ssh-util: allow cloudinit to merge all ssh keys into a custom user file, defined in AuthorizedKeysFile

[esposem]

Proposed Commit Message

This patch aims to fix LP1911680, by analyzing the files provided
in sshd_config and merge all keys into an user-specific file. Also
introduces additional tests to cover this specific case.

The file is picked by analizing the path given in AuthorizedKeysFile:

• if it points inside the current user folder (path is /home/user/...), it
  means it is an user-specific file, so we can copy all user-keys there.
• if it contains a %u or %h, it means that there will be a specific
  authorized_keys file for each user, so we can copy all user-keys there.
• if no path points to an user-specific file, for example when only
  /etc/ssh/authorized_keys is given, default to ~/.ssh/authorized_keys.

Note that if there are more than a single user-specific file, the last
one will be picked.

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

LP: #1911680
RHBZ:1862967

Additional Context

For example, given
AuthorizedKeysFile /etc/ssh/authorized_keys /home/%u/.ssh/authorized_keys2
and 2 users, cloud-user and test-user

where

cat /etc/ssh/authorized_keys
<SHARED_KEY>

cat /home/cloud-user/.ssh/authorized_keys2
<CLOUD_USER_KEY>

cat /home/test-user/.ssh/authorized_keys2
<TEST_USER_KEY>

the intended behavior is to have

• /home/cloud-user/.ssh/authorized_keys2 contains SHARED_KEY and CLOUD_USER_KEY
• /home/test-user/.ssh/authorized_keys2 contains SHARED_KEY and TEST_USER_KEY

Instead, if we apply commit b0e7381 (currently
reverted) we would get this result:

• /etc/ssh/authorized_keys contains SHARED_KEY, CLOUD_USER_KEY, TEST_USER_KEY
  (security issue)

Without this patch, we would instead get this result:

• /home/cloud-user/.ssh/authorized_keys contains SHARED_KEY and CLOUD_USER_KEY
• /home/test-user/.ssh/authorized_keys contains SHARED_KEY and TEST_USER_KEY
  (Note that it writes to .ssh/authorized_keys, and not to
  .ssh/authorized_keys2, locking out both users from ssh public-key access).

Test Steps

tox tests/unittests/test_sshutil.py

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

[otubo]

Hi Emanuele! Thanks a lot for working on this feature! I think overall it's a good start, but I have some comments. Since there's no reviewers assigned, we probably need to ping the maintainers on the IRC. I'm not sure you can assign one after you create a PR. Thanks for this patch!

[esposem]

@TheRealFalcon @OddBloke Hi, I forgot to assign any of you for this PR, and it seems I can't do it anymore now. Can you please take a look at it? Thank you in advance

[TheRealFalcon]

Thanks for this PR! I agree that this is a good start, and the new tests are very helpful.

I'm slightly -1 on changing render_authorizedkeysfile_paths in this way. It takes it from rendering the paths to rendering and also determining the default auth, which makes it harder to understand.

Could we instead determine which path to use in extract_authorized_keys? Something like this in the try:

key_paths = ssh_cfg.get("authorizedkeysfile", "%h/.ssh/authorized_keys")
auth_key_fns = render_authorizedkeysfile_paths(
    key_paths, pw_ent.pw_dir, username)

and then later:

for key_path, auth_key_fn in zip(key_paths.split(), auth_key_fns):
    if any([
        '%u' in key_path,
        '%h' in key_path,
        auth_key_fn.startswith(pw_ent.pw_dir)
    ]):
        user_authorizedkeys_file = auth_key_fn

Would something like this work for you?

[esposem]

Thanks for this PR! I agree that this is a good start, and the new tests are very helpful.

I'm slightly -1 on changing render_authorizedkeysfile_paths in this way. It takes it from rendering the paths to rendering and also determining the default auth, which makes it harder to understand.

Could we instead determine which path to use in extract_authorized_keys? Something like this in the try:

key_paths = ssh_cfg.get("authorizedkeysfile", "%h/.ssh/authorized_keys")
auth_key_fns = render_authorizedkeysfile_paths(
    key_paths, pw_ent.pw_dir, username)

and then later:

for key_path, auth_key_fn in zip(key_paths.split(), auth_key_fns):
    if any([
        '%u' in key_path,
        '%h' in key_path,
        auth_key_fn.startswith(pw_ent.pw_dir)
    ]):
        user_authorizedkeys_file = auth_key_fn

Would something like this work for you?

The reason for modifying render_authorizedkeysfile_paths was that I didn't want to add duplicate code. But I understand your solution, is even cleaner than mine, thank you for the suggestion. Will update the code and tests accordingly.

[TheRealFalcon]

Thanks for the updates! I added a few more comments, but nothing major.

[esposem]

@TheRealFalcon sorry again, do you know why the integration test in the build failed? I see

>       assert '# ssh-import-id gh:powersj' in ssh_output

E       AssertionError: assert '# ssh-import-id gh:powersj' in 'ssh-rsa
AAAAB3NzaC1yc2EAAAADAQABAAABAQDSuIqpiZlhWic/Fd1UmodKaskbBTx4h+kj9kbCKE7
mx3w44XesCnPlEJWo9OsgJAhwY3L8tlWrF4q9T...YPADZyybPvhDOj4uyvS6ZNapEpBM7YPz5
PG7akTDLTY9S05xNcRhyYXFOxk//D1 travis@travis-job-c9958220-ef0f-4c70-acbc-e4fbad2eb9c0'

for test_ssh_import_id, that has nothing to do with my fix.

[TheRealFalcon]

@esposem , I'm not entirely sure, but I think it's related to a separate issue I'm digging into. Using the following cloud-config:

#cloud-config
bootcmd:
 - sed -i 's;#AuthorizedKeysFile.*;AuthorizedKeysFile /etc/ssh/authorized_keys /var/tmp/%u/keys /home/test_user/.ssh/authorized_keys2 ;' /etc/ssh/sshd_config
ssh_authorized_keys:
 - ssh-rsa rootkey me@host
users:
- default
- name: test_user
  ssh_authorized_keys:
  - ssh-rsa <some_key>

The key specified for test_user is given to every user, which isn't right. I'm not entirely sure why yet, but I just noticed that extract_authorized_keys is also being called by both cc_ssh and cc_ssh_authkey_fingerprints. I'll let you know when I find out more.

[esposem]

@TheRealFalcon seems that the check has passed now, I guess you fixed something so thank you!
Anything else you want me to do for this PR? I force-pushed all your suggested changes.

[TheRealFalcon]

@esposem , yes, the error you saw was a transient error we haven't seen before. Since I can't reproduce it and it could have just been from a network blip, I'm not going to worry about it in this PR.

Regarding the other issue I mentioned, this is a problem on master as well, and something I think I'll put up a separate PR for.

I took the liberty of pushing (as a separate commit) an integration test along with a super small fix to user checking. It's not meant to be as exhaustive as the unit tests, but provides an end-to-end test of connecting via SSH. If you are ok with it, let me know. At this point, I think this PR is good to merge.

Thanks for contribution!

[esposem]

Sounds good! Thank you for the review and additional integration tests!

[blackboxsw]

+1 for this work @esposem and @TheRealFalcon . I also just filed a separate bug for tracking that cloud-init doesn't handle parsing Match sections properly because it treats the last line AuthorizedKeysFile config in /etc/ssh/sshd_config line as global. cloud-init might eventually want to grow user-specific Match section awareness in the event of custom configurations. LP: #1935857