libct/cap: switch to moby/sys/capability, lazy init #4358

kolyshkin · 2024-07-23T00:28:37Z

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.

This currently includes #4367; we can either merge it and rebase this PR, or just close that one in favor of this.

libcontainer/capabilities/capabilities.go

thaJeztah

LGTM; left one suggestion (not blocking)

rata

LGTM, thanks!

rata · 2024-07-30T09:00:34Z

This still LGTM. Let me know if it's ready, IMHO this is ready to merge.

thaJeztah · 2024-08-01T06:41:23Z

@kolyshkin was the last commit intended for this PR, or for a follow up?

thaJeztah · 2024-08-01T08:42:48Z

go.mod

@@ -10,14 +10,14 @@ require (
 	github.com/cyphar/filepath-securejoin v0.2.4
 	github.com/docker/go-units v0.5.0
 	github.com/godbus/dbus/v5 v5.1.0
+	github.com/kolyshkin/capability v0.1.0


Oh! I saw you work on this repository; should we try get this in either the opencontainers or moby org?

Or maybe the author of https://github.com/syndtr/gocapability is open to transferring maintenance (I think it's largely unmaintained, but could of course be just "moving slow")

@thaJeztah I was thinking of opencontainers but I'm not an org admin so can't create new repos here. Maybe moby is a good place; I guess I can transfer the repo.

Yeah, would be good; where possible I try to reduce dependencies from personal accounts. No discredit to those authors (you in this case), but not everyone may have familiarity with the author. Having the code in an org also can be more flexible and future-proof (in addition to being able to benefit from such orgs usually being on a (sponsored) enterprise plan, which may come with additional features on GitHub).

Either one would be fine with me; doing a very cursory check on where it's used (and thus potential candidates to switch), I see these;

AFAICS it's no longer a direct dependency in any of the projects in the Moby org, but I'd not consider that a blocker for having it in that org (personally). Happy to hear from others though! If we prefer opencontainers, I guess we can ask an owner to help transferring.

I was talking in another PR about maybe not using the personal account (#4362 (comment)). IMHO moby or opencontainers seem fine.

Shall we review and merge this or do we want to wait until we sort out the repo ownership?

Discussing with maintainers in the Moby call, and it looks like there's no objections against moving it in the moby org, so we may as well try to set that in motion (I can catch up with Kir on that)

Yes, let's move this to org repo first.

@caniszczyk can you help me move https://github.com/kolyshkin/capability to under opencontainers org?

opened kolyshkin/capability#14

In general you need an OCI TOB vote to add projects to opencontainers, I suspect putting it moby would be simpler. I'm open to proposing it to the TOB though.

kolyshkin · 2024-08-01T09:05:52Z

@kolyshkin was the last commit intended for this PR, or for a follow up?

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).

kolyshkin · 2024-08-02T05:34:55Z

we want mv github.com/kolyshkin/capability to github.com/opencontainers/capability can you help me move [https://github.com/kolyshkin/capability] to under opencontainers org? @gregkh @bfirsh

Please don't tag people you don't know. These are definitely not those who can help. We will figure it out.

rata

This mostly LGTM, left some comments

rata · 2024-08-02T10:06:47Z

libcontainer/capabilities/capabilities.go

+	//
+	// If mustHave argument is not nil, caps that are not present in mustHave
+	// are appended to ignoredCaps instead of the resulting slice.
+	capSlice := func(caps []string, mustHave []capability.Cap) []capability.Cap {


A "global" list of ignoredCaps that can be modified by any call to this function doesn't play nice with the fact that we only want these capabilities when we call this with ambient/inheritable. All the other calls, we don't want them to modify this.

Right now all the other calls use nil as the second param and only ambient/inheritable use non nil for the second param, so this works fine. But it seems a little error-prone.

What if it returns two caps set, the current value and the slice of caps not present in mustHave? Then only the call for ambient/inh will not ignore it? Or maybe we don't want to modify this capSlice func, maybe we want another function and use the slices package, like https://pkg.go.dev/slices#Compare and maybe delete, to get the ignored caps?

I understand that the mustHave we probably only want it for ambient/inh now, so it should be safe now. But if the userns capabilities are merged upstream, or we do some change, this might cause some bug. I'm not sure the risk is big and worth changing this, but I think the alternatives I proposed seems also easier to reason about.

What do you think?

At this point, I think it's best to document the capSlice function making it clear it modifies some variables, letting future devs know about it.

I just tried doing the above and realized it is already documented ("caps ... are appended to ignoredCaps").

Since this function is scoped inside New, its usage is quite limited. I understand the argument for being future proof, but it looks like adding more parameters (and, especially, return values) will decrease readability, which plays against us in "future proof" department.

A separate code to check that all ambient capabilities are also present in inheritable will be the best way, I guess. Using slices.Compare is not an option here, because:

our slices are not sorted;

inheritable set can contain capabilities that ambient set does not.

So we have to have a for (or something like slices.DeleteFunc). The problem here is we either compare a bunch of strings (which is inefficient) or deal with resulting slices of ints, but then we have to convert those capabilities back to string for the warning message (which is OK since the warning is rare, but it adds yet another complexity).

I spent some time trying to come up with something that sucks less than the code I have here, but I fail miserably. Perhaps you can suggest some code @rata?

For now, I just added cleaning the ignoredCaps map after use, hoping that if someone is going to reuse this, they will copy-paste the code 😁. Here's the diff for your review:

diff --git a/libcontainer/capabilities/capabilities.go b/libcontainer/capabilities/capabilities.go index 1111aac3..8082b4f0 100644 --- a/libcontainer/capabilities/capabilities.go +++ b/libcontainer/capabilities/capabilities.go @@ -89,6 +89,7 @@ func New(capConfig *configs.Capabilities) (*Caps, error) { ambient := capSlice(capConfig.Ambient, inheritable) if len(ignoredCaps) > 0 { logrus.Warn("unable to set Ambient capabilities which are not set in Inheritable; ignoring following Ambient capabilities: ", mapKeys(ignoredCaps)) + clear(ignoredCaps) } c.caps = map[capability.CapType][]capability.Cap{

True, it is not sorted so it would be more expensive, and it is not very expensive (n^2 in 15 elem lists is not too much), but it would imply allocations and so on.

Okay, I think the current comment is fine :-)

rata · 2024-08-02T10:08:21Z

libcontainer/capabilities/capabilities.go

 				out = append(out, v)
 			}
 		}
 		return out
 	}
+	inheritable := capSlice(capConfig.Inheritable, nil)
+	// Ambient vector can not contain values not raised in the Inheritable vector,


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.

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).

Commit 98fe566 removed setting inheritable capabilities from runc exec --cap, but neglected to also remove ambient capabilities. An ambient capability could only be set if the same inheritable capability is set, so as a result of the above change ambient capabilities were not set (but due to a bug in gocapability package, those errors are never reported). Once we start using a library with the fix [1], that bug will become apparent. Alas, we do not have any tests for runc exec --cap, so add one. [1]: kolyshkin/capability#3 Fixes: 98fe566 ("runc: do not set inheritable capabilities") Signed-off-by: Kir Kolyshkin <[email protected]>

Commit 98fe566 removed inheritable capabilities from the example spec (used by runc spec) and from the libcontainer/integration test config, but neglected to also remove ambient capabilities. An ambient capability could only be set if the same inheritable capability is set, so as a result of the above change ambient capabilities were not set (but due to a bug in gocapability package, those errors are never reported). Once we start using a library with the fix [1], that bug will become apparent (both bats-based and libct/int tests will fail). [1]: kolyshkin/capability#3 Fixes: 98fe566 ("runc: do not set inheritable capabilities") Signed-off-by: Kir Kolyshkin <[email protected]>

The example is too long since it lists too many capabilities. Simplify it, leaving only two capabilities. Also, remove ambient capabilities from the set. Inheritable capabilities were removed earlier by commit 98fe566, but ambient capabilities can't be raised without inheritable ones. Fixes: 98fe566 Signed-off-by: Kir Kolyshkin <[email protected]>

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. Fix the example spec (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]>

rata · 2024-09-19T09:54:33Z

@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 mentioned this pull request Jul 23, 2024

runc aways open "/proc/sys/kernel/cap_last_cap" #4356

Open

kolyshkin marked this pull request as ready for review July 23, 2024 17:45

thaJeztah reviewed Jul 23, 2024

View reviewed changes

libcontainer/capabilities/capabilities.go Outdated Show resolved Hide resolved

thaJeztah approved these changes Jul 23, 2024

View reviewed changes

rata approved these changes Jul 25, 2024

View reviewed changes

kolyshkin force-pushed the rm-cap-init branch from 351ab7e to 2bb6167 Compare July 29, 2024 20:37

rata added the status/4-merge label Jul 30, 2024

kolyshkin force-pushed the rm-cap-init branch from 276238b to cdb5b66 Compare August 1, 2024 08:29

kolyshkin changed the title ~~libct/cap: switch to lazy init~~ libct/cap: switch to kolyshkin/capability, lazy init, fix for ambient caps Aug 1, 2024

kolyshkin requested review from thaJeztah and rata August 1, 2024 08:33

thaJeztah reviewed Aug 1, 2024

View reviewed changes

kolyshkin force-pushed the rm-cap-init branch from cdb5b66 to bb7b102 Compare August 1, 2024 09:02

rata removed the status/4-merge label Aug 1, 2024

This comment was marked as outdated.

Sign in to view

kolyshkin force-pushed the rm-cap-init branch from bb7b102 to 1d0ff48 Compare August 2, 2024 05:05

thaJeztah self-requested a review August 2, 2024 08:08

rata reviewed Aug 2, 2024

View reviewed changes

This was referenced Aug 6, 2024

DRAFT:replace github.com/syndtr/gocapability with github.com/kolyshkin/capa… #4362

Closed

Move this repo to an org kolyshkin/capability#14

Open

kolyshkin changed the title ~~libct/cap: switch to kolyshkin/capability, lazy init, fix for ambient caps~~ libct/cap: switch to kolyshkin/capability, lazy init Aug 6, 2024

kolyshkin marked this pull request as draft August 6, 2024 19:22

kolyshkin force-pushed the rm-cap-init branch from 1d0ff48 to a4dfbc8 Compare August 7, 2024 02:50

kolyshkin mentioned this pull request Sep 6, 2024

Add capability package moby/sys#149

Merged

kolyshkin force-pushed the rm-cap-init branch from a4dfbc8 to 97b707f Compare September 12, 2024 02:38

kolyshkin changed the title ~~libct/cap: switch to kolyshkin/capability, lazy init~~ libct/cap: switch to moby/sys/capability, lazy init Sep 12, 2024

kolyshkin added 7 commits September 16, 2024 11:55

libct/cap: preallocate slices

9cfc264

Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cap: switch to lazy init

0e8e740

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]>

libct/cap: internalize capSlice

bfed3dc

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]>

kolyshkin force-pushed the rm-cap-init branch from 97b707f to 26aaa82 Compare September 16, 2024 19:00

Switch to github.com/moby/sys/capability v0.2.0

e63cec2

This removes the last unversioned package in runc's direct dependencies. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the rm-cap-init branch from 26aaa82 to e63cec2 Compare September 16, 2024 19:41

kolyshkin marked this pull request as ready for review September 16, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libct/cap: switch to moby/sys/capability, lazy init #4358

libct/cap: switch to moby/sys/capability, lazy init #4358

kolyshkin commented Jul 23, 2024 •

edited

Loading

thaJeztah left a comment

rata left a comment

rata commented Jul 30, 2024

thaJeztah commented Aug 1, 2024

thaJeztah Aug 1, 2024

thaJeztah Aug 1, 2024

kolyshkin Aug 1, 2024

thaJeztah Aug 1, 2024

rata Aug 1, 2024

rata Aug 1, 2024

thaJeztah Aug 1, 2024

kolyshkin Aug 1, 2024

kolyshkin Aug 6, 2024

cyphar Aug 7, 2024

kolyshkin commented Aug 1, 2024

This comment was marked as outdated.

kolyshkin commented Aug 2, 2024

rata left a comment

rata Aug 2, 2024

kolyshkin Aug 7, 2024

rata Aug 7, 2024

rata Aug 2, 2024

kolyshkin Aug 6, 2024

rata Aug 7, 2024

rata commented Sep 19, 2024

libct/cap: switch to moby/sys/capability, lazy init #4358

Are you sure you want to change the base?

libct/cap: switch to moby/sys/capability, lazy init #4358

Conversation

kolyshkin commented Jul 23, 2024 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

rata left a comment

Choose a reason for hiding this comment

rata commented Jul 30, 2024

thaJeztah commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Aug 1, 2024

This comment was marked as outdated.

kolyshkin commented Aug 2, 2024

rata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata commented Sep 19, 2024

kolyshkin commented Jul 23, 2024 •

edited

Loading