unshare: fix creating a userns when running as root #1415
Conversation
There was a problem hiding this comment.
What is this actually fixing?
Some situation where we are in a user namespace already, one that wasn’t created by this subpackage (no environment variables), we do have CAP_ADMIN (per IsRootless), but don’t have some other capability?
Something else?
Does that mean that the semantics of IsRootless is also incorrect and needs updating? (The documentation of IsRootless says basically nothing, so I have absolutely no idea.)
I’m not saying that this change is incorrect, I’m saying that I have no way to tell, and this might be a good opportunity to actually clarify the semantics.
| @@ -464,11 +464,6 @@ func bailOnError(err error, format string, a ...interface{}) { // nolint: golint | |||
|
|
|||
| // MaybeReexecUsingUserNamespace re-exec the process in a new namespace | |||
| func MaybeReexecUsingUserNamespace(evenForRoot bool) { | |||
There was a problem hiding this comment.
If this changes the semantics of the function in caller-visible ways, I’d expect that to show up at least in the documentation, if not in the name.
IMHO doc strings should be enough for a caller to understand what the function does (from the caller’s perspective) without needing to read the implementation, and this one is way below that standard.
this check would prevent the function to ever be used when running as root, since we won't check what capabilities are currently available to the process. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
fix-maybe-reexec-using-userns
branch
from
dbc6c4e
to
1af3928
Compare
the issue I've observed is that if we run as root but without CAP_SYS_ADMIN, then the function exits immediately without creating the user namespace as it was asked to do. I've changed the patch so we avoid the additional checks if we an unprivileged user (uid > 0) created the user namespace and we are not already running with euid == 0 |
|
can we please move this forward? It is needed for buildah inside a container |
This change broke my usage of buildah inside container. Before this change: After this change: |
|
@giuseppe WDYT? |
|
We've discussed that a bit in containers/buildah#4538, but since they're kinda separate issues, I've created containers/buildah#4563 specifically to address regression. |
this check would prevent the function to ever be used when running as root, since we won't check what capabilities are currently available to the process.
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com