capability defaulting #6470

pweil- · 2015-12-22T18:27:10Z

@smarterclayton @eparis - here is the WIP for adding required add/drops into SCCs. PTAL

The mechanics are:

You may define requiredCapabilities in the SCC which can include adds and drops
Defaulting will produce
Adds: all required caps + container requested adds - container requested drops (you can drop required adds if you want, just means your container can do less)
Drops: all required drops + container requested drops
Validation will ensure
Adds: all adds must be in either the required or allowed add lists of the SCC
Drops: all required drops are in the drop set

smarterclayton · 2015-12-22T19:23:04Z

Do we need to be smart and not add defaults when they are already dropped (by the container or scc)?

pweil- · 2015-12-22T19:39:57Z

Do we need to be smart and not add defaults when they are already dropped (by the container or scc)?

Since I'm using a string set and using the union it will not duplicate the adds. If the container is dropping something that is added by default it will be removed by the Difference call here: https://github.com/openshift/origin/pull/6470/files#diff-c7bf63e9488690fe18014ead17dbdd2cR61

smarterclayton · 2015-12-24T02:13:58Z

[test]

smarterclayton · 2016-01-02T22:57:08Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/v1/types.go

+	// the containers.  Any capability listed in add will be added to the container even if
+	// it is not requested.  Any capability listed in drop will be removed from the container
+	// even if it is requested to be added.
+	RequiredCapabilities Capabilities `json:"requiredCapabilities" description:"capabilities that must be added and dropped"`


added or dropped

jwhonce · 2016-01-05T15:49:50Z

@twiest FYI

smarterclayton · 2016-01-08T20:09:33Z

One more question. An admin wants to run a pod. They have access to various high privilige sccs. When they run a pod that wants to ping, will they get defaulted to drop raw net caps?

pweil- · 2016-01-11T19:52:04Z

One more question. An admin wants to run a pod. They have access to various high privilige sccs. When they run a pod that wants to ping, will they get defaulted to drop raw net caps?

In this case, the assumption would be that the admin has access to an SCC that does not require the net caps to be dropped. The admin should use prioritization to ensure that they get the defaults they expect when not requesting specific capabilities that would force validation failures (similar to requesting UID 0 before we could prioritize the anyuid SCC).

smarterclayton · 2016-01-11T20:15:58Z

Ok - those changes will be follow up / split PR?

On Mon, Jan 11, 2016 at 2:52 PM, Paul Weil notifications@github.com wrote:

One more question. An admin wants to run a pod. They have access to
various high privilige sccs. When they run a pod that wants to ping, will
they get defaulted to drop raw net caps?

In this case, the assumption would be that the admin has access to an SCC
that does not require the net caps to be dropped. The admin should use
prioritization to ensure that they get the defaults they expect when not
requesting specific capabilities that would force validation failures
(similar to requesting UID 0 before we could prioritize the anyuid SCC).

—
Reply to this email directly or view it on GitHub
#6470 (comment).

pweil- · 2016-01-11T20:25:25Z

Ok - those changes will be follow up / split PR?

Yes, wrapping up unit tests right now on this one for the mechanics and then we can do a follow up that will add in the default sets of caps to our bootstrap policies where appropriate.

pweil- · 2016-01-11T22:35:32Z

Updated with more tests and cleaned up comments. Ready for review.

@pmorie @liggitt - could I get some eyes on this one when you get a chance?

smarterclayton · 2016-01-11T23:45:16Z

Changes LGTM but want another sign off.

pmorie · 2016-01-13T21:05:12Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/v1/types.go

@@ -2597,7 +2597,19 @@ type SecurityContextConstraints struct {

 	// AllowPrivilegedContainer determines if a container can request to be run as privileged.
 	AllowPrivilegedContainer bool `json:"allowPrivilegedContainer" description:"allow containers to run as privileged"`
+	// Required capabilities are the capability set that must be set or defaulted into


How about RequiredCapabilities are defaulted into containers. ?

pweil- · 2016-01-13T21:33:12Z

@pmorie @liggitt @deads2k - any other comments on the mechanics here? I've run through my tests and it appears to be defaulting and restricting correctly. We can iterate on the actual values of the defaults separately (they are in #6627)

pweil- · 2016-01-13T22:02:18Z

[test]

pmorie · 2016-01-13T22:09:43Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/validation/validation_test.go

+	allowedCapListedInRequiredDrop.RequiredCapabilities = api.Capabilities{
+		Drop: []api.Capability{"foo"},
+	}
+	allowedCapListedInRequiredDrop.AllowedCapabilities = []api.Capability{"foo"}


what about a test case where a capability is in both add, drop, and allowed?

the validation doesn't care about that. It tests required.Add -> required.Drop and Allowed -> required.Drop. It is valid to have it in allowed and required.Add though I don't see why someone would do that.

Okay, I'll buy that for now. If people find it incredibly confusing we can add a validation later.

can we add a validation later if that would invalidate existing sccs?

I don't think we need validation for this but if someone really wants it I can add it. If you put something in both allowed and required.add it is just redundant but it isn't invalid. It is invalid to have something in (allowed or required.add) AND required.drop which is what we're validating.

pweil- · 2016-01-14T00:37:46Z

re[test]

smarterclayton · 2016-01-14T03:21:41Z

Any other comments on this?

On Wed, Jan 13, 2016 at 8:40 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8540/
)

—
Reply to this email directly or view it on GitHub
#6470 (comment).

soltysh · 2016-01-14T14:05:09Z

/sub

liggitt · 2016-01-14T20:03:30Z

pkg/security/admission/admission_test.go

+		},
+		// UC 3: if an SCC requires a cap then it should accept the pod request
+		// to add the cap.
+		"should accept cap add when in requied": {


pweil- · 2016-01-14T22:30:23Z

Updates:

Rename to InitialCapabilities
Rename of strategy
Added back case sensitivity - this seems the safest route because these are container runtime specific values
Added tests for case sensitivity for add/drop generation and validation
Added tests to ensure deduping of matched case items during generation of adds and drops
Fixed typos

@liggitt @pmorie

pweil- · 2016-01-14T23:39:33Z

re[test]

liggitt · 2016-01-15T14:10:25Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/v1/types.go

+	// 3.  Capabilities in InitialCapabilities.Drop will be dropped from the container
+	// 4.  Containers that request to add a capability that is required to be dropped will fail
+	//		validation.
+	InitialCapabilities Capabilities `json:"initialCapabilities" description:"capabilities that will be added or dropped"`


This name makes it seem like capabilities can change over time. "initial" needs a time context to be understood, and it's a little odd because the sequence is: 1) pod spec submitted to the API may specify capabilities, 2) admission drops some capabilities, adds some capabilities unless dropped by the pod, 3) object is created and capabilities can never be changed.

What about "assignedCapabilities"? They are what admission will assign to the pod at creation time, and the pod author can explicitly opt out of some of the added capabilities by dropping them in their submitted pod spec.

I know it is bike shedding, but 'assigned' sounds to me like it would blindly overwrite things. Which it only sort of does. You don't end up with the 'assigned' caps. You end up with some amalgamation.

I'm not arguing for Initial either, I'll try to think of something to propose instead of just being a negative nancy...

Last IRC suggestion was:

allowedCapabilities []kapi.Capbility - what you're allowed to add (already exists)
defaultAddCapabilitiess []kapi.Capbility - what you'll be given by default unless you drop one of them explicitly
requiredDropCapabilitiess []kapi.Capbility - you must drop these

@smarterclayton @eparis @liggitt - satisfactory?

allowedAddCapabilities

Can't change that one, it already exists

I'm not in love with the names but we don't have time to bike shed. Names are approved

pweil- · 2016-01-15T17:34:41Z

@liggitt @eparis - updated, PTAL

liggitt · 2016-01-15T18:08:11Z

...ps/_workspace/src/k8s.io/kubernetes/pkg/securitycontextconstraints/capabilities/mustrunas.go

+// 		container specifically is dropping the cap) and container requested adds
+// 2.  a capabilities.Drop set containing all the required drops and container requested drops
+func (s *defaultCapabilities) Generate(pod *api.Pod, container *api.Container) (*api.Capabilities, error) {
+	requiredAdd := makeCapSet(s.defaultAddCapabilities)


s/requiredAdd/defaultAdd/

pweil- · 2016-01-15T18:41:16Z

re[test]

pweil- · 2016-01-15T20:07:41Z

[test]

openshift-bot · 2016-01-15T21:35:25Z

Evaluated for origin test up to ddf3dd0

pweil- · 2016-01-15T21:39:06Z

@liggitt anything else on the revised code that looks fishy? I'm just fighting with jenkins at this point.

openshift-bot · 2016-01-15T22:46:28Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/45/)

liggitt · 2016-01-16T04:21:15Z

nope, LGTM

smarterclayton · 2016-01-16T05:33:06Z

[merge]

openshift-bot · 2016-01-16T06:20:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4646/) (Image: devenv-rhel7_3165)

smarterclayton · 2016-01-16T06:39:43Z

[merge]

openshift-bot · 2016-01-16T06:40:21Z

Evaluated for origin merge up to ddf3dd0

Merged by openshift-bot

pweil- force-pushed the scc-caps branch 3 times, most recently from 14452c0 to 1aef128 Compare December 22, 2015 20:24

smarterclayton reviewed Jan 2, 2016
View reviewed changes

pweil- force-pushed the scc-caps branch from 1aef128 to de8a66e Compare January 11, 2016 22:34

pweil- changed the title ~~WIP: capability defaulting~~ capability defaulting Jan 11, 2016

pweil- mentioned this pull request Jan 13, 2016

SCC caps defaults #6627

Merged

pweil- force-pushed the scc-caps branch from de8a66e to 97a638d Compare January 13, 2016 20:19

pmorie reviewed Jan 13, 2016
View reviewed changes

pweil- force-pushed the scc-caps branch 2 times, most recently from 509a0eb to 0e7c71f Compare January 13, 2016 21:19

pmorie reviewed Jan 13, 2016
View reviewed changes

pweil- force-pushed the scc-caps branch from 0e7c71f to 84aef2e Compare January 14, 2016 13:38

liggitt reviewed Jan 14, 2016
View reviewed changes

pweil- force-pushed the scc-caps branch from 84aef2e to 0d58a66 Compare January 14, 2016 22:25

liggitt reviewed Jan 15, 2016
View reviewed changes

pweil- force-pushed the scc-caps branch from 0d58a66 to ae1017a Compare January 15, 2016 17:22

pweil- force-pushed the scc-caps branch from ae1017a to 0c098b5 Compare January 15, 2016 17:40

liggitt reviewed Jan 15, 2016
View reviewed changes

pweil- force-pushed the scc-caps branch from 0c098b5 to cfdbb61 Compare January 15, 2016 18:23

liggitt added this to the 1.1.1 milestone Jan 15, 2016

Paul Weil added 2 commits January 15, 2016 15:45

UPSTREAM: <carry>: capability defaulting

5ff7ad4

admission tests and swagger

ddf3dd0

pweil- force-pushed the scc-caps branch from cfdbb61 to ddf3dd0 Compare January 15, 2016 21:33

openshift-bot pushed a commit that referenced this pull request Jan 16, 2016

Merge pull request #6470 from pweil-/scc-caps

9b59dab

Merged by openshift-bot

openshift-bot merged commit 9b59dab into openshift:master Jan 16, 2016

soltysh mentioned this pull request Jan 18, 2016

k8s rebase - 4a65fa1f35e98ae96785836d99bf4ec7712ab682 #6320

Merged

46 tasks

pweil- deleted the scc-caps branch January 19, 2016 14:13

capability defaulting #6470

capability defaulting #6470

Conversation

pweil- commented Dec 22, 2015

smarterclayton commented Dec 22, 2015

pweil- commented Dec 22, 2015

smarterclayton commented Dec 24, 2015

Choose a reason for hiding this comment

jwhonce commented Jan 5, 2016

smarterclayton commented Jan 8, 2016

pweil- commented Jan 11, 2016

smarterclayton commented Jan 11, 2016

pweil- commented Jan 11, 2016

pweil- commented Jan 11, 2016

smarterclayton commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Jan 13, 2016

pweil- commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Jan 14, 2016

smarterclayton commented Jan 14, 2016

soltysh commented Jan 14, 2016

Choose a reason for hiding this comment

pweil- commented Jan 14, 2016

pweil- commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Jan 15, 2016

pweil- commented Jan 15, 2016

openshift-bot commented Jan 15, 2016

pweil- commented Jan 15, 2016

openshift-bot commented Jan 15, 2016

liggitt commented Jan 16, 2016

smarterclayton commented Jan 16, 2016

openshift-bot commented Jan 16, 2016

smarterclayton commented Jan 16, 2016

openshift-bot commented Jan 16, 2016