Add PSP support for seccomp profiles #28300

pweil- · 2016-06-30T18:04:57Z

Seccomp support for PSP. There are still a couple of TODOs that need to be fixed but this is passing tests.

One thing of note, since seccomp is all being stored in annotations right now it breaks some of the assumptions we've stated for the provider in terms of mutating the passed in pod. I've put big warning comments around the pieces that do that to make sure it's clear and covered the rollback in admission if the policy fails to validate.

@sttts @pmorie @erictune @smarterclayton @liggitt

This change is

smarterclayton · 2016-06-30T21:35:51Z

pkg/apis/extensions/v1beta1/types.go

+	// SeccompProfiles is a slice of allowed profiles that may be set for the pod or
+	// container's seccomp annotations.  The wildcard '*' may be used to allow all.  When
+	// used to generate a value for a pod the first non-wildcard profile will be used as
+	// the default.


If this is unset, what happens? Describe here

Unset and also empty.

sttts · 2016-07-01T14:25:23Z

With all the annotation trouble. I wonder why we don't add the actual seccomp fields in the internal representation and translate it back to annotations on conversion.

smarterclayton · 2016-07-01T16:18:07Z

That works (we do it for init container) but it will break apply.

On Jul 1, 2016, at 7:25 AM, Dr. Stefan Schimanski notifications@github.com
wrote:

With all the annotation trouble. I wonder why we don't add the actual
seccomp fields in the internal representation and translate it back to
annotations on conversion.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#28300 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_aMMzJM4wKNh9lWOnedt826NZZIks5qRSNhgaJpZM4JCbSe
.

sttts · 2016-07-04T06:57:37Z

@smarterclayton which apply? kubectl-apply? Do you have a hint why it breaks?

pweil- · 2016-07-05T12:58:41Z

@sttts - are you going to change the internal type? If so I'll hold off on changes until I can rebase on that. I'd like to get this finished up this week and back ported to Origin, so that's what I'm shooting for.

sttts · 2016-07-05T13:00:46Z

@pweil- don't worry. just go ahead. I will adapt my code when your PR goes into master.

smarterclayton · 2016-07-05T15:59:19Z

kubectl-apply - it breaks because roundtripping the annotations through marshal and unmarshal loses the original formatting in the annotation (user provides { "some": "key" } and gets back {"some":"key"}, which means apply thinks the value has changed. There's really no way to preserve that (we don't know if the struct internally has changed).

I'm making that argument that for an alpha feature that's acceptable (users can work around it) and the problem is resolved once the feature is out of annotations.

pweil- · 2016-07-05T18:27:37Z

@sttts @smarterclayton comments addressed in code or responses here.

@sttts I spent some time rewriting the strategy since, as you pointed out, it was a little confusing to read. Let me know if you think the new code is easier to follow. It also now supports a missing pod level annotation so long as all containers have their own annotation that is valid.

pweil- · 2016-08-23T17:21:58Z

@sttts @timstclair - updated to better match the app armor implementation. There were two places I deviated that I want to call out:

I add a specific wild card for allowing any profile rather than having empty mean all are allowed
In my strategy signature I think it's easier to just generate a string and let the provider worry about applying it to the annotations. It avoids the extra copy as well.

Let me know what you think or if I've forgotten anything we talked about.

timstclair · 2016-08-23T18:40:29Z

pkg/security/podsecuritypolicy/seccomp/strategy.go

+// NewStrategy creates a new strategy that enforces seccomp profile constraints.
+func NewStrategy(pspAnnotations map[string]string) Strategy {
+	var allowedProfiles map[string]bool
+	allowAnyProfile := false


So by default, no seccomp is disabled? Is that the desired behavior? I would expect lack of AllowedProfilesAnnotationKey to imply AllowAny (that's how it works with AppArmor).

I was thinking about that today and I think it breaks the upgrade experience. Lacking the key, if we default to all enabled, we're running in a less restricted mode than we were previously. If we imply that it is 'allow none' you can safely upgrade to the new behavior without having to make changes to your api objects.

WDYT?

Yeah, that makes sense.

k8s-bot · 2016-09-12T09:12:11Z

GCE e2e build/test passed for commit faa4f4bc7078c9b6e14f65dcb726b186f2149ffe.

pweil- · 2016-10-17T19:14:05Z

@sttts @timstclair any other remaining items here?

timstclair

LGTM

sttts · 2016-10-18T08:38:49Z

lgtm

k8s-github-robot · 2016-10-18T15:45:17Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2016-10-18T16:04:02Z

Jenkins GCI GKE smoke e2e failed for commit 49e1474. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci 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 · 2016-10-18T16:26:09Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jun 30, 2016

k8s-github-robot assigned smarterclayton Jun 30, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Jun 30, 2016

smarterclayton reviewed Jun 30, 2016
View reviewed changes

pweil- force-pushed the psp-seccomp branch from b0047fa to 06d1811 Compare July 5, 2016 18:24

pweil- mentioned this pull request Jul 6, 2016

DO NOT MERGE SCC seccomp openshift/kubernetes#22

Closed

k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 23, 2016

timstclair reviewed Aug 23, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2016

pweil- force-pushed the psp-seccomp branch 2 times, most recently from faa4f4b to 555aa93 Compare September 12, 2016 08:31

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2016

support seccomp in psp

49e1474

pweil- force-pushed the psp-seccomp branch from 555aa93 to 49e1474 Compare October 17, 2016 19:12

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2016

timstclair approved these changes Oct 17, 2016

View reviewed changes

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2016

k8s-github-robot merged commit c592a46 into kubernetes:master Oct 18, 2016

timstclair mentioned this pull request Dec 12, 2016

Default seccomp profile cannot be configured without hardcoding it in Kubelet #34649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PSP support for seccomp profiles #28300

Add PSP support for seccomp profiles #28300

pweil- commented Jun 30, 2016 •

edited by k8s-oncall

Loading

smarterclayton Jun 30, 2016

sttts Jul 1, 2016

sttts commented Jul 1, 2016

smarterclayton commented Jul 1, 2016

sttts commented Jul 4, 2016

pweil- commented Jul 5, 2016

sttts commented Jul 5, 2016

smarterclayton commented Jul 5, 2016

pweil- commented Jul 5, 2016

pweil- commented Aug 23, 2016

timstclair Aug 23, 2016

pweil- Aug 23, 2016

timstclair Aug 23, 2016

k8s-bot commented Sep 12, 2016

pweil- commented Oct 17, 2016

timstclair left a comment

sttts commented Oct 18, 2016

k8s-github-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-github-robot commented Oct 18, 2016

Add PSP support for seccomp profiles #28300

Add PSP support for seccomp profiles #28300

Conversation

pweil- commented Jun 30, 2016 • edited by k8s-oncall Loading

smarterclayton Jun 30, 2016

Choose a reason for hiding this comment

sttts Jul 1, 2016

Choose a reason for hiding this comment

sttts commented Jul 1, 2016

smarterclayton commented Jul 1, 2016

sttts commented Jul 4, 2016

pweil- commented Jul 5, 2016

sttts commented Jul 5, 2016

smarterclayton commented Jul 5, 2016

pweil- commented Jul 5, 2016

pweil- commented Aug 23, 2016

timstclair Aug 23, 2016

Choose a reason for hiding this comment

pweil- Aug 23, 2016

Choose a reason for hiding this comment

timstclair Aug 23, 2016

Choose a reason for hiding this comment

k8s-bot commented Sep 12, 2016

pweil- commented Oct 17, 2016

timstclair left a comment

Choose a reason for hiding this comment

sttts commented Oct 18, 2016

k8s-github-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-github-robot commented Oct 18, 2016

pweil- commented Jun 30, 2016 •

edited by k8s-oncall

Loading