Merge pull request #56843 from CaoShuFeng/podsecuritypolicy

Automatic merge from submit-queue (batch tested with PRs 57021, 56843, 54983). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

[PSP] always check validated policy first for update operation

When update a pod with `kubernetes.io/psp` annotation set, we should
check this policy first. Because this saved policy is `usually` the
one we are looking for.


**Release note**:
```release-note
NONE
```

plugin/pkg/admission/security/podsecuritypolicy/BUILD

@@ -56,6 +56,7 @@ go_test(
         "//vendor/k8s.io/apiserver/pkg/authentication/serviceaccount:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/authorization/authorizerfactory:go_default_library",
     ],
 )
 

plugin/pkg/admission/security/podsecuritypolicy/admission.go

@@ -122,8 +122,8 @@ func (c *PodSecurityPolicyPlugin) Admit(a admission.Attributes) error {
 
 	pod := a.GetObject().(*api.Pod)
 
-	// compute the context
+	// compute the context. Mutation is allowed. ValidatedPSPAnnotation is not taken into account.
-	allowedPod, pspName, validationErrs, err := c.computeSecurityContext(a, pod, true)
+	allowedPod, pspName, validationErrs, err := c.computeSecurityContext(a, pod, true, "")
 	if err != nil {
 		return admission.NewForbidden(a, err)
 	}
@@ -152,8 +152,8 @@ func (c *PodSecurityPolicyPlugin) Validate(a admission.Attributes) error {
 
 	pod := a.GetObject().(*api.Pod)
 
-	// compute the context. Mutation is not allowed.
+	// compute the context. Mutation is not allowed. ValidatedPSPAnnotation is used as a hint to gain same speed-up.
-	allowedPod, _, validationErrs, err := c.computeSecurityContext(a, pod, false)
+	allowedPod, _, validationErrs, err := c.computeSecurityContext(a, pod, false, pod.ObjectMeta.Annotations[psputil.ValidatedPSPAnnotation])
 	if err != nil {
 		return admission.NewForbidden(a, err)
 	}
@@ -193,8 +193,9 @@ func shouldIgnore(a admission.Attributes) (bool, error) {
 
 // computeSecurityContext derives a valid security context while trying to avoid any changes to the given pod. I.e.
 // if there is a matching policy with the same security context as given, it will be reused. If there is no
-// matching policy the returned pod will be nil and the pspName empty.
+// matching policy the returned pod will be nil and the pspName empty. validatedPSPHint is the validated psp name
-func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes, pod *api.Pod, specMutationAllowed bool) (*api.Pod, string, field.ErrorList, error) {
+// saved in kubernetes.io/psp annotation. This psp is usually the one we are looking for.
+func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes, pod *api.Pod, specMutationAllowed bool, validatedPSPHint string) (*api.Pod, string, field.ErrorList, error) {
 	// get all constraints that are usable by the user
 	glog.V(4).Infof("getting pod security policies for pod %s (generate: %s)", pod.Name, pod.GenerateName)
 	var saInfo user.Info
@@ -213,9 +214,18 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes,
 		return pod, "", nil, nil
 	}
 
-	// sort by name to make order deterministic
+	// sort policies by name to make order deterministic
+	// If mutation is not allowed and validatedPSPHint is provided, check the validated policy first.
 	// TODO(liggitt): add priority field to allow admins to bucket differently
 	sort.SliceStable(policies, func(i, j int) bool {
+		if !specMutationAllowed {
+			if policies[i].Name == validatedPSPHint {
+				return true
+			}
+			if policies[j].Name == validatedPSPHint {
+				return false
+			}
+		}
 		return strings.Compare(policies[i].Name, policies[j].Name) < 0
 	})
 
@@ -244,7 +254,6 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes,
 		}
 
 		// the entire pod validated
-
 		mutated := !apiequality.Semantic.DeepEqual(pod, podCopy)
 		if mutated && !specMutationAllowed {
 			continue

plugin/pkg/admission/security/podsecuritypolicy/admission_test.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apiserver/pkg/authentication/serviceaccount"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
+	"k8s.io/apiserver/pkg/authorization/authorizerfactory"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	kapi "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/core/helper"
@@ -65,7 +66,7 @@ func NewTestAdmission(psps []*extensions.PodSecurityPolicy, authz authorizer.Aut
 	}
 }
 
-// TestAlwaysAllowedAuthorizer is a testing struct for testing that fulfills the authorizer interface.
+// TestAuthorizer is a testing struct for testing that fulfills the authorizer interface.
 type TestAuthorizer struct {
 	// usernameToNamespaceToAllowedPSPs contains the map of allowed PSPs.
 	// if nil, all PSPs are allowed.
@@ -344,6 +345,13 @@ func TestAdmitPreferNonmutating(t *testing.T) {
 	gcChangedPod.OwnerReferences = []metav1.OwnerReference{{Kind: "Foo", Name: "bar"}}
 	gcChangedPod.Finalizers = []string{"foo"}
 
+	podWithAnnotation := unprivilegedRunAsAnyPod.DeepCopy()
+	podWithAnnotation.ObjectMeta.Annotations = map[string]string{
+		// "mutating2" is lexicographically behind "mutating1", so "mutating1" should be
+		// chosen because it's the canonical PSP order.
+		psputil.ValidatedPSPAnnotation: mutating2.Name,
+	}
+
 	tests := map[string]struct {
 		operation             kadmission.Operation
 		pod                   *kapi.Pod
@@ -381,6 +389,15 @@ func TestAdmitPreferNonmutating(t *testing.T) {
 			expectedContainerUser: &mutating1.Spec.RunAsUser.Ranges[0].Min,
 			expectedPSP:           mutating1.Name,
 		},
+		"pod should use deterministic mutating PSP on create even if ValidatedPSPAnnotation is set": {
+			operation:             kadmission.Create,
+			pod:                   podWithAnnotation,
+			psps:                  []*extensions.PodSecurityPolicy{mutating2, mutating1},
+			shouldPassValidate:    true,
+			expectMutation:        true,
+			expectedContainerUser: &mutating1.Spec.RunAsUser.Ranges[0].Min,
+			expectedPSP:           mutating1.Name,
+		},
 		"pod should prefer non-mutating PSP on update": {
 			operation:             kadmission.Update,
 			pod:                   changedPodWithSC.DeepCopy(),
@@ -2130,7 +2147,6 @@ func TestPolicyAuthorizationErrors(t *testing.T) {
 	)
 
 	tests := map[string]struct {
-		priviliged           bool
 		inPolicies           []*extensions.PodSecurityPolicy
 		allowed              map[string]map[string]map[string]bool
 		expectValidationErrs int
@@ -2197,7 +2213,7 @@ func TestPolicyAuthorizationErrors(t *testing.T) {
 			plugin := NewTestAdmission(tc.inPolicies, authz)
 			attrs := kadmission.NewAttributesRecord(pod, nil, kapi.Kind("Pod").WithVersion("version"), ns, "", kapi.Resource("pods").WithVersion("version"), "", kadmission.Create, &user.DefaultInfo{Name: userName})
 
-			allowedPod, _, validationErrs, err := plugin.computeSecurityContext(attrs, pod, true)
+			allowedPod, _, validationErrs, err := plugin.computeSecurityContext(attrs, pod, true, "")
 			assert.Nil(t, allowedPod)
 			assert.NoError(t, err)
 			assert.Len(t, validationErrs, tc.expectValidationErrs)
@@ -2205,6 +2221,99 @@ func TestPolicyAuthorizationErrors(t *testing.T) {
 	}
 }
 
+func TestPreferValidatedPSP(t *testing.T) {
+	restrictivePSPWithName := func(name string) *extensions.PodSecurityPolicy {
+		p := restrictivePSP()
+		p.Name = name
+		return p
+	}
+
+	permissivePSPWithName := func(name string) *extensions.PodSecurityPolicy {
+		p := permissivePSP()
+		p.Name = name
+		return p
+	}
+
+	tests := map[string]struct {
+		inPolicies           []*extensions.PodSecurityPolicy
+		expectValidationErrs int
+		validatedPSPHint     string
+		expectedPSP          string
+	}{
+		"no policy saved in annotations, PSPs are ordered lexicographically": {
+			inPolicies: []*extensions.PodSecurityPolicy{
+				restrictivePSPWithName("001restrictive"),
+				restrictivePSPWithName("002restrictive"),
+				permissivePSPWithName("002permissive"),
+				permissivePSPWithName("001permissive"),
+				permissivePSPWithName("003permissive"),
+			},
+			expectValidationErrs: 0,
+			validatedPSPHint:     "",
+			expectedPSP:          "001permissive",
+		},
+		"policy saved in annotations is prefered": {
+			inPolicies: []*extensions.PodSecurityPolicy{
+				restrictivePSPWithName("001restrictive"),
+				restrictivePSPWithName("002restrictive"),
+				permissivePSPWithName("001permissive"),
+				permissivePSPWithName("002permissive"),
+				permissivePSPWithName("003permissive"),
+			},
+			expectValidationErrs: 0,
+			validatedPSPHint:     "002permissive",
+			expectedPSP:          "002permissive",
+		},
+		"policy saved in annotations is invalid": {
+			inPolicies: []*extensions.PodSecurityPolicy{
+				restrictivePSPWithName("001restrictive"),
+				restrictivePSPWithName("002restrictive"),
+			},
+			expectValidationErrs: 2,
+			validatedPSPHint:     "foo",
+			expectedPSP:          "",
+		},
+		"policy saved in annotations is disallowed anymore": {
+			inPolicies: []*extensions.PodSecurityPolicy{
+				restrictivePSPWithName("001restrictive"),
+				restrictivePSPWithName("002restrictive"),
+			},
+			expectValidationErrs: 2,
+			validatedPSPHint:     "001restrictive",
+			expectedPSP:          "",
+		},
+		"policy saved in annotations is disallowed anymore, but find another one": {
+			inPolicies: []*extensions.PodSecurityPolicy{
+				restrictivePSPWithName("001restrictive"),
+				restrictivePSPWithName("002restrictive"),
+				permissivePSPWithName("002permissive"),
+				permissivePSPWithName("001permissive"),
+			},
+			expectValidationErrs: 0,
+			validatedPSPHint:     "001restrictive",
+			expectedPSP:          "001permissive",
+		},
+	}
+	for desc, tc := range tests {
+		t.Run(desc, func(t *testing.T) {
+			authz := authorizerfactory.NewAlwaysAllowAuthorizer()
+			allowPrivilegeEscalation := true
+			pod := goodPod()
+			pod.Namespace = "ns"
+			pod.Spec.ServiceAccountName = "sa"
+			pod.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = &allowPrivilegeEscalation
+
+			plugin := NewTestAdmission(tc.inPolicies, authz)
+			attrs := kadmission.NewAttributesRecord(pod, nil, kapi.Kind("Pod").WithVersion("version"), "ns", "", kapi.Resource("pods").WithVersion("version"), "", kadmission.Update, &user.DefaultInfo{Name: "test"})
+
+			_, pspName, validationErrs, err := plugin.computeSecurityContext(attrs, pod, false, tc.validatedPSPHint)
+			assert.NoError(t, err)
+			assert.Len(t, validationErrs, tc.expectValidationErrs)
+			assert.Equal(t, pspName, tc.expectedPSP)
+		})
+	}
+}
+
 func restrictivePSP() *extensions.PodSecurityPolicy {
 	return &extensions.PodSecurityPolicy{
 		ObjectMeta: metav1.ObjectMeta{