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

compose: Support "preserve-passwd" option (enabled by default) #79

Merged
merged 1 commit into from
Dec 24, 2014

Conversation

cgwalters
Copy link
Member

The checking code from #56 landed, and started triggering for me on
the dockerroot user. It's nice to know it works. Then the issue
is... "what now"?

It turns out in the case of dockerroot it's actually unused, so we
could fix this by deleting it. But in general we need to support
dynamic uids/gids/. And we can't yet take a hard dep on #49.

So this patch changes things so we take a copy of the passwd/group
data from the previous commit. Any users subsequently added in the
new commit will be additive.

Closes: #78

The checking code from #56 landed, and started triggering for me on
the `dockerroot` user. It's nice to know it works. Then the issue
is... "what now"?

It turns out in the case of `dockerroot` it's actually unused, so we
could fix this by deleting it. But in general we need to support
dynamic uids/gids/. And we can't yet take a hard dep on #49.

So this patch changes things so we take a copy of the passwd/group
data from the previous commit.  Any users subsequently added in the
*new* commit will be additive.

Closes: coreos#78
@james-antill
Copy link
Contributor

ACK. Looks fine, one thing though: When GFile is creating the /etc/passwd and /usr/lib/passwd I assume the SELinux contexts will be wrong/bad, and I'm not sure that rpm will fix them if they preexist ... so can you check that is true?

Also are planning on doing a preserve-group option too or merge it in here? (I can't think of a reason to preserve one and not the other ... indeed, I imagine stuff would be confused)

@cgwalters
Copy link
Member Author

We currently ignore all SELinux labeling that RPM does; ostree has built in SELinux support, and it labels everything at the end. We do pick up file capabilities though.

And remember that we don't actually have /etc in the final tree - the defaults are in /usr/etc, and we copy those at deploy time, then relabel that subdir, then merge on top the current config changes. (This bit should be optimized to relabel as we copy).

cgwalters added a commit that referenced this pull request Dec 24, 2014
compose: Support "preserve-passwd" option (enabled by default)
@cgwalters cgwalters merged commit 526958a into coreos:master Dec 24, 2014
@cgwalters cgwalters deleted the wip/preserve-passwd branch December 24, 2014 17:17
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 7, 2015
I swear I tested this, but anyways
coreos#79
wasn't quite right.  We need to look at /usr/etc/{passwd,group}
for previous data.

We happily noticed there was no /etc/passwd in the tree, then
proceeded to do the merge and split again, with the result
of an empty /usr/etc/passwd in the new tree.

That in turn resulted in an empty /etc/passwd in an installed system,
i.e. with no "root" user, with obvious bad consequences, namely in my
case crashing Anaconda.

(Yes, I will write a testsuite for this)
cgwalters referenced this pull request in cgwalters/rpm-ostree Jan 7, 2015
Due to an intersection of #79 and #69, we ended up continually
accumulating copies in /usr/lib/{passwd,group}.  The fix here is to
deduplicate when constructing the temporary /etc/passwd that the RPM
install will operate on.

Closes: coreos#92
cgwalters referenced this pull request in cgwalters/rpm-ostree Jan 7, 2015
Due to an intersection of #79 and #69, we ended up continually
accumulating copies in /usr/lib/{passwd,group}.  The fix here is to
deduplicate when constructing the temporary /etc/passwd that the RPM
install will operate on.

Closes: coreos#92
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.

Support copying passwd data from previous commit
2 participants