This repository was archived by the owner on Mar 9, 2022. It is now read-only.
Adds seccomp support #219
Merged
Adds seccomp support #219
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 tasks
Member
Author
|
Generated an issue to track work on a docker/default seccomp helper needed in containerd/containerd |
Member
Author
|
Generated a PR over in containerd to provide the helpers we need: containerd/containerd#1493 |
Member
Author
|
rebased to pick up the build fix and new seccomp helpers from containerd. |
Member
Author
|
modified vendor commit to pick up the new critools seccomp test bucket |
Member
Author
|
@Random-Liu ok 137 - 128 maps to SIGKILL |
Member
Author
|
@Random-Liu ok I have one fix to push to the containerd/containerd code I was checking for runtime.GOARCH == "amd" not "amd64" so I didn't pick up the default arch_prctl permission. See: containerd/containerd#1532 |
Member
Author
|
@Random-Liu and the other issue is docker supported name: or names: where the oci spec expects names: for the permission white and black lists... I'll fix both.. |
Member
Author
|
Ok, with the two PRs list above (one for critools, and one for containerd/containerd) we will then pass all tests. As soon as those two PRs are committed I'll rebase this PR. |
Member
|
Cool! Thanks~
…On Sep 20, 2017 11:31 AM, "Mike Brown" ***@***.***> wrote:
Ok, with the two PRs list above (one for critools, and one for
containerd/containerd) we will then pass all tests. As soon as those two
PRs are committed I'll rebase this PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFjVu6GQihlB-rM9u7YYPegyBsMrExf7ks5skVoLgaJpZM4POXkD>
.
|
Member
Author
|
rebased.. @Random-Liu fyi containerd guys already merged containerd/containerd#1532 so just need your review and merge on the critools fix.. then I can vendor the fixes and we'll be all good. |
Member
|
@mikebrow I tried this PR with the newest seccomp test. Some tests are failing. |
Member
Author
|
Did you vendor over the fixes mentioned above to containerd? Those with routines get compiled into our code. I was going to vendor in the morning. |
Member
Author
|
rebased again... now need to vendor in the dependencies |
Member
Author
|
Ok finished vendoring and it looks good.. pushing the vendor updates.. |
Member
|
@mikebrow That may be the cause. I didn't update vendor thing. :) Will review this now. Thanks! |
Random-Liu
reviewed
Sep 21, 2017
| # FOCUS focuses the test to run. | ||
| FOCUS=${FOCUS:-} | ||
| # SKIP skips the test to skip. | ||
| SKIP=${SKIP:-"SeccompProfilePath"} |
pkg/server/container_create.go
Outdated
| // runtimeDefault indicates that we should use or create a runtime default apparmor profile. | ||
| // runtimeDefault indicates that we should use or create a runtime default profile. | ||
| runtimeDefault = "runtime/default" | ||
| // runtimeDefault indicates that we should use or create a docker default profile. |
pkg/server/container_create.go
Outdated
| // unconfinedProfile is a string indicating one should run a pod/containerd without a security profile | ||
| unconfinedProfile = "unconfined" | ||
| // seccompDefaultPodProfile is the default seccomp profile for pods. | ||
| seccompDefaultSandboxProfile = unconfinedProfile |
Member
There was a problem hiding this comment.
Why do we use different default for sandbox container? Do we have different default in dockershim? I don't think so.
Member
Author
There was a problem hiding this comment.
Cause it might be a different, don't see tie between pod and container here as is done with privileged. We should take this up with the overall seccomp CRI requirements.
Member
There was a problem hiding this comment.
I'm fine with have one default variable for container and one for sandbox.
However, both of them should be docker/default now. Currently, in your PR docker/default or runtime/default for sandbox will be unconfined, :)
| dockerDefault = "docker/default" | ||
| // appArmorDefaultProfileName is name to use when creating a default apparmor profile. | ||
| appArmorDefaultProfileName = "cri-containerd.apparmor.d" | ||
| // unconfinedProfile is a string indicating one should run a pod/containerd without a security profile |
Member
There was a problem hiding this comment.
nit: Change pod to sandbox? Since we are using sandbox in the variable name.
| } | ||
| } | ||
|
|
||
| // Set seccomp profile |
Member
There was a problem hiding this comment.
We should probably move this into a separate function, since the logic is the same with sandbox. I'm fine with doing that in another PR, up to you. :)
Member
Author
There was a problem hiding this comment.
yeah it's almost the same.. I figured there may be a reason for the logic to split but it didn't so far.
Member
|
Closes #250 |
Member
|
LGTM except: |
Signed-off-by: Mike Brown <[email protected]>
Signed-off-by: Mike Brown <[email protected]>
Member
Author
|
Good point @Random-Liu I changed pod's runtime and docker/default to docker/default as well. |
Member
|
LGTM |
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Generates runtime spec linux seccomp profile for Sandboxes and Containers with the following rules:
unset the seccomp profile, if seccomp is not enabled, or the security context is privileged, or the profile selected is unconfined.
with a future pr we'll allow a custom default profiles for cri-containerd, with this pr the "runtime/default" or "docker/default" is set to "unconfined" for pods and the docker/default for containers
if the seccomp profile is set to localhost/profileFileName, load and unmarshall the seccomp spec from the profileFileName
Signed-off-by: Mike Brown [email protected]