Merge ~amzn-cmiller/cloud-init:ssh_authorizedkeys into cloud-init:master

[Chad]

Mimic list-behavior of sshd AuthorizedKeyFile setting

Quoth sshd_config(5), "Multiple files may be listed, separated by whitespace". Use the first location specified as where to write
provided keys.

LP: #1404060

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

PASSED: Continuous integration, rev:a8a3b5489fb29be2b2e7b462569f61698e9f9c62
https://jenkins.ubuntu.com/server/job/cloud-init-ci/649/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

If you have evidence that shlex.split() is needed, then just provide it and I approve.

review: Needs Information

be685b5... by Chad on 2019-03-26

Split AuthorizedKeysFile as sshd does

[Chad (amzn-cmiller)]

We don't need `shlex.split()` per se, because it doesn't behave like sshd. Neither does "".split() .

Config listing `AuthorizedKeysFile /etc/ssh/"weird-file-name" %h/.ssh/authorized_keys`
looks for `/etc/ssh/weird-file-name`

Config listing `AuthorizedKeysFile "/etc/ssh/authkey /foo" %h/.ssh/authorized_keys`
looks for (as anticipated) `/etc/ssh/authkey /foo`

Config listing `AuthorizedKeysFile /etc/ssh/authkey\ /foo %h/.ssh/authorized_keys`
looks for `/etc/ssh/authkey\` !

shlex does both too much and too little. That ssh parsing is an irregular language, so a Regular Expression can't match or split it.

Your case of having quotes in the weird name would never have worked in ssh, and I don't see a way to include literal quotes (though I have not examined source code there).

But, backslash-space would not count as a literal space, but would in shlex.split() . I tested the directive in the comments in this patch to verify it behaves the same.

[Ryan Harper (raharper)]

I've pointed CI at this updated branch.

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

FAILED: Continuous integration, rev:be685b51802f80dce57909d80771720d0606616c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/672/
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/672/rebuild

review: Needs Fixing (continuous-integration)

[Ryan Harper (raharper)]

I've marked this branch Work-in-Progress. Once you've fixed the CI issue, please push changes and then mark the state back to Needs Review.

Unmerged commits

be685b5... by Chad on 2019-03-26

Split AuthorizedKeysFile as sshd does

a8a3b54... by Chad on 2019-03-25

Prove that space are allowed in files in authkeysfile directive

2476a40... by Matthew Yeazel on 2017-11-21

Add patch from smoser on LP bug

7e72941... by Matthew Yeazel on 2017-11-16

Fix flake error with import order

e77b7d5... by Matthew Yeazel on 2017-11-14

Fix more errors with flake8 and imports

8d11bc9... by Matthew Yeazel on 2017-11-13

Add test for one entry

e53d23b... 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/ssh_util.py b/cloudinit/ssh_util.py
index 3f99b58..5f4a8f8 100644
--- a/cloudinit/ssh_util.py
+++ b/cloudinit/ssh_util.py
@@ -8,6 +8,8 @@
 
 import os
 import pwd
+import re
+import shlex
 
 from cloudinit import log as logging
 from cloudinit import util
@@ -176,6 +178,26 @@ def parse_authorized_keys(fname):
     return contents
 
 
+def file_list_split(file_list_string):  # type: (Text) -> List(Text)
+    r"""Split filename list like sshd does. The difference from shlex
+    is insensitivity to backslash. So, escape all backslashes so shlex will
+    leave them intact. We need shlex.split for its parser.
+
+    >>> file_list_split(r'a "b c" d\ e f\g h\\i')
+    ['a', 'b c', 'd\\', 'e', 'f\\g', 'h\\\\i']
+
+    AuthorizedKeysFile    a "b c" d\ e f\g h\\i
+
+    Openssh (7.2) searches for these names (in user home because
+    nonabsolute names),   'a'   'b c'   'd\'   'e'   'f\g'   'h\\i'
+    which show up in log files with one layer of backslash-escaping.
+
+    @param file_list_string: filenames separated by space, opt quoted
+    @return: each filename as element of list
+    """
+    return shlex.split(re.sub(r"\\", r"\\\\", file_list_string))
+
+
 def update_authorized_keys(old_entries, keys):
     to_add = list([k for k in keys if k.valid()])
     for i in range(0, len(old_entries)):
@@ -223,6 +245,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 = file_list_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 73ae897..9756f9b 100644
--- a/tests/unittests/test_sshutil.py
+++ b/tests/unittests/test_sshutil.py
@@ -1,11 +1,15 @@
 # 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 cloudinit import util
 
+from mock import patch
+
 
 VALID_CONTENT = {
     'dsa': (
@@ -326,4 +330,63 @@ class TestUpdateSshConfig(test_helpers.CiTestCase):
         m_write_file.assert_not_called()
 
 
+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])
+
+    @mock.patch("cloudinit.ssh_util.users_ssh_info")
+    @mock.patch("cloudinit.ssh_util.parse_ssh_config_map")
+    def test_quoted_name(self, m_parse_ssh_config_map, m_users_ssh_info):
+        both_entries = '"/wi"t"h spa"ces/ak "second"'
+        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('/with spaces/ak', ret[0])
+
+    @mock.patch("cloudinit.ssh_util.users_ssh_info")
+    @mock.patch("cloudinit.ssh_util.parse_ssh_config_map")
+    def test_backslash_ignore(self, m_parse_ssh_config_map, m_users_ssh_info):
+        both_entries = r'/first\ second'
+        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('/first\\', ret[0])
+
 # vi: ts=4 expandtab