Merge ~yeazelm/cloud-init:ssh_authorizedkeys into cloud-init:master

[Matthew Yeazel]

Fix issue with multiple AuthorizedKeysFile entries

If a user puts two entries for AuthorizedKeysFile in an sshd_config,
then cloud-init would fail when trying to add keys.
The change is to pick the first entry.

LP: #1404060

[Scott Moser (smoser)]

Hi,
A couple things
a.) run 'tox' on your branch . (I'll point the c-i at it and it will report here, but it will complain about long lines in your dummy_users_ssh_info.

b.) can you put a test for "expected" path ? just one where there
is only a single filename in Authorizedkeysfile.

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/485/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/485/rebuild

review: Needs Fixing (continuous-integration)

50ad259... by Matthew Yeazel on 2017-11-13

Add test for one entry

[Matthew Yeazel (yeazelm)]

Added an additional test and fixed the formatting (I think). Is there a way I can kick off the CI?

[Scott Moser (smoser)]

I started one. It should post shortly

On November 13, 2017 4:59:59 PM EST, Matthew Yeazel <email address hidden> wrote:
>Added an additional test and fixed the formatting (I think). Is there a
>way I can kick off the CI?
>--
>https://code.launchpad.net/~yeazelm/cloud-init/+git/cloud-init/+merge/331897
>You are reviewing the proposed merge of
>~yeazelm/cloud-init:ssh_authorizedkeys into cloud-init:master.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/490/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/490/rebuild

review: Needs Fixing (continuous-integration)

4dd5d27... by Matthew Yeazel on 2017-11-14

Fix more errors with flake8 and imports

[Matthew Yeazel (yeazelm)]

Ok, from what I can tell I fixed my tests, but it looks like pre-existing tests still fail.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:d5de4e2fdb7960c0c37d07ff4e271ef562d141b2
https://jenkins.ubuntu.com/server/job/cloud-init-ci/503/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/503/rebuild

review: Needs Fixing (continuous-integration)

c02b357... by Matthew Yeazel on 2017-11-16

Fix flake error for unused variable

Removed the variable exc since it was causing flake8 errors

"F841 local variable 'exc' is assigned to but never used"

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ed2496354f000ebc08cbc2ab62df13cc59d8216f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/506/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/506/rebuild

review: Needs Fixing (continuous-integration)

1de7f2e... by Matthew Yeazel on 2017-11-16

Fix flake error with import order

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:3f65e992d942746f3102a8442eee15d17e559215
https://jenkins.ubuntu.com/server/job/cloud-init-ci/507/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/507/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:3f65e992d942746f3102a8442eee15d17e559215
https://jenkins.ubuntu.com/server/job/cloud-init-ci/527/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/527/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

Matthew,

Sorry for delay.
I took a look today, and had some more comments inline.

The mock.side_effect can definitely be confusing, but the way you did it does not get the change *undone*, so other tests will have your dummy_users_ssh_info used.

http://paste.ubuntu.com/26013468/

^ is a change that sums up what I tried to say in English.

[Matthew Yeazel (yeazelm)]

I agree with your comments, and your patch looks good. I'll push another commit with those changes.

adab261... by Matthew Yeazel on 2017-11-21

Add patch from smoser on LP bug

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:79b3c9e7b5098004a25d7e021997e4bcac8ccf31
https://jenkins.ubuntu.com/server/job/cloud-init-ci/531/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/531/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

OK.
Two things are causing your branch to fail integration test.
a.) your package is older than is in xenial. Thats not your fault
and I put a MP up to that at
 https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/334074

b.) the integration tests will actually show a failure on this.
I'm pretty sure its not just noise.

Even though 'a' should make you not need to do it, you should rebase your
branch on master (git rebase master).

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:764002694e67561534cf1e58d6af3f961aed6162
https://jenkins.ubuntu.com/server/job/cloud-init-ci/544/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/544/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:c5781d2c148d0f839b9004a35ed51fb91150e211
https://jenkins.ubuntu.com/server/job/cloud-init-ci/644/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/644/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:adab2612b3bea011da9d3d00786e11683da078ef
https://jenkins.ubuntu.com/server/job/cloud-init-ci/678/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/678/rebuild

review: Needs Fixing (continuous-integration)

Unmerged commits

adab261... by Matthew Yeazel on 2017-11-21

Add patch from smoser on LP bug

1de7f2e... by Matthew Yeazel on 2017-11-16

Fix flake error with import order

c02b357... by Matthew Yeazel on 2017-11-16

Fix flake error for unused variable

Removed the variable exc since it was causing flake8 errors

"F841 local variable 'exc' is assigned to but never used"

4dd5d27... by Matthew Yeazel on 2017-11-14

Fix more errors with flake8 and imports

50ad259... by Matthew Yeazel on 2017-11-13

Add test for one entry

895ba3c... by Matthew Yeazel on 2017-10-05

Fix issue with multiple AuthorizedKeysFile entries

If a user puts two entries in the AuthorizedKeysFile, cloud-init
will manage the keys in the first file.

https://bugs.launchpad.net/cloud-init/+bug/1404060

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index d1d0975..0b07aae 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -448,7 +448,7 @@ class DataSourceAzure(sources.DataSource):
                   self.ds_cfg['agent_command'])
         try:
             fabric_data = metadata_func()
-        except Exception as exc:
+        except Exception:
             LOG.warning(
                 "Error communicating with Azure fabric; You may experience."
                 "connectivity issues.", exc_info=True)
diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py
index b95b956..1f8feae 100644
--- a/cloudinit/ssh_util.py
+++ b/cloudinit/ssh_util.py
@@ -9,6 +9,8 @@
 import os
 import pwd
 
+from shlex import split as shell_split
+
 from cloudinit import log as logging
 from cloudinit import util
 
@@ -220,6 +222,11 @@ def extract_authorized_keys(username):
             # authenticated and %u is replaced by the username of that user.
             ssh_cfg = parse_ssh_config_map(DEF_SSHD_CFG)
             auth_key_fn = ssh_cfg.get("authorizedkeysfile", '').strip()
+            # AuthorizedKeysFile can have multiple entries separated with
+            # whitespace, this will set auth_key_fn to the first entry, we
+            # will manage keys in the first file listed, but not break
+            # configuration for more than one file.
+            auth_key_fn = shell_split(auth_key_fn)[0]
             if not auth_key_fn:
                 auth_key_fn = "%h/.ssh/authorized_keys"
             auth_key_fn = auth_key_fn.replace("%h", pw_ent.pw_dir)
diff --git a/tests/unittests/test_sshutil.py b/tests/unittests/test_sshutil.py
index 2a8e6ab..09109f6 100644
--- a/tests/unittests/test_sshutil.py
+++ b/tests/unittests/test_sshutil.py
@@ -1,10 +1,14 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
-from mock import patch
+from cloudinit.tests.helpers import mock
+
+from collections import namedtuple
 
 from cloudinit import ssh_util
 from cloudinit.tests import helpers as test_helpers
 
+from mock import patch
+
 
 VALID_CONTENT = {
     'dsa': (
@@ -193,4 +197,40 @@ class TestParseSSHConfig(test_helpers.TestCase):
         self.assertEqual('foo', ret[0].key)
         self.assertEqual('bar', ret[0].value)
 
+
+def _dummy_users_ssh_info(username):
+    # Faking a passwd entry for mocking
+    kwargs = {"pw_name": username, "pw_passwd": "***",
+              "pw_uid": 101, "pw_gid": 101, "pw_gecos": username,
+              "pw_dir": "/home/{}".format(username), "pw_shell": "/bin/bash"}
+    Passwd = namedtuple("Passwd", kwargs.keys())
+    return '/home/{}/.ssh'.format(username), Passwd(**kwargs)
+
+
+class TestExtractAuthorizedKeys(test_helpers.TestCase):
+
+    @mock.patch("cloudinit.ssh_util.users_ssh_info")
+    @mock.patch("cloudinit.ssh_util.parse_ssh_config_map")
+    def test_one_entry(self, m_parse_ssh_config_map, m_users_ssh_info):
+        one_entry = '%h/.ssh/authorized_keys'
+        m_users_ssh_info.side_effect = _dummy_users_ssh_info
+        m_parse_ssh_config_map.return_value = {
+            "authorizedkeysfile": one_entry
+        }
+
+        ret = ssh_util.extract_authorized_keys('testuser')
+        self.assertEqual('/home/testuser/.ssh/authorized_keys', ret[0])
+
+    @mock.patch("cloudinit.ssh_util.users_ssh_info")
+    @mock.patch("cloudinit.ssh_util.parse_ssh_config_map")
+    def test_two_entries(self, m_parse_ssh_config_map, m_users_ssh_info):
+        both_entries = '%h/.ssh/authorized_keys /etc/ssh/authorized_keys/%u'
+        m_users_ssh_info.side_effect = _dummy_users_ssh_info
+        m_parse_ssh_config_map.return_value = {
+            "authorizedkeysfile": both_entries
+        }
+
+        ret = ssh_util.extract_authorized_keys('testuser')
+        self.assertEqual('/home/testuser/.ssh/authorized_keys', ret[0])
+
 # vi: ts=4 expandtab