From 7a7fc9e6b9c7d0eb9984ee9c6402dd9c563e7e7d Mon Sep 17 00:00:00 2001 From: Eduardo Otubo Date: Fri, 28 Jun 2019 14:44:16 +0200 Subject: [PATCH 1/5] Multiple file fix for AuthorizedKeysFile config Currently cloud-init does not know how to handle multiple file configuration on section AuthorizedKeysFile of ssh configuration. cloud-init will mess up the home user directory by creating bogus folders inside it. This patch provides a fix for this erroneous behavior. It gathers all keys from all the files listed on the section AuthorizedKeysFile of ssh configuration and merge all of them inside home user ~/.ssh/authorized_keys of the vm deployed. Signed-off-by: Eduardo Otubo --- cloudinit/ssh_util.py | 76 +++++++++++++++------- tests/unittests/test_sshutil.py | 112 ++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 24 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 3f99b58ccb3..bc984e413d1 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -8,6 +8,7 @@ import os import pwd +import re from cloudinit import log as logging from cloudinit import util @@ -160,19 +161,20 @@ def parse_ssh_key(ent): comment=comment, options=options) -def parse_authorized_keys(fname): +def parse_authorized_keys(fnames): lines = [] - try: - if os.path.isfile(fname): - lines = util.load_file(fname).splitlines() - except (IOError, OSError): - util.logexc(LOG, "Error reading lines from %s", fname) - lines = [] - parser = AuthKeyLineParser() contents = [] - for line in lines: - contents.append(parser.parse(line)) + for fname in fnames: + try: + if os.path.isfile(fname): + lines = util.load_file(fname).splitlines() + for line in lines: + contents.append(parser.parse(line)) + except (IOError, OSError): + util.logexc(LOG, "Error reading lines from %s", fname) + lines = [] + return contents @@ -211,32 +213,58 @@ def users_ssh_info(username): return (os.path.join(pw_ent.pw_dir, '.ssh'), pw_ent) -def extract_authorized_keys(username): +def render_authorizedkeysfile(value, homedir, username): + if value is None: + value = "%h/.ssh/authorized_keys" + for macro, field in (("%h", homedir), ("%u", username), ("%%", "%")): + value = value.replace(macro, field) + if not value.startswith("/"): + value = os.path.join(homedir, value) + return value + + +def extract_authorized_keys(username, sshd_cfg_file=DEF_SSHD_CFG): (ssh_dir, pw_ent) = users_ssh_info(username) - auth_key_fn = None + auth_key_fns = [] with util.SeLinuxGuard(ssh_dir, recursive=True): try: + default_authorizedkeys_file = "%h/.ssh/authorized_keys" + # The 'AuthorizedKeysFile' may contain tokens # of the form %T which are substituted during connection set-up. # The following tokens are defined: %% is replaced by a literal # '%', %h is replaced by the home directory of the user being # 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() - if not auth_key_fn: - auth_key_fn = "%h/.ssh/authorized_keys" - auth_key_fn = auth_key_fn.replace("%h", pw_ent.pw_dir) - auth_key_fn = auth_key_fn.replace("%u", username) - auth_key_fn = auth_key_fn.replace("%%", '%') - if not auth_key_fn.startswith('/'): - auth_key_fn = os.path.join(pw_ent.pw_dir, auth_key_fn) + ssh_cfg = parse_ssh_config_map(sshd_cfg_file) + auth_key_fns = re.split( + r'(? 1: + util.logexc( + LOG, + "Looks like there's more than one authorizedkeys file" + " configured. Make sure there's no white spaces in" + " their paths. If they do, escape with '\\'.") + + for i in range(len(auth_key_fns)): + auth_key_fns[i] = render_authorizedkeysfile( + auth_key_fns[i], + pw_ent.pw_dir, + username) + print(auth_key_fns[i]) + except (IOError, OSError): # Give up and use a default key filename - auth_key_fn = os.path.join(ssh_dir, 'authorized_keys') + auth_key_fns[0] = os.path.join(ssh_dir, 'authorized_keys') util.logexc(LOG, "Failed extracting 'AuthorizedKeysFile' in ssh " "config from %r, using 'AuthorizedKeysFile' file " - "%r instead", DEF_SSHD_CFG, auth_key_fn) - return (auth_key_fn, parse_authorized_keys(auth_key_fn)) + "%r instead", DEF_SSHD_CFG, auth_key_fns[0]) + + # always store all the keys in the user's private file + return (default_authorizedkeys_file, parse_authorized_keys(auth_key_fns)) def setup_user_keys(keys, username, options=None): diff --git a/tests/unittests/test_sshutil.py b/tests/unittests/test_sshutil.py index 73ae897f2f0..62b9f9ef784 100644 --- a/tests/unittests/test_sshutil.py +++ b/tests/unittests/test_sshutil.py @@ -5,6 +5,7 @@ from cloudinit import ssh_util from cloudinit.tests import helpers as test_helpers from cloudinit import util +from cloudinit import distros VALID_CONTENT = { @@ -57,6 +58,45 @@ 'user \"root\".\';echo;sleep 10"') +class MyBaseDistro(distros.Distro): + # MyBaseDistro is here to test TestMultipleSshAuthorizedKeysFile + # And for that we need to create a new user + + def __init__(self, name="basedistro", cfg=None, paths=None): + if not cfg: + cfg = {} + if not paths: + paths = {} + super(MyBaseDistro, self).__init__(name, cfg, paths) + + def install_packages(self, pkglist): + raise NotImplementedError() + + def _write_network(self, settings): + raise NotImplementedError() + + def package_command(self, cmd, args=None, pkgs=None): + raise NotImplementedError() + + def update_package_sources(self): + raise NotImplementedError() + + def apply_locale(self, locale, out_fn=None): + raise NotImplementedError() + + def set_timezone(self, tz): + raise NotImplementedError() + + def _read_hostname(self, filename, default=None): + raise NotImplementedError() + + def _write_hostname(self, hostname, filename): + raise NotImplementedError() + + def _read_system_hostname(self): + raise NotImplementedError() + + class TestAuthKeyLineParser(test_helpers.CiTestCase): def test_simple_parse(self): @@ -326,4 +366,76 @@ def test_not_modified(self): m_write_file.assert_not_called() +class TestMultipleSshAuthorizedKeysFile(test_helpers.CiTestCase): + username = 'foouser' + dist = MyBaseDistro() + dist.create_user(username) + key_entries = [] + + def test_multiple_authorizedkeys_file_order1(self): + authorized_keys = self.tmp_path('/tmp/authorized_keys') + util.write_file(authorized_keys, VALID_CONTENT['rsa']) + + user_keys = self.tmp_path('/tmp/user_keys') + util.write_file(user_keys, VALID_CONTENT['dsa']) + + sshd_config = self.tmp_path('/tmp/sshd_config') + util.write_file( + sshd_config, + "AuthorizedKeysFile /tmp/authorized_keys /tmp/user_keys") + + (auth_key_fn, auth_key_entries) = ssh_util.extract_authorized_keys( + self.username, sshd_config) + content = ssh_util.update_authorized_keys( + auth_key_entries, + self.key_entries) + + self.assertEqual(auth_key_fn, "%h/.ssh/authorized_keys") + self.assertTrue(VALID_CONTENT['rsa'] in content) + self.assertTrue(VALID_CONTENT['dsa'] in content) + + def test_multiple_authorizedkeys_file_order2(self): + authorized_keys = self.tmp_path('/tmp/authorized_keys') + util.write_file(authorized_keys, VALID_CONTENT['rsa']) + + user_keys = self.tmp_path('/tmp/user_keys') + util.write_file(user_keys, VALID_CONTENT['dsa']) + + sshd_config = self.tmp_path('/tmp/sshd_config') + util.write_file( + sshd_config, + "AuthorizedKeysFile /tmp/user_keys /tmp/authorized_keys") + + (auth_key_fn, auth_key_entries) = ssh_util.extract_authorized_keys( + self.username, sshd_config) + content = ssh_util.update_authorized_keys( + auth_key_entries, self.key_entries) + + self.assertEqual(auth_key_fn, "%h/.ssh/authorized_keys") + self.assertTrue(VALID_CONTENT['rsa'] in content) + self.assertTrue(VALID_CONTENT['dsa'] in content) + + def test_white_spaces(self): + authorized_keys = self.tmp_path('/tmp/authorized_keys') + util.write_file(authorized_keys, VALID_CONTENT['rsa']) + + user_keys = self.tmp_path('/tmp/user\\ keys') + util.write_file(user_keys, VALID_CONTENT['dsa']) + + sshd_config = self.tmp_path('/tmp/sshd_config') + util.write_file( + sshd_config, + "AuthorizedKeysFile /tmp/authorized_keys /tmp/user\\ keys") + + (auth_key_fn, auth_key_entries) = ssh_util.extract_authorized_keys( + self.username, + sshd_config) + content = ssh_util.update_authorized_keys( + auth_key_entries, + self.key_entries) + + self.assertEqual(auth_key_fn, "%h/.ssh/authorized_keys") + self.assertTrue(VALID_CONTENT['rsa'] in content) + self.assertTrue(VALID_CONTENT['dsa'] in content) + # vi: ts=4 expandtab From 3fd94105b4938ec250de9829b9f378a15d04136d Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 22 Nov 2019 14:07:17 -0500 Subject: [PATCH 2/5] Simplify testing. This adds render_authorizedkeysfile_paths which does not require any interaction with OS. It is then much easier to test it. The 4 added tests give pretty good coverage of the new code. Note, I had to comment out the 'dist.create_user' call as it was actually trying to create a user. --- cloudinit/ssh_util.py | 57 ++++++-------- tests/unittests/test_sshutil.py | 133 +++++++++++++------------------- 2 files changed, 74 insertions(+), 116 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index bc984e413d1..cce5834bfe0 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -173,7 +173,6 @@ def parse_authorized_keys(fnames): contents.append(parser.parse(line)) except (IOError, OSError): util.logexc(LOG, "Error reading lines from %s", fname) - lines = [] return contents @@ -213,52 +212,40 @@ def users_ssh_info(username): return (os.path.join(pw_ent.pw_dir, '.ssh'), pw_ent) -def render_authorizedkeysfile(value, homedir, username): - if value is None: +def render_authorizedkeysfile_paths(value, homedir, username): + # The 'AuthorizedKeysFile' may contain tokens + # of the form %T which are substituted during connection set-up. + # The following tokens are defined: %% is replaced by a literal + # '%', %h is replaced by the home directory of the user being + # authenticated and %u is replaced by the username of that user. + macros = (("%h", homedir), ("%u", username), ("%%", "%")) + if not value: value = "%h/.ssh/authorized_keys" - for macro, field in (("%h", homedir), ("%u", username), ("%%", "%")): - value = value.replace(macro, field) - if not value.startswith("/"): - value = os.path.join(homedir, value) - return value + paths = re.split(r'(? 1: - util.logexc( - LOG, - "Looks like there's more than one authorizedkeys file" - " configured. Make sure there's no white spaces in" - " their paths. If they do, escape with '\\'.") - - for i in range(len(auth_key_fns)): - auth_key_fns[i] = render_authorizedkeysfile( - auth_key_fns[i], - pw_ent.pw_dir, - username) - print(auth_key_fns[i]) + auth_key_fns = render_authorizedkeysfile_paths( + ssh_cfg.get("authorizedkeysfile", "%h/.ssh/authorized_keys"), + pw_ent.pw_dir, username) except (IOError, OSError): # Give up and use a default key filename - auth_key_fns[0] = os.path.join(ssh_dir, 'authorized_keys') + auth_key_fns[0] = default_authorizedkeys_file util.logexc(LOG, "Failed extracting 'AuthorizedKeysFile' in ssh " "config from %r, using 'AuthorizedKeysFile' file " "%r instead", DEF_SSHD_CFG, auth_key_fns[0]) diff --git a/tests/unittests/test_sshutil.py b/tests/unittests/test_sshutil.py index 62b9f9ef784..b227c20bb01 100644 --- a/tests/unittests/test_sshutil.py +++ b/tests/unittests/test_sshutil.py @@ -1,11 +1,18 @@ # This file is part of cloud-init. See LICENSE file for license information. from mock import patch +from collections import namedtuple from cloudinit import ssh_util from cloudinit.tests import helpers as test_helpers from cloudinit import util -from cloudinit import distros + +# https://stackoverflow.com/questions/11351032/ +FakePwEnt = namedtuple( + 'FakePwEnt', + ['pw_dir', 'pw_gecos', 'pw_name', 'pw_passwd', 'pw_shell', 'pwd_uid']) +FakePwEnt.__new__.__defaults__ = tuple( + "UNSET_%s" % n for n in FakePwEnt._fields) VALID_CONTENT = { @@ -58,45 +65,6 @@ 'user \"root\".\';echo;sleep 10"') -class MyBaseDistro(distros.Distro): - # MyBaseDistro is here to test TestMultipleSshAuthorizedKeysFile - # And for that we need to create a new user - - def __init__(self, name="basedistro", cfg=None, paths=None): - if not cfg: - cfg = {} - if not paths: - paths = {} - super(MyBaseDistro, self).__init__(name, cfg, paths) - - def install_packages(self, pkglist): - raise NotImplementedError() - - def _write_network(self, settings): - raise NotImplementedError() - - def package_command(self, cmd, args=None, pkgs=None): - raise NotImplementedError() - - def update_package_sources(self): - raise NotImplementedError() - - def apply_locale(self, locale, out_fn=None): - raise NotImplementedError() - - def set_timezone(self, tz): - raise NotImplementedError() - - def _read_hostname(self, filename, default=None): - raise NotImplementedError() - - def _write_hostname(self, hostname, filename): - raise NotImplementedError() - - def _read_system_hostname(self): - raise NotImplementedError() - - class TestAuthKeyLineParser(test_helpers.CiTestCase): def test_simple_parse(self): @@ -366,75 +334,78 @@ def test_not_modified(self): m_write_file.assert_not_called() -class TestMultipleSshAuthorizedKeysFile(test_helpers.CiTestCase): - username = 'foouser' - dist = MyBaseDistro() - dist.create_user(username) - key_entries = [] +class TestBasicAuthorizedKeyParse(test_helpers.CiTestCase): + def test_user(self): + self.assertEqual( + ["/opt/bobby/keys"], + ssh_util.render_authorizedkeysfile_paths( + "/opt/%u/keys", "/home/bobby", "bobby")) - def test_multiple_authorizedkeys_file_order1(self): - authorized_keys = self.tmp_path('/tmp/authorized_keys') - util.write_file(authorized_keys, VALID_CONTENT['rsa']) + def test_multiple(self): + self.assertEqual( + ["/keys/path1", "/keys/path2"], + ssh_util.render_authorizedkeysfile_paths( + "/keys/path1 /keys/path2", "/home/bobby", "bobby")) - user_keys = self.tmp_path('/tmp/user_keys') - util.write_file(user_keys, VALID_CONTENT['dsa']) + def test_relative(self): + self.assertEqual( + ["/home/bobby/.secret/keys"], + ssh_util.render_authorizedkeysfile_paths( + ".secret/keys", "/home/bobby", "bobby")) - sshd_config = self.tmp_path('/tmp/sshd_config') - util.write_file( - sshd_config, - "AuthorizedKeysFile /tmp/authorized_keys /tmp/user_keys") + def test_home(self): + self.assertEqual( + ["/homedirs/bobby/.keys"], + ssh_util.render_authorizedkeysfile_paths( + "%h/.keys", "/homedirs/bobby", "bobby")) - (auth_key_fn, auth_key_entries) = ssh_util.extract_authorized_keys( - self.username, sshd_config) - content = ssh_util.update_authorized_keys( - auth_key_entries, - self.key_entries) - self.assertEqual(auth_key_fn, "%h/.ssh/authorized_keys") - self.assertTrue(VALID_CONTENT['rsa'] in content) - self.assertTrue(VALID_CONTENT['dsa'] in content) +class TestMultipleSshAuthorizedKeysFile(test_helpers.CiTestCase): - def test_multiple_authorizedkeys_file_order2(self): - authorized_keys = self.tmp_path('/tmp/authorized_keys') + @patch("cloudinit.ssh_util.pwd.getpwnam") + def test_multiple_authorizedkeys_file_order1(self, m_getpwnam): + fpw = FakePwEnt(pw_name='bobby', pw_dir='/home2/bobby') + m_getpwnam.return_value = fpw + authorized_keys = self.tmp_path('authorized_keys') util.write_file(authorized_keys, VALID_CONTENT['rsa']) - user_keys = self.tmp_path('/tmp/user_keys') + user_keys = self.tmp_path('user_keys') util.write_file(user_keys, VALID_CONTENT['dsa']) - sshd_config = self.tmp_path('/tmp/sshd_config') + sshd_config = self.tmp_path('sshd_config') util.write_file( sshd_config, - "AuthorizedKeysFile /tmp/user_keys /tmp/authorized_keys") + "AuthorizedKeysFile %s %s" % (authorized_keys, user_keys)) (auth_key_fn, auth_key_entries) = ssh_util.extract_authorized_keys( - self.username, sshd_config) + fpw.pw_name, sshd_config) content = ssh_util.update_authorized_keys( - auth_key_entries, self.key_entries) + auth_key_entries, []) - self.assertEqual(auth_key_fn, "%h/.ssh/authorized_keys") + self.assertEqual("%s/.ssh/authorized_keys" % fpw.pw_dir, auth_key_fn) self.assertTrue(VALID_CONTENT['rsa'] in content) self.assertTrue(VALID_CONTENT['dsa'] in content) - def test_white_spaces(self): - authorized_keys = self.tmp_path('/tmp/authorized_keys') + @patch("cloudinit.ssh_util.pwd.getpwnam") + def test_multiple_authorizedkeys_file_order2(self, m_getpwnam): + fpw = FakePwEnt(pw_name='suzie', pw_dir='/home/suzie') + m_getpwnam.return_value = fpw + authorized_keys = self.tmp_path('authorized_keys') util.write_file(authorized_keys, VALID_CONTENT['rsa']) - user_keys = self.tmp_path('/tmp/user\\ keys') + user_keys = self.tmp_path('user_keys') util.write_file(user_keys, VALID_CONTENT['dsa']) - sshd_config = self.tmp_path('/tmp/sshd_config') + sshd_config = self.tmp_path('sshd_config') util.write_file( sshd_config, - "AuthorizedKeysFile /tmp/authorized_keys /tmp/user\\ keys") + "AuthorizedKeysFile %s %s" % (authorized_keys, user_keys)) (auth_key_fn, auth_key_entries) = ssh_util.extract_authorized_keys( - self.username, - sshd_config) - content = ssh_util.update_authorized_keys( - auth_key_entries, - self.key_entries) + fpw.pw_name, sshd_config) + content = ssh_util.update_authorized_keys(auth_key_entries, []) - self.assertEqual(auth_key_fn, "%h/.ssh/authorized_keys") + self.assertEqual("%s/.ssh/authorized_keys" % fpw.pw_dir, auth_key_fn) self.assertTrue(VALID_CONTENT['rsa'] in content) self.assertTrue(VALID_CONTENT['dsa'] in content) From 523617a9021efe785954f70c0ada54fdf251db5e Mon Sep 17 00:00:00 2001 From: Eduardo Otubo Date: Thu, 5 Dec 2019 10:23:53 +0100 Subject: [PATCH 3/5] Replacing re.split() regex by simple split() --- cloudinit/ssh_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index cce5834bfe0..7dca332d5eb 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -221,7 +221,7 @@ def render_authorizedkeysfile_paths(value, homedir, username): macros = (("%h", homedir), ("%u", username), ("%%", "%")) if not value: value = "%h/.ssh/authorized_keys" - paths = re.split(r'(? Date: Thu, 5 Dec 2019 10:31:30 +0100 Subject: [PATCH 4/5] removing import re, since not used --- cloudinit/ssh_util.py | 1 - 1 file changed, 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 7dca332d5eb..f963010af73 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -8,7 +8,6 @@ import os import pwd -import re from cloudinit import log as logging from cloudinit import util From 5819a290df0794327ecaf90a2d885b2d6b25e036 Mon Sep 17 00:00:00 2001 From: Eduardo Otubo Date: Thu, 5 Dec 2019 17:22:30 +0100 Subject: [PATCH 5/5] Removing strip() --- cloudinit/ssh_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index f963010af73..bcb23a5ae98 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -220,7 +220,7 @@ def render_authorizedkeysfile_paths(value, homedir, username): macros = (("%h", homedir), ("%u", username), ("%%", "%")) if not value: value = "%h/.ssh/authorized_keys" - paths = value.strip().split() + paths = value.split() rendered = [] for path in paths: for macro, field in macros: