From d5cc8b6896eaecfc63ac9b733a23d8ef1a4e26f6 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 17 Aug 2021 10:01:49 -0500 Subject: [PATCH 1/6] Fix home permissions modified by ssh module In #956, we updated the file and directory permissions for keys not in the user's home directory. We also unintentionally modified the permissions within the home directory as well. These should not change, and this commit changes that back. LP: #1940233 --- cloudinit/ssh_util.py | 18 ++++- .../modules/test_ssh_keysfile.py | 74 +++++++++++++++---- 2 files changed, 72 insertions(+), 20 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index b8a3c8f75ae..f2e0d31b15b 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -321,7 +321,8 @@ def check_create_path(username, filename, strictmodes): home_folder = os.path.dirname(user_pwent.pw_dir) for directory in directories: parent_folder += "/" + directory - if home_folder.startswith(parent_folder): + if (home_folder.startswith(parent_folder) or + parent_folder == user_pwent.pw_dir): continue if not os.path.isdir(parent_folder): @@ -329,9 +330,18 @@ def check_create_path(username, filename, strictmodes): # create the directory, and make it accessible by everyone # but owned by root, as it might be used by many users. with util.SeLinuxGuard(parent_folder): - os.makedirs(parent_folder, mode=0o755, exist_ok=True) - util.chownbyid(parent_folder, root_pwent.pw_uid, - root_pwent.pw_gid) + mode = 0o755 + uid = root_pwent.pw_uid + gid = root_pwent.pw_gid + if parent_folder.startswith(user_pwent.pw_dir): + mode = 0o700 + uid = user_pwent.pw_uid + gid = user_pwent.pw_gid + try: + os.mkdir(parent_folder, mode=mode) + except FileExistsError: + continue + util.chownbyid(parent_folder, uid, gid) permissions = check_permissions(username, parent_folder, filename, False, strictmodes) diff --git a/tests/integration_tests/modules/test_ssh_keysfile.py b/tests/integration_tests/modules/test_ssh_keysfile.py index f82d76494ed..b2948f12fdf 100644 --- a/tests/integration_tests/modules/test_ssh_keysfile.py +++ b/tests/integration_tests/modules/test_ssh_keysfile.py @@ -10,10 +10,10 @@ TEST_USER2_KEYS = get_test_rsa_keypair('test2') TEST_DEFAULT_KEYS = get_test_rsa_keypair('test3') -USERDATA = """\ +_USERDATA = """\ #cloud-config bootcmd: - - sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile /etc/ssh/authorized_keys %h/.ssh/authorized_keys2;' /etc/ssh/sshd_config + - {bootcmd} ssh_authorized_keys: - {default} users: @@ -24,26 +24,20 @@ - name: test_user2 ssh_authorized_keys: - {user2} -""".format( # noqa: E501 +""".format( + bootcmd='{bootcmd}', default=TEST_DEFAULT_KEYS.public_key, user1=TEST_USER1_KEYS.public_key, user2=TEST_USER2_KEYS.public_key, ) +DEFAULT_KEYS_USERDATA = _USERDATA.format(bootcmd='""') +MODIFIED_KEYS_USERDATA = _USERDATA.format(bootcmd=( + "sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile " + "/etc/ssh/authorized_keys %h/.ssh/authorized_keys2;' " + "/etc/ssh/sshd_config")) -@pytest.mark.ubuntu -@pytest.mark.user_data(USERDATA) -def test_authorized_keys(client: IntegrationInstance): - expected_keys = [ - ('test_user1', '/home/test_user1/.ssh/authorized_keys2', - TEST_USER1_KEYS), - ('test_user2', '/home/test_user2/.ssh/authorized_keys2', - TEST_USER2_KEYS), - ('ubuntu', '/home/ubuntu/.ssh/authorized_keys2', - TEST_DEFAULT_KEYS), - ('root', '/root/.ssh/authorized_keys2', TEST_DEFAULT_KEYS), - ] - +def common_verify(client, expected_keys): for user, filename, keys in expected_keys: contents = client.read_from_file(filename) if user in ['ubuntu', 'root']: @@ -83,3 +77,51 @@ def test_authorized_keys(client: IntegrationInstance): look_for_keys=False, allow_agent=False, ) + + # Also ensure we haven't messed with any /home permissions + # See LP: #1940233 + home_dir = '/home/{}'.format(user) + home_perms = '755' + if user == 'root': + home_dir = '/root' + home_perms = '700' + assert '{} {}'.format(user, home_perms) == client.execute( + 'stat -c "%U %a" {}'.format(home_dir) + ) + assert '{} 700'.format(user) == client.execute( + 'stat -c "%U %a" {}/.ssh'.format(home_dir) + ) + assert '{} 600'.format(user) == client.execute( + 'stat -c "%U %a" {}'.format(filename) + ) + assert 'root 755' == client.execute('stat -c "%U %a" /home') + + +@pytest.mark.ubuntu +@pytest.mark.user_data(DEFAULT_KEYS_USERDATA) +def test_authorized_keys_default(client: IntegrationInstance): + expected_keys = [ + ('test_user1', '/home/test_user1/.ssh/authorized_keys', + TEST_USER1_KEYS), + ('test_user2', '/home/test_user2/.ssh/authorized_keys', + TEST_USER2_KEYS), + ('ubuntu', '/home/ubuntu/.ssh/authorized_keys', + TEST_DEFAULT_KEYS), + ('root', '/root/.ssh/authorized_keys', TEST_DEFAULT_KEYS), + ] + common_verify(client, expected_keys) + + +@pytest.mark.ubuntu +@pytest.mark.user_data(MODIFIED_KEYS_USERDATA) +def test_authorized_keys_modified(client: IntegrationInstance): + expected_keys = [ + ('test_user1', '/home/test_user1/.ssh/authorized_keys2', + TEST_USER1_KEYS), + ('test_user2', '/home/test_user2/.ssh/authorized_keys2', + TEST_USER2_KEYS), + ('ubuntu', '/home/ubuntu/.ssh/authorized_keys2', + TEST_DEFAULT_KEYS), + ('root', '/root/.ssh/authorized_keys2', TEST_DEFAULT_KEYS), + ] + common_verify(client, expected_keys) From 240a0795d9484a18b4a5798fb0f08bd5b7c139bf Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 17 Aug 2021 13:14:11 -0500 Subject: [PATCH 2/6] handle symlinks --- cloudinit/ssh_util.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index f2e0d31b15b..720f6baaebb 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -325,7 +325,7 @@ def check_create_path(username, filename, strictmodes): parent_folder == user_pwent.pw_dir): continue - if not os.path.isdir(parent_folder): + if not os.path.exists(parent_folder): # directory does not exist, and permission so far are good: # create the directory, and make it accessible by everyone # but owned by root, as it might be used by many users. @@ -339,6 +339,11 @@ def check_create_path(username, filename, strictmodes): gid = user_pwent.pw_gid try: os.mkdir(parent_folder, mode=mode) + except NotADirectoryError: + LOG.debug( + 'Cannot create directory. File exists in path: ' + '{}'.format(parent_folder)) + return False except FileExistsError: continue util.chownbyid(parent_folder, uid, gid) From 1a358bcfff92d71b9200715dd4aff10a6e756e3a Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 17 Aug 2021 14:12:43 -0500 Subject: [PATCH 3/6] Update integration test --- cloudinit/ssh_util.py | 4 +- .../modules/test_ssh_keysfile.py | 68 ++++++++++++++++--- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 720f6baaebb..4e216762d66 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -341,8 +341,8 @@ def check_create_path(username, filename, strictmodes): os.mkdir(parent_folder, mode=mode) except NotADirectoryError: LOG.debug( - 'Cannot create directory. File exists in path: ' - '{}'.format(parent_folder)) + 'Cannot create directory. File exists in path: %s', + parent_folder) return False except FileExistsError: continue diff --git a/tests/integration_tests/modules/test_ssh_keysfile.py b/tests/integration_tests/modules/test_ssh_keysfile.py index b2948f12fdf..0c27fc7bf78 100644 --- a/tests/integration_tests/modules/test_ssh_keysfile.py +++ b/tests/integration_tests/modules/test_ssh_keysfile.py @@ -30,15 +30,11 @@ user1=TEST_USER1_KEYS.public_key, user2=TEST_USER2_KEYS.public_key, ) -DEFAULT_KEYS_USERDATA = _USERDATA.format(bootcmd='""') -MODIFIED_KEYS_USERDATA = _USERDATA.format(bootcmd=( - "sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile " - "/etc/ssh/authorized_keys %h/.ssh/authorized_keys2;' " - "/etc/ssh/sshd_config")) def common_verify(client, expected_keys): for user, filename, keys in expected_keys: + # Ensure key is in the key file contents = client.read_from_file(filename) if user in ['ubuntu', 'root']: # Our personal public key gets added by pycloudlib @@ -88,15 +84,19 @@ def common_verify(client, expected_keys): assert '{} {}'.format(user, home_perms) == client.execute( 'stat -c "%U %a" {}'.format(home_dir) ) - assert '{} 700'.format(user) == client.execute( - 'stat -c "%U %a" {}/.ssh'.format(home_dir) - ) + if client.execute("test -f {}/.ssh".format(home_dir)).ok: + assert '{} 700'.format(user) == client.execute( + 'stat -c "%U %a" {}/.ssh'.format(home_dir) + ) assert '{} 600'.format(user) == client.execute( 'stat -c "%U %a" {}'.format(filename) ) assert 'root 755' == client.execute('stat -c "%U %a" /home') +DEFAULT_KEYS_USERDATA = _USERDATA.format(bootcmd='""') + + @pytest.mark.ubuntu @pytest.mark.user_data(DEFAULT_KEYS_USERDATA) def test_authorized_keys_default(client: IntegrationInstance): @@ -112,9 +112,15 @@ def test_authorized_keys_default(client: IntegrationInstance): common_verify(client, expected_keys) +AUTHORIZED_KEYS2_USERDATA = _USERDATA.format(bootcmd=( + "sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile " + "/etc/ssh/authorized_keys %h/.ssh/authorized_keys2;' " + "/etc/ssh/sshd_config")) + + @pytest.mark.ubuntu -@pytest.mark.user_data(MODIFIED_KEYS_USERDATA) -def test_authorized_keys_modified(client: IntegrationInstance): +@pytest.mark.user_data(AUTHORIZED_KEYS2_USERDATA) +def test_authorized_keys2(client: IntegrationInstance): expected_keys = [ ('test_user1', '/home/test_user1/.ssh/authorized_keys2', TEST_USER1_KEYS), @@ -125,3 +131,45 @@ def test_authorized_keys_modified(client: IntegrationInstance): ('root', '/root/.ssh/authorized_keys2', TEST_DEFAULT_KEYS), ] common_verify(client, expected_keys) + + +NESTED_KEYS_USERDATA = _USERDATA.format(bootcmd=( + "sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile " + "/etc/ssh/authorized_keys %h/foo/bar/ssh/keys;' " + "/etc/ssh/sshd_config")) + + +@pytest.mark.ubuntu +@pytest.mark.user_data(NESTED_KEYS_USERDATA) +def test_nested_keys(client: IntegrationInstance): + expected_keys = [ + ('test_user1', '/home/test_user1/foo/bar/ssh/keys', + TEST_USER1_KEYS), + ('test_user2', '/home/test_user2/foo/bar/ssh/keys', + TEST_USER2_KEYS), + ('ubuntu', '/home/ubuntu/foo/bar/ssh/keys', + TEST_DEFAULT_KEYS), + ('root', '/root/foo/bar/ssh/keys', TEST_DEFAULT_KEYS), + ] + common_verify(client, expected_keys) + + +EXTERNAL_KEYS_USERDATA = _USERDATA.format(bootcmd=( + "sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile " + "/etc/ssh/authorized_keys /etc/ssh/authorized_keys/%u/keys;' " + "/etc/ssh/sshd_config")) + + +@pytest.mark.ubuntu +@pytest.mark.user_data(EXTERNAL_KEYS_USERDATA) +def test_external_keys(client: IntegrationInstance): + expected_keys = [ + ('test_user1', '/etc/ssh/authorized_keys/test_user1/keys', + TEST_USER1_KEYS), + ('test_user2', '/etc/ssh/authorized_keys/test_user2/keys', + TEST_USER2_KEYS), + ('ubuntu', '/etc/ssh/authorized_keys/ubuntu/keys', + TEST_DEFAULT_KEYS), + ('root', '/etc/ssh/authorized_keys/root/keys', TEST_DEFAULT_KEYS), + ] + common_verify(client, expected_keys) From bb786eb9ace987f4a09d63d516a6d33853b92069 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 18 Aug 2021 09:08:53 -0500 Subject: [PATCH 4/6] Disallowed unexpected symlinks/files/directories --- cloudinit/ssh_util.py | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 4e216762d66..a0abd36360c 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -321,6 +321,21 @@ def check_create_path(username, filename, strictmodes): home_folder = os.path.dirname(user_pwent.pw_dir) for directory in directories: parent_folder += "/" + directory + + # security check, disallow symlinks in the AuthorizedKeysFile path. + if os.path.islink(parent_folder): + LOG.debug( + "Invalid directory. Symlink exists in path: %s", + parent_folder) + return False + + # check no file exists + if os.path.isfile(parent_folder): + LOG.debug( + "Invalid directory. File exists in path: %s", + parent_folder) + return False + if (home_folder.startswith(parent_folder) or parent_folder == user_pwent.pw_dir): continue @@ -337,15 +352,7 @@ def check_create_path(username, filename, strictmodes): mode = 0o700 uid = user_pwent.pw_uid gid = user_pwent.pw_gid - try: - os.mkdir(parent_folder, mode=mode) - except NotADirectoryError: - LOG.debug( - 'Cannot create directory. File exists in path: %s', - parent_folder) - return False - except FileExistsError: - continue + os.makedirs(parent_folder, mode=mode, exist_ok=True) util.chownbyid(parent_folder, uid, gid) permissions = check_permissions(username, parent_folder, @@ -353,6 +360,10 @@ def check_create_path(username, filename, strictmodes): if not permissions: return False + if os.path.islink(filename) or os.path.isdir(filename): + LOG.debug("%s is not a file!" % filename) + return False + # check the file if not os.path.exists(filename): # if file does not exist: we need to create it, since the From 98b6b940af8d44cd3ff0700d646238bc73e97fbf Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 18 Aug 2021 09:35:35 -0500 Subject: [PATCH 5/6] pylint --- cloudinit/ssh_util.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index a0abd36360c..9ccadf0945b 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -329,7 +329,6 @@ def check_create_path(username, filename, strictmodes): parent_folder) return False - # check no file exists if os.path.isfile(parent_folder): LOG.debug( "Invalid directory. File exists in path: %s", @@ -361,7 +360,7 @@ def check_create_path(username, filename, strictmodes): return False if os.path.islink(filename) or os.path.isdir(filename): - LOG.debug("%s is not a file!" % filename) + LOG.debug("%s is not a file!", filename) return False # check the file From 3245b4c01d39d328abe02ff0d181772a9e075ea5 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 20 Aug 2021 16:28:19 -0500 Subject: [PATCH 6/6] review comments --- .../integration_tests/modules/test_ssh_keysfile.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/modules/test_ssh_keysfile.py b/tests/integration_tests/modules/test_ssh_keysfile.py index 0c27fc7bf78..3159feb924d 100644 --- a/tests/integration_tests/modules/test_ssh_keysfile.py +++ b/tests/integration_tests/modules/test_ssh_keysfile.py @@ -74,7 +74,7 @@ def common_verify(client, expected_keys): allow_agent=False, ) - # Also ensure we haven't messed with any /home permissions + # Ensure we haven't messed with any /home permissions # See LP: #1940233 home_dir = '/home/{}'.format(user) home_perms = '755' @@ -84,13 +84,23 @@ def common_verify(client, expected_keys): assert '{} {}'.format(user, home_perms) == client.execute( 'stat -c "%U %a" {}'.format(home_dir) ) - if client.execute("test -f {}/.ssh".format(home_dir)).ok: + if client.execute("test -d {}/.ssh".format(home_dir)).ok: assert '{} 700'.format(user) == client.execute( 'stat -c "%U %a" {}/.ssh'.format(home_dir) ) assert '{} 600'.format(user) == client.execute( 'stat -c "%U %a" {}'.format(filename) ) + + # Also ensure ssh-keygen works as expected + client.execute('mkdir {}/.ssh'.format(home_dir)) + assert client.execute( + "ssh-keygen -b 2048 -t rsa -f {}/.ssh/id_rsa -q -N ''".format( + home_dir) + ).ok + assert client.execute('test -f {}/.ssh/id_rsa'.format(home_dir)) + assert client.execute('test -f {}/.ssh/id_rsa.pub'.format(home_dir)) + assert 'root 755' == client.execute('stat -c "%U %a" /home')