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

Add ConfigMap support for seccomp custom profiles #1269

Closed
wants to merge 12 commits into from
Closed

Add ConfigMap support for seccomp custom profiles #1269

wants to merge 12 commits into from

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Oct 2, 2019

This proposal focuses on allowing users to load user-defined seccomp profiles from ConfigMap objects instead of loading them from physical files placed on nodes.

Feature: #135

For context:
kubernetes/kubernetes#52827
kubernetes/kubernetes#20870

/sig-node
/sig-auth

/priority important-longterm

/assign @tallclair
/cc @jessfraz

@k8s-ci-robot k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Oct 2, 2019
@k8s-ci-robot k8s-ci-robot requested a review from jessfraz October 2, 2019 08:05
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Oct 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pjbgf
To complete the pull request process, please assign dchen1107
You can assign the PR to them by writing /assign @dchen1107 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dchen1107 dchen1107 added this to the v1.18 milestone Oct 15, 2019
@dchen1107
Copy link
Member

We discussed this proposal at today's SIG Node meeting. It is a usability improve for seccomp feature and would like to bundle it together with seccomp GA in v1.18 even there is no strict dependencies.

@tallclair
Copy link
Member

I haven't had a chance to look through the details yet, but I think it would be helpful to add a section (maybe under alternatives) justifying the use of a ConfigMap over a CRD. There's a good list of when each is appropriate here: https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/#should-i-use-a-configmap-or-a-custom-resource, and so noting which of those bullets apply in this situation would be helpful.

@pjbgf
Copy link
Member Author

pjbgf commented Oct 17, 2019

@tallclair I have added the reasoning for choosing ConfigMap vs CRDs. PTAL

@pjbgf
Copy link
Member Author

pjbgf commented Oct 29, 2019

@tallclair PTAL

@tallclair tallclair mentioned this pull request Jan 9, 2020
22 tasks
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 27, 2020
@palnabarun
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 27, 2020
@palnabarun
Copy link
Member

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.18 milestone Mar 6, 2020
@evrardjp
Copy link

evrardjp commented Apr 21, 2020

I am on the fence. This proposal is indeed more convient, and simple for users. However, I have to admit that passing big blobs to container runtimes for each container doesn't sound right to me.

The file based approach is simple, and just require configuration management.
I am pretty sure a few of those already exist, so that we won't need to do these changes in the container runtimes :p

Replace OCI with CRI

Co-Authored-By: Sascha Grunert <sgrunert@suse.com>
@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Apr 24, 2020
@pjbgf
Copy link
Member Author

pjbgf commented Apr 24, 2020

@saschagrunert @evrardjp thank you for taking the time reviewing the KEP.

I don't mind changing the direction of this KEP to go for a file-based approach (which is already implemented and used by "localhost/" profiles). But given that seccomp is a security feature, I believe we should mitigate (or have some protection against) "profile race tampering".

In a file-based approach, between the creation of the file (by the kubelet, based off the ConfigMap/CRD) and its consumption (by the container runtime), the profile can be tampered with effectively disabling its protections. The end-users would be oblivious to this, as the containers would still run. It may be even harder to diagnose if it happens on a single node within their cluster.

At present there is no such protection, but I believe this would be a valid "requirement" for GA.
Implementing a tamper-resistant file based approach, could require further changes to the CRI (and the container run-times), as they would have to receive and validate the payload hashes.

On the other hand, passing the entire profile to the CRI removes such need. As a level of "tamper resistance" could be obtained at transport level, which is something important between the kubelet and CRI regardless of this feature and therefore would be off-scope of this KEP.

The "least-effort" compromise could be to go for the file-based approach, using the existing infrastructure and use file permissions to protect against this. Having this well-documented and working together with kubeadmin/kops/kubespray to provide the community with an automated way to ensure file permissions are up to par. However, that feels to me like the weakest link in the seccomp chain implementation.

Please do share your thoughts and potential workarounds. I am quite keen to get this progressed and pushed over the line sooner rather than later.

@saschagrunert
Copy link
Member

@saschagrunert @evrardjp thank you for taking the time reviewing the KEP.

I don't mind changing the direction of this KEP to go for a file-based approach (which is already implemented and used by "localhost/" profiles). But given that seccomp is a security feature, I believe we should mitigate (or have some protection against) "profile race tampering".

I acknowledge your arguments and the only issue I see is answering the question on: How much information we want to transfer via the CRI? We right now have a state in the CRI where some information are transferred directly and others via local paths. If we say we generally want to move away from referencing local paths then I'm totally fine with going your proposed way.


Another more general point: We'd like to support you with your efforts to make Kubernetes more easily secure-able. With taking the seccomp GA KEP as well as the new proposed release cycle into account, do you think we could bring this feature with 1.19 (together)?

We're planning to work in parallel on a more high level seccomp-operator in a separate repository. The idea is to provide a much higher level abstraction for seccomp (via the CRD) and sync the state into either files (like now) or config-maps (like your proposed KEP). What do you think about that? I'd love to see us working together on the overall story.

(Later on we could promote your KEP to GA by adding the CRD from the seccomp-operator project, but this will be a tough run. Shipping a CRD via a feature gate is right now an unresolved question in Kubernetes.)

@pjbgf
Copy link
Member Author

pjbgf commented Apr 24, 2020

Another more general point: We'd like to support you with your efforts to make Kubernetes more easily secure-able. With taking the seccomp GA KEP as well as the new proposed release cycle into account, do you think we could bring this feature with 1.19 (together)?

That's brilliant, I do think we can deliver this for 1.19, probably with a few small changes (i.e. use file-based profiles). The tamper-resistance can be a KEP on its own to consider Seccomp Profiles and AppArmor, which would consider the CRI impact.

The seccomp operator sounds like a great idea and I would be keen to part take. Around the subject, but not as comprehensive as the operator, I got another KEP open that would bring some Kubernetes native profiles types. And on a side note, I have a pet project that auto-generates seccomp profiles reasonably for some workloads. Putting this all together may result in a better end-user experience.

I think it may be worth having a catch-up to consider the "big-picture" and decide what pieces of the puzzle we shall focus first to achieve that. From mid-next week I will have plenty of extra time to focus on this. I will reach you out on slack.

@saschagrunert
Copy link
Member

saschagrunert commented Apr 24, 2020

The seccomp operator sounds like a great idea and I would be keen to part take. Around the subject, but not as comprehensive as the operator, I got another KEP open that would bring some Kubernetes native profiles types. And on a side note, I have a pet project that auto-generates seccomp profiles reasonably for some workloads. Putting this all together may result in a better end-user experience.

That sounds great, seccomp profile recording via OCI hooks is mentioned in the RFC of the operator too. I think this would be a very valuable addition in the future. 👍

I think it may be worth having a catch-up to consider the "big-picture" and decide what pieces of the puzzle we shall focus first to achieve that. From mid-next week I will have plenty of extra time to focus on this. I will reach you out on slack.

Yes, I setup a scheduled Zoom meeting for the whole topic, everyone if free to join:

Time: Apr 30, 2020 02:00 PM Amsterdam, Berlin, Rome, Stockholm, Vienna
Meeting: https://zoom.us/j/91379118387

@pjbgf
Copy link
Member Author

pjbgf commented Apr 29, 2020

Yes, I setup a scheduled Zoom meeting for the whole topic, everyone if free to join:
Time: Apr 30, 2020 02:00 PM Amsterdam, Berlin, Rome, Stockholm, Vienna
Meeting: https://zoom.us/j/91379118387

cc @tallclair

@pjbgf
Copy link
Member Author

pjbgf commented Apr 29, 2020

@saschagrunert I created a google doc to set an initial agenda. In it I also added a mind-map for me to talk through my understanding of the current state/outstanding work. I added a point for you to talk a bit about the seccomp-operator, feel free to add more stuff to it.

@pjbgf
Copy link
Member Author

pjbgf commented May 6, 2020

This KEP is being withdrawn in favour of the seccomp-operator, which will eventually bring similar features out of tree.

@pjbgf pjbgf closed this May 6, 2020
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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants