capability: add separate API for ambient capabilities #165

lifubang · 2024-09-30T03:39:13Z

In runtime-spec said[1]:
Runtimes SHOULD NOT fail if the container configuration requests capabilities that cannot be granted, for example, if the runtime operates in a restricted environment with a limited set of capabilities.
So this is the default mode when requesting capabilities. As a package used by runc, I think most of code about capabilities should be in this project, not in runc.

For the first edition of ambient implementation, though it masked the err because of a bug, but there was no return when got an error, it is different from the error handling in other place, so I think it was the author's original ideal to raise as many ambient cap sets as possible when got some errors.
There is no return in ambient caps apply:

sys/capability/capability/capability_linux.go

Lines 477 to 482 in 7b553f5

    
           err := prctl(pr_CAP_AMBIENT, action, uintptr(i), 0, 0) 
        
           // Ignore EINVAL as not supported on kernels before 4.3 
        
           if errno, ok := err.(syscall.Errno); ok && errno == syscall.EINVAL { 
        
           	err = nil 
        
           	continue 
        
           }

There is a return in other place:

sys/capability/capability/capability_linux.go

Lines 451 to 459 in 7b553f5

    
           err = prctl(syscall.PR_CAPBSET_DROP, uintptr(i), 0, 0, 0) 
        
           if err != nil { 
        
           	// Ignore EINVAL since the capability may not be supported in this system. 
        
           	if errno, ok := err.(syscall.Errno); ok && errno == syscall.EINVAL { 
        
           		err = nil 
        
           		continue 
        
           	} 
        
           	return 
        
           }

~~So, I think we should provide a choice to let users make the decision, for ambient cap sets apply, we should provide at least two modes:~~
~~1. Greedy mode: it should be the default mode to be compatible with the past, it means raise ambient caps as many as possible and return the last error;~~
~~2. Stop on error mode: it means that we should stop to raise other ambient caps when got an error.~~

So, I think we should provide the abilities to let users can make a decision to ignore the ambient cap raise/lower error or not.

[1] https://github.com/opencontainers/runtime-spec/blob/8f3fbc881602d85699e5c448634ec1288860d966/config.md?plain=1#L286-L292

lifubang · 2024-09-30T03:40:12Z

Once #164 merged, I'll rebase this PR.

kolyshkin · 2024-09-30T22:25:34Z

I think we should just make "greedy" mode a default.

Out of curiosity, have you checked what other implementations do? libcap C and Go versions, for example?

lifubang · 2024-10-01T01:05:43Z

Out of curiosity, have you checked what other implementations do? libcap C and Go versions, for example?

It seems like there is no function provided in libcap C version to raise more than one ambient caps in one time, so the users have to call cap_set_ambient one by one; Users have a choice in their code to ignore the error or not;

For go version, it provides such type function, there are two difference comparing with us:

It doesn't ignore any errors from syscall, includes EINVAL;
Like the name Apply on our project, it provides a single function SetAmbient to do this, but users can pass one or more ambient caps in params(This is very important).
Though it returns error immediately, but I think if users want to ignore the SetAmbient errors, users have a choice to call it one by one.

The go version implementation code is:

//go:uintptrescapes
func (sc *syscaller) setAmbient(enable bool, val ...Value) error {
	dir := uintptr(prCapAmbientLower)
	if enable {
		dir = prCapAmbientRaise
	}
	for _, v := range val {
		_, err := sc.prctlwcall6(prCapAmbient, dir, uintptr(v), 0, 0, 0)
		if err != nil {
			return err
		}
	}
	return nil
}

So I think we can have some choices:

Add greedy and stop on error mode like this PR;
Add two functions for ambient:

func SetAmbient(enable bool, val ...Value) error
func ResetAmbient() error
WDYT @kolyshkin

lifubang · 2024-10-01T01:20:15Z

So I think we can have some choices:

Add greedy and stop on error mode like this PR;

Add two functions for ambient:

func SetAmbient(enable bool, val ...Value) error

func ResetAmbient() error
WDYT @kolyshkin

In fact I’m prefer to the first solution, this is our own distinguishing feature, and I think it’s more convenient for the callers.

kolyshkin · 2024-10-03T18:27:51Z

I'd like to follow the KISS principle here. In other words, a separate function that changes a way errors are handled is a bit too much.

kolyshkin · 2024-10-03T18:31:19Z

In fact, if we provide a function to apply capabilities one by one, a user can control what do to about errors.

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

In fact, if we provide a function to apply capabilities one by one, a user can control what do to about errors.

Quite agree, I add 3 APIs for ambient cap, maybe runc can use these APIs to support ambient cap set.
PTAL

kolyshkin · 2024-10-06T21:19:15Z

capability/capability_linux.go

@@ -117,6 +117,11 @@ func newPid(pid int) (c Capabilities, retErr error) {
 	return
 }

+func ignorableError(err error) bool {
+	// Ignore EINVAL as not supported on kernels before 4.3


See, the original code has two different comments about why we're ignoring EINVAL:

For PR_CAPBSET_DROP, it says "Ignore EINVAL since the capability may not be supported in this system."

For PR_CAPSET_AMBIENT, it says "Ignore EINVAL as not supported on kernels before 4.3."

With this change, you remove the first comment, moved the second one into this function, and thus it is now applicable to both cases, which is incorrect.

kolyshkin · 2024-10-06T21:26:38Z

capability/capability_linux.go

-						continue
-					}
-					return
+				if err != nil && !ignorableError(err) {


Also, with this change, in case kind only has BOUNDS and the kernel returns EINVAL for the last PR_CAPBSET_DROP, it will be returned, while the code before your changes returns nil in the same case.

I don't think so. ignorableError is not the main reason of this refactor, removing the named return param is the main refactor, err is now a local val, not the return name. It will help to avoid causing other issues like the original issue of ambient implementation.

kolyshkin

Regarding the first commit -- the implementation in this PR is incorrect for a few reasons (see comments above). See #166 for a more correct implementation of what you're trying to achieve.

lifubang · 2024-10-07T01:31:47Z

Regarding the first commit -- the implementation in this PR is incorrect for a few reasons (see comments above). See #166 for a more correct implementation of what you're trying to achieve.

Fixed the comment.
Without this incorrect comment, do you think this implementation is valuable for runc?

kolyshkin · 2024-10-08T01:17:46Z

do you think this implementation is valuable for runc?

Yes, I think, we should set ambient capabilities separately and one by one, so we have a chance to ignore or warn errors.

I'd rename functions to AmbientRaise, AmbientLower and AmbientLowerAll or something like this.

lifubang · 2024-10-08T02:45:49Z

See #166 for a more correct implementation of what you're trying to achieve.

Changed like yours.

lifubang · 2024-10-08T02:46:35Z

I'd rename functions to AmbientRaise, AmbientLower and AmbientLowerAll or something like this.

Changed all function names.

kolyshkin · 2024-10-08T18:31:08Z

capability/capability_linux.go

@@ -117,6 +117,14 @@ func newPid(pid int) (c Capabilities, retErr error) {
 	return
 }

+func ignoreEINVAL(err error) error {
+	// Ignore EINVAL since the op or the capability may not be supported in this system.


Can you please remove this comment? Instead, explain why we're ignoring EINVAL when we call this function.

capability/capability_linux.go

kolyshkin · 2024-10-08T21:51:28Z

I just realized this package API is flawed. You start with NewPid2(pid), but then some capabilities can only be applied to current process (when pid == 0 or pid == gettid()`). Meaning, if you do something like this:

c, err := capability.NewPid(somePid)
...
c.Set(capability.AMBIENT, capability.CAP_CHOWN)
c.Apply(capability.AMBIENT)

The ambient capability will be applied to the current process, rather than the process identified by somePid.

😓

Will open a new issue.

This PR is related, since it adds new methods to the above API. Since we can only raise/lower/clear own capabilities, Those should probably just be functions.

I'm also thinking we should only accept only one single capability as the argument, since otherwise we have the same problem with the errors (whether to continue or return an error if we caught one).

func AmbientRaise(Cap) error
func AmbientLower(Cap) error
func AmbientClearAll() error

lifubang · 2024-10-09T01:49:33Z

The ambient capability will be applied to the current process, rather than the process identified by somePid.

👍 This is really a long time bug.

This PR is related, since it adds new methods to the above API. Since we can only raise/lower/clear own capabilities, Those should probably just be functions.

Yes, for other users, to have a compatibility, we may also need to add a pid check in Apply.

I'm also thinking we should only accept only one single capability as the argument, since otherwise we have the same problem with the errors (whether to continue or return an error if we caught one).

It's not very important, we can keep support a cap array, someone who want to ignore the error, he can pass a single cap one by one.
So, I'd prefer to support multi-caps param.

capability/capability.go

capability/capability_linux.go

capability/capability_test.go

kolyshkin · 2024-10-10T17:24:51Z

Needs a rebase (#164 is merged).

lifubang · 2024-10-11T01:13:53Z

Can we have a fix for #165 as a separate PR? This one is becoming too big to consume.

OK, I'll split this PR to two.

capability/capability_linux.go

capability/capability_test.go

Signed-off-by: lifubang <[email protected]>

kolyshkin · 2024-10-24T06:35:01Z

Looks like this actually works now!

=== RUN   TestAmbientCapAPI
    capability_test.go:205: output from child:
        === RUN   TestAmbientCapAPI
--- PASS: TestAmbientCapAPI (0.00s)

kolyshkin

LGTM, thanks!

kolyshkin · 2024-10-24T06:39:36Z

@lifubang thanks, only one nit: the last commit subject should be prefixed with capability:. Or, you can squash the last two commits since they are about the same thing.

kolyshkin

I am looking into C library (cap) and maybe we should design API here in the same way.

They have:

       int cap_set_ambient(cap_value_t cap, cap_flag_value_t value);
       int cap_reset_ambient(void);

Maybe we should do something similar?

// AmbientSet raises or lowers specified ambient capabilities for the calling process.
func AmbientSet(raise bool, cap ...Cap) error

or

// AmbientSet raises or lowers the specified ambient capability for the calling process.
func AmbientSet(cap Cap, raise bool) error

I'm thinking about the last form because I'm still not very convinced we should provide a function which sets multiple capabilities. It adds some complexity but on the caller's side it's just a simple for loop.

kolyshkin · 2024-10-24T07:57:24Z

I'm thinking about the last form because I'm still not very convinced we should provide a function which sets multiple capabilities. It adds some complexity but on the caller's side it's just a simple for loop.

Also because we can provide AmbientIsSet(which Cap) bool, which can only check a single capability.

lifubang mentioned this pull request Sep 30, 2024

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

Open

lifubang force-pushed the ambient-apply-mode branch from a336f24 to e502b72 Compare September 30, 2024 03:43

lifubang force-pushed the ambient-apply-mode branch from e502b72 to 3b5d22e Compare October 5, 2024 02:22

kolyshkin reviewed Oct 6, 2024

View reviewed changes

kolyshkin requested changes Oct 6, 2024

View reviewed changes

lifubang force-pushed the ambient-apply-mode branch from 3b5d22e to 27a611c Compare October 7, 2024 01:21

This comment was marked as outdated.

Sign in to view

lifubang changed the title ~~Add ApplyMode and implement it for AMBIENT~~ Improve implementation of AMBIENT cap Oct 8, 2024

lifubang mentioned this pull request Oct 8, 2024

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

Open

lifubang force-pushed the ambient-apply-mode branch 2 times, most recently from 6886234 to 6153b26 Compare October 8, 2024 02:43

kolyshkin reviewed Oct 8, 2024

View reviewed changes

capability/capability_linux.go Show resolved Hide resolved

kolyshkin reviewed Oct 8, 2024

View reviewed changes

capability/capability_linux.go Show resolved Hide resolved

lifubang force-pushed the ambient-apply-mode branch 2 times, most recently from c5a04f3 to 3ce9f85 Compare October 9, 2024 03:33