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

lib/commit: Don't chown objects to repo target owner #1754

Closed

Conversation

dbnicholson
Copy link
Member

The idea is that if the process is running as root, it can change
ownership of newly written files to match the owner of the repo.
Unfortunately, it currently applies in the other direction, too - a
non-root user writing to a root owned repository. If the repo is
writable by the user but owned by root, it can still create files and
directories there, but it can't change ownership of them.

This feature comes from
https://bugzilla.gnome.org/show_bug.cgi?id=738954. As it turns out, this
feature was never completed. It only works on content objects and not
metadata objects, refs, deltas, summaries, etc. Rather than try to fix
all of those, remove the feature until someone has interest in
completing it.

The idea is that if the process is running as root, it can change
ownership of newly written files to match the owner of the repo.
Unfortunately, it currently applies in the other direction, too - a
non-root user writing to a root owned repository. If the repo is
writable by the user but owned by root, it can still create files and
directories there, but it can't change ownership of them.

This feature comes from
https://bugzilla.gnome.org/show_bug.cgi?id=738954. As it turns out, this
feature was never completed. It only works on content objects and not
metadata objects, refs, deltas, summaries, etc. Rather than try to fix
all of those, remove the feature until someone has interest in
completing it.
@cgwalters
Copy link
Member

Yeah...valid. Ironically I was just thinking about this again today in relation to coreos/coreos-assembler#165 but...you're right it's incomplete and lacks tests and obviously has bugs.

@rh-atomic-bot r+ 7f6a100

@rh-atomic-bot
Copy link

⌛ Testing commit 7f6a100 with merge 43d9cac...

@dbnicholson
Copy link
Member Author

Yeah...valid. Ironically I was just thinking about this again today in relation to coreos/coreos-assembler#165 but...you're right it's incomplete and lacks tests and obviously has bugs.

It's definitely a worthy idea in certain scenarios. The alternative is just to add a getuid() == 0 to the conditional, but I figured since no one had worked on it in 4 years it probably wasn't going to be finished.

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 43d9cac to master...

@dbnicholson dbnicholson deleted the no-chown-repo-owner branch October 12, 2018 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants