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

chroot: fix many issues with chroot #7057

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

jfinkels
Copy link
Collaborator

@jfinkels jfinkels commented Jan 3, 2025

This merge request fixes several issues in the behavior of chroot, mostly around setting
group IDs. It ensures that supplemental groups provided by --groups are properly handled with respect to the groups implied by --userspec. It ensures that we fall back to numeric parsing when attempting to lookup user and group IDs from their names.

It changes the type of Options.groups to be Option<Vec<String>> so that
the absence of the --groups option is meaningfully distinguished from
an empty list of groups. This is important because chroot --groups=''
is used specifically to disable supplementary group lookup.

It improves the parsing of the --groups parameter to chroot so that it
handles more error cases and better matches the behavior of GNU
chroot. For example, multiple adjacent commas are allowed:

--groups=a,,,b

but spaces between commas are not allowed:

--groups="a, , ,b"

This fixes all but one of the test cases in the GNU test file tests/chroot/chroot-credentials.sh.

@jfinkels jfinkels linked an issue Jan 3, 2025 that may be closed by this pull request
Copy link

github-actions bot commented Jan 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/id/setgid is no longer failing!
Congrats! The gnu test tests/touch/now-owned-by-other is no longer failing!
Congrats! The gnu test tests/truncate/truncate-owned-by-other is no longer failing!

@jfinkels jfinkels marked this pull request as ready for review January 3, 2025 01:08
Improve the parsing of the `--groups` parameter to `chroot` so that it
handles more error cases and better matches the behavior of GNU
`chroot`. For example, multiple adjacent commas are allowed:

    --groups=a,,,b

but spaces between commas are not allowed:

    --groups="a, , ,b"
Change the type of `Options.groups` to be `Option<Vec<String>>` so that
the absence of the `--groups` option is meaningfully distinguished from
an empty list of groups. This is important because `chroot --groups=''`
is used specifically to disable supplementary group lookup.
Fix several issues in the behavior of `chroot`, mostly around setting
group IDs. This commit ensures that supplemental groups provided by
`--groups` are properly handled with respect to the groups implied by
`--userspec`. This commit also ensures that we fall back to numeric
parsing when attempting to lookup user and group IDs from their names.
Copy link

github-actions bot commented Jan 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/id/setgid is no longer failing!
Congrats! The gnu test tests/touch/now-owned-by-other is no longer failing!
Congrats! The gnu test tests/truncate/truncate-owned-by-other is no longer failing!

@sylvestre
Copy link
Contributor

Three fixes. Wahoo. It is unexpected :)

@sylvestre sylvestre merged commit 8ab7bba into uutils:main Jan 3, 2025
65 checks passed
@sylvestre
Copy link
Contributor

i guess there isnt a way to reuse some stuff in uucore or to share some code with other programs

@jfinkels jfinkels deleted the chroot-set-groups branch January 3, 2025 18:53
@jfinkels
Copy link
Collaborator Author

jfinkels commented Jan 3, 2025

Hmm I'm not sure, but I'll definitely keep it in mind if I see some common code that can be refactored.

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.

chroot: userspec argument is not respected in chroot environment
2 participants