capability: can't raise ambient and drop bounding caps for other process

[lifubang]

There may be two ways to fix #168:

1. Return an error when pid is not 0;
2. Do raise and drop operation when pid is 0.

I think we should choose the second way, because besides the bug fix,
we can also have a compatibility with before.

[kolyshkin]

There may be two ways to fix #168:

1. Return an error when pid is not 0;
2. Do raise and drop operation when pid is 0.

I think we should choose the second way, because besides the bug fix, we can also have a compatibility with before.

There is a compatibility issue either way. With the second way, the behavior of this package has changed:

• it used to ignore the pid argument and set capabilities for the current process;
• now it doesn't do that.

So, for some code which does, for example, NewPid(os.Getpid()), this will be a breaking change. What's worse, they won't know about it, as no error is returned.

So I think the first way is better. If someone is doing something wrong, it's better to return an error so they know they are doing it wrong, rather than silently stop doing something.

[lifubang]

So I think the first way is better. If someone is doing something wrong, it's better to return an error so they know they are doing it wrong, rather than silently stop doing something.

Agree, I have changed to return an error.

[kolyshkin]

Do we need these errors as part of a public API? If yes, why?

[lifubang]

Maybe someone use it to check the error type, like what to do in the test?
But maybe no, so I'll change it.

[kolyshkin]

Maybe someone use it to check the error type, like what to do in the test?

For the test case, you can add export_test.go file having something like this:

package capability

// Make some internal details available for tests.

var ErrBoundingNotMine = errBoundingNotMine

[kolyshkin]

I think this should be

Suggested change

	cmd := exec.Command("sleep", "sleep", "infinity")
	cmd := exec.Command("sleep", "infinity")

[kolyshkin]

The error went unnoticed because this test is not being run, because the previous test (TestAmbientCapSet) removes CAP_SET_PCAP from the test process. Oh well

[kolyshkin]

See here

[kolyshkin]

We have a big problem

=== RUN   TestAmbientCapSet
--- PASS: TestAmbientCapSet (0.00s)
=== RUN   TestApplyCapsForOtherProcess
    capability_test.go:159: The test needs `CAP_SETPCAP`.
--- SKIP: TestApplyCapsForOtherProcess (0.00s)

TestAmbientCapSet removes some capabilities from the binary, and thus all other tests that require CAP_SETPCAP (or root) won't work.

Glad we found it now before it's too late.

The solution here is to spawn such test cases in a separate binary, so it won't affect others.

I will come up with a patch.

[kolyshkin]

Suggested change

	errBoundingNotMine = errors.New("not support drop bounding cap of other process")
	errAmbientNotMine = errors.New("not support modify ambient cap of other process")
	errBoundingNotMine = errors.New("unable to modify bounding capabilities of another process")
	errAmbientNotMine = errors.New("unable to modify ambient capabilities of another process")

[kolyshkin]

Suggested change

	t.Fatalf("expected not support error when drop bounding caps for other process, but got: %v", err)
	t.Fatalf("want error %v, got %v", errBoundingNotMine, err)

[kolyshkin]

Suggested change

	t.Fatalf("expected not support error when rasing ambient caps for other process, but got: %v", err)
	t.Fatalf("want error %v, got %v", errAmbientNotMine, err)

[kolyshkin]

I think I have added this comment already but it looks like it is lost.

I deliberately changed this in commit 7208f83 so these tests can change public API only, and I don't like changing it back.

You have a few options here:

1. Just check for an error.
2. export errors (as described in capability: can't raise ambient and drop bounding caps for other process #171 (comment)).
3. create a different test file which will have package capability and thus would be able to test and access internals.

I would prefer way 2.

[kolyshkin]

I will come up with a patch.

See #173.

[kolyshkin]

Closing in favor of #174. @lifubang please help reviewing that one.