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.

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?

go.mod

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

libcontainer/capabilities/capabilities.go

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

kolyshkin · 2024-09-25T22:50:17Z

@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?

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 · 2024-09-26T05:42:07Z

No longer a draft; PTAL @lifubang @AkihiroSuda @thaJeztah

lifubang · 2024-09-27T15:37:36Z

libcontainer/capabilities/capabilities.go

+	// 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 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)

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.

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.

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.

It looks like such configuration is not working in the current runc version:

31be074

kolyshkin · 2024-09-27T21:21:25Z

@cyphar @AkihiroSuda PTAL

lifubang · 2024-10-08T02:24:31Z

@AkihiroSuda There is at least one error in this PR(#4358 (comment)), I suggest to wait moby/sys/capability to make an new release(moby/sys#165).

rata · 2024-10-09T11:11:00Z

@lifubang Thanks, but can you elaborate on what is the error exactly? The link to a test case, what does that mean? Should that test fail? Or is it working and it shouldn't? Is that before/after the PR? What do you want to show exactly? I'll check-out this branch and test myself, but it would be great if you can elaborate more on your concern.

lifubang · 2024-10-09T12:40:27Z

but can you elaborate on what is the error exactly?

#4358 (comment)
This PR will cause at least these two regressions.

The link to a test case, what does that mean?

It means the regression 1. So, how about merge this test case first to ensure we can't introduce any regression in the future?

lifubang · 2024-10-09T12:54:23Z

I think we can split this PR to 3 PRs, then we can push the progress more easily.
PR 1: Switch to github.com/moby/sys/capability, this PR should not change any ~~logic~~behavior of runc, because we should replace the dependency package which has a long time bug and is in a unmaintained state ASAP;
PR 2: libct/cap: switch to lazy init;
PR 3: Improve the error msgs of cap set.

But unfortunately, there are still some bugs in github.com/moby/sys/capability. So we need to wait a new release of it.

rata · 2024-10-09T13:04:17Z

@lifubang I fully agree, let's make this PR be 1 or 1 and 2. And PR 3 can be done later, maybe even after the 1.2.0 release (I'm not sure I'd hold the release for adding a warning, especially something that has been like this for a long time).

@kolyshkin what do you think?

kolyshkin · 2024-10-12T19:30:21Z

PR 1: Switch to github.com/moby/sys/capability, this PR should not change any ~~logic~~behavior of runc, because we should replace the dependency package which has a long time bug and is in a unmaintained state ASAP;

Unfortunately it does change the behavior, because of the bug in the original package which is fixed in the fork (kolyshkin/capability#3).

Even if we try to work around this (set the AMBIENT caps separately and ignore the error), it will still change the behavior, because the fork returns the error as soon as it has it, while the original package keep going and tries to set all capabilities.

To solve this, we need something like AmbientRaise which is being added by moby/sys#165. As soon as it is merged and a new release is made, we can make the "PR 1 " work with maximum level of compatibility with the current version.

kolyshkin · 2024-10-12T19:33:23Z

But unfortunately, there are still some bugs in github.com/moby/sys/capability. So we need to wait a new release of it.

More like "missing functionality" (of setting ambient capabilities one by one), although we can actually use x/sys/unix for that.

The only bug I'm aware of is this one, and it does not affect runc in any way.

lifubang · 2024-10-15T13:22:11Z

To solve this, we need something like AmbientRaise which is being added by moby/sys#165. As soon as it is merged and a new release is made, we can make the "PR 1 " work with maximum level of compatibility with the current version.

Yes, like #4418, it is the final usage after we release moby/sys/capability v0.4.0.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

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 <kolyshkin@gmail.com>

This removes the last unversioned package in runc's direct dependencies. Draft pending moby/sys#176 merge and v0.4.0 release. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

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

go.mod Outdated Show resolved Hide resolved

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 marked this pull request as draft September 25, 2024 22:49

kolyshkin force-pushed the rm-cap-init branch 2 times, most recently from 0583622 to ff2e3dd Compare September 26, 2024 05:40

kolyshkin requested a review from cyphar September 26, 2024 05:41

lifubang marked this pull request as ready for review September 26, 2024 06:07

lifubang mentioned this pull request Sep 27, 2024

libct/cap: switch to moby/sys/capability #4418

Open

lifubang reviewed Sep 27, 2024

View reviewed changes

kolyshkin requested a review from lifubang September 27, 2024 20:18

kolyshkin force-pushed the rm-cap-init branch from ff2e3dd to e954d45 Compare September 27, 2024 20:18

cyphar mentioned this pull request Oct 2, 2024

Blockers for v1.2.0 #4114

Closed

21 tasks

kolyshkin mentioned this pull request Oct 4, 2024

Treating unknown/unavailable capabilities #4426

Open

kolyshkin force-pushed the rm-cap-init branch from e954d45 to 4b54b98 Compare October 5, 2024 00:23

AkihiroSuda approved these changes Oct 8, 2024

View reviewed changes

AkihiroSuda requested a review from rata October 8, 2024 02:03

kolyshkin force-pushed the rm-cap-init branch from 4b54b98 to b9a4909 Compare November 1, 2024 17:17

kolyshkin added 3 commits November 1, 2024 10:17

libct/cap: preallocate slices

947e76c

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct/cap: switch to lazy init

d20c770

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 <kolyshkin@gmail.com>

[draft] Switch to github.com/moby/sys/capability v0.4.0

490ab7a

This removes the last unversioned package in runc's direct dependencies. Draft pending moby/sys#176 merge and v0.4.0 release. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the rm-cap-init branch from b9a4909 to 490ab7a Compare November 1, 2024 17:17

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

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 6, 2024

rata Aug 7, 2024

kolyshkin commented Sep 25, 2024

kolyshkin commented Sep 26, 2024

lifubang Sep 27, 2024

kolyshkin Sep 27, 2024

lifubang Sep 27, 2024

kolyshkin Sep 28, 2024 •

edited

Loading

lifubang Sep 28, 2024

kolyshkin commented Sep 27, 2024

lifubang commented Oct 8, 2024

rata commented Oct 9, 2024

lifubang commented Oct 9, 2024

lifubang commented Oct 9, 2024 •

edited

Loading

rata commented Oct 9, 2024

kolyshkin commented Oct 12, 2024

kolyshkin commented Oct 12, 2024

lifubang commented Oct 15, 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

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

rata Aug 2, 2024

Choose a reason for hiding this comment

kolyshkin Aug 6, 2024

Choose a reason for hiding this comment

rata Aug 7, 2024

Choose a reason for hiding this comment

kolyshkin commented Sep 25, 2024

kolyshkin commented Sep 26, 2024

lifubang Sep 27, 2024

Choose a reason for hiding this comment

kolyshkin Sep 27, 2024

Choose a reason for hiding this comment

lifubang Sep 27, 2024

Choose a reason for hiding this comment

kolyshkin Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

lifubang Sep 28, 2024

Choose a reason for hiding this comment

kolyshkin commented Sep 27, 2024

lifubang commented Oct 8, 2024

rata commented Oct 9, 2024

lifubang commented Oct 9, 2024

lifubang commented Oct 9, 2024 • edited Loading

rata commented Oct 9, 2024

kolyshkin commented Oct 12, 2024

kolyshkin commented Oct 12, 2024

lifubang commented Oct 15, 2024

kolyshkin commented Jul 23, 2024 •

edited

Loading

kolyshkin Sep 28, 2024 •

edited

Loading

lifubang commented Oct 9, 2024 •

edited

Loading