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

CRI: add seccomp profile root path to runtime #38971

Closed
wants to merge 3 commits into from

Conversation

feiskyer
Copy link
Member

This PR adds a new security.alpha.kubernetes.io/seccomp/root annotations to sandboxConfig to pass seccomp profile root path to runtime.

Fixes #36997.

cc @yujuhong @euank

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@feiskyer
Copy link
Member Author

cc @kubernetes/sig-node-misc

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Dec 19, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 32de1fb. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

@feiskyer PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2016
@euank
Copy link
Contributor

euank commented Dec 22, 2016

I did a little more investigation, and as far as I can tell a seccomp root and the associated file structure is a docker specific (invented here) concept.

If we wanted to make this container runtime agnostic, the Kubelet could convert from that json to protobuf structs which clearly specify the exact details.

I think this is attractive for a number of reasons. Most importantly, the current specification is relying on incidental communication of state via the filesystem (passing references to files over an API is basically always an anti-pattern).

There are also things, such as no-new-privileges, which interact in some way with certain seccomp options, and so it makes sense for the kubelet to resolve a seccomp specification fully itself prior to reaching out to the runtime so that the kubelet may make any additional decisions that must be made.

That would also reduce the number of annotations that have magic powers, which is always a good thing in my book.

cc @lucab @jonboulle

@resouer resouer requested a review from vishh December 22, 2016 01:41
@feiskyer
Copy link
Member Author

feiskyer commented Dec 22, 2016

If we wanted to make this container runtime agnostic, the Kubelet could convert from that json to protobuf structs which clearly specify the exact details.

Yes, I'm planning to add another issue to focus on the format of seccomp profile (in both kubernetes api and kubelet CRI). But it is not ready yet.

There are also things, such as no-new-privileges, which interact in some way with certain seccomp options, and so it makes sense for the kubelet to resolve a seccomp specification fully itself prior to reaching out to the runtime so that the kubelet may make any additional decisions that must be made.

If so, we need to spec the seccomp profile format first. (opened #39128 for this)

@yujuhong yujuhong assigned timstclair and unassigned vishh Jan 4, 2017
@timstclair
Copy link

Can you explain the motivation for this? There should be a single seccomp root path for the host, so this seems like a value that should be configured in the shim (e.g. flag), as opposed to a per-pod annotation.

@euank - I agree with you, but that's also not how seccomp works today. I'd like to be able to source seccomp (and AppArmor) profiles from ConfigMaps, and maybe at that point we could deprecate local profiles.

@feiskyer
Copy link
Member Author

feiskyer commented Jan 5, 2017

@timstclair This is a try to fix #36997.

I'd like to be able to source seccomp (and AppArmor) profiles from ConfigMaps, and maybe at that point we could deprecate local profiles.

If we have such plan, then we should pass full profile instead of path to runtimes.

@feiskyer
Copy link
Member Author

feiskyer commented Jan 5, 2017

Let's close this and move to #36997 to discuss more.

@feiskyer feiskyer closed this Jan 5, 2017
@feiskyer feiskyer deleted the seccomp-path branch June 9, 2017 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants