Fix home permissions modified by ssh module (SC-338) by TheRealFalcon · Pull Request #984 · canonical/cloud-init

TheRealFalcon · 2021-08-17T15:04:40Z

Proposed Commit Message

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

Additional Context

See #956 and https://bugs.launchpad.net/cloud-init/+bug/1940233

Test Steps

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

TheRealFalcon · 2021-08-17T15:05:21Z

FYI @esposem

In canonical#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

esposem · 2021-08-17T15:49:12Z

+                        gid = user_pwent.pw_gid
+                    try:
+                        os.mkdir(parent_folder, mode=mode)
+                    except FileExistsError:


So with this change

/, /home/ and /home/<user> are skipped and not created in any case.

then, we can have either a path that is not in the user home folder, and we chmod it to root ownership, or inside the user folder, and we chmod it as user-owned.

Only one question: what if /home/ or /home/<user> do not exist? os.mkdir should launch a FileNotFoundError I think, we should handle also this case right?

I think I agree with @paride in https://bugs.launchpad.net/cloud-init/+bug/1940233/comments/4 when he says it's not the responsibility of this module to create a user's home directory.

A couple of thoughts about this:

The authorized_keys file is always created user-owned (chmod 600), even if its path is outside the user's $HOME directory. I think this is good, so users are still be able to manage their own authorized_keys.

Given that we've got bitten by this, I think it we should test two more scenarios:

AuthorizedKeysFile outside of $HOME, e.g. in /etc/ssh/authorized_keys/%u/

AuthorizedKeysFile deeper in $HOME, e.g. %h/foo/bar/ssh/

Wydt?

@TheRealFalcon ok, seems reasonable
@paride regarding the right permission w.r.t. authorized_keys files, look here. The mistake in this code is that I did not think about the permissions that other ssh tools need, in this case ssh-keygen. Because if .ssh has permission 755 and it's owned by root, it should work w.r.t. AuthorizedKeysFile.

After a quick test I see that the mode doesn't matter, the ownership is the problem (which makes sense because with root owner and 755 ssh-keygen on behalf of the user cannot write in the folder).

Regarding the first scenario, it is covered in this and this test.

Added the requested integration tests. There's some overlap with the unit tests (and the unit tests are much more comprehensive), but better to have a basic integration check of the other use cases too.

Looks good to me, thank you!

LGTM, thanks!

paride · 2021-08-17T17:12:16Z

+                    parent_folder == user_pwent.pw_dir):
                continue

            if not os.path.isdir(parent_folder):


There is one weird case that is not covered by this if: parent_folder existing but not-a-directory (file, symlink). In this case cloud-init is going to fail creating deeper subdirectories or the authorized_keys file, possibly doing unexpected things in the case of a symlink. Do you think we should cover this case, returning early when not os.path.isdir(parent_folder) but os.path.exists(parent_folder)?

More specifically I'm worried by a user doing e.g. ln -s .ssh /root/.ssh, and of what would cloud-init do.

Pushed 240a079 , which I think addresses this.

TheRealFalcon · 2021-08-18T13:28:54Z

@esposem , Yep, my bad. I misread Paride's comment and went a direction based off a wrong assumption. What you say makes sense.

esposem · 2021-08-18T19:58:14Z

Everything now looks good to me, thank you for working on this!

blackboxsw · 2021-08-20T20:07:44Z

+        assert '{} {}'.format(user, home_perms) == client.execute(
+            'stat -c "%U %a" {}'.format(home_dir)
+        )
+        if client.execute("test -f {}/.ssh".format(home_dir)).ok:


Does this actually work? I get exit 1 from this call meaning we don't drop in for the perms check.

Also can we attempt to run something like the example ssh-keygen call in the bug report to confirm that all the plumbing works as expected without error?

blackboxsw

LGTM! Needs rebase and CI pass but otherwise +1

TheRealFalcon requested a review from blackboxsw August 17, 2021 15:04

TheRealFalcon changed the title ~~Fix home permissions modified by ssh module~~ Fix home permissions modified by ssh module (SC-338) Aug 17, 2021

TheRealFalcon force-pushed the ssh-home-perms branch from 428f36d to d5cc8b6 Compare August 17, 2021 15:28

esposem suggested changes Aug 17, 2021

View reviewed changes

paride reviewed Aug 17, 2021

View reviewed changes

TheRealFalcon added 2 commits August 17, 2021 13:14

handle symlinks

240a079

Update integration test

1a358bc

Disallowed unexpected symlinks/files/directories

bb786eb

TheRealFalcon force-pushed the ssh-home-perms branch from ac8a184 to bb786eb Compare August 18, 2021 14:15

pylint

98b6b94

blackboxsw reviewed Aug 20, 2021

View reviewed changes

review comments

3245b4c

blackboxsw approved these changes Aug 20, 2021

View reviewed changes

Merge branch 'main' into ssh-home-perms

f02bb61

TheRealFalcon merged commit 7d3f5d7 into canonical:main Aug 20, 2021

TheRealFalcon deleted the ssh-home-perms branch August 20, 2021 22:09

This was referenced May 12, 2023

cloud-init in impish makes /home/ubuntu/.ssh root.root #3894

Closed

Release 21.3 #3898

Closed

Conversation

TheRealFalcon commented Aug 17, 2021

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

TheRealFalcon commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paride Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esposem Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paride Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon commented Aug 18, 2021

Uh oh!

esposem commented Aug 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paride Aug 17, 2021 •

edited

Loading

esposem Aug 17, 2021 •

edited

Loading

paride Aug 17, 2021 •

edited

Loading