Security Context Constraints: prefer non-mutating SCC on update.

Author: php-coder

pkg/security/admission/admission.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/golang/glog"
 
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	kapihelper "k8s.io/kubernetes/pkg/apis/core/helper"
 	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
@@ -115,20 +116,54 @@ func (c *constraint) Admit(a admission.Attributes) error {
 		return admission.NewForbidden(a, fmt.Errorf("no providers available to validate pod request"))
 	}
 
+	// TODO(liggitt): allow spec mutation during initializing updates?
+	specMutationAllowed := a.GetOperation() == admission.Create
+
 	// all containers in a single pod must validate under a single provider or we will reject the request
 	validationErrs := field.ErrorList{}
+	var (
+		allowedPod       *kapi.Pod
+		allowingProvider scc.SecurityContextConstraintsProvider
+	)
+
+loop:
 	for _, provider := range providers {
-		if errs := scc.AssignSecurityContext(provider, pod, field.NewPath(fmt.Sprintf("provider %s: ", provider.GetSCCName()))); len(errs) > 0 {
+		podCopy := pod.DeepCopy()
+
+		if errs := scc.AssignSecurityContext(provider, podCopy, field.NewPath(fmt.Sprintf("provider %s: ", provider.GetSCCName()))); len(errs) > 0 {
 			validationErrs = append(validationErrs, errs...)
 			continue
 		}
 
-		// the entire pod validated, annotate and accept the pod
-		glog.V(4).Infof("pod %s (generate: %s) validated against provider %s", pod.Name, pod.GenerateName, provider.GetSCCName())
+		// the entire pod validated
+		switch {
+		case specMutationAllowed:
+			// if mutation is allowed, use the first found SCC that allows the pod.
+			// This behavior is different from Kubernetes which tries to search a non-mutating provider
+			// even on creating. We prefer most restrictive SCC in this case even if it mutates a pod.
+			allowedPod = podCopy
+			allowingProvider = provider
+			glog.V(6).Infof("pod %s (generate: %s) validated against provider %s with mutation", pod.Name, pod.GenerateName, provider.GetSCCName())
+			break loop
+		case apiequality.Semantic.DeepEqual(pod, podCopy):
+			// if we don't allow mutation, only use the validated pod if it didn't require any spec changes
+			allowedPod = podCopy
+			allowingProvider = provider
+			glog.V(6).Infof("pod %s (generate: %s) validated against provider %s without mutation", pod.Name, pod.GenerateName, provider.GetSCCName())
+			break loop
+		default:
+			glog.V(6).Infof("pod %s (generate: %s) validated against provider %s, but required mutation, skipping", pod.Name, pod.GenerateName, provider.GetSCCName())
+		}
+	}
+
+	if allowedPod != nil {
+		*pod = *allowedPod
+		// annotate and accept the pod
+		glog.V(4).Infof("pod %s (generate: %s) validated against provider %s", pod.Name, pod.GenerateName, allowingProvider.GetSCCName())
 		if pod.ObjectMeta.Annotations == nil {
 			pod.ObjectMeta.Annotations = map[string]string{}
 		}
-		pod.ObjectMeta.Annotations[allocator.ValidatedSCCAnnotation] = provider.GetSCCName()
+		pod.ObjectMeta.Annotations[allocator.ValidatedSCCAnnotation] = allowingProvider.GetSCCName()
 		return nil
 	}
 

pkg/security/admission/admission_test.go

@@ -140,16 +140,17 @@ func TestAdmitCaps(t *testing.T) {
 	for i := 0; i < 2; i++ {
 		for k, v := range tc {
 			v.pod.Spec.Containers, v.pod.Spec.InitContainers = v.pod.Spec.InitContainers, v.pod.Spec.Containers
+
+			testSCCAdmit(k, v.sccs, v.pod, v.shouldPass, t)
+
 			containers := v.pod.Spec.Containers
 			if i == 0 {
 				containers = v.pod.Spec.InitContainers
 			}
 
-			testSCCAdmit(k, v.sccs, v.pod, v.shouldPass, t)
-
 			if v.expectedCapabilities != nil {
 				if !reflect.DeepEqual(v.expectedCapabilities, containers[0].SecurityContext.Capabilities) {
-					t.Errorf("%s resulted in caps that were not expected - expected: %v, received: %v", k, v.expectedCapabilities, containers[0].SecurityContext.Capabilities)
+					t.Errorf("%s resulted in caps that were not expected - expected: %#v, received: %#v", k, v.expectedCapabilities, containers[0].SecurityContext.Capabilities)
 				}
 			}
 		}
@@ -239,43 +240,44 @@ func TestAdmitSuccess(t *testing.T) {
 		"specifyUIDInRange": {
 			pod:                 specifyUIDInRange,
 			expectedPodSC:       podSC(seLinuxLevelFromNamespace, defaultGroup, defaultGroup),
-			expectedContainerSC: containerSC(seLinuxLevelFromNamespace, goodUID),
+			expectedContainerSC: containerSC(nil, goodUID),
 		},
 		"specifyLabels": {
 			pod:                 specifyLabels,
 			expectedPodSC:       podSC(seLinuxLevelFromNamespace, defaultGroup, defaultGroup),
-			expectedContainerSC: containerSC(seLinuxLevelFromNamespace, 1),
+			expectedContainerSC: containerSC(&seLinuxLevelFromNamespace, 1),
 		},
 		"specifyFSGroup": {
 			pod:                 specifyFSGroupInRange,
 			expectedPodSC:       podSC(seLinuxLevelFromNamespace, goodFSGroup, defaultGroup),
-			expectedContainerSC: containerSC(seLinuxLevelFromNamespace, 1),
+			expectedContainerSC: containerSC(nil, 1),
 		},
 		"specifySupGroup": {
 			pod:                 specifySupGroup,
 			expectedPodSC:       podSC(seLinuxLevelFromNamespace, defaultGroup, 3),
-			expectedContainerSC: containerSC(seLinuxLevelFromNamespace, 1),
+			expectedContainerSC: containerSC(nil, 1),
 		},
 		"specifyPodLevelSELinuxLevel": {
 			pod:                 specifyPodLevelSELinux,
 			expectedPodSC:       podSC(seLinuxLevelFromNamespace, defaultGroup, defaultGroup),
-			expectedContainerSC: containerSC(seLinuxLevelFromNamespace, 1),
+			expectedContainerSC: containerSC(nil, 1),
 		},
 	}
 
 	for i := 0; i < 2; i++ {
 		for k, v := range testCases {
 			v.pod.Spec.Containers, v.pod.Spec.InitContainers = v.pod.Spec.InitContainers, v.pod.Spec.Containers
-			containers := v.pod.Spec.Containers
-			if i == 0 {
-				containers = v.pod.Spec.InitContainers
-			}
 
 			hasErrors := testSCCAdmission(v.pod, p, saSCC.Name, k, t)
 			if hasErrors {
 				continue
 			}
 
+			containers := v.pod.Spec.Containers
+			if i == 0 {
+				containers = v.pod.Spec.InitContainers
+			}
+
 			if !reflect.DeepEqual(v.expectedPodSC, v.pod.Spec.SecurityContext) {
 				t.Errorf("%s unexpected pod SecurityContext diff:\n%s", k, diff.ObjectGoPrintSideBySide(v.expectedPodSC, v.pod.Spec.SecurityContext))
 			}
@@ -934,6 +936,85 @@ func TestAdmitSeccomp(t *testing.T) {
 
 }
 
+func TestAdmitPreferNonmutatingWhenPossible(t *testing.T) {
+
+	mutatingSCC := restrictiveSCC()
+	mutatingSCC.Name = "mutating-scc"
+
+	nonMutatingSCC := laxSCC()
+	nonMutatingSCC.Name = "non-mutating-scc"
+
+	simplePod := goodPod()
+	simplePod.Spec.Containers[0].Name = "simple-pod"
+	simplePod.Spec.Containers[0].Image = "test-image:0.1"
+
+	modifiedPod := simplePod.DeepCopy()
+	modifiedPod.Spec.Containers[0].Image = "test-image:0.2"
+
+	tests := map[string]struct {
+		oldPod      *kapi.Pod
+		newPod      *kapi.Pod
+		operation   kadmission.Operation
+		sccs        []*securityapi.SecurityContextConstraints
+		shouldPass  bool
+		expectedSCC string
+	}{
+		"creation: the first SCC (even if it mutates) should be used": {
+			newPod:      simplePod.DeepCopy(),
+			operation:   kadmission.Create,
+			sccs:        []*securityapi.SecurityContextConstraints{mutatingSCC, nonMutatingSCC},
+			shouldPass:  true,
+			expectedSCC: mutatingSCC.Name,
+		},
+		"updating: the first non-mutating SCC should be used": {
+			oldPod:      simplePod.DeepCopy(),
+			newPod:      modifiedPod.DeepCopy(),
+			operation:   kadmission.Update,
+			sccs:        []*securityapi.SecurityContextConstraints{mutatingSCC, nonMutatingSCC},
+			shouldPass:  true,
+			expectedSCC: nonMutatingSCC.Name,
+		},
+		"updating: a pod should be rejected when there are only mutating SCCs": {
+			oldPod:     simplePod.DeepCopy(),
+			newPod:     modifiedPod.DeepCopy(),
+			operation:  kadmission.Update,
+			sccs:       []*securityapi.SecurityContextConstraints{mutatingSCC},
+			shouldPass: false,
+		},
+	}
+
+	for testCaseName, testCase := range tests {
+		// We can't use testSCCAdmission() here because it doesn't support Update operation.
+		// We can't use testSCCAdmit() here because it doesn't support Update operation and doesn't check for the SCC annotation.
+
+		tc := setupClientSet()
+		lister := createSCCLister(t, testCase.sccs)
+		plugin := NewTestAdmission(lister, tc)
+
+		attrs := kadmission.NewAttributesRecord(testCase.newPod, testCase.oldPod, kapi.Kind("Pod").WithVersion("version"), testCase.newPod.Namespace, testCase.newPod.Name, kapi.Resource("pods").WithVersion("version"), "", testCase.operation, &user.DefaultInfo{})
+		err := plugin.(kadmission.MutationInterface).Admit(attrs)
+
+		if testCase.shouldPass {
+			if err != nil {
+				t.Errorf("%s expected no errors but received %v", testCaseName, err)
+			} else {
+				validatedSCC, ok := testCase.newPod.Annotations[allocator.ValidatedSCCAnnotation]
+				if !ok {
+					t.Errorf("expected %q to find the validated annotation on the pod for the scc but found none", testCaseName)
+
+				} else if validatedSCC != testCase.expectedSCC {
+					t.Errorf("%q should have validated against %q but found %q", testCaseName, testCase.expectedSCC, validatedSCC)
+				}
+			}
+		} else {
+			if err == nil {
+				t.Errorf("%s expected errors but received none", testCaseName)
+			}
+		}
+	}
+
+}
+
 // testSCCAdmission is a helper to admit the pod and ensure it was validated against the expected
 // SCC. Returns true when errors have been encountered.
 func testSCCAdmission(pod *kapi.Pod, plugin kadmission.Interface, expectedSCC, testName string, t *testing.T) bool {
@@ -1087,15 +1168,16 @@ func goodPod() *kapi.Pod {
 	}
 }
 
-func containerSC(seLinuxLevel string, uid int64) *kapi.SecurityContext {
-	no := false
-	return &kapi.SecurityContext{
-		Privileged: &no,
-		RunAsUser:  &uid,
-		SELinuxOptions: &kapi.SELinuxOptions{
-			Level: seLinuxLevel,
-		},
+func containerSC(seLinuxLevel *string, uid int64) *kapi.SecurityContext {
+	sc := &kapi.SecurityContext{
+		RunAsUser: &uid,
+	}
+	if seLinuxLevel != nil {
+		sc.SELinuxOptions = &kapi.SELinuxOptions{
+			Level: *seLinuxLevel,
+		}
 	}
+	return sc
 }
 
 func podSC(seLinuxLevel string, fsGroup, supGroup int64) *kapi.PodSecurityContext {

pkg/security/securitycontextconstraints/matcher.go

@@ -13,7 +13,6 @@ import (
 	"k8s.io/apiserver/pkg/authentication/user"
 	kapi "k8s.io/kubernetes/pkg/apis/core"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
-	sc "k8s.io/kubernetes/pkg/securitycontext"
 
 	allocator "github.com/openshift/origin/pkg/security"
 	securityapi "github.com/openshift/origin/pkg/security/apis/security"
@@ -71,87 +70,44 @@ func ConstraintAppliesTo(constraint *securityapi.SecurityContextConstraints, use
 // and validates that the sc falls within the scc constraints.  All containers must validate against
 // the same scc or is not considered valid.
 func AssignSecurityContext(provider SecurityContextConstraintsProvider, pod *kapi.Pod, fldPath *field.Path) field.ErrorList {
-	generatedSCs := make([]*kapi.SecurityContext, len(pod.Spec.InitContainers)+len(pod.Spec.Containers))
-
 	errs := field.ErrorList{}
 
 	psc, generatedAnnotations, err := provider.CreatePodSecurityContext(pod)
 	if err != nil {
 		errs = append(errs, field.Invalid(fldPath.Child("spec", "securityContext"), pod.Spec.SecurityContext, err.Error()))
 	}
 
-	// save the original PSC and validate the generated PSC.  Leave the generated PSC
-	// set for container generation/validation.  We will reset to original post container
-	// validation.
-	originalPSC := pod.Spec.SecurityContext
-	originalAnnotations := pod.Annotations
-
 	pod.Spec.SecurityContext = psc
 	pod.Annotations = generatedAnnotations
 	errs = append(errs, provider.ValidatePodSecurityContext(pod, fldPath.Child("spec", "securityContext"))...)
 
-	// Note: this is not changing the original container, we will set container SCs later so long
-	// as all containers validated under the same SCC.
-	containerPath := fldPath.Child("spec", "initContainers")
-	for i, containerCopy := range pod.Spec.InitContainers {
-		csc, resolutionErrs := resolveContainerSecurityContext(provider, pod, &containerCopy, containerPath.Index(i))
-		errs = append(errs, resolutionErrs...)
-		if len(resolutionErrs) > 0 {
+	for i := range pod.Spec.InitContainers {
+		sc, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.InitContainers[i])
+		if err != nil {
+			errs = append(errs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("securityContext"), "", err.Error()))
 			continue
 		}
-		generatedSCs[i] = csc
+		pod.Spec.InitContainers[i].SecurityContext = sc
+		errs = append(errs, provider.ValidateContainerSecurityContext(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i).Child("securityContext"))...)
 	}
 
-	base := len(pod.Spec.InitContainers)
-
-	// Note: this is not changing the original container, we will set container SCs later so long
-	// as all containers validated under the same SCC.
-	containerPath = fldPath.Child("spec", "containers")
-	for i, containerCopy := range pod.Spec.Containers {
-		csc, resolutionErrs := resolveContainerSecurityContext(provider, pod, &containerCopy, containerPath.Index(i))
-		errs = append(errs, resolutionErrs...)
-		if len(resolutionErrs) > 0 {
+	for i := range pod.Spec.Containers {
+		sc, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[i])
+		if err != nil {
+			errs = append(errs, field.Invalid(field.NewPath("spec", "containers").Index(i).Child("securityContext"), "", err.Error()))
 			continue
 		}
-		generatedSCs[i+base] = csc
+		pod.Spec.Containers[i].SecurityContext = sc
+		errs = append(errs, provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i).Child("securityContext"))...)
 	}
+
 	if len(errs) > 0 {
-		// ensure psc is not mutated if there are errors
-		pod.Spec.SecurityContext = originalPSC
-		pod.Annotations = originalAnnotations
 		return errs
 	}
 
-	// if we've reached this code then we've generated and validated an SC for every container in the
-	// pod so let's apply what we generated.  Note: the psc is already applied.
-	for i := range pod.Spec.InitContainers {
-		pod.Spec.InitContainers[i].SecurityContext = generatedSCs[i]
-	}
-	for i := range pod.Spec.Containers {
-		pod.Spec.Containers[i].SecurityContext = generatedSCs[i+base]
-	}
 	return nil
 }
 
-// resolveContainerSecurityContext checks the provided container against the provider, returning any
-// validation errors encountered on the resulting security context, or the security context that was
-// resolved. The SecurityContext field of the container is updated, so ensure that a copy of the original
-// container is passed here if you wish to preserve the original input.
-func resolveContainerSecurityContext(provider SecurityContextConstraintsProvider, pod *kapi.Pod, container *kapi.Container, path *field.Path) (*kapi.SecurityContext, field.ErrorList) {
-	// We will determine the effective security context for the container and validate against that
-	// since that is how the sc provider will eventually apply settings in the runtime.
-	// This results in an SC that is based on the Pod's PSC with the set fields from the container
-	// overriding pod level settings.
-	container.SecurityContext = sc.InternalDetermineEffectiveSecurityContext(pod, container)
-
-	csc, err := provider.CreateContainerSecurityContext(pod, container)
-	if err != nil {
-		return nil, field.ErrorList{field.Invalid(path.Child("securityContext"), "", err.Error())}
-	}
-	container.SecurityContext = csc
-	return csc, provider.ValidateContainerSecurityContext(pod, container, path.Child("securityContext"))
-}
-
 // constraintSupportsGroup checks that group is in constraintGroups.
 func constraintSupportsGroup(group string, constraintGroups []string) bool {
 	for _, g := range constraintGroups {