admission: don't update psp annotation on update #55486

sttts · 2017-11-10T15:03:28Z

Follow-up of #54689.

Related to #55435 as istio-like initializer-based container injection cannot contribute to SC mutations.

The PodSecurityPolicy annotation `kubernetes.io/psp` on pods is only set once on create.

sttts · 2017-11-10T15:06:44Z

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

-		// if failOnNoPolicies is false.
-		// TODO: if failOnNoPolicies is toggled from false to true, we will never update the annotation anymore. Is this desired?
-		pod.ObjectMeta.Annotations[psputil.ValidatedPSPAnnotation] = pspName
+		pod.ObjectMeta.Annotations[psputil.InitiallyValidatedPSPAnnotation] = pspName


note that before this PR we did not allow mutation in updates either (i.e. no serious mutation), but we were updating the annotation. Now we completely skip the mutating admission phase for update requests.

sttts · 2017-11-10T15:09:56Z

We need the annotation in tests. An alternative would be to define a secret annotation on a pod: if that is found during admission, we update it to the chosen PSP. This secret annotation can be kubernetes.io/psp or kubernetes.io/initial-psp-on-create, or anything else.

liggitt · 2017-11-10T15:15:53Z

pkg/security/podsecuritypolicy/util/util.go

@@ -26,7 +26,7 @@ import (
 )

 const (
-	ValidatedPSPAnnotation = "kubernetes.io/psp"


I wouldn't change the existing annotation

liggitt · 2017-11-10T16:01:21Z

nit on not changing the annotation name, LGTM otherwise

sttts · 2017-11-10T17:31:59Z

@liggitt addressed the annotation comment. Will give the west-coast reviewers some time to review.

liggitt · 2017-11-10T17:32:02Z

/lgtm

tallclair · 2017-11-10T22:33:18Z

We need the annotation in tests.

This feels to me like the tests are testing the internal implementation, not the public interface. Maybe the solution is to rewrite those tests to not depend on this annotation?

As a longer term solution, this feels like something that might be useful in audit logs, along with things like which RBAC role authorized a user. I'm not sure exactly how that should be incorporated, but it seems like a more appropriate place for this sort of information. /cc @crassirostris

tallclair · 2017-11-10T22:35:05Z

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

@@ -395,7 +400,7 @@ func TestAdmitPreferNonmutating(t *testing.T) {
 			pod:                   unprivilegedRunAsAnyPod.DeepCopy(),
 			oldPod:                changedPod.DeepCopy(),
 			psps:                  []*extensions.PodSecurityPolicy{mutating2, mutating1},
-			shouldPassAdmit:       false,
+			shouldPassAdmit:       true,


"pod should not allow mutation on update"

Change the description of this case.

tallclair · 2017-11-10T22:37:49Z

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

-			pod:                   unprivilegedRunAsAnyPod.DeepCopy(),
-			oldPod:                changedPod.DeepCopy(),
+			pod:                   podWithSC.DeepCopy(),
+			oldPod:                changedPodWithSC.DeepCopy(),


nit: I think these names are backwards... changedPod* sohuld be the new one, not the "oldPod". Same for other test cases.

Tried to follow the existing pattern there, but it confused me as well.

sttts · 2017-11-13T09:30:05Z

This feels to me like the tests are testing the internal implementation, not the public interface. Maybe the solution is to rewrite those tests to not depend on this annotation?

Our API is implicit, i.e. not introspectable. Our tests want to verify that the right PSP was chosen. If we don't want such an annotation, we have to find another way to detect that. Might be tricky.

sttts · 2017-11-13T10:15:41Z

As a longer term solution, this feels like something that might be useful in audit logs, along with things like which RBAC role authorized a user. I'm not sure exactly how that should be incorporated, but it seems like a more appropriate place for this sort of information. /cc @crassirostris

I like that idea. @crassirostris can you create an issue of audit+admission? With webhooks this will get even more important.

sttts · 2017-11-13T10:20:48Z

@tallclair updated. ptal

php-coder · 2017-11-13T11:03:33Z

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

@@ -119,11 +119,15 @@ func (c *PodSecurityPolicyPlugin) Admit(a admission.Attributes) error {
 		return nil
 	}

+	// TODO(liggitt): allow spec mutation during initializing updates?
+	if a.GetOperation() != admission.Create {


Why not move this check into the shouldIgnore()?

It's not the same for Admit and Validate. We only skip the mutation on non-Create requests in mutating admission (= Admit).

Ok, thanks!

The logic become more and more sophisticated... If you will add a comment here, it would be good addition IMHO.

sttts · 2017-11-13T13:52:35Z

/retest

sttts · 2017-11-13T16:34:26Z

@tallclair @liggitt this is waiting for approval.

liggitt · 2017-11-13T16:55:09Z

/approve

sttts · 2017-11-13T17:20:52Z

/retest

deads2k · 2017-11-13T18:13:05Z

/approve no-issue

k8s-github-robot · 2017-11-13T18:13:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, sttts

Associated issue: 55435

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~plugin/pkg/admission/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-14T00:45:27Z

Automatic merge from submit-queue (batch tested with PRs 54005, 55127, 53850, 55486, 53440). If you want to cherry-pick this change to another branch, please follow the instructions here.

crassirostris · 2017-11-20T16:17:05Z

@sttts @tallclair Sorry I'm little bit late to the party. Could you please clarify what you're suggesting here:

I like that idea. @crassirostris can you create an issue of audit+admission? With webhooks this will get even more important.

I'm not sure I understand the context

sttts · 2017-11-20T19:22:49Z

I'm not sure I understand the context

The PSP admission plugin matches a pod with a PodSecurityPolicy. If no policy matches, the pod is rejected. It would be helpful to know which policy made the pod pass the admission step.

crassirostris · 2017-11-22T12:15:37Z

@sttts Thanks! Created #56209

sttts assigned liggitt, mikedanese, deads2k and tallclair Nov 10, 2017

sttts force-pushed the sttts-psp-admission-annotation branch from 264ef95 to 65ce2e0 Compare November 10, 2017 15:05

sttts commented Nov 10, 2017

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 10, 2017

sttts added area/admission-control area/security labels Nov 10, 2017

liggitt reviewed Nov 10, 2017

View reviewed changes

sttts force-pushed the sttts-psp-admission-annotation branch 3 times, most recently from 3b5fb6f to 8a3a102 Compare November 10, 2017 17:30

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

tallclair reviewed Nov 10, 2017

View reviewed changes

sttts force-pushed the sttts-psp-admission-annotation branch from 8a3a102 to b21b23b Compare November 13, 2017 10:16

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 13, 2017

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 13, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

php-coder reviewed Nov 13, 2017

View reviewed changes

admission: don't update psp annotation on update

3d5849f

sttts force-pushed the sttts-psp-admission-annotation branch from b21b23b to 3d5849f Compare November 13, 2017 16:10

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2017

k8s-github-robot merged commit 560a310 into kubernetes:master Nov 14, 2017

crassirostris mentioned this pull request Nov 22, 2017

Add PodSecurityPolicy admission information to audit logs #56209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admission: don't update psp annotation on update #55486

admission: don't update psp annotation on update #55486

sttts commented Nov 10, 2017 •

edited

sttts Nov 10, 2017

sttts commented Nov 10, 2017 •

edited

liggitt Nov 10, 2017

sttts Nov 10, 2017

liggitt commented Nov 10, 2017

sttts commented Nov 10, 2017

liggitt commented Nov 10, 2017

tallclair commented Nov 10, 2017

tallclair Nov 10, 2017

sttts Nov 13, 2017

tallclair Nov 10, 2017

sttts Nov 13, 2017

sttts Nov 13, 2017

sttts commented Nov 13, 2017

sttts commented Nov 13, 2017

sttts commented Nov 13, 2017

php-coder Nov 13, 2017

sttts Nov 13, 2017

php-coder Nov 13, 2017

sttts Nov 13, 2017

sttts Nov 13, 2017

sttts commented Nov 13, 2017

sttts commented Nov 13, 2017

liggitt commented Nov 13, 2017

sttts commented Nov 13, 2017

deads2k commented Nov 13, 2017

k8s-github-robot commented Nov 13, 2017

k8s-github-robot commented Nov 14, 2017

crassirostris commented Nov 20, 2017

sttts commented Nov 20, 2017

crassirostris commented Nov 22, 2017

admission: don't update psp annotation on update #55486

admission: don't update psp annotation on update #55486

Conversation

sttts commented Nov 10, 2017 • edited

Choose a reason for hiding this comment

sttts commented Nov 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 10, 2017

sttts commented Nov 10, 2017

liggitt commented Nov 10, 2017

tallclair commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Nov 13, 2017

sttts commented Nov 13, 2017

sttts commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Nov 13, 2017

sttts commented Nov 13, 2017

liggitt commented Nov 13, 2017

sttts commented Nov 13, 2017

deads2k commented Nov 13, 2017

k8s-github-robot commented Nov 13, 2017

k8s-github-robot commented Nov 14, 2017

crassirostris commented Nov 20, 2017

sttts commented Nov 20, 2017

crassirostris commented Nov 22, 2017

sttts commented Nov 10, 2017 •

edited

sttts commented Nov 10, 2017 •

edited