From e5d2cad7b959ecf9ccbe2b79d8b0465c6d980613 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Mon, 25 Mar 2019 11:01:28 -0700 Subject: [PATCH] Refactor PSP provider --- pkg/security/podsecuritypolicy/provider.go | 37 +++++++-- .../podsecuritypolicy/provider_test.go | 76 ++++++++++--------- pkg/security/podsecuritypolicy/types.go | 14 ++-- .../security/podsecuritypolicy/admission.go | 29 +------ 4 files changed, 79 insertions(+), 77 deletions(-) diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go index d347bcbf53d..9c120c23f80 100644 --- a/pkg/security/podsecuritypolicy/provider.go +++ b/pkg/security/podsecuritypolicy/provider.go @@ -59,10 +59,10 @@ func NewSimpleProvider(psp *policy.PodSecurityPolicy, namespace string, strategy }, nil } -// DefaultPodSecurityContext sets the default values of the required but not filled fields. -// It modifies the SecurityContext and annotations of the provided pod. Validation should be -// used after the context is defaulted to ensure it complies with the required restrictions. -func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error { +// MutatePod sets the default values of the required but not filled fields. +// Validation should be used after the context is defaulted to ensure it +// complies with the required restrictions. +func (s *simpleProvider) MutatePod(pod *api.Pod) error { sc := securitycontext.NewPodSecurityContextMutator(pod.Spec.SecurityContext) if sc.SupplementalGroups() == nil { @@ -104,13 +104,25 @@ func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error { pod.Spec.SecurityContext = sc.PodSecurityContext() + for i := range pod.Spec.InitContainers { + if err := s.mutateContainer(pod, &pod.Spec.InitContainers[i]); err != nil { + return err + } + } + + for i := range pod.Spec.Containers { + if err := s.mutateContainer(pod, &pod.Spec.Containers[i]); err != nil { + return err + } + } + return nil } -// DefaultContainerSecurityContext sets the default values of the required but not filled fields. +// mutateContainer sets the default values of the required but not filled fields. // It modifies the SecurityContext of the container and annotations of the pod. Validation should // be used after the context is defaulted to ensure it complies with the required restrictions. -func (s *simpleProvider) DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error { +func (s *simpleProvider) mutateContainer(pod *api.Pod, container *api.Container) error { sc := securitycontext.NewEffectiveContainerSecurityContextMutator( securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext), securitycontext.NewContainerSecurityContextMutator(container.SecurityContext), @@ -282,11 +294,22 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList { } } } + + fldPath := field.NewPath("spec", "initContainers") + for i := range pod.Spec.InitContainers { + allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.InitContainers[i], fldPath.Index(i))...) + } + + fldPath = field.NewPath("spec", "containers") + for i := range pod.Spec.Containers { + allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.Containers[i], fldPath.Index(i))...) + } + return allErrs } // Ensure a container's SecurityContext is in compliance with the given constraints -func (s *simpleProvider) ValidateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList { +func (s *simpleProvider) validateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} podSC := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext) diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go index ad6b31ffcfc..a833da80102 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -31,7 +31,6 @@ import ( policy "k8s.io/api/policy/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" - "k8s.io/apimachinery/pkg/util/validation/field" api "k8s.io/kubernetes/pkg/apis/core" k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1" "k8s.io/kubernetes/pkg/security/apparmor" @@ -41,7 +40,7 @@ import ( const defaultContainerName = "test-c" -func TestDefaultPodSecurityContextNonmutating(t *testing.T) { +func TestMutatePodNonmutating(t *testing.T) { // Create a pod with a security context that needs filling in createPod := func() *api.Pod { return &api.Pod{ @@ -89,7 +88,7 @@ func TestDefaultPodSecurityContextNonmutating(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - err = provider.DefaultPodSecurityContext(pod) + err = provider.MutatePod(pod) if err != nil { t.Fatalf("unable to create psc %v", err) } @@ -98,14 +97,14 @@ func TestDefaultPodSecurityContextNonmutating(t *testing.T) { // since all the strategies were permissive if !reflect.DeepEqual(createPod(), pod) { diffs := diff.ObjectDiff(createPod(), pod) - t.Errorf("pod was mutated by DefaultPodSecurityContext. diff:\n%s", diffs) + t.Errorf("pod was mutated by MutatePod. diff:\n%s", diffs) } if !reflect.DeepEqual(createPSP(), psp) { - t.Error("psp was mutated by DefaultPodSecurityContext") + t.Error("psp was mutated by MutatePod") } } -func TestDefaultContainerSecurityContextNonmutating(t *testing.T) { +func TestMutateContainerNonmutating(t *testing.T) { untrue := false tests := []struct { security *api.SecurityContext @@ -134,7 +133,6 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) { Name: "psp-sa", Annotations: map[string]string{ seccomp.AllowedProfilesAnnotationKey: "*", - seccomp.DefaultProfileAnnotationKey: "foo", }, }, Spec: policy.PodSecurityPolicySpec{ @@ -165,7 +163,7 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0]) + err = provider.MutatePod(pod) if err != nil { t.Fatalf("unable to create container security context %v", err) } @@ -174,15 +172,15 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) { // since all the strategies were permissive if !reflect.DeepEqual(createPod(), pod) { diffs := diff.ObjectDiff(createPod(), pod) - t.Errorf("pod was mutated by DefaultContainerSecurityContext. diff:\n%s", diffs) + t.Errorf("pod was mutated. diff:\n%s", diffs) } if !reflect.DeepEqual(createPSP(), psp) { - t.Error("psp was mutated by DefaultContainerSecurityContext") + t.Error("psp was mutated") } } } -func TestValidatePodSecurityContextFailures(t *testing.T) { +func TestValidatePodFailures(t *testing.T) { failHostNetworkPod := defaultPod() failHostNetworkPod.Spec.SecurityContext.HostNetwork = true @@ -504,6 +502,7 @@ func TestValidateContainerFailures(t *testing.T) { }, } failSELinuxPod := defaultPod() + failSELinuxPod.Spec.SecurityContext.SELinuxOptions = &api.SELinuxOptions{Level: "foo"} failSELinuxPod.Spec.Containers[0].SecurityContext.SELinuxOptions = &api.SELinuxOptions{ Level: "bar", } @@ -620,22 +619,24 @@ func TestValidateContainerFailures(t *testing.T) { } for k, v := range errorCases { - provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory()) - if err != nil { - t.Fatalf("unable to create provider %v", err) - } - errs := provider.ValidateContainer(v.pod, &v.pod.Spec.Containers[0], field.NewPath("")) - if len(errs) == 0 { - t.Errorf("%s expected validation failure but did not receive errors", k) - continue - } - if !strings.Contains(errs[0].Error(), v.expectedError) { - t.Errorf("%s received unexpected error %v\nexpected: %s", k, errs, v.expectedError) - } + t.Run(k, func(t *testing.T) { + provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory()) + if err != nil { + t.Fatalf("unable to create provider %v", err) + } + errs := provider.ValidatePod(v.pod) + if len(errs) == 0 { + t.Errorf("expected validation failure but did not receive errors") + return + } + if !strings.Contains(errs[0].Error(), v.expectedError) { + t.Errorf("unexpected error %v\nexpected: %s", errs, v.expectedError) + } + }) } } -func TestValidatePodSecurityContextSuccess(t *testing.T) { +func TestValidatePodSuccess(t *testing.T) { hostNetworkPSP := defaultPSP() hostNetworkPSP.Spec.HostNetwork = true hostNetworkPod := defaultPod() @@ -941,6 +942,7 @@ func TestValidateContainerSuccess(t *testing.T) { }, } seLinuxPod := defaultPod() + seLinuxPod.Spec.SecurityContext.SELinuxOptions = &api.SELinuxOptions{Level: "foo"} seLinuxPod.Spec.Containers[0].SecurityContext.SELinuxOptions = &api.SELinuxOptions{ Level: "foo", } @@ -1007,6 +1009,7 @@ func TestValidateContainerSuccess(t *testing.T) { seccompPod := defaultPod() seccompPod.Annotations = map[string]string{ + api.SeccompPodAnnotationKey: "foo", api.SeccompContainerAnnotationKeyPrefix + seccompPod.Spec.Containers[0].Name: "foo", } @@ -1074,15 +1077,16 @@ func TestValidateContainerSuccess(t *testing.T) { } for k, v := range successCases { - provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory()) - if err != nil { - t.Fatalf("unable to create provider %v", err) - } - errs := provider.ValidateContainer(v.pod, &v.pod.Spec.Containers[0], field.NewPath("")) - if len(errs) != 0 { - t.Errorf("%s expected validation pass but received errors %v\n%s", k, errs, spew.Sdump(v.pod.ObjectMeta)) - continue - } + t.Run(k, func(t *testing.T) { + provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory()) + if err != nil { + t.Fatalf("unable to create provider %v", err) + } + errs := provider.ValidatePod(v.pod) + if len(errs) != 0 { + t.Errorf("%s expected validation pass but received errors %v\n%s", k, errs, spew.Sdump(v.pod.ObjectMeta)) + } + }) } } @@ -1146,7 +1150,7 @@ func TestGenerateContainerSecurityContextReadOnlyRootFS(t *testing.T) { t.Errorf("%s unable to create provider %v", k, err) continue } - err = provider.DefaultContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0]) + err = provider.MutatePod(v.pod) if err != nil { t.Errorf("%s unable to create container security context %v", k, err) continue @@ -1351,10 +1355,10 @@ func TestAllowPrivilegeEscalation(t *testing.T) { provider, err := NewSimpleProvider(psp, "namespace", NewSimpleStrategyFactory()) require.NoError(t, err) - err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0]) + err = provider.MutatePod(pod) require.NoError(t, err) - errs := provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs := provider.ValidatePod(pod) if test.expectErr { assert.NotEmpty(t, errs, "expected validation error") } else { diff --git a/pkg/security/podsecuritypolicy/types.go b/pkg/security/podsecuritypolicy/types.go index 8caba94dbaf..ea4694ae143 100644 --- a/pkg/security/podsecuritypolicy/types.go +++ b/pkg/security/podsecuritypolicy/types.go @@ -32,16 +32,12 @@ import ( // Provider provides the implementation to generate a new security // context based on constraints or validate an existing security context against constraints. type Provider interface { - // DefaultPodSecurityContext sets the default values of the required but not filled fields. - // It modifies the SecurityContext and annotations of the provided pod. - DefaultPodSecurityContext(pod *api.Pod) error - // DefaultContainerSecurityContext sets the default values of the required but not filled fields. - // It modifies the SecurityContext of the container and annotations of the pod. - DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error - // Ensure a pod is in compliance with the given constraints. + // MutatePod sets the default values of the required but not filled fields of the pod and all + // containers in the pod. + MutatePod(pod *api.Pod) error + // ValidatePod ensures a pod and all its containers are in compliance with the given constraints. + // ValidatePod MUST NOT mutate the pod. ValidatePod(pod *api.Pod) field.ErrorList - // Ensure a container's SecurityContext is in compliance with the given constraints. - ValidateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList // Get the name of the PSP that this provider was initialized with. GetPSPName() string } diff --git a/plugin/pkg/admission/security/podsecuritypolicy/admission.go b/plugin/pkg/admission/security/podsecuritypolicy/admission.go index f980b932212..704324568c5 100644 --- a/plugin/pkg/admission/security/podsecuritypolicy/admission.go +++ b/plugin/pkg/admission/security/podsecuritypolicy/admission.go @@ -306,35 +306,14 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes, func assignSecurityContext(provider psp.Provider, pod *api.Pod) field.ErrorList { errs := field.ErrorList{} - err := provider.DefaultPodSecurityContext(pod) - if err != nil { - errs = append(errs, field.Invalid(field.NewPath("spec", "securityContext"), pod.Spec.SecurityContext, err.Error())) + if err := provider.MutatePod(pod); err != nil { + // TODO(tallclair): MutatePod should return a field.ErrorList + errs = append(errs, field.Invalid(field.NewPath(""), pod, err.Error())) } errs = append(errs, provider.ValidatePod(pod)...) - for i := range pod.Spec.InitContainers { - err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.InitContainers[i]) - if err != nil { - errs = append(errs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("securityContext"), "", err.Error())) - continue - } - errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i))...) - } - - for i := range pod.Spec.Containers { - err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[i]) - if err != nil { - errs = append(errs, field.Invalid(field.NewPath("spec", "containers").Index(i).Child("securityContext"), "", err.Error())) - continue - } - errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i))...) - } - - if len(errs) > 0 { - return errs - } - return nil + return errs } // createProvidersFromPolicies creates providers from the constraints supplied.