-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Ensure that --userns=keep-id
sets user in config
#9942
Conversation
Still need to write tests |
@edsantiago Poke - do we have an issue for exec throwing warnings to the effect of:
I think I remember one but I can't find it. Anyways, that's fixed here. |
0ccf569
to
752a39b
Compare
@mheon if I do a podman inspect after this is done, does the user say the UID/GID of the calling user? |
@rhatdan Yep:
Before:
|
@mheon that error doesn't look familiar, and I can't find any emails with "ctl" and "resizing" nor "resizing" and "exec" in my well-indexed archives. If you want tests, take a look at the new https://github.com/containers/podman/blob/master/test/system/450-interactive.bats |
Unfortunately, it still says:
I think that's a separate, more involved fix, though. |
@edsantiago Huh. It is a race, so if CI isn't seeing it, my working theory is that CI is too slow to actually experience it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One of the side-effects of the `--userns=keep-id` command is switching the default user of the container to the UID of the user running Podman (though this can still be overridden by the `--user` flag). However, it did this by setting the UID and GID in the OCI spec, and not by informing Libpod of its intention to switch users via the `WithUser()` option. Because of this, a lot of the code that should have triggered when the container ran with a non-root user was not triggering. In the case of the issue that this fixed, the code to remove capabilities from non-root users was not triggering. Adjust the keep-id code to properly inform Libpod of our intention to use a non-root user to fix this. Also, fix an annoying race around short-running exec sessions where Podman would always print a warning that the exec session had already stopped. Fixes containers#9919 Signed-off-by: Matthew Heon <[email protected]>
Added a check in Libpod to make sure this doesn't happen again. Test added. Should be good to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, mheon, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
One of the side-effects of the
--userns=keep-id
command is switching the default user of the container to the UID of the user running Podman (though this can still be overridden by the--user
flag). However, it did this by setting the UID and GID in the OCI spec, and not by informing Libpod of its intention to switch users via theWithUser()
option. Because of this, a lot of the code that should have triggered when the container ran with a non-root user was not triggering. In the case of the issue that this fixed, the code to remove capabilities from non-root users was not triggering. Adjust the keep-id code to properly inform Libpod of our intention to use a non-root user to fix this.Also, fix an annoying race around short-running exec sessions where Podman would always print a warning that the exec session had already stopped.
Fixes #9919