From 5ace0f03d806e77c4b59aa64459a45654bdd54d8 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 17 Jul 2018 17:41:50 -0700 Subject: [PATCH] Cleanup & fix PodSecurityPolicy field path usage --- .../podsecuritypolicy/capabilities/BUILD | 1 + .../capabilities/capabilities.go | 8 +-- .../capabilities/capabilities_test.go | 5 +- .../podsecuritypolicy/capabilities/types.go | 2 +- pkg/security/podsecuritypolicy/factory.go | 4 +- pkg/security/podsecuritypolicy/group/BUILD | 5 +- .../podsecuritypolicy/group/mustrunas.go | 10 ++-- .../podsecuritypolicy/group/mustrunas_test.go | 9 +-- .../podsecuritypolicy/group/runasany.go | 2 +- .../podsecuritypolicy/group/runasany_test.go | 4 +- pkg/security/podsecuritypolicy/group/types.go | 2 +- pkg/security/podsecuritypolicy/provider.go | 59 +++++++------------ .../podsecuritypolicy/provider_test.go | 32 +++++----- pkg/security/podsecuritypolicy/types.go | 6 +- .../podsecuritypolicy/user/mustrunas.go | 6 +- .../podsecuritypolicy/user/nonroot.go | 8 +-- .../podsecuritypolicy/user/runasany.go | 2 +- pkg/security/podsecuritypolicy/user/types.go | 3 +- .../security/podsecuritypolicy/admission.go | 10 ++-- .../podsecuritypolicy/admission_test.go | 2 +- 20 files changed, 86 insertions(+), 94 deletions(-) diff --git a/pkg/security/podsecuritypolicy/capabilities/BUILD b/pkg/security/podsecuritypolicy/capabilities/BUILD index c57050b140..c86bcd5b90 100644 --- a/pkg/security/podsecuritypolicy/capabilities/BUILD +++ b/pkg/security/podsecuritypolicy/capabilities/BUILD @@ -29,6 +29,7 @@ go_test( deps = [ "//pkg/apis/core:go_default_library", "//pkg/apis/policy:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", ], ) diff --git a/pkg/security/podsecuritypolicy/capabilities/capabilities.go b/pkg/security/podsecuritypolicy/capabilities/capabilities.go index 0e1f41c617..f2fd012a2f 100644 --- a/pkg/security/podsecuritypolicy/capabilities/capabilities.go +++ b/pkg/security/podsecuritypolicy/capabilities/capabilities.go @@ -81,7 +81,7 @@ func (s *defaultCapabilities) Generate(pod *api.Pod, container *api.Container) ( } // Validate ensures that the specified values fall within the range of the strategy. -func (s *defaultCapabilities) Validate(pod *api.Pod, container *api.Container, capabilities *api.Capabilities) field.ErrorList { +func (s *defaultCapabilities) Validate(fldPath *field.Path, pod *api.Pod, container *api.Container, capabilities *api.Capabilities) field.ErrorList { allErrs := field.ErrorList{} if capabilities == nil { @@ -94,7 +94,7 @@ func (s *defaultCapabilities) Validate(pod *api.Pod, container *api.Container, c // container has no requested caps but we have required caps. We should have something in // at least the drops on the container. - allErrs = append(allErrs, field.Invalid(field.NewPath("capabilities"), capabilities, + allErrs = append(allErrs, field.Invalid(fldPath, capabilities, "required capabilities are not set on the securityContext")) return allErrs } @@ -112,7 +112,7 @@ func (s *defaultCapabilities) Validate(pod *api.Pod, container *api.Container, c for _, cap := range capabilities.Add { sCap := string(cap) if !defaultAdd.Has(sCap) && !allowedAdd.Has(sCap) { - allErrs = append(allErrs, field.Invalid(field.NewPath("capabilities", "add"), sCap, "capability may not be added")) + allErrs = append(allErrs, field.Invalid(fldPath.Child("add"), sCap, "capability may not be added")) } } @@ -122,7 +122,7 @@ func (s *defaultCapabilities) Validate(pod *api.Pod, container *api.Container, c for _, requiredDrop := range s.requiredDropCapabilities { sDrop := string(requiredDrop) if !containerDrops.Has(sDrop) { - allErrs = append(allErrs, field.Invalid(field.NewPath("capabilities", "drop"), capabilities.Drop, + allErrs = append(allErrs, field.Invalid(fldPath.Child("drop"), capabilities.Drop, fmt.Sprintf("%s is required to be dropped but was not found", sDrop))) } } diff --git a/pkg/security/podsecuritypolicy/capabilities/capabilities_test.go b/pkg/security/podsecuritypolicy/capabilities/capabilities_test.go index 6f752a0315..2bf32b8599 100644 --- a/pkg/security/podsecuritypolicy/capabilities/capabilities_test.go +++ b/pkg/security/podsecuritypolicy/capabilities/capabilities_test.go @@ -20,6 +20,7 @@ import ( "reflect" "testing" + "k8s.io/apimachinery/pkg/util/validation/field" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/policy" ) @@ -329,7 +330,7 @@ func TestValidateAdds(t *testing.T) { t.Errorf("%s failed: %v", k, err) continue } - errs := strategy.Validate(nil, nil, v.containerCaps) + errs := strategy.Validate(field.NewPath("capabilities"), nil, nil, v.containerCaps) if v.expectedError == "" && len(errs) > 0 { t.Errorf("%s should have passed but had errors %v", k, errs) continue @@ -390,7 +391,7 @@ func TestValidateDrops(t *testing.T) { t.Errorf("%s failed: %v", k, err) continue } - errs := strategy.Validate(nil, nil, v.containerCaps) + errs := strategy.Validate(field.NewPath("capabilities"), nil, nil, v.containerCaps) if v.expectedError == "" && len(errs) > 0 { t.Errorf("%s should have passed but had errors %v", k, errs) continue diff --git a/pkg/security/podsecuritypolicy/capabilities/types.go b/pkg/security/podsecuritypolicy/capabilities/types.go index 47226fabab..093653bdb6 100644 --- a/pkg/security/podsecuritypolicy/capabilities/types.go +++ b/pkg/security/podsecuritypolicy/capabilities/types.go @@ -26,5 +26,5 @@ type Strategy interface { // Generate creates the capabilities based on policy rules. Generate(pod *api.Pod, container *api.Container) (*api.Capabilities, error) // Validate ensures that the specified values fall within the range of the strategy. - Validate(pod *api.Pod, container *api.Container, capabilities *api.Capabilities) field.ErrorList + Validate(fldPath *field.Path, pod *api.Pod, container *api.Container, capabilities *api.Capabilities) field.ErrorList } diff --git a/pkg/security/podsecuritypolicy/factory.go b/pkg/security/podsecuritypolicy/factory.go index 05c7f0d8bb..cfa13ee10c 100644 --- a/pkg/security/podsecuritypolicy/factory.go +++ b/pkg/security/podsecuritypolicy/factory.go @@ -139,7 +139,7 @@ func createFSGroupStrategy(opts *policy.FSGroupStrategyOptions) (group.GroupStra case policy.FSGroupStrategyRunAsAny: return group.NewRunAsAny() case policy.FSGroupStrategyMustRunAs: - return group.NewMustRunAs(opts.Ranges, fsGroupField) + return group.NewMustRunAs(opts.Ranges) default: return nil, fmt.Errorf("Unrecognized FSGroup strategy type %s", opts.Rule) } @@ -151,7 +151,7 @@ func createSupplementalGroupStrategy(opts *policy.SupplementalGroupsStrategyOpti case policy.SupplementalGroupsStrategyRunAsAny: return group.NewRunAsAny() case policy.SupplementalGroupsStrategyMustRunAs: - return group.NewMustRunAs(opts.Ranges, supplementalGroupsField) + return group.NewMustRunAs(opts.Ranges) default: return nil, fmt.Errorf("Unrecognized SupplementalGroups strategy type %s", opts.Rule) } diff --git a/pkg/security/podsecuritypolicy/group/BUILD b/pkg/security/podsecuritypolicy/group/BUILD index 7bead94cef..20f49accde 100644 --- a/pkg/security/podsecuritypolicy/group/BUILD +++ b/pkg/security/podsecuritypolicy/group/BUILD @@ -30,7 +30,10 @@ go_test( "runasany_test.go", ], embed = [":go_default_library"], - deps = ["//pkg/apis/policy:go_default_library"], + deps = [ + "//pkg/apis/policy:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + ], ) filegroup( diff --git a/pkg/security/podsecuritypolicy/group/mustrunas.go b/pkg/security/podsecuritypolicy/group/mustrunas.go index 9e2b8b8791..544f473424 100644 --- a/pkg/security/podsecuritypolicy/group/mustrunas.go +++ b/pkg/security/podsecuritypolicy/group/mustrunas.go @@ -28,19 +28,17 @@ import ( // mustRunAs implements the GroupStrategy interface type mustRunAs struct { ranges []policy.IDRange - field string } var _ GroupStrategy = &mustRunAs{} // NewMustRunAs provides a new MustRunAs strategy based on ranges. -func NewMustRunAs(ranges []policy.IDRange, field string) (GroupStrategy, error) { +func NewMustRunAs(ranges []policy.IDRange) (GroupStrategy, error) { if len(ranges) == 0 { return nil, fmt.Errorf("ranges must be supplied for MustRunAs") } return &mustRunAs{ ranges: ranges, - field: field, }, nil } @@ -61,17 +59,17 @@ func (s *mustRunAs) GenerateSingle(_ *api.Pod) (*int64, error) { // Validate ensures that the specified values fall within the range of the strategy. // Groups are passed in here to allow this strategy to support multiple group fields (fsgroup and // supplemental groups). -func (s *mustRunAs) Validate(_ *api.Pod, groups []int64) field.ErrorList { +func (s *mustRunAs) Validate(fldPath *field.Path, _ *api.Pod, groups []int64) field.ErrorList { allErrs := field.ErrorList{} if len(groups) == 0 && len(s.ranges) > 0 { - allErrs = append(allErrs, field.Invalid(field.NewPath(s.field), groups, "unable to validate empty groups against required ranges")) + allErrs = append(allErrs, field.Invalid(fldPath, groups, "unable to validate empty groups against required ranges")) } for _, group := range groups { if !s.isGroupValid(group) { detail := fmt.Sprintf("group %d must be in the ranges: %v", group, s.ranges) - allErrs = append(allErrs, field.Invalid(field.NewPath(s.field), groups, detail)) + allErrs = append(allErrs, field.Invalid(fldPath, groups, detail)) } } diff --git a/pkg/security/podsecuritypolicy/group/mustrunas_test.go b/pkg/security/podsecuritypolicy/group/mustrunas_test.go index 3d7c17e33e..3cd5fdce99 100644 --- a/pkg/security/podsecuritypolicy/group/mustrunas_test.go +++ b/pkg/security/podsecuritypolicy/group/mustrunas_test.go @@ -20,6 +20,7 @@ import ( "strings" "testing" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/apis/policy" ) @@ -40,7 +41,7 @@ func TestMustRunAsOptions(t *testing.T) { } for k, v := range tests { - _, err := NewMustRunAs(v.ranges, "") + _, err := NewMustRunAs(v.ranges) if v.pass && err != nil { t.Errorf("error creating strategy for %s: %v", k, err) } @@ -77,7 +78,7 @@ func TestGenerate(t *testing.T) { } for k, v := range tests { - s, err := NewMustRunAs(v.ranges, "") + s, err := NewMustRunAs(v.ranges) if err != nil { t.Errorf("error creating strategy for %s: %v", k, err) } @@ -161,11 +162,11 @@ func TestValidate(t *testing.T) { } for k, v := range tests { - s, err := NewMustRunAs(v.ranges, "") + s, err := NewMustRunAs(v.ranges) if err != nil { t.Errorf("error creating strategy for %s: %v", k, err) } - errs := s.Validate(nil, v.groups) + errs := s.Validate(field.NewPath(""), nil, v.groups) if v.expectedError == "" && len(errs) > 0 { t.Errorf("unexpected errors for %s: %v", k, errs) } diff --git a/pkg/security/podsecuritypolicy/group/runasany.go b/pkg/security/podsecuritypolicy/group/runasany.go index 3890f84991..4fb9e86f33 100644 --- a/pkg/security/podsecuritypolicy/group/runasany.go +++ b/pkg/security/podsecuritypolicy/group/runasany.go @@ -43,7 +43,7 @@ func (s *runAsAny) GenerateSingle(_ *api.Pod) (*int64, error) { } // Validate ensures that the specified values fall within the range of the strategy. -func (s *runAsAny) Validate(_ *api.Pod, groups []int64) field.ErrorList { +func (s *runAsAny) Validate(fldPath *field.Path, _ *api.Pod, groups []int64) field.ErrorList { return field.ErrorList{} } diff --git a/pkg/security/podsecuritypolicy/group/runasany_test.go b/pkg/security/podsecuritypolicy/group/runasany_test.go index 86c1066224..788b6363ff 100644 --- a/pkg/security/podsecuritypolicy/group/runasany_test.go +++ b/pkg/security/podsecuritypolicy/group/runasany_test.go @@ -18,6 +18,8 @@ package group import ( "testing" + + "k8s.io/apimachinery/pkg/util/validation/field" ) func TestRunAsAnyGenerate(t *testing.T) { @@ -53,7 +55,7 @@ func TestRunAsAnyValidte(t *testing.T) { if err != nil { t.Fatalf("unexpected error initializing NewRunAsAny %v", err) } - errs := s.Validate(nil, nil) + errs := s.Validate(field.NewPath(""), nil, nil) if len(errs) != 0 { t.Errorf("unexpected errors: %v", errs) } diff --git a/pkg/security/podsecuritypolicy/group/types.go b/pkg/security/podsecuritypolicy/group/types.go index dbf1520421..479469ba11 100644 --- a/pkg/security/podsecuritypolicy/group/types.go +++ b/pkg/security/podsecuritypolicy/group/types.go @@ -31,5 +31,5 @@ type GroupStrategy interface { // value to return if configured with multiple ranges. This is used for FSGroup. GenerateSingle(pod *api.Pod) (*int64, error) // Validate ensures that the specified values fall within the range of the strategy. - Validate(pod *api.Pod, groups []int64) field.ErrorList + Validate(fldPath *field.Path, pod *api.Pod, groups []int64) field.ErrorList } diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go index 7458eb8a50..da82327f32 100644 --- a/pkg/security/podsecuritypolicy/provider.go +++ b/pkg/security/podsecuritypolicy/provider.go @@ -27,13 +27,6 @@ import ( "k8s.io/kubernetes/pkg/securitycontext" ) -// used to pass in the field being validated for reusable group strategies so they -// can create informative error messages. -const ( - fsGroupField = "fsGroup" - supplementalGroupsField = "supplementalGroups" -) - // simpleProvider is the default implementation of Provider. type simpleProvider struct { psp *policy.PodSecurityPolicy @@ -180,31 +173,32 @@ func (s *simpleProvider) DefaultContainerSecurityContext(pod *api.Pod, container } // ValidatePod ensure a pod is in compliance with the given constraints. -func (s *simpleProvider) ValidatePod(pod *api.Pod, fldPath *field.Path) field.ErrorList { +func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList { allErrs := field.ErrorList{} sc := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext) + scPath := field.NewPath("spec", "securityContext") - fsGroups := []int64{} + var fsGroups []int64 if fsGroup := sc.FSGroup(); fsGroup != nil { - fsGroups = append(fsGroups, *fsGroup) + fsGroups = []int64{*fsGroup} } - allErrs = append(allErrs, s.strategies.FSGroupStrategy.Validate(pod, fsGroups)...) - allErrs = append(allErrs, s.strategies.SupplementalGroupStrategy.Validate(pod, sc.SupplementalGroups())...) + allErrs = append(allErrs, s.strategies.FSGroupStrategy.Validate(scPath.Child("fsGroup"), pod, fsGroups)...) + allErrs = append(allErrs, s.strategies.SupplementalGroupStrategy.Validate(scPath.Child("supplementalGroups"), pod, sc.SupplementalGroups())...) allErrs = append(allErrs, s.strategies.SeccompStrategy.ValidatePod(pod)...) - allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, nil, sc.SELinuxOptions())...) + allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(scPath.Child("seLinuxOptions"), pod, nil, sc.SELinuxOptions())...) 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")) + allErrs = append(allErrs, field.Invalid(scPath.Child("hostNetwork"), sc.HostNetwork(), "Host network 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")) + allErrs = append(allErrs, field.Invalid(scPath.Child("hostPID"), sc.HostPID(), "Host PID 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, field.Invalid(scPath.Child("hostIPC"), sc.HostIPC(), "Host IPC is not allowed to be used")) } allErrs = append(allErrs, s.strategies.SysctlsStrategy.Validate(pod)...) @@ -268,7 +262,7 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod, fldPath *field.Path) field.Er } if !found { allErrs = append(allErrs, - field.Invalid(fldPath.Child("volumes").Index(i).Child("driver"), driver, + field.Invalid(field.NewPath("spec", "volumes").Index(i).Child("driver"), driver, "Flexvolume driver is not allowed to be used")) } } @@ -278,52 +272,43 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod, fldPath *field.Path) field.Er } // Ensure a container's SecurityContext is in compliance with the given constraints -func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, container *api.Container, fldPath *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) sc := securitycontext.NewEffectiveContainerSecurityContextAccessor(podSC, securitycontext.NewContainerSecurityContextMutator(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())...) + scPath := containerPath.Child("securityContext") + allErrs = append(allErrs, s.strategies.RunAsUserStrategy.Validate(scPath, pod, container, sc.RunAsNonRoot(), sc.RunAsUser())...) + allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(scPath.Child("seLinuxOptions"), pod, container, sc.SELinuxOptions())...) allErrs = append(allErrs, s.strategies.AppArmorStrategy.Validate(pod, container)...) allErrs = append(allErrs, s.strategies.SeccompStrategy.ValidateContainer(pod, container)...) 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, field.Invalid(scPath.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(scPath.Child("capabilities"), pod, container, sc.Capabilities())...) - containersPath := fldPath.Child("containers") - for idx, c := range pod.Spec.Containers { - idxPath := containersPath.Index(idx) - allErrs = append(allErrs, s.hasInvalidHostPort(&c, idxPath)...) - } - - containersPath = fldPath.Child("initContainers") - for idx, c := range pod.Spec.InitContainers { - idxPath := containersPath.Index(idx) - allErrs = append(allErrs, s.hasInvalidHostPort(&c, idxPath)...) - } + allErrs = append(allErrs, s.hasInvalidHostPort(container, containerPath)...) if s.psp.Spec.ReadOnlyRootFilesystem { 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")) + allErrs = append(allErrs, field.Invalid(scPath.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")) + allErrs = append(allErrs, field.Invalid(scPath.Child("readOnlyRootFilesystem"), *readOnly, "ReadOnlyRootFilesystem must be set to true")) } } 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")) + allErrs = append(allErrs, field.Invalid(scPath.Child("allowPrivilegeEscalation"), allowEscalation, "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")) + allErrs = append(allErrs, field.Invalid(scPath.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 a7b8174a96..4ef7129ea6 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -412,7 +412,7 @@ func TestValidatePodSecurityContextFailures(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - errs := provider.ValidatePod(v.pod, field.NewPath("")) + errs := provider.ValidatePod(v.pod) if len(errs) == 0 { t.Errorf("%s expected validation failure but did not receive errors", k) continue @@ -445,7 +445,7 @@ func allowFlexVolumesPSP(allowAllFlexVolumes, allowAllVolumes bool) *policy.PodS return psp } -func TestValidateContainerSecurityContextFailures(t *testing.T) { +func TestValidateContainerFailures(t *testing.T) { // fail user strategy failUserPSP := defaultPSP() uid := int64(999) @@ -577,7 +577,7 @@ func TestValidateContainerSecurityContextFailures(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - errs := provider.ValidateContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0], field.NewPath("")) + 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 @@ -836,7 +836,7 @@ func TestValidatePodSecurityContextSuccess(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - errs := provider.ValidatePod(v.pod, field.NewPath("")) + errs := provider.ValidatePod(v.pod) if len(errs) != 0 { t.Errorf("%s expected validation pass but received errors %v", k, errs) continue @@ -844,7 +844,7 @@ func TestValidatePodSecurityContextSuccess(t *testing.T) { } } -func TestValidateContainerSecurityContextSuccess(t *testing.T) { +func TestValidateContainerSuccess(t *testing.T) { // success user strategy userPSP := defaultPSP() uid := int64(999) @@ -1001,7 +1001,7 @@ func TestValidateContainerSecurityContextSuccess(t *testing.T) { if err != nil { t.Fatalf("unable to create provider %v", err) } - errs := provider.ValidateContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0], field.NewPath("")) + 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 @@ -1198,7 +1198,7 @@ func TestValidateAllowedVolumes(t *testing.T) { } // expect a denial for this PSP and test the error message to ensure it's related to the volumesource - errs := provider.ValidatePod(pod, field.NewPath("")) + errs := provider.ValidatePod(pod) if len(errs) != 1 { t.Errorf("expected exactly 1 error for %s but got %v", fieldVal.Name, errs) } else { @@ -1209,14 +1209,14 @@ func TestValidateAllowedVolumes(t *testing.T) { // now add the fstype directly to the psp and it should validate psp.Spec.Volumes = []policy.FSType{fsType} - errs = provider.ValidatePod(pod, field.NewPath("")) + errs = provider.ValidatePod(pod) if len(errs) != 0 { t.Errorf("directly allowing volume expected no errors for %s but got %v", fieldVal.Name, errs) } // now change the psp to allow any volumes and the pod should still validate psp.Spec.Volumes = []policy.FSType{policy.All} - errs = provider.ValidatePod(pod, field.NewPath("")) + errs = provider.ValidatePod(pod) if len(errs) != 0 { t.Errorf("wildcard volume expected no errors for %s but got %v", fieldVal.Name, errs) } @@ -1241,7 +1241,7 @@ func TestValidateAllowPrivilegeEscalation(t *testing.T) { } // expect a denial for this PSP and test the error message to ensure it's related to allowPrivilegeEscalation - errs := provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs := provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 1 { t.Errorf("expected exactly 1 error but got %v", errs) } else { @@ -1252,7 +1252,7 @@ func TestValidateAllowPrivilegeEscalation(t *testing.T) { // now add allowPrivilegeEscalation to the podSecurityPolicy psp.Spec.AllowPrivilegeEscalation = true - errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs = provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 0 { t.Errorf("directly allowing privilege escalation expected no errors but got %v", errs) } @@ -1278,7 +1278,7 @@ func TestValidateDefaultAllowPrivilegeEscalation(t *testing.T) { } // expect a denial for this PSP and test the error message to ensure it's related to allowPrivilegeEscalation - errs := provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs := provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 1 { t.Errorf("expected exactly 1 error but got %v", errs) } else { @@ -1294,7 +1294,7 @@ func TestValidateDefaultAllowPrivilegeEscalation(t *testing.T) { // expect a denial for this PSP because we did not allowPrivilege Escalation via the PodSecurityPolicy // and test the error message to ensure it's related to allowPrivilegeEscalation - errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs = provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 1 { t.Errorf("expected exactly 1 error but got %v", errs) } else { @@ -1305,7 +1305,7 @@ func TestValidateDefaultAllowPrivilegeEscalation(t *testing.T) { // Now set AllowPrivilegeEscalation psp.Spec.AllowPrivilegeEscalation = true - errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs = provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 0 { t.Errorf("directly allowing privilege escalation expected no errors but got %v", errs) } @@ -1313,7 +1313,7 @@ func TestValidateDefaultAllowPrivilegeEscalation(t *testing.T) { // Now set the psp spec to false and reset AllowPrivilegeEscalation psp.Spec.AllowPrivilegeEscalation = false pod.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = nil - errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs = provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 1 { t.Errorf("expected exactly 1 error but got %v", errs) } else { @@ -1325,7 +1325,7 @@ func TestValidateDefaultAllowPrivilegeEscalation(t *testing.T) { // Now unset both AllowPrivilegeEscalation psp.Spec.AllowPrivilegeEscalation = true pod.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = nil - errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath("")) + errs = provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath("")) if len(errs) != 0 { t.Errorf("resetting allowing privilege escalation expected no errors but got %v", errs) } diff --git a/pkg/security/podsecuritypolicy/types.go b/pkg/security/podsecuritypolicy/types.go index ee7fdae1e9..1127337b4e 100644 --- a/pkg/security/podsecuritypolicy/types.go +++ b/pkg/security/podsecuritypolicy/types.go @@ -39,9 +39,9 @@ type Provider interface { // 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. - ValidatePod(pod *api.Pod, fldPath *field.Path) field.ErrorList - // Ensure a container's SecurityContext is in compliance with the given constraints - ValidateContainerSecurityContext(pod *api.Pod, container *api.Container, fldPath *field.Path) field.ErrorList + 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/pkg/security/podsecuritypolicy/user/mustrunas.go b/pkg/security/podsecuritypolicy/user/mustrunas.go index fdcf519ed4..995e785388 100644 --- a/pkg/security/podsecuritypolicy/user/mustrunas.go +++ b/pkg/security/podsecuritypolicy/user/mustrunas.go @@ -49,17 +49,17 @@ func (s *mustRunAs) Generate(pod *api.Pod, container *api.Container) (*int64, er } // Validate ensures that the specified values fall within the range of the strategy. -func (s *mustRunAs) Validate(fldPath *field.Path, _ *api.Pod, _ *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList { +func (s *mustRunAs) Validate(scPath *field.Path, _ *api.Pod, _ *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList { allErrs := field.ErrorList{} if runAsUser == nil { - allErrs = append(allErrs, field.Required(fldPath.Child("runAsUser"), "")) + allErrs = append(allErrs, field.Required(scPath.Child("runAsUser"), "")) return allErrs } if !s.isValidUID(*runAsUser) { detail := fmt.Sprintf("must be in the ranges: %v", s.opts.Ranges) - allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsUser"), *runAsUser, detail)) + allErrs = append(allErrs, field.Invalid(scPath.Child("runAsUser"), *runAsUser, detail)) } return allErrs } diff --git a/pkg/security/podsecuritypolicy/user/nonroot.go b/pkg/security/podsecuritypolicy/user/nonroot.go index 64c32caa03..04c26dcaeb 100644 --- a/pkg/security/podsecuritypolicy/user/nonroot.go +++ b/pkg/security/podsecuritypolicy/user/nonroot.go @@ -41,18 +41,18 @@ func (s *nonRoot) Generate(pod *api.Pod, container *api.Container) (*int64, erro // 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(fldPath *field.Path, _ *api.Pod, _ *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList { +func (s *nonRoot) Validate(scPath *field.Path, _ *api.Pod, _ *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList { allErrs := field.ErrorList{} if runAsNonRoot == nil && runAsUser == nil { - allErrs = append(allErrs, field.Required(fldPath.Child("runAsNonRoot"), "must be true")) + allErrs = append(allErrs, field.Required(scPath.Child("runAsNonRoot"), "must be true")) return allErrs } if runAsNonRoot != nil && *runAsNonRoot == false { - allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsNonRoot"), *runAsNonRoot, "must be true")) + allErrs = append(allErrs, field.Invalid(scPath.Child("runAsNonRoot"), *runAsNonRoot, "must be true")) return allErrs } if runAsUser != nil && *runAsUser == 0 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsUser"), *runAsUser, "running with the root UID is forbidden")) + allErrs = append(allErrs, field.Invalid(scPath.Child("runAsUser"), *runAsUser, "running with the root UID is forbidden")) return allErrs } return allErrs diff --git a/pkg/security/podsecuritypolicy/user/runasany.go b/pkg/security/podsecuritypolicy/user/runasany.go index 935338978f..ea31b6dd0a 100644 --- a/pkg/security/podsecuritypolicy/user/runasany.go +++ b/pkg/security/podsecuritypolicy/user/runasany.go @@ -38,6 +38,6 @@ func (s *runAsAny) Generate(pod *api.Pod, container *api.Container) (*int64, err } // Validate ensures that the specified values fall within the range of the strategy. -func (s *runAsAny) Validate(fldPath *field.Path, _ *api.Pod, _ *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList { +func (s *runAsAny) Validate(_ *field.Path, _ *api.Pod, _ *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList { return field.ErrorList{} } diff --git a/pkg/security/podsecuritypolicy/user/types.go b/pkg/security/podsecuritypolicy/user/types.go index fbcc34c79a..035d0607b4 100644 --- a/pkg/security/podsecuritypolicy/user/types.go +++ b/pkg/security/podsecuritypolicy/user/types.go @@ -26,5 +26,6 @@ type RunAsUserStrategy interface { // Generate creates the uid based on policy rules. Generate(pod *api.Pod, container *api.Container) (*int64, error) // Validate ensures that the specified values fall within the range of the strategy. - Validate(fldPath *field.Path, pod *api.Pod, container *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList + // scPath is the field path to the container's security context + Validate(scPath *field.Path, pod *api.Pod, container *api.Container, runAsNonRoot *bool, runAsUser *int64) field.ErrorList } diff --git a/plugin/pkg/admission/security/podsecuritypolicy/admission.go b/plugin/pkg/admission/security/podsecuritypolicy/admission.go index 1c1b571f82..ec5ccb0240 100644 --- a/plugin/pkg/admission/security/podsecuritypolicy/admission.go +++ b/plugin/pkg/admission/security/podsecuritypolicy/admission.go @@ -258,7 +258,7 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes, for _, provider := range providers { podCopy := pod.DeepCopy() - if errs := assignSecurityContext(provider, podCopy, field.NewPath(fmt.Sprintf("provider %s: ", provider.GetPSPName()))); len(errs) > 0 { + if errs := assignSecurityContext(provider, podCopy); len(errs) > 0 { validationErrs[provider.GetPSPName()] = errs continue } @@ -303,7 +303,7 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes, // assignSecurityContext creates a security context for each container in the pod // and validates that the sc falls within the psp constraints. All containers must validate against // the same psp or is not considered valid. -func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.Path) field.ErrorList { +func assignSecurityContext(provider psp.Provider, pod *api.Pod) field.ErrorList { errs := field.ErrorList{} err := provider.DefaultPodSecurityContext(pod) @@ -311,7 +311,7 @@ func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.P errs = append(errs, field.Invalid(field.NewPath("spec", "securityContext"), pod.Spec.SecurityContext, err.Error())) } - errs = append(errs, provider.ValidatePod(pod, field.NewPath("spec", "securityContext"))...) + errs = append(errs, provider.ValidatePod(pod)...) for i := range pod.Spec.InitContainers { err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.InitContainers[i]) @@ -319,7 +319,7 @@ func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.P errs = append(errs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("securityContext"), "", err.Error())) continue } - errs = append(errs, provider.ValidateContainerSecurityContext(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i).Child("securityContext"))...) + errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i))...) } for i := range pod.Spec.Containers { @@ -328,7 +328,7 @@ func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.P errs = append(errs, field.Invalid(field.NewPath("spec", "containers").Index(i).Child("securityContext"), "", err.Error())) continue } - errs = append(errs, provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i).Child("securityContext"))...) + errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i))...) } if len(errs) > 0 { diff --git a/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go b/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go index 92d4a36da5..24c9153ee0 100644 --- a/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go +++ b/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go @@ -1876,7 +1876,7 @@ func TestAssignSecurityContext(t *testing.T) { } for k, v := range testCases { - errs := assignSecurityContext(provider, v.pod, nil) + errs := assignSecurityContext(provider, v.pod) if v.shouldValidate && len(errs) > 0 { t.Errorf("%s expected to validate but received errors %v", k, errs) continue