Promote sysctl annotations to API fields

pkg/security/podsecuritypolicy/factory.go

@@ -77,15 +77,7 @@ func (f *simpleStrategyFactory) CreateStrategies(psp *policy.PodSecurityPolicy,
 		errs = append(errs, err)
 	}
 
-	var unsafeSysctls []string
-	if ann, found := psp.Annotations[policy.SysctlsPodSecurityPolicyAnnotationKey]; found {
-		var err error
-		unsafeSysctls, err = policy.SysctlsFromPodSecurityPolicyAnnotation(ann)
-		if err != nil {
-			errs = append(errs, err)
-		}
-	}
-	sysctlsStrat := createSysctlsStrategy(unsafeSysctls)
+	sysctlsStrat := createSysctlsStrategy(sysctl.SafeSysctlWhitelist(), psp.Spec.AllowedUnsafeSysctls, psp.Spec.ForbiddenSysctls)
 
 	if len(errs) > 0 {
 		return nil, errors.NewAggregate(errs)
@@ -170,7 +162,7 @@ func createCapabilitiesStrategy(defaultAddCaps, requiredDropCaps, allowedCaps []
 	return capabilities.NewDefaultCapabilities(defaultAddCaps, requiredDropCaps, allowedCaps)
 }
 
-// createSysctlsStrategy creates a new unsafe sysctls strategy.
-func createSysctlsStrategy(sysctlsPatterns []string) sysctl.SysctlsStrategy {
-	return sysctl.NewMustMatchPatterns(sysctlsPatterns)
+// createSysctlsStrategy creates a new sysctls strategy.
+func createSysctlsStrategy(safeWhitelist, allowedUnsafeSysctls, forbiddenSysctls []string) sysctl.SysctlsStrategy {
+	return sysctl.NewMustMatchPatterns(safeWhitelist, allowedUnsafeSysctls, forbiddenSysctls)
 }

pkg/security/podsecuritypolicy/provider_test.go

@@ -267,17 +267,34 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
 		},
 	}
 
-	failOtherSysctlsAllowedPSP := defaultPSP()
-	failOtherSysctlsAllowedPSP.Annotations[policy.SysctlsPodSecurityPolicyAnnotationKey] = "bar,abc"
+	failSysctlDisallowedPSP := defaultPSP()
+	failSysctlDisallowedPSP.Spec.ForbiddenSysctls = []string{"kernel.shm_rmid_forced"}
 
-	failNoSysctlAllowedPSP := defaultPSP()
-	failNoSysctlAllowedPSP.Annotations[policy.SysctlsPodSecurityPolicyAnnotationKey] = ""
+	failNoSafeSysctlAllowedPSP := defaultPSP()
+	failNoSafeSysctlAllowedPSP.Spec.ForbiddenSysctls = []string{"*"}
 
-	failSafeSysctlFooPod := defaultPod()
-	failSafeSysctlFooPod.Annotations[api.SysctlsPodAnnotationKey] = "foo=1"
+	failAllUnsafeSysctlsPSP := defaultPSP()
+	failAllUnsafeSysctlsPSP.Spec.AllowedUnsafeSysctls = []string{}
 
-	failUnsafeSysctlFooPod := defaultPod()
-	failUnsafeSysctlFooPod.Annotations[api.UnsafeSysctlsPodAnnotationKey] = "foo=1"
+	failSafeSysctlKernelPod := defaultPod()
+	failSafeSysctlKernelPod.Spec.SecurityContext = &api.PodSecurityContext{
+		Sysctls: []api.Sysctl{
+			{
+				Name:  "kernel.shm_rmid_forced",
+				Value: "1",
+			},
+		},
+	}
+
+	failUnsafeSysctlPod := defaultPod()
+	failUnsafeSysctlPod.Spec.SecurityContext = &api.PodSecurityContext{
+		Sysctls: []api.Sysctl{
+			{
+				Name:  "kernel.sem",
+				Value: "32000",
+			},
+		},
+	}
 
 	failSeccompProfilePod := defaultPod()
 	failSeccompProfilePod.Annotations = map[string]string{api.SeccompPodAnnotationKey: "foo"}
@@ -359,25 +376,20 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
 			psp:           failHostPathReadOnlyPSP,
 			expectedError: "must be read-only",
 		},
-		"failSafeSysctlFooPod with failNoSysctlAllowedSCC": {
-			pod:           failSafeSysctlFooPod,
-			psp:           failNoSysctlAllowedPSP,
-			expectedError: "sysctls are not allowed",
+		"failSafeSysctlKernelPod with failNoSafeSysctlAllowedPSP": {
+			pod:           failSafeSysctlKernelPod,
+			psp:           failNoSafeSysctlAllowedPSP,
+			expectedError: "sysctl \"kernel.shm_rmid_forced\" is not allowed",
 		},
-		"failUnsafeSysctlFooPod with failNoSysctlAllowedSCC": {
-			pod:           failUnsafeSysctlFooPod,
-			psp:           failNoSysctlAllowedPSP,
-			expectedError: "sysctls are not allowed",
+		"failSafeSysctlKernelPod with failSysctlDisallowedPSP": {
+			pod:           failSafeSysctlKernelPod,
+			psp:           failSysctlDisallowedPSP,
+			expectedError: "sysctl \"kernel.shm_rmid_forced\" is 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",
+		"failUnsafeSysctlPod with failAllUnsafeSysctlsPSP": {
+			pod:           failUnsafeSysctlPod,
+			psp:           failAllUnsafeSysctlsPSP,
+			expectedError: "unsafe sysctl \"kernel.sem\" is not allowed",
 		},
 		"failInvalidSeccomp": {
 			pod:           failSeccompProfilePod,
@@ -707,14 +719,29 @@ func TestValidatePodSecurityContextSuccess(t *testing.T) {
 		{PathPrefix: "/foo"},
 	}
 
-	sysctlAllowFooPSP := defaultPSP()
-	sysctlAllowFooPSP.Annotations[policy.SysctlsPodSecurityPolicyAnnotationKey] = "foo"
+	sysctlAllowAllPSP := defaultPSP()
+	sysctlAllowAllPSP.Spec.ForbiddenSysctls = []string{}
+	sysctlAllowAllPSP.Spec.AllowedUnsafeSysctls = []string{"*"}
 
-	safeSysctlFooPod := defaultPod()
-	safeSysctlFooPod.Annotations[api.SysctlsPodAnnotationKey] = "foo=1"
+	safeSysctlKernelPod := defaultPod()
+	safeSysctlKernelPod.Spec.SecurityContext = &api.PodSecurityContext{
+		Sysctls: []api.Sysctl{
+			{
+				Name:  "kernel.shm_rmid_forced",
+				Value: "1",
+			},
+		},
+	}
 
-	unsafeSysctlFooPod := defaultPod()
-	unsafeSysctlFooPod.Annotations[api.UnsafeSysctlsPodAnnotationKey] = "foo=1"
+	unsafeSysctlKernelPod := defaultPod()
+	unsafeSysctlKernelPod.Spec.SecurityContext = &api.PodSecurityContext{
+		Sysctls: []api.Sysctl{
+			{
+				Name:  "kernel.sem",
+				Value: "32000",
+			},
+		},
+	}
 
 	seccompPSP := defaultPSP()
 	seccompPSP.Annotations = map[string]string{
@@ -766,21 +793,13 @@ 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 safe kernel sysctl": {
+			pod: safeSysctlKernelPod,
+			psp: sysctlAllowAllPSP,
 		},
-		"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(),
+		"pass sysctl specific profile with unsafe kernel sysctl": {
+			pod: unsafeSysctlKernelPod,
+			psp: sysctlAllowAllPSP,
 		},
 		"pass hostDir allowed directory validating PSP": {
 			pod: hostPathDirPod,

pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns.go

@@ -22,12 +22,26 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	api "k8s.io/kubernetes/pkg/apis/core"
-	"k8s.io/kubernetes/pkg/apis/core/helper"
 )
 
+// SafeSysctlWhitelist returns the whitelist of safe sysctls and safe sysctl patterns (ending in *).
+//
+// A sysctl is called safe iff
+// - it is namespaced in the container or the pod
+// - it is isolated, i.e. has no influence on any other pod on the same node.
+func SafeSysctlWhitelist() []string {
+	return []string{
+		"kernel.shm_rmid_forced",
+		"net.ipv4.ip_local_port_range",
+		"net.ipv4.tcp_syncookies",
+	}
+}
+
 // mustMatchPatterns implements the SysctlsStrategy interface
 type mustMatchPatterns struct {
-	patterns []string
+	safeWhitelist        []string
+	allowedUnsafeSysctls []string
+	forbiddenSysctls     []string
 }
 
 var (
@@ -38,56 +52,75 @@ var (
 
 // 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 {
-	if patterns == nil {
-		patterns = defaultSysctlsPatterns
-	}
+func NewMustMatchPatterns(safeWhitelist, allowedUnsafeSysctls, forbiddenSysctls []string) SysctlsStrategy {
 	return &mustMatchPatterns{
-		patterns: patterns,
+		safeWhitelist:        safeWhitelist,
+		allowedUnsafeSysctls: allowedUnsafeSysctls,
+		forbiddenSysctls:     forbiddenSysctls,
 	}
 }
 
+func (s *mustMatchPatterns) isForbidden(sysctlName string) bool {
+	// Is the sysctl forbidden?
+	for _, s := range s.forbiddenSysctls {
+		if strings.HasSuffix(s, "*") {
+			prefix := strings.TrimSuffix(s, "*")
+			if strings.HasPrefix(sysctlName, prefix) {
+				return true
+			}
+		} else if sysctlName == s {
+			return true
+		}
+	}
+	return false
+}
+
+func (s *mustMatchPatterns) isSafe(sysctlName string) bool {
+	for _, ws := range s.safeWhitelist {
+		if sysctlName == ws {
+			return true
+		}
+	}
+	return false
+}
+
+func (s *mustMatchPatterns) isAllowedUnsafe(sysctlName string) bool {
+	for _, s := range s.allowedUnsafeSysctls {
+		if strings.HasSuffix(s, "*") {
+			prefix := strings.TrimSuffix(s, "*")
+			if strings.HasPrefix(sysctlName, prefix) {
+				return true
+			}
+		} else if sysctlName == s {
+			return true
+		}
+	}
+	return false
+}
+
 // Validate ensures that the specified values fall within the range of the strategy.
 func (s *mustMatchPatterns) Validate(pod *api.Pod) field.ErrorList {
 	allErrs := field.ErrorList{}
-	allErrs = append(allErrs, s.validateAnnotation(pod, api.SysctlsPodAnnotationKey)...)
-	allErrs = append(allErrs, s.validateAnnotation(pod, api.UnsafeSysctlsPodAnnotationKey)...)
-	return allErrs
-}
 
-func (s *mustMatchPatterns) validateAnnotation(pod *api.Pod, key string) field.ErrorList {
-	allErrs := field.ErrorList{}
-
-	fieldPath := field.NewPath("pod", "metadata", "annotations").Key(key)
-
-	sysctls, err := helper.SysctlsFromPodAnnotation(pod.Annotations[key])
-	if err != nil {
-		allErrs = append(allErrs, field.Invalid(fieldPath, pod.Annotations[key], err.Error()))
+	var sysctls []api.Sysctl
+	if pod.Spec.SecurityContext != nil {
+		sysctls = pod.Spec.SecurityContext.Sysctls
 	}
 
-	if len(sysctls) > 0 {
-		if len(s.patterns) == 0 {
-			allErrs = append(allErrs, field.Invalid(fieldPath, pod.Annotations[key], "sysctls are not allowed"))
-		} else {
-			for i, sysctl := range sysctls {
-				allErrs = append(allErrs, s.ValidateSysctl(sysctl.Name, fieldPath.Index(i))...)
-			}
+	fieldPath := field.NewPath("pod", "spec", "securityContext").Child("sysctls")
+
+	for i, sysctl := range sysctls {
+		switch {
+		case s.isForbidden(sysctl.Name):
+			allErrs = append(allErrs, field.ErrorList{field.Forbidden(fieldPath.Index(i), fmt.Sprintf("sysctl %q is not allowed", sysctl.Name))}...)
+		case s.isSafe(sysctl.Name):
+			continue
+		case s.isAllowedUnsafe(sysctl.Name):
+			continue
+		default:
+			allErrs = append(allErrs, field.ErrorList{field.Forbidden(fieldPath.Index(i), fmt.Sprintf("unsafe sysctl %q is not allowed", sysctl.Name))}...)
 		}
 	}
 
 	return allErrs
 }
-
-func (s *mustMatchPatterns) ValidateSysctl(sysctlName string, fldPath *field.Path) field.ErrorList {
-	for _, s := range s.patterns {
-		if s[len(s)-1] == '*' {
-			prefix := s[:len(s)-1]
-			if strings.HasPrefix(sysctlName, string(prefix)) {
-				return nil
-			}
-		} else if sysctlName == s {
-			return nil
-		}
-	}
-	return field.ErrorList{field.Forbidden(fldPath, fmt.Sprintf("sysctl %q is not allowed", sysctlName))}
-}

pkg/security/podsecuritypolicy/sysctl/mustmatchpatterns_test.go

@@ -17,48 +17,47 @@ limitations under the License.
 package sysctl
 
 import (
-	api "k8s.io/kubernetes/pkg/apis/core"
-	"k8s.io/kubernetes/pkg/apis/core/helper"
 	"testing"
+
+	api "k8s.io/kubernetes/pkg/apis/core"
 )
 
 func TestValidate(t *testing.T) {
 	tests := map[string]struct {
-		patterns   []string
-		allowed    []string
-		disallowed []string
+		whitelist     []string
+		forbiddenSafe []string
+		allowedUnsafe []string
+		allowed       []string
+		disallowed    []string
 	}{
 		// no container requests
-		"nil": {
-			patterns: nil,
-			allowed:  []string{"foo"},
+		"with allow all": {
+			whitelist: []string{"foo"},
+			allowed:   []string{"foo"},
 		},
 		"empty": {
-			patterns:   []string{},
-			disallowed: []string{"foo"},
+			whitelist:     []string{"foo"},
+			forbiddenSafe: []string{"*"},
+			disallowed:    []string{"foo"},
 		},
 		"without wildcard": {
-			patterns:   []string{"a", "a.b"},
+			whitelist:  []string{"a", "a.b"},
 			allowed:    []string{"a", "a.b"},
 			disallowed: []string{"b"},
 		},
-		"with catch-all wildcard": {
-			patterns: []string{"*"},
-			allowed:  []string{"a", "a.b"},
-		},
 		"with catch-all wildcard and non-wildcard": {
-			patterns: []string{"a.b.c", "*"},
-			allowed:  []string{"a", "a.b", "a.b.c", "b"},
+			allowedUnsafe: []string{"a.b.c", "*"},
+			allowed:       []string{"a", "a.b", "a.b.c", "b"},
 		},
 		"without catch-all wildcard": {
-			patterns:   []string{"a.*", "b.*", "c.d.e", "d.e.f.*"},
-			allowed:    []string{"a.b", "b.c", "c.d.e", "d.e.f.g.h"},
-			disallowed: []string{"a", "b", "c", "c.d", "d.e", "d.e.f"},
+			allowedUnsafe: []string{"a.*", "b.*", "c.d.e", "d.e.f.*"},
+			allowed:       []string{"a.b", "b.c", "c.d.e", "d.e.f.g.h"},
+			disallowed:    []string{"a", "b", "c", "c.d", "d.e", "d.e.f"},
 		},
 	}
 
 	for k, v := range tests {
-		strategy := NewMustMatchPatterns(v.patterns)
+		strategy := NewMustMatchPatterns(v.whitelist, v.allowedUnsafe, v.forbiddenSafe)
 
 		pod := &api.Pod{}
 		errs := strategy.Validate(pod)
@@ -66,37 +65,40 @@ func TestValidate(t *testing.T) {
 			t.Errorf("%s: unexpected validaton errors for empty sysctls: %v", k, errs)
 		}
 
-		sysctls := []api.Sysctl{}
-		for _, s := range v.allowed {
-			sysctls = append(sysctls, api.Sysctl{
-				Name:  s,
-				Value: "dummy",
-			})
-		}
-		testAllowed := func(key string, category string) {
-			pod.Annotations = map[string]string{
-				key: helper.PodAnnotationsFromSysctls(sysctls),
+		testAllowed := func() {
+			sysctls := []api.Sysctl{}
+			for _, s := range v.allowed {
+				sysctls = append(sysctls, api.Sysctl{
+					Name:  s,
+					Value: "dummy",
+				})
+			}
+			pod.Spec.SecurityContext = &api.PodSecurityContext{
+				Sysctls: sysctls,
 			}
 			errs = strategy.Validate(pod)
 			if len(errs) != 0 {
-				t.Errorf("%s: unexpected validaton errors for %s sysctls: %v", k, category, errs)
+				t.Errorf("%s: unexpected validaton errors for sysctls: %v", k, errs)
 			}
 		}
-		testDisallowed := func(key string, category string) {
+		testDisallowed := func() {
 			for _, s := range v.disallowed {
-				pod.Annotations = map[string]string{
-					key: helper.PodAnnotationsFromSysctls([]api.Sysctl{{Name: s, Value: "dummy"}}),
+				pod.Spec.SecurityContext = &api.PodSecurityContext{
+					Sysctls: []api.Sysctl{
+						{
+							Name:  s,
+							Value: "dummy",
+						},
+					},
 				}
 				errs = strategy.Validate(pod)
 				if len(errs) == 0 {
-					t.Errorf("%s: expected error for %s sysctl %q", k, category, s)
+					t.Errorf("%s: expected error for sysctl %q", k, s)
 				}
 			}
 		}
 
-		testAllowed(api.SysctlsPodAnnotationKey, "safe")
-		testAllowed(api.UnsafeSysctlsPodAnnotationKey, "unsafe")
-		testDisallowed(api.SysctlsPodAnnotationKey, "safe")
-		testDisallowed(api.UnsafeSysctlsPodAnnotationKey, "unsafe")
+		testAllowed()
+		testDisallowed()
 	}
 }