Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Sep 27, 2024

There is a long standing bug in github.com/syndtr/gocapability package:
It will always ignore errors when setting ambient caps.
(Please see kolyshkin/capability#3)
We need to have a compatibility with before even though this bug has been fixed.

As we can learn from the man page for capabilities(7) and PR_CAP_AMBIENT_RAISE(2const):

Ambient (since Linux 4.3)
    This is a set of capabilities that are preserved across an
    [execve(2)](https://man7.org/linux/man-pages/man2/execve.2.html) of a program that is not privileged.  The
    ambient capability set obeys the invariant that no
    capability can ever be ambient if it is not both permitted
    and inheritable.
ERRORS
       EINVAL cap does not specify a valid capability.

       EPERM  either the capability specified in cap is not present in
              the process's permitted and inheritable capability sets,
              or the PR_CAP_AMBIENT_LOWER securebit has been set.

There are at least 2 conditions to return error when setting ambient caps:

  1. The ambient cap sets is not in both permitted and inheritable cap sets;
  2. The PR_CAP_AMBIENT_LOWER securebit has been set.

So, it is hard to know whether there is an error before we are doing the really ambient caps set action.
The easiest way is to ignore all ambient caps errors and output a warning log.

PS: Also found another thing we can do more better: moby/sys#163


The original PR(#4358) description:
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.

@kolyshkin
Copy link
Contributor

To reviewers: as much as I can figure out this is an alternative to (not a carry of) #4358, which basically replaces its commit:
c7d80c1
with the one which applies ambient caps separately, and ignores its error (as the old library used to do).

@lifubang it's not correct to use git cherry-pick -s -x here, because it's not a backport but an alternative (and so the "cherry picked from xxx" will point to non-existent commits. Besides, it changes the dates of the original commits (it's not super important though). In cases like this I use git format-patch and git am rather than cherry-picks to retain the original commits as much as possible.

@kolyshkin
Copy link
Contributor

  1. I like libct/cap: switch to moby/sys/capability, lazy init #4358 because is clearly states which ambient caps could not be applied and why, allowing a user understand what's wrong in container spec and how to fix it. The warning in libct/cap: switch to moby/sys/capability, lazy init #4358 looks like this:

WARN: unable to set Ambient capabilities which are not set in Inheritable; ignoring following Ambient capabilities: [CAP_SYSLOG CAP_AUDIT_WRITE]

Thereas in this PR the warning is like this:

WARN: unable to set Ambient capabilities: permission denied

which is less helpful.

  1. As ambient capabilities are set one by one in Apply, with this PR we may get a situation when some legitimate capabilities won't be applied. libct/cap: switch to moby/sys/capability, lazy init #4358 don't have this issue.

Here's a repro:

@test "runc run [ambient caps not set in inheritable result in a warning]" {
        update_config '   .process.capabilities.inheritable = ["CAP_KILL"]
                        | .process.capabilities.ambient = ["CAP_KILL", "CAP_CHOWN"]'
        runc run test_amb  
        [ "$status" -eq 0 ]
        # This should result in CAP_KILL (0x20) set in ambient.
        # CAP_KILL is 5, the bit mask is 0x20 (1 << 5).
        [[ "${output}" == *"CapAmb:     0000000000000020"* ]]
}

Obviously we can also fix this in moby/sys/capability. Maybe even introduce a "backward compatibility mode" which returns warnings not errors.

OTOH I like this PR's approach for its simplicity.

@kolyshkin
Copy link
Contributor

In any case, a warning should be a temporary measure, and we should switch to an error in (say) runc 1.3.

@lifubang
Copy link
Member Author

lifubang commented Sep 28, 2024

Thereas in this PR the warning is like this:

Yes, it’s helpful, but maybe error in sometimes.
#4358 (comment)

I think maybe it should be the job of ‘ github.com/moby/sys/capabilit’, the ‘Apply’ function should return a detailed error, not only the syscall errorno. I wanted to file a proposal in ‘moby/sys’ when I was writing this PR, but I think that it will changes this API’s return type, it belongs to a break change, I still have no good suggestions.

@lifubang
Copy link
Member Author

2. s ambient capabilities are set one by one in Apply, with this PR we may get a situation when some legitimate capabilities won't be applied.

Indeed.
Please see moby/sys#165

@cyphar cyphar mentioned this pull request Oct 2, 2024
21 tasks
@kolyshkin
Copy link
Contributor

I think maybe it should be the job of ‘ github.com/moby/sys/capabilit’, the ‘Apply’ function should return a detailed error, not only the syscall errorno. I wanted to file a proposal in ‘moby/sys’ when I was writing this PR, but I think that it will changes this API’s return type, it belongs to a break change, I still have no good suggestions.

I guess we can make Apply try to set all capabilities and return a more detailed error message. As long as it's using %w to embed the low-level error, and the users are using errors.Is or the like, we're good to go.

@lifubang lifubang changed the title [Carry #4358] libct/cap: switch to moby/sys/capability, lazy init libct/cap: switch to moby/sys/capability Oct 15, 2024
This is an example to explain how to keep the behavior
or runc after we repalce the package capability.

Signed-off-by: lfbzhm <[email protected]>
@@ -7,6 +7,8 @@ go 1.22
// Note that toolchain does not impose a requirement on other modules using runc.
toolchain go1.22.4

replace github.com/moby/sys/capability v0.3.0 => github.com/lifubang/moby_sys/capability v0.0.0-20241013102214-92ccf7035c8d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which PR? This?

moby/sys#165

Comment on lines -40 to +44
list := capability.List()
list, err := capability.ListSupported()
if err != nil {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not right.

runc features generally show all known features, flags etc., even those not supported by the current kernel, platform etc.

I was thinking about adding --supported flags but never got around to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants