Stop storing system keys in user's authorized keys file and check for folder permissions

[esposem]

Proposed Commit Message

This is the continuation of PR #948. In addition to the fix proposed by @TheRealFalcon, it contains the permission fix discussed in the PR, with additional test cases and the fix for the old ones

In /etc/ssh/sshd_config, it is possible to define a custom authorized_keys file
that will contain the keys allowed to access the machine via
the AuthorizedKeysFile option.
Cloudinit is able to add user-specific keys to the existing ones, but we need
to be careful on which of the authorized_keys files listed to pick.
Chosing for example a file that is shared by all user will cause security
issues, because the owner of that key can then access also other users.
    
We therefore pick an authorized_keys file only if it satisfies the following
conditions:
    1. it is not a "global" file, ie it must be defined in AuthorizedKeysFile
        with %u, %h or be in  /home/<user>. This avoids security issues.
    2. it must comply with ssh permission requirements, otherwise the ssh agent
        won't use that file.
    
We also need to consider the case when the chosen authorized_keys file
does not exist. In this case, the existing behavior of cloud-init is
to create the new file. We therefore need to be sure that the file complies
with ssh permissions too, by setting:
    - the actual file to permission 600, and owned by the user
    - the directories in the path that do not exist must be root owned and with
      permission 755.

Test Steps

See PR #948

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[esposem]

@TheRealFalcon I updated the PR with what I believe are the final changes for this fix.
The things that I added and are not mentioned in PR #948 are:

• support to create the chosen user-specific authorized_keys file if it doesn't exist, with the right permissions.
• we stop at the first user-specific authorized_keys file that we can use. If we don't do that, we would create folders and empty authorized_keys files for all user-specific options given in AuthorizedKeysFile.

The issue still lays in the unit tests, we have:

1. Right now I noted that FakePwEnt does not support pw_gid, but even adding it creates problems when I try to fetch which group the fake user owns to
2. the permission on the files for the fake users are wrong.
   I am clearly doing something wrong here, so please advise.

[esposem]

@blackboxsw since you approved my original PR #937, I thought about including you too to this PR.

[TheRealFalcon]

@esposem , yes, I see the problem. Since we're dealing with files that get written to the local filesystem, I think we have no choice but to mock the permissions related functions.

It gets a bit more cumbersome, but something like this should work (save and apply with git apply <filename):

diff --git a/tests/unittests/test_sshutil.py b/tests/unittests/test_sshutil.py
index d1b64bd7..9cf971ec 100644
--- a/tests/unittests/test_sshutil.py
+++ b/tests/unittests/test_sshutil.py
@@ -1,6 +1,7 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 from collections import namedtuple
+from functools import partial
 from unittest.mock import patch
 
 from cloudinit import ssh_util
@@ -8,13 +9,40 @@ from cloudinit.tests import helpers as test_helpers
 from cloudinit import util
 
 # https://stackoverflow.com/questions/11351032/
-FakePwEnt = namedtuple(
-    'FakePwEnt',
-    ['pw_dir', 'pw_gecos', 'pw_name', 'pw_passwd', 'pw_shell', 'pwd_uid'])
+FakePwEnt = namedtuple('FakePwEnt', [
+    'pw_name',
+    'pw_passwd',
+    'pw_uid',
+    'pw_gid',
+    'pw_gecos',
+    'pw_dir',
+    'pw_shell',
+])
 FakePwEnt.__new__.__defaults__ = tuple(
     "UNSET_%s" % n for n in FakePwEnt._fields)
 
 
+def mock_get_owner(updated_permissions, value):
+    try:
+        return updated_permissions[value][0]
+    except ValueError:
+        return util.get_owner(value)
+
+
+def mock_get_group(updated_permissions, value):
+    try:
+        return updated_permissions[value][1]
+    except ValueError:
+        return util.get_group(value)
+
+
+def mock_get_permissions(updated_permissions, value):
+    try:
+        return updated_permissions[value][2]
+    except ValueError:
+        return util.get_permissions(value)
+
+
 # Do not use these public keys, most of them are fetched from
 # the testdata for OpenSSH, and their private keys are available
 # https://github.com/openssh/openssh-portable/tree/master/regress/unittests/sshkey/testdata
@@ -658,8 +686,24 @@ class TestMultipleSshAuthorizedKeysFile(test_helpers.CiTestCase):
         self.assertTrue(VALID_CONTENT['dsa'] in content)
 
     @patch("cloudinit.ssh_util.pwd.getpwnam")
-    def test_single_user_local_global_files(self, m_getpwnam):
+    @patch("cloudinit.util.get_permissions")
+    @patch("cloudinit.util.get_owner")
+    @patch("cloudinit.util.get_group")
+    def test_single_user_local_global_files(
+        self, m_get_group, m_get_owner, m_get_permissions, m_getpwnam
+    ):
         fpw = FakePwEnt(pw_name='bobby', pw_dir='/tmp/home/bobby')
+        mock_permissions = {
+            '/tmp': ('bobby', 'bobby', 0o700),
+            '/tmp/home': ('bobby', 'bobby', 0o700),
+            '/tmp/home/bobby': ('bobby', 'bobby', 0o700),
+            '/tmp/home/bobby/.ssh': ('bobby', 'bobby', 0o700),
+            '/tmp/home/bobby/.ssh/user_keys': ('bobby', 'bobby', 0o600),
+        }
+        m_get_permissions.side_effect = partial(
+            mock_get_permissions, mock_permissions)
+        m_get_owner.side_effect = partial(mock_get_owner, mock_permissions)
+        m_get_group.side_effect = partial(mock_get_group, mock_permissions)
         m_getpwnam.return_value = fpw
         user_ssh_folder = "%s/.ssh" % fpw.pw_dir
 

I only applied it to one of the failing tests, but you can see how it works.

Does this help you?

[esposem]

@TheRealFalcon exactly what I needed, thank you! I extended your code a little bit to cover also get_user_groups and getpwnam, but the idea remains the same. I fixed all tests and added 5 new ones.
Let me know if you have any feedback!

[esposem]

Once again same integration test error as in my original PR:
#937 (comment)

[otubo]

@esposem I believe that looks good me! Worth all the discussion and brain puzzle we had :-D

[TheRealFalcon]

@esposem , this is looking really good. Thanks. I've added some inline comments, but all minor things.

Once again same integration test error as in my original PR

Dang, I thought I fixed that. I'm not sure what's causing it as I'm unable to reproduce it locally. I can trigger a re-run if it happens again.

[TheRealFalcon]

@esposem , This looks good. Thanks for driving this!