Merge pull request kubernetes#33821 from sttts/sttts-sysctl-psp-fixes

Automatic merge from submit-queue

Improve sysctl psp tests

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://github.com/kubernetes/kubernetes/blob/master/CONTRIBUTING.md and developer guide https://github.com/kubernetes/kubernetes/blob/master/docs/devel/development.md
2. If you want *faster* PR reviews, read how: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/faster_reviews.md
3. Follow the instructions for writing a release note: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes
-->

**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:
<!--  Steps to write your release note:
1. Use the release-note-* labels to set the release note state (if you have access) 
2. Enter your extended release note in the below block; leaving it blank means using the PR title as the release note. If no release note is required, just write `NONE`. 
-->
```release-note
```

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]

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"}},

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)
 }

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

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.

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)

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,