Multiple file fix for AuthorizedKeysFile config by otubo · Pull Request #60 · canonical/cloud-init

otubo · 2019-11-22T14:01:40Z

Currently cloud-init does not know how to handle multiple file
configuration on section AuthorizedKeysFile of ssh configuration.
cloud-init will mess up the home user directory by creating bogus
folders inside it.

This patch provides a fix for this erroneous behavior. It gathers all
keys from all the files listed on the section AuthorizedKeysFile of ssh
configuration and merge all of them inside home user
~/.ssh/authorized_keys of the vm deployed.

Resolves: rhbz#1642008

Signed-off-by: Eduardo Otubo otubo@redhat.com

Currently cloud-init does not know how to handle multiple file configuration on section AuthorizedKeysFile of ssh configuration. cloud-init will mess up the home user directory by creating bogus folders inside it. This patch provides a fix for this erroneous behavior. It gathers all keys from all the files listed on the section AuthorizedKeysFile of ssh configuration and merge all of them inside home user ~/.ssh/authorized_keys of the vm deployed. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

smoser · 2019-11-22T19:13:28Z

@otubo see what you think of this. It makes testing much easier.

0001-Simplify-testing.txt

smoser · 2019-11-26T15:41:38Z

@otubo
I put up some further changes in a branch at
https://github.com/smoser/cloud-init/tree/cleanup/multiple-ssh-keys

The big change is using mock to patch pwent with a namedtuple, which is just less code than the whole fake distro.

I don't want you to feel like I'm taking this over or telling you you have to do it like so... I'm just trying to help, because I appreciate your work and know how frustrating it can be when someone asks you to add tests or refactor code.

otubo · 2019-11-28T07:37:53Z

@otubo
I put up some further changes in a branch at
https://github.com/smoser/cloud-init/tree/cleanup/multiple-ssh-keys

The big change is using mock to patch pwent with a namedtuple, which is just less code than the whole fake distro.

I don't want you to feel like I'm taking this over or telling you you have to do it like so... I'm just trying to help, because I appreciate your work and know how frustrating it can be when someone asks you to add tests or refactor code.

No, please, I really do appreciate all the help. I applied your patche on top of mine and it looks a lot better. Thanks.

This adds render_authorizedkeysfile_paths which does not require any interaction with OS. It is then much easier to test it. The 4 added tests give pretty good coverage of the new code. Note, I had to comment out the 'dist.create_user' call as it was actually trying to create a user.

smoser

I'm ok with this at this point. but I would like review from someone else also.

I think TestMultipleSshAuthorizedKeysFile could use some more tests... the coverage report will probably tell us where we're not testing. I don't think we probaly end up testing the 'default_authorized_file' path.

If no one else is able to take a look, I'd be fine to mark approve.

raharper

Thanks for updating. Is there a downstream or upstream cloud-init bug that tracked this issue?

raharper · 2019-12-02T20:24:36Z

-        if not value.startswith("/"):
-            value = os.path.join(homedir, value)
-    return value
+    paths = re.split(r'(?<!\\) ', value)


Could we get a comment here to explain what the regex is achieving?

@otubo can you add the above and then we can land this?

I think we may be able to just use split() now. @otubo was trying to account for escaping spaces in sshd_config to account for spaces in paths, but I think reality is they are just not supported.

So, the right solution might just be to use split().

That's exactly what I was aiming for, as @smoser mentioned.
I'm gonna replace it and push again.

Also, this is the bugzilla for that: https://bugzilla.redhat.com/show_bug.cgi?id=1642008

smoser

I added 'Resolves: rhbz#1642008' to the description.
Not sure if that is how we are doing things on github or not.

I'm fine happy with this PR at this point.

Thank you, Eduardo.

raharper

Thanks!

Currently cloud-init does not know how to handle multiple file configuration on section AuthorizedKeysFile of ssh configuration. cloud-init will mess up the home user directory by creating bogus folders inside it. This patch provides a fix for this erroneous behavior. It gathers all keys from all the files listed on the section AuthorizedKeysFile of ssh configuration and merge all of them inside home user ~/.ssh/authorized_keys of the vm deployed. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

This reverts commit f1094b1a539044c0193165a41501480de0f8df14. See canonical/cloud-init#60.

pzakha · 2020-02-11T16:33:53Z

Sorry I'm a bit late to the party here, I've just seen this change as it has made it to the Ubuntu package. I'm planning to open a PR if needed, but I have a few questions first.

Regarding the idea of copying the keys from all the files in AuthorizedKeysFile into the "default_authorizedkeys_file", why are we doing this? From what I understand the main reason why we are copying things around in the first place is because we want to write new keys into a file that is processed by sshd. As such, wouldn't it make more sense to only take the keys from the first file in AuthorizedKeysFile, then add the new keys to those and write them back into the same file. I think copying keys from all the files into a single file would not be a behavior that people would expect.

Regarding the new tests, was it intended that the only difference between test_multiple_authorizedkeys_file_order1 and test_multiple_authorizedkeys_file_order2 is the non-standard "home2" home directory in the first one? I also noticed that the exception handling logic in extract_authorized_keys is now broken, i.e. auth_key_fns[0] = default_authorizedkeys_file fails with IndexError: list assignment index out of range, so we may want to add another test to handle that exception logic.

OddBloke · 2020-02-11T17:45:31Z

Regarding the idea of copying the keys from all the files in AuthorizedKeysFile into the "default_authorizedkeys_file", why are we doing this?

@pzakha I agree that this doesn't sound like the ideal behaviour; could you file a bug with this (at https://bugs.launchpad.net/cloud-init/+filebug), so we can discuss (and address) it?

ask0n · 2020-02-14T15:02:59Z

Looks like configurations similar to #149:

AuthorizedKeysFile /etc/ssh/authorized_keys/%u

now are completely broken....

The following commit merged all ssh keys into a default user file "~/.ssh/authorized_keys" in sshd_config had multiple files configured for AuthorizedKeysFile: commit f1094b1 Author: Eduardo Otubo <otubo@redhat.com> Date: Thu Dec 5 17:37:35 2019 +0100 Multiple file fix for AuthorizedKeysFile config (canonical#60) This commit ignored the case when sshd_config would have a single file for AuthorizedKeysFile, but a non default configuration, for example "~/.ssh/authorized_keys_foobar". In this case cloud-init would grab all keys from this file and write a new one, the default "~/.ssh/authorized_keys" causing the bug. rhbz: #1862967 Signed-off-by: Eduardo Otubo <otubo@redhat.com>

The following commit merged all ssh keys into a default user file `~/.ssh/authorized_keys` in sshd_config had multiple files configured for AuthorizedKeysFile: commit f1094b1 Author: Eduardo Otubo <otubo@redhat.com> Date: Thu Dec 5 17:37:35 2019 +0100 Multiple file fix for AuthorizedKeysFile config (#60) This commit ignored the case when sshd_config would have a single file for AuthorizedKeysFile, but a non default configuration, for example `~/.ssh/authorized_keys_foobar`. In this case cloud-init would grab all keys from this file and write a new one, the default `~/.ssh/authorized_keys` causing the bug. rhbz: #1862967 Signed-off-by: Eduardo Otubo <otubo@redhat.com>

raharper suggested changes Nov 22, 2019

View reviewed changes

Comment thread tests/unittests/test_sshutil.py Outdated

Comment thread cloudinit/ssh_util.py Outdated

Comment thread cloudinit/ssh_util.py Outdated

smoser reviewed Nov 22, 2019

View reviewed changes

Comment thread cloudinit/ssh_util.py

otubo requested a review from raharper November 28, 2019 09:18

smoser reviewed Dec 2, 2019

View reviewed changes

raharper reviewed Dec 2, 2019

View reviewed changes

raharper approved these changes Dec 2, 2019

View reviewed changes

otubo added 2 commits December 5, 2019 10:23

Replacing re.split() regex by simple split()

523617a

removing import re, since not used

e64f99d

smoser approved these changes Dec 5, 2019

View reviewed changes

raharper suggested changes Dec 5, 2019

View reviewed changes

Comment thread cloudinit/ssh_util.py Outdated

Removing strip()

5819a29

raharper approved these changes Dec 5, 2019

View reviewed changes

raharper merged commit f1094b1 into canonical:master Dec 5, 2019

pzakha added a commit to pzakha/cloud-init that referenced this pull request Feb 11, 2020

Revert "Multiple file fix for AuthorizedKeysFile config (delphix#60)"

b2ff0b0

This reverts commit f1094b1a539044c0193165a41501480de0f8df14. See canonical/cloud-init#60.

pzakha mentioned this pull request Feb 11, 2020

Merge conflict dealing with multiple entries for AuthorizedKeysFile delphix/cloud-init#14

Merged

11 tasks

raharper mentioned this pull request Mar 13, 2020

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

Closed

TheRealFalcon mentioned this pull request Jul 19, 2021

Stop storing system keys in user's authorized keys file #948

Closed

This was referenced May 12, 2023

Release 19.4 #3522

Closed

cloud-init regenerating ssh-keys #3753

Closed

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 20.4 #3811

Closed

Conversation

otubo commented Nov 22, 2019 • edited by smoser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smoser commented Nov 22, 2019

Uh oh!

smoser commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

otubo commented Nov 28, 2019

Uh oh!

smoser left a comment

Choose a reason for hiding this comment

Uh oh!

raharper left a comment

Choose a reason for hiding this comment

Uh oh!

raharper Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

powersj Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

smoser Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

otubo Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

otubo Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

smoser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raharper left a comment

Choose a reason for hiding this comment

Uh oh!

pzakha commented Feb 11, 2020

Uh oh!

OddBloke commented Feb 11, 2020

Uh oh!

ask0n commented Feb 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

otubo commented Nov 22, 2019 •

edited by smoser

Loading

smoser commented Nov 26, 2019 •

edited

Loading