From a5f722e181420cec5ef5533a111487034a713d7b Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 14 Oct 2017 15:07:09 -0400 Subject: [PATCH] PodSecurityPolicy: avoid unnecessary securitycontext mutation --- pkg/security/podsecuritypolicy/BUILD | 1 + pkg/security/podsecuritypolicy/provider.go | 145 ++++++++---------- .../podsecuritypolicy/provider_test.go | 44 ++---- 3 files changed, 74 insertions(+), 116 deletions(-) diff --git a/pkg/security/podsecuritypolicy/BUILD b/pkg/security/podsecuritypolicy/BUILD index 9216455dcf..55a68dec3b 100644 --- a/pkg/security/podsecuritypolicy/BUILD +++ b/pkg/security/podsecuritypolicy/BUILD @@ -26,6 +26,7 @@ go_library( "//pkg/security/podsecuritypolicy/sysctl:go_default_library", "//pkg/security/podsecuritypolicy/user:go_default_library", "//pkg/security/podsecuritypolicy/util:go_default_library", + "//pkg/securitycontext:go_default_library", "//pkg/util/maps:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go index 712eca1715..fadacec1dd 100644 --- a/pkg/security/podsecuritypolicy/provider.go +++ b/pkg/security/podsecuritypolicy/provider.go @@ -24,6 +24,7 @@ import ( "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/apis/extensions" psputil "k8s.io/kubernetes/pkg/security/podsecuritypolicy/util" + "k8s.io/kubernetes/pkg/securitycontext" "k8s.io/kubernetes/pkg/util/maps" ) @@ -66,42 +67,32 @@ func NewSimpleProvider(psp *extensions.PodSecurityPolicy, namespace string, stra // Create a PodSecurityContext based on the given constraints. If a setting is already set // on the PodSecurityContext it will not be changed. Validate should be used after the context // is created to ensure it complies with the required restrictions. -// -// NOTE: this method works on a copy of the PodSecurityContext. It is up to the caller to -// apply the PSC if validation passes. func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurityContext, map[string]string, error) { - var sc *api.PodSecurityContext = nil - if pod.Spec.SecurityContext != nil { - // work with a copy - copy := *pod.Spec.SecurityContext - sc = © - } else { - sc = &api.PodSecurityContext{} - } + sc := securitycontext.NewPodSecurityContextMutator(pod.Spec.SecurityContext) annotations := maps.CopySS(pod.Annotations) - if sc.SupplementalGroups == nil { + if sc.SupplementalGroups() == nil { supGroups, err := s.strategies.SupplementalGroupStrategy.Generate(pod) if err != nil { return nil, nil, err } - sc.SupplementalGroups = supGroups + sc.SetSupplementalGroups(supGroups) } - if sc.FSGroup == nil { + if sc.FSGroup() == nil { fsGroup, err := s.strategies.FSGroupStrategy.GenerateSingle(pod) if err != nil { return nil, nil, err } - sc.FSGroup = fsGroup + sc.SetFSGroup(fsGroup) } - if sc.SELinuxOptions == nil { + if sc.SELinuxOptions() == nil { seLinux, err := s.strategies.SELinuxStrategy.Generate(pod, nil) if err != nil { return nil, nil, err } - sc.SELinuxOptions = seLinux + sc.SetSELinuxOptions(seLinux) } // This is only generated on the pod level. Containers inherit the pod's profile. If the @@ -116,40 +107,34 @@ func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurit } annotations[api.SeccompPodAnnotationKey] = seccompProfile } - return sc, annotations, nil + return sc.PodSecurityContext(), annotations, nil } // Create a SecurityContext based on the given constraints. If a setting is already set on the // container's security context then it will not be changed. Validation should be used after // the context is created to ensure it complies with the required restrictions. -// -// NOTE: this method works on a copy of the SC of the container. It is up to the caller to apply -// the SC if validation passes. func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container *api.Container) (*api.SecurityContext, map[string]string, error) { - var sc *api.SecurityContext = nil - if container.SecurityContext != nil { - // work with a copy of the original - copy := *container.SecurityContext - sc = © - } else { - sc = &api.SecurityContext{} - } + sc := securitycontext.NewEffectiveContainerSecurityContextMutator( + securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext), + securitycontext.NewContainerSecurityContextMutator(container.SecurityContext), + ) + annotations := maps.CopySS(pod.Annotations) - if sc.RunAsUser == nil { + if sc.RunAsUser() == nil { uid, err := s.strategies.RunAsUserStrategy.Generate(pod, container) if err != nil { return nil, nil, err } - sc.RunAsUser = uid + sc.SetRunAsUser(uid) } - if sc.SELinuxOptions == nil { + if sc.SELinuxOptions() == nil { seLinux, err := s.strategies.SELinuxStrategy.Generate(pod, container) if err != nil { return nil, nil, err } - sc.SELinuxOptions = seLinux + sc.SetSELinuxOptions(seLinux) } annotations, err := s.strategies.AppArmorStrategy.Generate(annotations, container) @@ -160,67 +145,64 @@ 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 sc.RunAsNonRoot == nil && sc.RunAsUser == nil && s.psp.Spec.RunAsUser.Rule == extensions.RunAsUserStrategyMustRunAsNonRoot { + if sc.RunAsNonRoot() == nil && sc.RunAsUser() == nil && s.psp.Spec.RunAsUser.Rule == extensions.RunAsUserStrategyMustRunAsNonRoot { nonRoot := true - sc.RunAsNonRoot = &nonRoot + sc.SetRunAsNonRoot(&nonRoot) } caps, err := s.strategies.CapabilitiesStrategy.Generate(pod, container) if err != nil { return nil, nil, err } - sc.Capabilities = caps + sc.SetCapabilities(caps) // if the PSP requires a read only root filesystem and the container has not made a specific // request then default ReadOnlyRootFilesystem to true. - if s.psp.Spec.ReadOnlyRootFilesystem && sc.ReadOnlyRootFilesystem == nil { + if s.psp.Spec.ReadOnlyRootFilesystem && sc.ReadOnlyRootFilesystem() == nil { readOnlyRootFS := true - sc.ReadOnlyRootFilesystem = &readOnlyRootFS + sc.SetReadOnlyRootFilesystem(&readOnlyRootFS) } // if the PSP sets DefaultAllowPrivilegeEscalation and the container security context // allowPrivilegeEscalation is not set, then default to that set by the PSP. - if s.psp.Spec.DefaultAllowPrivilegeEscalation != nil && sc.AllowPrivilegeEscalation == nil { - sc.AllowPrivilegeEscalation = s.psp.Spec.DefaultAllowPrivilegeEscalation + if s.psp.Spec.DefaultAllowPrivilegeEscalation != nil && sc.AllowPrivilegeEscalation() == nil { + sc.SetAllowPrivilegeEscalation(s.psp.Spec.DefaultAllowPrivilegeEscalation) } // if the PSP sets psp.AllowPrivilegeEscalation to false set that as the default - if !s.psp.Spec.AllowPrivilegeEscalation && sc.AllowPrivilegeEscalation == nil { - sc.AllowPrivilegeEscalation = &s.psp.Spec.AllowPrivilegeEscalation + if !s.psp.Spec.AllowPrivilegeEscalation && sc.AllowPrivilegeEscalation() == nil { + sc.SetAllowPrivilegeEscalation(&s.psp.Spec.AllowPrivilegeEscalation) } - return sc, annotations, nil + return sc.ContainerSecurityContext(), annotations, nil } // Ensure a pod's SecurityContext is in compliance with the given constraints. func (s *simpleProvider) ValidatePodSecurityContext(pod *api.Pod, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if pod.Spec.SecurityContext == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("securityContext"), pod.Spec.SecurityContext, "No security context is set")) - return allErrs - } + sc := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext) fsGroups := []int64{} - if pod.Spec.SecurityContext.FSGroup != nil { - fsGroups = append(fsGroups, *pod.Spec.SecurityContext.FSGroup) + if fsGroup := sc.FSGroup(); fsGroup != nil { + fsGroups = append(fsGroups, *fsGroup) } allErrs = append(allErrs, s.strategies.FSGroupStrategy.Validate(pod, fsGroups)...) - allErrs = append(allErrs, s.strategies.SupplementalGroupStrategy.Validate(pod, pod.Spec.SecurityContext.SupplementalGroups)...) + allErrs = append(allErrs, s.strategies.SupplementalGroupStrategy.Validate(pod, sc.SupplementalGroups())...) allErrs = append(allErrs, s.strategies.SeccompStrategy.ValidatePod(pod)...) - allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, nil, pod.Spec.SecurityContext.SELinuxOptions)...) + allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, nil, sc.SELinuxOptions())...) - if !s.psp.Spec.HostNetwork && pod.Spec.SecurityContext.HostNetwork { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), pod.Spec.SecurityContext.HostNetwork, "Host network is not allowed to be used")) + if !s.psp.Spec.HostNetwork && sc.HostNetwork() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), sc.HostNetwork(), "Host network is not allowed to be used")) } - if !s.psp.Spec.HostPID && pod.Spec.SecurityContext.HostPID { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), pod.Spec.SecurityContext.HostPID, "Host PID is not allowed to be used")) + if !s.psp.Spec.HostPID && sc.HostPID() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), sc.HostPID(), "Host PID is not allowed to be used")) } - if !s.psp.Spec.HostIPC && pod.Spec.SecurityContext.HostIPC { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), pod.Spec.SecurityContext.HostIPC, "Host IPC is not allowed to be used")) + if !s.psp.Spec.HostIPC && sc.HostIPC() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), sc.HostIPC(), "Host IPC is not allowed to be used")) } allErrs = append(allErrs, s.strategies.SysctlsStrategy.Validate(pod)...) @@ -261,25 +243,23 @@ func (s *simpleProvider) ValidatePodSecurityContext(pod *api.Pod, fldPath *field func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, container *api.Container, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if container.SecurityContext == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("securityContext"), container.SecurityContext, "No security context is set")) - return allErrs - } + podSC := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext) + sc := securitycontext.NewEffectiveContainerSecurityContextAccessor(podSC, securitycontext.NewContainerSecurityContextMutator(container.SecurityContext)) - sc := container.SecurityContext - allErrs = append(allErrs, s.strategies.RunAsUserStrategy.Validate(fldPath.Child("securityContext"), pod, container, sc.RunAsNonRoot, sc.RunAsUser)...) - allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, container, sc.SELinuxOptions)...) + allErrs = append(allErrs, s.strategies.RunAsUserStrategy.Validate(fldPath.Child("securityContext"), pod, container, sc.RunAsNonRoot(), sc.RunAsUser())...) + allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, container, sc.SELinuxOptions())...) allErrs = append(allErrs, s.strategies.AppArmorStrategy.Validate(pod, container)...) allErrs = append(allErrs, s.strategies.SeccompStrategy.ValidateContainer(pod, container)...) - if !s.psp.Spec.Privileged && sc.Privileged != nil && *sc.Privileged { - allErrs = append(allErrs, field.Invalid(fldPath.Child("privileged"), *sc.Privileged, "Privileged containers are not allowed")) + privileged := sc.Privileged() + if !s.psp.Spec.Privileged && privileged != nil && *privileged { + allErrs = append(allErrs, field.Invalid(fldPath.Child("privileged"), *privileged, "Privileged containers are not allowed")) } - allErrs = append(allErrs, s.strategies.CapabilitiesStrategy.Validate(pod, container, sc.Capabilities)...) + allErrs = append(allErrs, s.strategies.CapabilitiesStrategy.Validate(pod, container, sc.Capabilities())...) - if !s.psp.Spec.HostNetwork && pod.Spec.SecurityContext.HostNetwork { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), pod.Spec.SecurityContext.HostNetwork, "Host network is not allowed to be used")) + if !s.psp.Spec.HostNetwork && podSC.HostNetwork() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), podSC.HostNetwork(), "Host network is not allowed to be used")) } containersPath := fldPath.Child("containers") @@ -294,29 +274,30 @@ func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, containe allErrs = append(allErrs, s.hasInvalidHostPort(&c, idxPath)...) } - if !s.psp.Spec.HostPID && pod.Spec.SecurityContext.HostPID { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), pod.Spec.SecurityContext.HostPID, "Host PID is not allowed to be used")) + if !s.psp.Spec.HostPID && podSC.HostPID() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), podSC.HostPID(), "Host PID is not allowed to be used")) } - if !s.psp.Spec.HostIPC && pod.Spec.SecurityContext.HostIPC { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), pod.Spec.SecurityContext.HostIPC, "Host IPC is not allowed to be used")) + if !s.psp.Spec.HostIPC && podSC.HostIPC() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), podSC.HostIPC(), "Host IPC is not allowed to be used")) } if s.psp.Spec.ReadOnlyRootFilesystem { - if sc.ReadOnlyRootFilesystem == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), sc.ReadOnlyRootFilesystem, "ReadOnlyRootFilesystem may not be nil and must be set to true")) - } else if !*sc.ReadOnlyRootFilesystem { - allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), *sc.ReadOnlyRootFilesystem, "ReadOnlyRootFilesystem must be set to true")) + readOnly := sc.ReadOnlyRootFilesystem() + if readOnly == nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), readOnly, "ReadOnlyRootFilesystem may not be nil and must be set to true")) + } else if !*readOnly { + allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), *readOnly, "ReadOnlyRootFilesystem must be set to true")) } } - if !s.psp.Spec.AllowPrivilegeEscalation && sc.AllowPrivilegeEscalation == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("allowPrivilegeEscalation"), sc.AllowPrivilegeEscalation, "Allowing privilege escalation for containers is not allowed")) - + allowEscalation := sc.AllowPrivilegeEscalation() + if !s.psp.Spec.AllowPrivilegeEscalation && allowEscalation == nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("allowPrivilegeEscalation"), allowEscalation, "Allowing privilege escalation for containers is not allowed")) } - if !s.psp.Spec.AllowPrivilegeEscalation && sc.AllowPrivilegeEscalation != nil && *sc.AllowPrivilegeEscalation { - allErrs = append(allErrs, field.Invalid(fldPath.Child("allowPrivilegeEscalation"), *sc.AllowPrivilegeEscalation, "Allowing privilege escalation for containers is not allowed")) + if !s.psp.Spec.AllowPrivilegeEscalation && allowEscalation != nil && *allowEscalation { + allErrs = append(allErrs, field.Invalid(fldPath.Child("allowPrivilegeEscalation"), *allowEscalation, "Allowing privilege escalation for containers is not allowed")) } return allErrs diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go index d61dc96247..cfba7786c6 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -55,30 +55,21 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) { Name: "psp-sa", Annotations: map[string]string{ seccomp.AllowedProfilesAnnotationKey: "*", - seccomp.DefaultProfileAnnotationKey: "foo", }, }, Spec: extensions.PodSecurityPolicySpec{ - DefaultAddCapabilities: []api.Capability{"foo"}, - RequiredDropCapabilities: []api.Capability{"bar"}, + AllowPrivilegeEscalation: true, RunAsUser: extensions.RunAsUserStrategyOptions{ Rule: extensions.RunAsUserStrategyRunAsAny, }, SELinux: extensions.SELinuxStrategyOptions{ Rule: extensions.SELinuxStrategyRunAsAny, }, - // these are pod mutating strategies that are tested above FSGroup: extensions.FSGroupStrategyOptions{ - Rule: extensions.FSGroupStrategyMustRunAs, - Ranges: []extensions.GroupIDRange{ - {Min: 1, Max: 1}, - }, + Rule: extensions.FSGroupStrategyRunAsAny, }, SupplementalGroups: extensions.SupplementalGroupsStrategyOptions{ - Rule: extensions.SupplementalGroupsStrategyMustRunAs, - Ranges: []extensions.GroupIDRange{ - {Min: 1, Max: 1}, - }, + Rule: extensions.SupplementalGroupsStrategyRunAsAny, }, }, } @@ -91,17 +82,13 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - sc, _, err := provider.CreatePodSecurityContext(pod) + _, _, err = provider.CreatePodSecurityContext(pod) if err != nil { t.Fatalf("unable to create psc %v", err) } - // The generated security context should have filled in missing options, so they should differ - if reflect.DeepEqual(sc, &pod.Spec.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 + // since all the strategies were permissive if !reflect.DeepEqual(createPod(), pod) { diffs := diff.ObjectDiff(createPod(), pod) t.Errorf("pod was mutated by CreatePodSecurityContext. diff:\n%s", diffs) @@ -134,7 +121,6 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) { // Create a PSP with strategies that will populate a blank security context createPSP := func() *extensions.PodSecurityPolicy { - uid := int64(1) return &extensions.PodSecurityPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "psp-sa", @@ -144,25 +130,19 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) { }, }, Spec: extensions.PodSecurityPolicySpec{ - DefaultAddCapabilities: []api.Capability{"foo"}, - RequiredDropCapabilities: []api.Capability{"bar"}, + AllowPrivilegeEscalation: true, RunAsUser: extensions.RunAsUserStrategyOptions{ - Rule: extensions.RunAsUserStrategyMustRunAs, - Ranges: []extensions.UserIDRange{{Min: uid, Max: uid}}, + Rule: extensions.RunAsUserStrategyRunAsAny, }, SELinux: extensions.SELinuxStrategyOptions{ - Rule: extensions.SELinuxStrategyMustRunAs, - SELinuxOptions: &api.SELinuxOptions{User: "you"}, + Rule: extensions.SELinuxStrategyRunAsAny, }, - // 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, }, } } @@ -174,17 +154,13 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - sc, _, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[0]) + _, _, 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") - } - // Creating the provider or the security context should not have mutated the psp or pod + // since all the strategies were permissive if !reflect.DeepEqual(createPod(), pod) { diffs := diff.ObjectDiff(createPod(), pod) t.Errorf("pod was mutated by CreateContainerSecurityContext. diff:\n%s", diffs)