diff --git a/pkg/api/helpers.go b/pkg/api/helpers.go index c78ef0e89a..f70f959f01 100644 --- a/pkg/api/helpers.go +++ b/pkg/api/helpers.go @@ -577,7 +577,7 @@ func SysctlsFromPodAnnotation(annotation string) ([]Sysctl, error) { sysctls := make([]Sysctl, len(kvs)) for i, kv := range kvs { cs := strings.Split(kv, "=") - if len(cs) != 2 { + if len(cs) != 2 || len(cs[0]) == 0 { return nil, fmt.Errorf("sysctl %q not of the format sysctl_name=value", kv) } sysctls[i].Name = cs[0] diff --git a/pkg/api/helpers_test.go b/pkg/api/helpers_test.go index 70abca2a1b..1e8a5bd222 100644 --- a/pkg/api/helpers_test.go +++ b/pkg/api/helpers_test.go @@ -508,6 +508,14 @@ func TestSysctlsFromPodAnnotation(t *testing.T) { annotation: "foo.bar", expectErr: true, }, + { + annotation: "=123", + expectErr: true, + }, + { + annotation: "foo.bar=", + expectValue: []Sysctl{{Name: "foo.bar", Value: ""}}, + }, { annotation: "foo.bar=42", expectValue: []Sysctl{{Name: "foo.bar", Value: "42"}}, diff --git a/pkg/security/podsecuritypolicy/factory.go b/pkg/security/podsecuritypolicy/factory.go index 8b187a45ab..64480b38d1 100644 --- a/pkg/security/podsecuritypolicy/factory.go +++ b/pkg/security/podsecuritypolicy/factory.go @@ -79,10 +79,7 @@ func (f *simpleStrategyFactory) CreateStrategies(psp *extensions.PodSecurityPoli errs = append(errs, err) } } - sysctlsStrat, err := createSysctlsStrategy(unsafeSysctls) - if err != nil { - errs = append(errs, err) - } + sysctlsStrat := createSysctlsStrategy(unsafeSysctls) if len(errs) > 0 { return nil, errors.NewAggregate(errs) @@ -162,6 +159,6 @@ func createCapabilitiesStrategy(defaultAddCaps, requiredDropCaps, allowedCaps [] } // createSysctlsStrategy creates a new unsafe sysctls strategy. -func createSysctlsStrategy(sysctlsPatterns []string) (sysctl.SysctlsStrategy, error) { +func createSysctlsStrategy(sysctlsPatterns []string) sysctl.SysctlsStrategy { return sysctl.NewMustMatchPatterns(sysctlsPatterns) } diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go index 3529cd46dd..ea06aa4d7a 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -226,6 +226,18 @@ func TestValidatePodSecurityContextFailures(t *testing.T) { }, } + failOtherSysctlsAllowedPSP := defaultPSP() + failOtherSysctlsAllowedPSP.Annotations[extensions.SysctlsPodSecurityPolicyAnnotationKey] = "bar,abc" + + failNoSysctlAllowedPSP := defaultPSP() + failNoSysctlAllowedPSP.Annotations[extensions.SysctlsPodSecurityPolicyAnnotationKey] = "" + + failSafeSysctlFooPod := defaultPod() + failSafeSysctlFooPod.Annotations[api.SysctlsPodAnnotationKey] = "foo=1" + + failUnsafeSysctlFooPod := defaultPod() + failUnsafeSysctlFooPod.Annotations[api.UnsafeSysctlsPodAnnotationKey] = "foo=1" + errorCases := map[string]struct { pod *api.Pod psp *extensions.PodSecurityPolicy @@ -281,6 +293,26 @@ func TestValidatePodSecurityContextFailures(t *testing.T) { psp: defaultPSP(), expectedError: "hostPath volumes are not allowed to be used", }, + "failSafeSysctlFooPod with failNoSysctlAllowedSCC": { + pod: failSafeSysctlFooPod, + psp: failNoSysctlAllowedPSP, + expectedError: "sysctls are not allowed", + }, + "failUnsafeSysctlFooPod with failNoSysctlAllowedSCC": { + pod: failUnsafeSysctlFooPod, + psp: failNoSysctlAllowedPSP, + expectedError: "sysctls are not allowed", + }, + "failSafeSysctlFooPod with failOtherSysctlsAllowedSCC": { + pod: failSafeSysctlFooPod, + psp: failOtherSysctlsAllowedPSP, + expectedError: "sysctl \"foo\" is not allowed", + }, + "failUnsafeSysctlFooPod with failOtherSysctlsAllowedSCC": { + pod: failUnsafeSysctlFooPod, + psp: failOtherSysctlsAllowedPSP, + expectedError: "sysctl \"foo\" is not allowed", + }, } for k, v := range errorCases { provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory()) @@ -471,6 +503,15 @@ func TestValidatePodSecurityContextSuccess(t *testing.T) { Level: "level", } + sysctlAllowFooPSP := defaultPSP() + sysctlAllowFooPSP.Annotations[extensions.SysctlsPodSecurityPolicyAnnotationKey] = "foo" + + safeSysctlFooPod := defaultPod() + safeSysctlFooPod.Annotations[api.SysctlsPodAnnotationKey] = "foo=1" + + unsafeSysctlFooPod := defaultPod() + unsafeSysctlFooPod.Annotations[api.UnsafeSysctlsPodAnnotationKey] = "foo=1" + errorCases := map[string]struct { pod *api.Pod psp *extensions.PodSecurityPolicy @@ -499,6 +540,22 @@ func TestValidatePodSecurityContextSuccess(t *testing.T) { pod: seLinuxPod, psp: seLinuxPSP, }, + "pass sysctl specific profile with safe sysctl": { + pod: safeSysctlFooPod, + psp: sysctlAllowFooPSP, + }, + "pass sysctl specific profile with unsafe sysctl": { + pod: unsafeSysctlFooPod, + psp: sysctlAllowFooPSP, + }, + "pass empty profile with safe sysctl": { + pod: safeSysctlFooPod, + psp: defaultPSP(), + }, + "pass empty profile with unsafe sysctl": { + pod: unsafeSysctlFooPod, + psp: defaultPSP(), + }, } for k, v := range errorCases { @@ -755,7 +812,8 @@ func TestGenerateContainerSecurityContextReadOnlyRootFS(t *testing.T) { func defaultPSP() *extensions.PodSecurityPolicy { return &extensions.PodSecurityPolicy{ ObjectMeta: api.ObjectMeta{ - Name: "psp-sa", + Name: "psp-sa", + Annotations: map[string]string{}, }, Spec: extensions.PodSecurityPolicySpec{ RunAsUser: extensions.RunAsUserStrategyOptions{ @@ -777,6 +835,9 @@ func defaultPSP() *extensions.PodSecurityPolicy { func defaultPod() *api.Pod { var notPriv bool = false return &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Annotations: map[string]string{}, + }, Spec: api.PodSpec{ SecurityContext: &api.PodSecurityContext{ // fill in for test cases diff --git a/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns.go b/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns.go index 66b11f7144..f7480e4297 100644 --- a/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns.go +++ b/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns.go @@ -35,15 +35,15 @@ var ( defaultSysctlsPatterns = []string{"*"} ) -// NewMustMatchPatterns creates a new mustMatchPattern strategy that will provide validation. +// NewMustMatchPatterns creates a new mustMatchPatterns strategy that will provide validation. // Passing nil means the default pattern, passing an empty list means to disallow all sysctls. -func NewMustMatchPatterns(patterns []string) (SysctlsStrategy, error) { +func NewMustMatchPatterns(patterns []string) SysctlsStrategy { if patterns == nil { patterns = defaultSysctlsPatterns } return &mustMatchPatterns{ patterns: patterns, - }, nil + } } // Validate ensures that the specified values fall within the range of the strategy. diff --git a/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns_test.go b/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns_test.go index 23aefbcaab..6716175bc4 100644 --- a/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns_test.go +++ b/pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns_test.go @@ -58,11 +58,7 @@ func TestValidate(t *testing.T) { } for k, v := range tests { - strategy, err := NewMustMatchPatterns(v.patterns) - if err != nil { - t.Errorf("%s failed: %v", k, err) - continue - } + strategy := NewMustMatchPatterns(v.patterns) pod := &api.Pod{} errs := strategy.Validate(pod) diff --git a/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go b/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go index a0631d6d2b..3deb21be62 100644 --- a/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go +++ b/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go @@ -1106,6 +1106,38 @@ func TestAdmitSysctls(t *testing.T) { psps: []*extensions.PodSecurityPolicy{emptySysctls}, shouldPass: false, }, + "pod with unsafe sysctls a, b request disallowed under aSysctls SCC": { + pod: podWithSysctls([]string{}, []string{"a", "b"}), + psps: []*extensions.PodSecurityPolicy{aSysctl}, + shouldPass: false, + }, + "pod with unsafe sysctls b request disallowed under aSysctls SCC": { + pod: podWithSysctls([]string{}, []string{"b"}), + psps: []*extensions.PodSecurityPolicy{aSysctl}, + shouldPass: false, + }, + "pod with unsafe sysctls a request allowed under aSysctls SCC": { + pod: podWithSysctls([]string{}, []string{"a"}), + psps: []*extensions.PodSecurityPolicy{aSysctl}, + shouldPass: true, + expectedPSP: aSysctl.Name, + }, + "pod with safe sysctls a, b request disallowed under aSysctls SCC": { + pod: podWithSysctls([]string{"a", "b"}, []string{}), + psps: []*extensions.PodSecurityPolicy{aSysctl}, + shouldPass: false, + }, + "pod with safe sysctls b request disallowed under aSysctls SCC": { + pod: podWithSysctls([]string{"b"}, []string{}), + psps: []*extensions.PodSecurityPolicy{aSysctl}, + shouldPass: false, + }, + "pod with safe sysctls a request allowed under aSysctls SCC": { + pod: podWithSysctls([]string{"a"}, []string{}), + psps: []*extensions.PodSecurityPolicy{aSysctl}, + shouldPass: true, + expectedPSP: aSysctl.Name, + }, "pod with unsafe sysctls request disallowed under emptySysctls PSP": { pod: podWithSysctls([]string{}, []string{"a", "b"}), psps: []*extensions.PodSecurityPolicy{emptySysctls}, @@ -1117,12 +1149,12 @@ func TestAdmitSysctls(t *testing.T) { shouldPass: true, expectedPSP: mixedSysctls.Name, }, - "pod with not-matching unsafe sysctls request allowed under mixedSysctls PSP": { + "pod with not-matching unsafe sysctls request disallowed under mixedSysctls PSP": { pod: podWithSysctls([]string{"a.b", "b.c", "c", "d.e.f"}, []string{"e"}), psps: []*extensions.PodSecurityPolicy{mixedSysctls}, shouldPass: false, }, - "pod with not-matching safe sysctls request allowed under mixedSysctls PSP": { + "pod with not-matching safe sysctls request disallowed under mixedSysctls PSP": { pod: podWithSysctls([]string{"a.b", "b.c", "c", "d.e.f", "e"}, []string{}), psps: []*extensions.PodSecurityPolicy{mixedSysctls}, shouldPass: false,