Drop unnecessary and wrong group detection on host#401
Drop unnecessary and wrong group detection on host#401debarshiray merged 1 commit intocontainers:masterfrom martinpitt:group-detection
Conversation
|
This can be demonstrated entirely without toolbox. On a Fedora host there is the The current detection code looks at the host groups, but that doesn't apply to the container: |
|
With toolbox, the reproducer is this: |
|
Build failed.
|
martinpitt
changed the title
|
This fails the tests as it breaks the So the fix is even easier -- just drop the redundant "outer" group detection. |
|
Build failed.
|
|
Build succeeded.
|
|
This is revealed/caused by a fix in libpod introduced in 1.8.1. |
|
Can this be merged in ? |
|
My hunch is that the maintainers now ignore this project as they are working full steam on the Go rewrite. I haven't tested that at all yet (it's not yet in Fedora 32 or even Rawhide). It would still be nice to land this, as this project is really easy to work on and run straight out of git. |
|
I would also like to see this PR being approved and merged in master and consequently released as a package... |
|
I'd like to add my vote for merging this fix as well. |
|
The associated code in the Go variant is here: https://github.com/containers/toolbox/blob/master/src/cmd/create.go#L193 The change to remove this setting in Go can probably be added to this pull request as well to ensure this is remains fixed when the Go version becomes official. |
Don't call get_group_for_sudo() on the host during create(). That runs on the host, and thus will check which sudo group exists on the host. But that is entirely irrelevant for sudo inside the container, and it breaks when trying to create a Debian or Ubuntu based toolbox on a Fedora host (or vice versa). This also causes problem on CoreOS[0][1] Also, there is no point in running the `podman create` command with an extra sudo group, normal user privileges are just fine. init_container() will call get_group_for_sudo() inside the container and initialize the groups correctly there. containers#401 [0] containers#423 [1] coreos/fedora-coreos-tracker#458
|
I took the liberty of updating your PR to reflect your original changes in the new Go code. I tried this change on a CoreOS host myself and it really makes rootless Toolbox work. @debarshiray, this should be good to be merged. Thank you very much for working on this! |
|
Build failed.
|
|
The tests succeeded in April when I sent this, but the run yesterday (triggered by @HarryMichal ?) now fails with That looks like it needs to be fixed in your CI? |
|
The CI is being fixed in a separate PR. |
|
Any traction on this? Toolbox is a daily important tool we use. I appreciate the go rewrite, and look forward to it. But the backlog of simple and good bug fixes backing up here is taking its toll. |
|
Build failed.
|
... and other hybrid set-ups where the host and container OSes aren't the same. The entry point of a toolbox container already runs as root:root. Therefore, there's no need to run it with an additional group. Interactive shells spawned by 'sudo su -' both inside the container and on the host don't run with such an additional group either. They run just as root:root. This prevented toolbox containers from starting up on Fedora CoreOS hosts, because CoreOS has both the 'sudo' and 'wheel' groups but the fedora-toolbox images only have the 'wheel' group. Therefore, it ended up calling 'podman create --group-add sudo ...', and since the 'sudo' group was missing from the image, the container failed to start. The --group-add flag was added in commit 4bda42d when the entry point ran as $USER as specified in the user-specific customized image. The additional group was specified to retain consistency with interactive shells run as $USER. Since then, things have changed. There's no longer any user-specific customized image and commit f74400f made the entry point run as root:root. The --group-add flag should have been removed as part of those changes. #423
|
Build failed.
|
|
Looks like this fix for #423 had an unintended side-effect. Earlier, inside the container, we'd get: ... but now: Notice how the |
Owen chased it down the stack and fixed it in crun:
So, all good! |
9 participants
Don't call get_group_for_sudo() on the host during create(). That runs
on the host, and thus will check which sudo group exists on the host.
But that is entirely irrelevant for sudo inside the container, and it
breaks when trying to create a Debian or Ubuntu based toolbox on a
Fedora host (or vice versa).
Also, there is no point in running the
podman createcommand with anextra sudo group, normal user privileges are just fine.
init_container() will call get_group_for_sudo() inside the container and
initialize the groups correctly there.