Do not change permissions for system-wide folders with ssh authorized_keys

[ask0n]

cloud-init’s code by default makes a wrong assumption that all users using private .ssh folder for authorized_keys.
But AuthorizedKeysFile directive in sshd_config could be used to have a system-wide folder with user keys, which are managed by the configuration management systems, for example puppet.
The idea is to have a configuration like AuthorizedKeysFile /etc/ssh/authorized_keys/%u to prevent users manage their authorized_keys or to rootkit other users from user who had a sudo permissions on host.
Folder /etc/ssh/authorized_keys/ and all files inside are owned by root because user shouldn’t be able to modify this files. When cloud-init change mode to 700 for such folder it breaks whole concept.
For private .ssh folder cloud-init sets permission on line https://github.com/canonical/cloud-init/blob/master/cloudinit/ssh_util.py#L260 there is no need to set this permission again for system-wide folder.

Steps to reproduce:

1. Launch new EC2 instance
2. Create a new system user, for example test-user
3. mkdir /etc/ssh/authorized_keys/
4. chmod 755 /etc/ssh/authorized_keys/
5. chown root /etc/ssh/authorized_keys/
6. Modify /etc/ssh/sshd_config file:

AuthorizedKeysFile /etc/ssh/authorized_keys/%u

7. Put ssh public key for test-user into /etc/ssh/authorized_keys/test-user
8. Restart sshd
9. Check that you can login as test-user via ssh
10. Create AMI from running instance
11. Launch new EC2 from created AMI.
12. Cloud-init will be smart enough to put authorized_keys file into preconfigured folder /etc/ssh/authorized_keys/ but after chmod 700 user test-user will not be able to login into newly launched instance.

[OddBloke]

Hi Anton, thanks for submitting this pull request! To accept it, we will need you to sign the cloud-init CLA. You can find instructions on how to do that in our HACKING doc.

This looks like it's fixing https://bugs.launchpad.net/cloud-init/+bug/1839061, but I think the fix here is too simplistic. We do need to create $USER/.ssh when per-user SSH keys are being used, and we should set the permissions on it correctly. This change will fix your specific case, but changes behaviour for per-user keys. Does that sound correct, or have I missed something?

[ask0n]

Hi @OddBloke

We do need to create $USER/.ssh when per-user SSH keys are being used, and we should set the permissions on it correctly.

Correct, this happens exactly on line https://github.com/canonical/cloud-init/blob/master/cloudinit/ssh_util.py#L260. Which totally makes sense. On that line cloud-init changes something which was created by cloud-init.
My fix looks "simplistic" but it is how cloud-init's code is for that annoying bug. I checked that this code was just copied from version to version since 2007 without any modification. It doesn't changes behavior for per-user keys.
Right now we also fixing permissions with:

runcmd:
- "awk  '/^AuthorizedKeysFile/ {print $2}' /etc/ssh/sshd_config | xargs dirname | xargs chmod 755"

but this will not help if you want to use your AMI within different accounts of configuration that doesn't have that "fix" in runcmd.

[OddBloke]

Aha, thanks for the explanation! I'll want to double-check with people who have touched this code before (because there may be some deeper reason for doing this twice that neither of us know of), but I think it's worth you signing the CLA so that we can land this if they're happy. :-)

[ask0n]

Sure, CLA is signed, just got confirmation form Sally.

[OddBloke]

Great, thanks! We unfortunately need you to do a manual step to prove that your GitHub and Launchpad accounts are owned by the same person. Look for "For first-time signers, or for existing contributors who have already signed the agreement in Launchpad" in https://cloudinit.readthedocs.io/en/latest/topics/hacking.html and follow the steps there.

[raharper]

I've a suggested change.

[raharper]

I spent some time putting together a unittest for this:

class TestSetupUserKeys(test_helpers.FilesystemMockingTestCase):               
                                                                               
    @patch("cloudinit.ssh_util.parse_ssh_config_map")                          
    @patch("cloudinit.ssh_util.pwd.getpwnam")                                  
    def test_setup_userkeys_create_dir(self, m_getpwnam, m_parse_ssh):         
        fpw = FakePwEnt(pw_name='test', pw_dir='/home2/test',                  
                        pw_uid=9999, pw_gid=9999)                              
        m_getpwnam.return_value = fpw                                          
        m_parse_ssh.return_value = {                                           
            'authorizedkeysfile': '/etc/ssh/authorized_keys/%u'                
        }                                                                      
        key_entries = [                                                        
            ' '.join(('rsa', VALID_CONTENT['rsa'], 'new_comment1')), ]         
                                                                               
        root = self.reRoot()                                                   
        expected_paths = [                                                     
            os.path.join(root, 'etc/ssh/authorized_keys/%s' % fpw.pw_name),    
            os.path.join(root, 'home2/test/.ssh/authorized_keys'),             
        ]                                                                      
        target_dir = os.path.dirname(expected_paths[0])                        
        self.assertFalse(all(map(os.path.exists, expected_paths)))             
        ssh_util.setup_user_keys(key_entries, 'test', None)                    
        self.assertTrue(all(map(os.path.exists, expected_paths)))              
        file_stat = os.stat(target_dir)                                        
        mode = stat.S_IMODE(file_stat.st_mode)                                 
        self.assertEqual(0o700, mode)                                          
                                                                               
    @patch("cloudinit.ssh_util.parse_ssh_config_map")                          
    @patch("cloudinit.ssh_util.pwd.getpwnam")                                  
    def test_setup_userkeys_mkdir_if_needed(self, m_getpwnam, m_parse_ssh):    
        fpw = FakePwEnt(pw_name='test', pw_dir='/home2/test',                  
                        pw_uid=9999, pw_gid=9999)                              
        m_getpwnam.return_value = fpw                                          
        m_parse_ssh.return_value = {                                           
            'authorizedkeysfile': '/etc/ssh/authorized_keys/%u'                
        }                                                                      
        key_entries = [                                                        
            ' '.join(('rsa', VALID_CONTENT['rsa'], 'new_comment1')), ]         
                                                                               
        root = self.reRoot()                                                   
        expected_paths = [                                                     
            os.path.join(root, 'etc/ssh/authorized_keys/%s' % fpw.pw_name),    
            os.path.join(root, 'home2/test/.ssh/authorized_keys'),             
        ]                                                                      
        target_dir = os.path.dirname(expected_paths[0])                        
        self.assertFalse(all(map(os.path.exists, expected_paths)))             
        # precreate dir with 0755 mode                                         
        util.ensure_dir(target_dir, mode=0o755)                                
        ssh_util.setup_user_keys(key_entries, 'test', None)                    
        self.assertTrue(all(map(os.path.exists, expected_paths)))              
                                                                               
        file_stat = os.stat(target_dir)                                        
        mode = stat.S_IMODE(file_stat.st_mode)                                 
        self.assertEqual(0o755, mode)                   

However, it seems since this PR was started, the multiple AuthorizationKeyFile PR #60
landed, where cloud-init writes authorized_keys file has changed. In particular, the location that cloud-init updates now is always the default user .ssh/authorized keys file.

This means that cloud-init no longer runs ensure_dir on the AuthorizedKeysFile paths, only on default user's ~/.ssh;

I've tested your scenario where we have a system-level test-user with a key in /etc/ssh/authorized_keys/ dir with mode 0755 and a test-user owned key file. AuthorizedKeyFiles
is /etc/ssh/authorized_keys/%u, Restart sshd, and verify test-user can ssh into the instance.

On new instance, the Ubuntu user is created, the provided ssh_authorized_keys value is added to /home/ubuntu/.ssh/.authorized key file. No change is made to /etc/ssh/authorized_keys/ dir. test-user can ssh in. The default user cannot as sshd is now looking only in /etc/ssh/authorized_keys/%u in which there is no 'ubuntu' file with keys. The default user's ~/.ssh/authorized_keys file only has the public key from user-data. Test-user does not have a ~/.ssh dir at all.

Would you be able to test your scenario out on an Ubuntu Focal image and confirm that you're desired use-case is function? If it is, then I think we can close this PR.

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging powersj, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

[igalic]

i wonder if @github-actions bot should directly @ the person who opened the issue / pr in this ⬆️ message (in this case @ask0n )