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

allow setting container capabilities in challenge.yaml #301

Merged
merged 4 commits into from
Jun 14, 2021

Conversation

sroettger
Copy link
Collaborator

We were always overwriting the capabilities of the challenge container so it was not possible to add custom caps.
Change this by checking for existence before overwriting.

Note that I'm not merging the Capabilities.Add list, instead the challenge author will have to add SYS_ADMIN manually if they want to change it.

@sirdarckcat
Copy link
Member

This probably needs documentation (that cap_sys_admin is required by the template nsjail and not enforced by the operator) and some testing for troubleshooting. Specially the security consequences of it.

Also, this is more of a feature, so maybe better to push on v1.1? Unless we consider the old behavior a bug.

@sroettger
Copy link
Collaborator Author

It's really just a bug fix, we just ignored the value in the template before.
Also writing documentation for this seems overkill since I don't expect this feature to be used much and the error when you don't have sys_admin should be pretty straightforward to figure out. (mount will fail with EPERM)

@sirdarckcat
Copy link
Member

It does look easy to troubleshoot https://www.google.com/search?q=mount+eperm - but it is surprising that adding a capability actually removes a capability.

Given that the podTemplate behavior has other cases like these (setting cpu limits is enforced to the config and volume mounts appends). Before we could say that in order to ensure compatibility we always enforce configurations that work for limits, mounts and capabilities, but now we can't say that (some times we overwrite them, sometimes we let them overwrite the configuration).

It would be nice to add a comment here:

// PodTemplate is used to set the template for the deployment's pod,
// so that an author can add volumeMounts and other extra features
PodTemplate *corev1.PodTemplate `json:"podTemplate"`

So at least it makes sense why we are inconsistent

@sroettger
Copy link
Collaborator Author

setting cpu limits is enforced to the config
but that's just another bug. Why would we force CPU limits, we have no idea about the requirements of the challenge

@sirdarckcat
Copy link
Member

Hmm yea, I don't remember why we set them to 0.9/0.45 - looks like it was meant to be for the template task, but we ended up putting it on the base, which then made it all the way to the operator.

3ede18e
https://github.com/google/kctf/blob/alpha/base/k8s/deployment/containers.yaml#L35

Appending looks good though, maybe we should just delete the lines that set the limits, but that can be done separately.

@sroettger
Copy link
Collaborator Author

at first I didn't append since I thought it would break if you append the same capability twice. But just tried it out and seems to work fine. So this should be what users would expect it to do.

@sroettger sroettger merged commit 3b162f4 into v1.0 Jun 14, 2021
@sroettger sroettger deleted the custom_capabilities branch June 14, 2021 13:01
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.

2 participants