kubernetes · k8s-github-robot · Jun 14, 2017 · Jun 6, 2017
diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go
@@ -165,7 +165,7 @@ func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container
 	// if we're using the non-root strategy set the marker that this container should not be
 	// run as root which will signal to the kubelet to do a final check either on the runAsUser
 	// or, if runAsUser is not set, the image UID will be checked.
-	if s.psp.Spec.RunAsUser.Rule == extensions.RunAsUserStrategyMustRunAsNonRoot {
+	if sc.RunAsNonRoot == nil && s.psp.Spec.RunAsUser.Rule == extensions.RunAsUserStrategyMustRunAsNonRoot {
 		nonRoot := true
 		sc.RunAsNonRoot = &nonRoot
 	}

diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go
@@ -112,76 +112,86 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) {
 }
 
 func TestCreateContainerSecurityContextNonmutating(t *testing.T) {
-	// Create a pod with a security context that needs filling in
-	createPod := func() *api.Pod {
-		return &api.Pod{
-			Spec: api.PodSpec{
-				Containers: []api.Container{{
-					SecurityContext: &api.SecurityContext{},
-				}},
-			},
-		}
+	untrue := false
+	tests := []struct {
+		security *api.SecurityContext
+	}{
+		{nil},
+		{&api.SecurityContext{RunAsNonRoot: &untrue}},
 	}
 
-	// Create a PSP with strategies that will populate a blank security context
-	createPSP := func() *extensions.PodSecurityPolicy {
-		uid := types.UnixUserID(1)
-		return &extensions.PodSecurityPolicy{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "psp-sa",
-				Annotations: map[string]string{
-					seccomp.AllowedProfilesAnnotationKey: "*",
-					seccomp.DefaultProfileAnnotationKey:  "foo",
+	for _, tc := range tests {
+		// Create a pod with a security context that needs filling in
+		createPod := func() *api.Pod {
+			return &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{{
+						SecurityContext: tc.security,
+					}},
 				},
-			},
-			Spec: extensions.PodSecurityPolicySpec{
-				DefaultAddCapabilities:   []api.Capability{"foo"},
-				RequiredDropCapabilities: []api.Capability{"bar"},
-				RunAsUser: extensions.RunAsUserStrategyOptions{
-					Rule:   extensions.RunAsUserStrategyMustRunAs,
-					Ranges: []extensions.UserIDRange{{Min: uid, Max: uid}},
-				},
-				SELinux: extensions.SELinuxStrategyOptions{
-					Rule:           extensions.SELinuxStrategyMustRunAs,
-					SELinuxOptions: &api.SELinuxOptions{User: "you"},
-				},
-				// these are pod mutating strategies that are tested above
-				FSGroup: extensions.FSGroupStrategyOptions{
-					Rule: extensions.FSGroupStrategyRunAsAny,
+			}
+		}
+
+		// Create a PSP with strategies that will populate a blank security context
+		createPSP := func() *extensions.PodSecurityPolicy {
+			uid := types.UnixUserID(1)
+			return &extensions.PodSecurityPolicy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "psp-sa",
+					Annotations: map[string]string{
+						seccomp.AllowedProfilesAnnotationKey: "*",
+						seccomp.DefaultProfileAnnotationKey:  "foo",
+					},
 				},
-				SupplementalGroups: extensions.SupplementalGroupsStrategyOptions{
-					Rule: extensions.SupplementalGroupsStrategyRunAsAny,
+				Spec: extensions.PodSecurityPolicySpec{
+					DefaultAddCapabilities:   []api.Capability{"foo"},
+					RequiredDropCapabilities: []api.Capability{"bar"},
+					RunAsUser: extensions.RunAsUserStrategyOptions{
+						Rule:   extensions.RunAsUserStrategyMustRunAs,
+						Ranges: []extensions.UserIDRange{{Min: uid, Max: uid}},
+					},
+					SELinux: extensions.SELinuxStrategyOptions{
+						Rule:           extensions.SELinuxStrategyMustRunAs,
+						SELinuxOptions: &api.SELinuxOptions{User: "you"},
+					},
+					// these are pod mutating strategies that are tested above
+					FSGroup: extensions.FSGroupStrategyOptions{
+						Rule: extensions.FSGroupStrategyRunAsAny,
+					},
+					SupplementalGroups: extensions.SupplementalGroupsStrategyOptions{
+						Rule: extensions.SupplementalGroupsStrategyRunAsAny,
+					},
+					// mutates the container SC by defaulting to true if container sets nil
+					ReadOnlyRootFilesystem: true,
 				},
-				// mutates the container SC by defaulting to true if container sets nil
-				ReadOnlyRootFilesystem: true,
-			},
+			}
 		}
-	}
 
-	pod := createPod()
-	psp := createPSP()
+		pod := createPod()
+		psp := createPSP()
 
-	provider, err := NewSimpleProvider(psp, "namespace", NewSimpleStrategyFactory())
-	if err != nil {
-		t.Fatalf("unable to create provider %v", err)
-	}
-	sc, _, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[0])
-	if err != nil {
-		t.Fatalf("unable to create container security context %v", err)
-	}
+		provider, err := NewSimpleProvider(psp, "namespace", NewSimpleStrategyFactory())
+		if err != nil {
+			t.Fatalf("unable to create provider %v", err)
+		}
+		sc, _, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[0])
+		if err != nil {
+			t.Fatalf("unable to create container security context %v", err)
+		}
 
-	// The generated security context should have filled in missing options, so they should differ
-	if reflect.DeepEqual(sc, &pod.Spec.Containers[0].SecurityContext) {
-		t.Error("expected created security context to be different than container's, but they were identical")
-	}
+		// The generated security context should have filled in missing options, so they should differ
+		if reflect.DeepEqual(sc, &pod.Spec.Containers[0].SecurityContext) {
+			t.Error("expected created security context to be different than container's, but they were identical")
+		}
 
-	// Creating the provider or the security context should not have mutated the psp or pod
-	if !reflect.DeepEqual(createPod(), pod) {
-		diffs := diff.ObjectDiff(createPod(), pod)
-		t.Errorf("pod was mutated by CreateContainerSecurityContext. diff:\n%s", diffs)
-	}
-	if !reflect.DeepEqual(createPSP(), psp) {
-		t.Error("psp was mutated by CreateContainerSecurityContext")
+		// Creating the provider or the security context should not have mutated the psp or pod
+		if !reflect.DeepEqual(createPod(), pod) {
+			diffs := diff.ObjectDiff(createPod(), pod)
+			t.Errorf("pod was mutated by CreateContainerSecurityContext. diff:\n%s", diffs)
+		}
+		if !reflect.DeepEqual(createPSP(), psp) {
+			t.Error("psp was mutated by CreateContainerSecurityContext")
+		}
 	}
 }
 

diff --git a/pkg/security/podsecuritypolicy/user/nonroot.go b/pkg/security/podsecuritypolicy/user/nonroot.go
@@ -41,8 +41,9 @@ func (s *nonRoot) Generate(pod *api.Pod, container *api.Container) (*types.UnixU
 
 // Validate ensures that the specified values fall within the range of the strategy.  Validation
 // of this will pass if either the UID is not set, assuming that the image will provided the UID
-// or if the UID is set it is not root.  In order to work properly this assumes that the kubelet
-// performs a final check on runAsUser or the image UID when runAsUser is nil.
+// or if the UID is set it is not root.  Validation will fail if RunAsNonRoot is set to false.
+// In order to work properly this assumes that the kubelet performs a final check on runAsUser
+// or the image UID when runAsUser is nil.
 func (s *nonRoot) Validate(pod *api.Pod, container *api.Container) field.ErrorList {
 	allErrs := field.ErrorList{}
 	securityContextPath := field.NewPath("securityContext")
@@ -51,8 +52,13 @@ func (s *nonRoot) Validate(pod *api.Pod, container *api.Container) field.ErrorLi
 		allErrs = append(allErrs, field.Invalid(securityContextPath, container.SecurityContext, detail))
 		return allErrs
 	}
+	if container.SecurityContext.RunAsNonRoot != nil && *container.SecurityContext.RunAsNonRoot == false {
+		detail := fmt.Sprintf("RunAsNonRoot must be true for container %s", container.Name)
+		allErrs = append(allErrs, field.Invalid(securityContextPath.Child("runAsNonRoot"), *container.SecurityContext.RunAsNonRoot, detail))
+		return allErrs
+	}
 	if container.SecurityContext.RunAsUser != nil && *container.SecurityContext.RunAsUser == 0 {
-		detail := fmt.Sprintf("running with the root UID is forbidden by the pod security policy %s", container.Name)
+		detail := fmt.Sprintf("running with the root UID is forbidden by the pod security policy for container %s", container.Name)
 		allErrs = append(allErrs, field.Invalid(securityContextPath.Child("runAsUser"), *container.SecurityContext.RunAsUser, detail))
 		return allErrs
 	}

diff --git a/pkg/security/podsecuritypolicy/user/nonroot_test.go b/pkg/security/podsecuritypolicy/user/nonroot_test.go
@@ -52,30 +52,70 @@ func TestNonRootGenerate(t *testing.T) {
 func TestNonRootValidate(t *testing.T) {
 	goodUID := types.UnixUserID(1)
 	badUID := types.UnixUserID(0)
+	untrue := false
+	unfalse := true
 	s, err := NewRunAsNonRoot(&extensions.RunAsUserStrategyOptions{})
 	if err != nil {
 		t.Fatalf("unexpected error initializing NewMustRunAs %v", err)
 	}
-	container := &api.Container{
-		SecurityContext: &api.SecurityContext{
-			RunAsUser: &badUID,
+	tests := []struct {
+		container   *api.Container
+		expectedErr bool
+		msg         string
+	}{
+		{
+			container: &api.Container{
+				SecurityContext: &api.SecurityContext{
+					RunAsUser: &badUID,
+				},
+			},
+			expectedErr: true,
+			msg:         "in test case %d, expected errors from root uid but got none: %v",
+		},
+		{
+			container: &api.Container{
+				SecurityContext: &api.SecurityContext{
+					RunAsUser: &goodUID,
+				},
+			},
+			expectedErr: false,
+			msg:         "in test case %d, expected no errors from non-root uid but got %v",
+		},
+		{
+			container: &api.Container{
+				SecurityContext: &api.SecurityContext{
+					RunAsNonRoot: &untrue,
+				},
+			},
+			expectedErr: true,
+			msg:         "in test case %d, expected errors from RunAsNonRoot but got none: %v",
+		},
+		{
+			container: &api.Container{
+				SecurityContext: &api.SecurityContext{
+					RunAsNonRoot: &unfalse,
+					RunAsUser:    &goodUID,
+				},
+			},
+			expectedErr: false,
+			msg:         "in test case %d, expected no errors from non-root uid but got %v",
+		},
+		{
+			container: &api.Container{
+				SecurityContext: &api.SecurityContext{
+					RunAsNonRoot: &unfalse,
+					RunAsUser:    &badUID,
+				},
+			},
+			expectedErr: true,
+			msg:         "in test case %d, expected errors from root uid but got %v",
 		},
 	}
 
-	errs := s.Validate(nil, container)
-	if len(errs) == 0 {
-		t.Errorf("expected errors from root uid but got none")
-	}
-
-	container.SecurityContext.RunAsUser = &goodUID
-	errs = s.Validate(nil, container)
-	if len(errs) != 0 {
-		t.Errorf("expected no errors from non-root uid but got %v", errs)
-	}
-
-	container.SecurityContext.RunAsUser = nil
-	errs = s.Validate(nil, container)
-	if len(errs) != 0 {
-		t.Errorf("expected no errors from nil uid but got %v", errs)
+	for i, tc := range tests {
+		errs := s.Validate(nil, tc.container)
+		if (len(errs) == 0) == tc.expectedErr {
+			t.Errorf(tc.msg, i, errs)
+		}
 	}
 }