From 158f17b9bb3cc1d2ba1378fc56db017e8326f865 Mon Sep 17 00:00:00 2001 From: Quintin Lee Date: Tue, 6 Jun 2017 12:28:15 -0700 Subject: [PATCH] PodSecurityPolicy should respect and validate user-supplied RunAsNonRoot fields. --- pkg/security/podsecuritypolicy/provider.go | 2 +- .../podsecuritypolicy/provider_test.go | 132 ++++++++++-------- .../podsecuritypolicy/user/nonroot.go | 12 +- .../podsecuritypolicy/user/nonroot_test.go | 76 +++++++--- 4 files changed, 139 insertions(+), 83 deletions(-) diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go index e92c6419fc..9fc80fa889 100644 --- 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 index 8cdfd65f07..71d241d86a 100644 --- 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}}, + } + + 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, + }}, + }, + } } - } - // 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", + // 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", + }, }, - }, - Spec: extensions.PodSecurityPolicySpec{ - DefaultAddCapabilities: []api.Capability{"foo"}, - RequiredDropCapabilities: []api.Capability{"bar"}, - RunAsUser: extensions.RunAsUserStrategyOptions{ - Rule: extensions.RunAsUserStrategyMustRunAs, - Ranges: []extensions.UserIDRange{{Min: uid, Max: uid}}, + 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, }, - 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, - }, + } } - } - 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 index 9b0455509a..7d16883607 100644 --- 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 index 819a2bcb3e..3e8662cb56 100644 --- 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) + } } }