From 5bfe2ee2b063d87e6fd255d6c5e63123aa3f6de0 Mon Sep 17 00:00:00 2001

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Date: Sat, 21 Aug 2021 13:55:53 +0200

Subject: [PATCH] Fix home permissions modified by ssh module (SC-338) (#984)

RH-Author: Emanuele Giuseppe Esposito <eesposit@redhat.com>

RH-MergeRequest: 9: Fix home permissions modified by ssh module

RH-Commit: [1/1] ab55db88aa1bf2f77acaca5e76ffabbab72b1fb2 (eesposit/cloud-init-centos-)

RH-Bugzilla: 1995843

RH-Acked-by: Mohamed Gamal Morsy <mmorsy@redhat.com>

RH-Acked-by: Eduardo Otubo <otubo@redhat.com>

TESTED: By me and QA

BREW: 39178085

Fix home permissions modified by ssh module (SC-338) (#984)

commit 7d3f5d750f6111c2716143364ea33486df67c927

Author: James Falcon <therealfalcon@gmail.com>

Date:   Fri Aug 20 17:09:49 2021 -0500

    Fix home permissions modified by ssh module (SC-338) (#984)

    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

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

---

 cloudinit/ssh_util.py                         |  35 ++++-

 .../modules/test_ssh_keysfile.py              | 132 +++++++++++++++---

 2 files changed, 146 insertions(+), 21 deletions(-)

diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py

index b8a3c8f7..9ccadf09 100644

--- a/cloudinit/ssh_util.py

+++ b/cloudinit/ssh_util.py

@@ -321,23 +321,48 @@ 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):

+

+            # 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

+

+            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

-            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.

                 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

+                    os.makedirs(parent_folder, mode=mode, exist_ok=True)

+                    util.chownbyid(parent_folder, uid, gid)

             permissions = check_permissions(username, parent_folder,

                                             filename, False, 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

diff --git a/tests/integration_tests/modules/test_ssh_keysfile.py b/tests/integration_tests/modules/test_ssh_keysfile.py

index f82d7649..3159feb9 100644

--- a/tests/integration_tests/modules/test_ssh_keysfile.py

+++ b/tests/integration_tests/modules/test_ssh_keysfile.py

@@ -10,10 +10,10 @@ TEST_USER1_KEYS = get_test_rsa_keypair('test1')

 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,27 +24,17 @@ users:

 - 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,

 )

-@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:

+        # 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

@@ -83,3 +73,113 @@ def test_authorized_keys(client: IntegrationInstance):

                     look_for_keys=False,

                     allow_agent=False,

                 )

+

+        # 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)

+        )

+        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')

+

+

+DEFAULT_KEYS_USERDATA = _USERDATA.format(bootcmd='""')

+

+

+@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)

+

+

+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(AUTHORIZED_KEYS2_USERDATA)

+def test_authorized_keys2(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)

+

+

+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)

-- 

2.27.0