capabilities: be more graceful in resetting ambient

[evanphx]

Similar to when SetAmbient() can fail, runc should be graceful about ResetAmbient failing.

This functionality previously worked under gvisor, which doesn't implement ambient capabilities atm. The hard error on reset broke gvisor usage.

[cyphar]

I'm not sure we can safely ignore an error from dropping caps -- in theory you shouldn't get an error in practice, but if we did fail to clear the caps then running the container at that point seems unsafe (ambient caps not being dropped properly was the cause of some very old Docker CVEs IIRC).

[evanphx]

Totally understand and I'm going to work on this on the gvisor side. If you don't want to accept this, totally fine. My thinking was that if we were allowing SetAmbient through, we should allow ResetAmbient through too.

[kolyshkin]

The reason why we warn rather than error out on SetAmbient is explained in the code, and the cause is backward compatibility concerns (old gocapability package used to ignore any errors). We didn't have such thing with ResetAmbient (in fact, we did not use ResetAmbient until recently).

But I wonder which error are you getting from gvisor, @evanphx? If that's EINVAL, it should actually be ignored as kernels older than 4.3 did not support ambient capabilities and thus EINVAL should be treated as "ambient caps are not supported, we warn about it but continue".

[evanphx]

It is EINVAL, yeah. Looks like the ResetAmbient code path doesn't accommodate any errors currently, so I'd presume that pre 4.3 is broken atm too.

[kolyshkin]

It is EINVAL, yeah. Looks like the ResetAmbient code path doesn't accommodate any errors currently, so I'd presume that pre 4.3 is broken atm too.

Can you change this to warn (or ignore) on EINVAL only? I think such change belongs here (rather than moby/sys/capability) as in there it is a new API and I'd like it to be a tad lower-level (so a user can actually see ambient is not supported).

[evanphx]

No problem!

[evanphx]

@kolyshkin Went ahead and didn't warn on EINVAL for now, especially because the code below will warn when it tries to actually set up the specific caps anyway.

[kolyshkin]

Just thought we can skip setting Ambient entirely upon getting EINVAL from ResetAmbient, as this can only mean one thing -- ambient caps are not supported.

OTOH warnings from SetAmbient might be nice to have, so maybe it's fine as it is.

[kolyshkin]

@opencontainers/runc-maintainers PTAL