Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Host users take ownership of existing home directories #47107

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eriktate
Copy link
Contributor

@eriktate eriktate commented Oct 2, 2024

This PR updates CreateHomeDirectory such that it takes ownership of existing home directories and their contents. I originally thought we were already chowning the directory contents and we were just omitting the directory itself. However, I realized while implementing that this only happened for the files copied from skel so we'll need to figure out if this is something we want to move forward with or not

changelog: Allows host user creation to take ownership of existing home directories on behalf of the newly created user.

Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to recursively chown the whole home directory tree rather than make just the directory owned, readable(?) and accessible by the user? What if there's something in there that's owned by root and is not supposed to be read by the user?

If we do this we should definitely at least check and see if the uid that owns the directory isn't a user that already exists in the system.

Comment on lines 256 to 257
// filepath.WalkDir is documented to walk the paths in lexical order, iterating
// in the reverse order ensures that files are always Lchowned before their parent directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? Collecting a slice of unbounded size of paths doesn't feel great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably isn't 🤔 This was copied from the existing function that used to be in utils, but I don't see any reason to keep this behavior

@gravitational gravitational deleted a comment from github-actions bot Oct 2, 2024
@gravitational gravitational deleted a comment from github-actions bot Oct 2, 2024
@eriktate
Copy link
Contributor Author

eriktate commented Oct 3, 2024

Do we have to recursively chown the whole home directory tree rather than make just the directory owned, readable(?) and accessible by the user? What if there's something in there that's owned by root and is not supposed to be read by the user?

If we do this we should definitely at least check and see if the uid that owns the directory isn't a user that already exists in the system.

I think it depends on the expected behavior here. As far as I understand, the primary usage would be in moving users over to teleport that were previously managed some other way. Which would benefit from taking ownership of everything since it's meant to be effectively the same user. @rosstimothy do you think it would be acceptable to only take ownership of the directory and not the contents?

Validating the UIDs are "safe" to overwrite is a great idea 👍

Copy link

github-actions bot commented Oct 4, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@eriktate eriktate force-pushed the eriktate/host-user-existing-home-ownership branch from 179162b to de9c464 Compare October 4, 2024 17:15
Copy link

github-actions bot commented Oct 4, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@eriktate eriktate force-pushed the eriktate/host-user-existing-home-ownership branch 2 times, most recently from e0a1f57 to 884db9a Compare October 4, 2024 17:27
@eriktate eriktate force-pushed the eriktate/host-user-existing-home-ownership branch 3 times, most recently from 8e30806 to b214b0c Compare October 4, 2024 19:32
@eriktate eriktate force-pushed the eriktate/host-user-existing-home-ownership branch from b214b0c to 53c3e8e Compare October 4, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants