Do not drop effective&permitted set#45511
Conversation
|
ref #45491 |
|
There's little point in making this change against master as it will have no effect. An unintended consequence of #44804 is that |
corhere
added
status/2-code-review
kind/bugfix
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
I am now convinced that zeroing out the Effective and Permitted sets was never the right thing to do. This change is not strictly necessary to fix the bug on v24 or master because of the aforementioned oversight, but is still good to have in so we don't accidentally regress.
Note for reviewers: despite setting the Effective and Permitted sets in the container spec, in practice the container's PID 1 will have the respective capability sets zeroed out in most cases when UID > 0 because that's what execve does. The only exception is when the entrypoint has file capabilities set, which is the case that is being fixed.
|
@xpivarc could you have a look at adding an integration test for this? (let us know if you need help doing so 🤗 ) |
|
@thaJeztah I should have some time this week. Sorry for the delay. |
|
@justincormack PTAL |
Currently moby drops ep sets before the entrypoint is executed. This does mean that with combination of no-new-privileges the file capabilities stops working with non-root containers. This is undesired as the usability of such containers is harmed comparing to running root containers. This commit therefore sets the effective/permitted set in order to allow use of file capabilities or libcap(3)/prctl(2) respectively with combination of no-new-privileges and without respectively. For no-new-privileges the container will be able to obtain capabilities that are requested. Signed-off-by: Luboslav Pivarc <[email protected]> Signed-off-by: Bjorn Neergaard <[email protected]>
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Still some discussion happening on the change. Let me put a "request changes" until those are all settled
|
@thaJeztah PTAL |
|
Thanks @corhere |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM after the tests are fixed
corhere
force-pushed
the
capabilites
branch
2 times, most recently
from
f92519b to
954584e
Compare
Verify non-root containers are able to use file capabilities. Signed-off-by: Luboslav Pivarc <[email protected]> Co-authored-by: Cory Snider <[email protected]> Signed-off-by: Cory Snider <[email protected]>
|
@thaJeztah @neersighted are there any plans for backport? |
|
Yes, as evidenced by the cherry-pick labels. You are welcome to create them yourself, using prior backport PRs as a model, if you want 😄 |
5 participants
--security-opt=no-new-privileges#45491- What I did
Currently moby drops ep sets before the entrypoint is executed. This does mean that with combination of no-new-privileges the file capabilities stops working with non-root containers. This is undesired as the usability of such containers is harmed comparing to running root containers.
This commit therefore sets the effective/permitted set in order to allow use of file capabilities or libcap(3)/prctl(2) respectively with combination of no-new-privileges and without respectively.
For no-new-privileges the container will be able to obtain capabilities that are requested.
- How I did it
- How to verify it
Use the below as Dockerfile
docker run --security-opt=no-new-privileges --user=100 --cap-add sys_admin <tag of the build Dockerfile> capsh --printYou should see
Current: = cap_sys_admin+eprather thanCurrent:- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)