-
Notifications
You must be signed in to change notification settings - Fork 229
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
Add capabilities check #104
Conversation
dffa4da
to
900bc2b
Compare
900bc2b
to
d3687b4
Compare
internal/templates/containercapabilities/internal/params/params.go
Outdated
Show resolved
Hide resolved
b48c3bd
to
d579041
Compare
|
||
// Any capability from scCaps should not match with any from paramCaps | ||
for _, paramCapMatcher := range paramCapMatchers { | ||
for _, scCap := range scCaps.Add { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the tricky thing here is that containers can have some capabilities implicitly, through defaults. This means we should either:
- Use the default list of defaults from here, and assume they are in the container.
- Be extra-restrictive, and say that to forbid a capability, you must drop it explicitly. (This is the same as assuming that containers have all capabilities by default.)
As written, the NET_RAW
check will not fire by default on an empty capabilities list. But it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will not fire on checking ADD list, but it still fires on checking DROP list right?
var found bool | ||
for _, scCap := range scCaps.Drop { | ||
// User can specify to drop "all" under containers as well | ||
if paramCapMatcher(string(scCap)) || literalReservedCapabilitiesAllMatcher(string(scCap)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I think we should just compute the effective list of capabilities by looking at the "add" and "drop", and pass those into this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I thought about it as well, but that requires keeping a base list that is consistent with the default caps, which I was not sure if it changes or not.
The cleanest way of doing this would be:
- 1: find a base list
- 2: perform add and drop
- 3: validate against the user specified params.
However, since we don't have that base list, and not sure how safe it is to just hard code one (not only this can change, this has to also work with different versions of kubernetes server), so instead I chose to check it by making sure unwanted caps are:
- 1: not found in ADD list since this overwrites even if it is included in DROP list
- 2: dropped in DROP list
d579041
to
65b0b1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This resolves #32