capability: add separate ambient and bound API #176

kolyshkin · 2024-10-31T01:32:10Z

The API being added is the same as in kernel.org/pub/linux/libs/security/libcap/cap
package, although the implementation is a bit simpler (here we only set
capabilities for the calling thread).

With this, we can switch runc to moby/sys/capability.

Co-authored-by: @lifubang
Closes: #165

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

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

All the prctl calls that we make (or can potentially make) are limited to 3 arguments, so it's sufficient to use Syscall (rather than Syscall6). This is mostly a cosmetic change. Signed-off-by: Kir Kolyshkin <[email protected]>

RawSyscall is preferable for syscalls that do not block, and neither of the ones used by this package do. This makes the whole thing a bit faster. Signed-off-by: Kir Kolyshkin <[email protected]>

We need to lock OS thread as this library deals with per-thread capabilities. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2024-10-31T01:35:48Z

@lifubang PTAL (this is similar to #165 plus

adds GetAmbient (for completeness);
makes the API similar to that of cap package;
some nits here and there.

thaJeztah · 2024-10-31T01:52:39Z

capability/capability_linux.go

+func ignoreEINVAL(err error) error {
+	if errors.Is(err, syscall.EINVAL) {
+		err = nil


Reminds me that I stumbled on this package which had a "consider integrating with moby/sys todo; https://github.com/moby/moby/blob/dc225798cbddebd47bfaa0fd8337d145c91fc6ba/internal/unix_noeintr/fs_unix.go#L3-L5

That thing in there is something entirely different -- it's a retry-on-EINTR, and here we have ignore-EINVAL.

As to autogenerating stuff, even github.com/golang/go doesn't do it -- they use helper functions here and there but it's all manual.

thaJeztah

LGTM

thaJeztah · 2024-10-31T02:03:44Z

capability/capability_test.go

+			log.Fatalf("Get(AMBIENT, %s): want %v, got %v", cap, want, got)
+		}
+	}
+	os.Exit(0)


LOL, had to look twice, but then saw it's called from testInChild

Yeah, it's somewhat complicated here since we modify capabilities and if we do it in a current process the following tests are toasted.

I'm way outside my understanding, but isn't that what runtime.LockOSThread is for/does?
(makes sure the capabilities are adjusted for the thread we're on, and then if we don't unlock the thread, it exits instead of being reused?)

We are actually re-executing the test binary here (or, rather, in testInChild), so here we have a separate process (not a bare go thread) running.

It never occurred to be we can test all this in a separate go thread (rather than a process). Let me give it a try :)

I quickly tried that and it did't work. I'll add it to TODO and merge this as is for now, since it's just a test case.

Ah, the runtime.LockOSThread(), as added, is needed so that Go runtime won't suddenly switch the goroutine to other OS thread while we run the test.

Practically, for users of this package, it means we need to change capabilities (in a locked OS thread) and then re-exec (which is what runc does).

Yeah, I was looking at capabilities being a per thread attribute and scratching my beard 😂 sorry it didn't work out, but at least now you know!

thaJeztah · 2024-10-31T02:04:14Z

@AkihiroSuda @tianon ptal

thaJeztah · 2024-10-31T02:10:06Z

capability/syscall_linux.go

-func prctl(option int, arg2, arg3, arg4, arg5 uintptr) (err error) {
-	_, _, e1 := syscall.Syscall6(syscall.SYS_PRCTL, uintptr(option), arg2, arg3, arg4, arg5, 0)
+func prctl(option int, arg2, arg3 uintptr) (err error) {
+	_, _, e1 := syscall.Syscall(syscall.SYS_PRCTL, uintptr(option), arg2, arg3)


I could've sworn these numbered ones were deprecated, but that was for windows there they added a SyscallN(); https://github.com/golang/go/blob/6d39245514c675cdea5c7fd7f778e97bf0728dd5/src/syscall/dll_windows.go#L27-L45

Wonder why they did this change only for Windows. Perhaps it's better aligned to native windows apis.

The API is the same as in kernel.org/pub/linux/libs/security/libcap/cap package, although the implementation is a bit simpler (here we only set capabilities for the calling thread). Co-authored-by: lifubang <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>

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 <[email protected]>

tianon

Looks good (and IMO totally fine to merge as-is) -- I left a couple questions, but I don't think they're super important.

tianon · 2024-11-04T21:45:39Z

capability/capability_linux.go

 		}
 		if (1<<uint(CAP_SETPCAP))&data[0].effective != 0 {
 			for i := Cap(0); i <= last; i++ {
 				if c.Get(BOUNDING, i) {
 					continue
 				}
-				err = prctl(syscall.PR_CAPBSET_DROP, uintptr(i), 0, 0, 0)
+				// Ignore EINVAL since the capability may not be supported in this system.
+				err = ignoreEINVAL(prctl(syscall.PR_CAPBSET_DROP, uintptr(i), 0))


Not a huge deal (certainly not a dealbreaker), but it does seem weird to still use prctl directly here when all the rest have switched to your new wrappers -- was there a reason not to use dropBound here?

Nice catch, thanks!

I added GetBound/DropBound at the last moment, and this here is part of an earlier patch. Still makes sense to use dropBound here -- updated.

tianon · 2024-11-04T21:50:37Z

capability/capability_test.go

+			log.Fatalf("Get(AMBIENT, %s): want %v, got %v", cap, want, got)
+		}
+	}
+	os.Exit(0)


I'm way outside my understanding, but isn't that what runtime.LockOSThread is for/does?
(makes sure the capabilities are adjusted for the thread we're on, and then if we don't unlock the thread, it exits instead of being reused?)

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

tianon

I'm helping!! 😂❤️

kolyshkin and others added 5 commits October 30, 2024 16:41

capability: fix a typo

0b071ef

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

capability: improve error handling for func Apply

bb713bc

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

capability: prctl nit

8f1f88b

All the prctl calls that we make (or can potentially make) are limited to 3 arguments, so it's sufficient to use Syscall (rather than Syscall6). This is mostly a cosmetic change. Signed-off-by: Kir Kolyshkin <[email protected]>

capability: use raw syscalls

f092a6d

RawSyscall is preferable for syscalls that do not block, and neither of the ones used by this package do. This makes the whole thing a bit faster. Signed-off-by: Kir Kolyshkin <[email protected]>

capability: test fix

a902dc6

We need to lock OS thread as this library deals with per-thread capabilities. Signed-off-by: Kir Kolyshkin <[email protected]>

thaJeztah reviewed Oct 31, 2024

View reviewed changes

thaJeztah approved these changes Oct 31, 2024

View reviewed changes

thaJeztah reviewed Oct 31, 2024

View reviewed changes

kolyshkin force-pushed the cap-ambient branch from 6449fa9 to c61ae88 Compare November 1, 2024 01:54

kolyshkin changed the title ~~capability: add GetAmbient, SetAmbient, ResetAmbient API~~ capability: add separate ambient and bound API Nov 1, 2024

tianon approved these changes Nov 4, 2024

View reviewed changes

capability: add GetBound, DropBound

c1ade77

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

kolyshkin force-pushed the cap-ambient branch from c61ae88 to c1ade77 Compare November 7, 2024 03:47

kolyshkin merged commit 638aa7c into moby:main Nov 7, 2024
20 checks passed

tianon approved these changes Nov 7, 2024

View reviewed changes

kolyshkin mentioned this pull request Nov 7, 2024

capability: v0.4.0 release changelog #177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

capability: add separate ambient and bound API #176

capability: add separate ambient and bound API #176

kolyshkin commented Oct 31, 2024

kolyshkin commented Oct 31, 2024

thaJeztah Oct 31, 2024

kolyshkin Oct 31, 2024

thaJeztah left a comment

thaJeztah Oct 31, 2024

kolyshkin Nov 1, 2024

tianon Nov 4, 2024

kolyshkin Nov 7, 2024

kolyshkin Nov 7, 2024

kolyshkin Nov 7, 2024

tianon Nov 7, 2024

thaJeztah commented Oct 31, 2024

thaJeztah Oct 31, 2024

kolyshkin Nov 1, 2024

tianon left a comment

tianon Nov 4, 2024

kolyshkin Nov 7, 2024

tianon Nov 4, 2024

tianon left a comment

capability: add separate ambient and bound API #176

capability: add separate ambient and bound API #176

Conversation

kolyshkin commented Oct 31, 2024

kolyshkin commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah 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

Choose a reason for hiding this comment

thaJeztah commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon 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

tianon left a comment

Choose a reason for hiding this comment