libct/cap: switch to moby/sys/capability, lazy init #4358
Conversation
|
This still LGTM. Let me know if it's ready, IMHO this is ready to merge. |
|
@kolyshkin was the last commit intended for this PR, or for a follow up? |
kolyshkin
changed the title
I can split this PR into two, but really I want all this to be merged (and you don't have to re-review the first two commits). |
This comment was marked as outdated.
This comment was marked as outdated.
| out = append(out, v) | ||
| } | ||
| } | ||
| return out | ||
| } | ||
| inheritable := capSlice(capConfig.Inheritable, nil) | ||
| // Ambient vector can not contain values not raised in the Inheritable vector, |
There was a problem hiding this comment.
The whole sentence from: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/cap#section-readme is:
Note, the Ambient vector cannot contain values not raised in the Inh vector, so setting values directly in one vector may have the side effect of mirroring the value in the other vector to maintain this constraint
I guess we want to ignore it, instead of adding it, as we are doing for a long time now. But wanted to raise this, just in case.
There was a problem hiding this comment.
Yes, that library raises inheritable bits where the corresponding ambient bits are raised.
I agree that we should not change the behavior (and instead maybe document that if you want to raise ambient caps, you have to raise inheritable ones as well).
kolyshkin
changed the title
kolyshkin
force-pushed
the
rm-cap-init
branch
from
a4dfbc8
to
97b707f
Compare
kolyshkin
changed the title
kolyshkin
force-pushed
the
rm-cap-init
branch
2 times, most recently
from
26aaa82
to
e63cec2
Compare
|
@kolyshkin I see you marked this as ready, but as you didn't re-request a review, it doesn't notify anyone. Is this ready for review? I see the first 3 commits here are also in #4367, which seems confusing (and splitting the threads of review for those). Do you want us to review the rest of the commits only here, but those are here to avoid conflicts/something? Can you explain a little bit what is your intention here? |
kolyshkin
force-pushed
the
rm-cap-init
branch
from
e63cec2
to
d921a08
Compare
Sorry for the confusion; I think we should merge #4367 first, and I am still working on it (need to add a new test case by @lifubang). |
kolyshkin
force-pushed
the
rm-cap-init
branch
2 times, most recently
from
0583622
to
ff2e3dd
Compare
|
No longer a draft; PTAL @lifubang @AkihiroSuda @thaJeztah |
| // compatibility we have to ignore those Ambient caps that are not also raised | ||
| // in Inheritable (and issue a warning). | ||
| ambient := capSlice(capConfig.Ambient, inheritable) | ||
| if len(ignoredCaps) > 0 { |
There was a problem hiding this comment.
There are at least 2 conditions to return error when setting ambient caps:
- The ambient cap sets is not in both permitted and inheritable cap sets;
- The PR_CAP_AMBIENT_LOWER securebit has been set.
So maybe this is not enough. I take another easy way to implement this compatibility. (#4418)
There was a problem hiding this comment.
This PR handles condition 1. As for condition 2, it works as it used to work before, no changes needed here.
In general, I think, we should return an error when we're unable to set a capability requested. The warning which this PR adds is a temporary measure, to ensure we're not breaking our users. Eventually this warning should become an error.
There was a problem hiding this comment.
This PR handles condition 1.
Because of ‘both permitted and inheritable cap sets’.
Even in condition 1, I think you need to add check ‘permitted’ too, not only ‘inheritable’. Please see my first commit in #4418 .
As for condition 2, it works as it used to work before, no changes needed here.
I think this error was also masked before this PR, but will be failed in the future. It should have a compatibility like condition 1.
There was a problem hiding this comment.
Even in condition 1, I think you need to add check ‘permitted’ too, not only ‘inheritable’. Please see my first commit in #4418 .
It looks like such configuration is not working in the current runc version:
@test "runc run [raise inheritable bit not set in permitted]" {
update_config ' .process.capabilities.inheritable = ["CAP_KILL"]
| .process.capabilities.permitted = ["CAP_CHOWN"]'
# | .process.capabilities.ambient = ["CAP_KILL", "CAP_CHOWN"]'
runc run testct
[ "$status" -eq 0 ] runc run testct (status=1):
time="2024-09-27T17:31:28-07:00" level=error msg="runc run failed: unable to start container process: error during container init: unable to apply caps: operation not permitted"
This means:
- If it's not working now, it should not start work in the future.
- We don't have to check for permitted bit set when checking ambient.
As for condition 2, it works as it used to work before, no changes needed here.
I think this error was also masked before this PR, but will be failed in the future. It should have a compatibility like condition 1.
You might be right, I need to check it.
There was a problem hiding this comment.
It looks like such configuration is not working in the current runc version:
kolyshkin
force-pushed
the
rm-cap-init
branch
from
ff2e3dd
to
e954d45
Compare
|
@cyphar @AkihiroSuda PTAL |
Signed-off-by: Kir Kolyshkin <[email protected]>
A map which is created in func init is only used by capSlice, which is only used by New, which is only used by runc init. Switch to lazy init to slightly save on startup time. Signed-off-by: Kir Kolyshkin <[email protected]>
Move capSlice to be an internal function of New. This way, we don't have to pass most parameters. This is a preparation for the next commit. Signed-off-by: Kir Kolyshkin <[email protected]>
Fixing a long standing bug in github.com/syndtr/gocapability package
(ignoring errors when setting ambient caps, see [1]) revealed that
it's not possible to raise those ambient capabilities for which
inheritable capabilities are not raised. In other words, "the Ambient
vector cannot contain values not raised in the Inh vector" ([2]).
The example spec in libct/specconv had a few ambient capabilities set
but no inheritable ones. As a result, when capability package with fix
from [1] is used, we get an error trying to start a container ("unable
to apply caps: permission denied").
The only decent way to fix this is to ignore raised ambient capabilities
for which inheritable capabilities are not raised (essentially mimicking
the old behavior). Let's also add a warning about ignored capabilities,
with an intention to change it to an error later.
Fix the example spec accordingly (remove the ambient caps).
This is in preparation to switch to github.com/kolyshkin/capability.
[1]: kolyshkin/capability#3
[2]: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/cap#IAB.SetVector
Signed-off-by: Kir Kolyshkin <[email protected]>
This removes the last unversioned package in runc's direct dependencies. Signed-off-by: Kir Kolyshkin <[email protected]>
This has started as a simple way to reduce init() overhead in libcontainer/capabilities, but ended up switching to the fork of gocapability package, and also fixing a big issue in handling of ambient capabilities.
Please see individual commits for details; I will open an issue about ambient caps tomorrow.