Add authz to psp admission #33080

pweil- · 2016-09-20T11:02:19Z

Add authz integration to PSP admission to enable granting access to use specific PSPs on a per-user and per-service account basis. This allows an administrator to use multiple policies in a cluster that grant different levels of access for different types of users.

Builds on #32555. Second commit adds authz check to matching policy function in psp admission.

@deads2k @sttts @timstclair

This change is

erictune · 2016-09-22T21:50:48Z

@kubernetes/sig-auth
@cjcullen

This is saying you can create pods if you can get a matching PodSecurityPolicy object (not namespaces).

That means that I cannot view all the PodSecurityPolicies so see which ones I want to ask my cluster admin to use.

What if the verb was something distinctive. I like "use".

Then you can give people read permission without allowing them to match the policy.

Bonus points if there is a "use" subresource on the PodSecurityPolicy object that returns 200 if you are authorized to use that PSP, and 403 if you are not.

Double bonus points if there is a "match" subresource on a PodSecurityPolicy object that takes a Pod and returns 200, plus a JSON message that says "match: true" or "match: false".

erictune · 2016-09-22T21:51:44Z

Should have said this first: I love that this is getting finished up. Thank you for sending it.

liggitt · 2016-09-22T21:55:50Z

Agree that get is the wrong verb. Not sure about use, but nothing clearly better springs to mind

erictune · 2016-09-26T17:06:09Z

@alex-mohr @roberthbailey @cjcullen
This may be of interest to you.
PSP can be used to block access to hostDir, privileged containers, and other interesting features in a Kubernetes cluster. Currently it is a cluster-wide setting.

This PR would make it able to be per-user, which is interesting. You could allow an addon-adder-user to create hostDir containers, but not allow ordinary users to do so. Robert we chatted about this friday. This requires some authz support for the new "use" verb.

pweil- · 2016-10-17T17:36:41Z

@erictune @liggitt - updated this to use "use" as the verb. Since this is a synthetic check it doesn't require any plumbing so it's just matching what is in admission to what is in policy. Easy to change if "use" isn't good.

Also added an example walk through of using PSP with RBAC to allow creation of privileged containers since it was referred to in #33080 (comment).

PTAL

pweil- · 2016-10-17T17:40:53Z

Will take these as additional issues so this doesn't grow too much larger since it's at XL

Bonus points if there is a "use" subresource on the PodSecurityPolicy object that returns 200 if you are authorized to use that PSP, and 403 if you are not.

Double bonus points if there is a "match" subresource on a PodSecurityPolicy object that takes a Pod and returns 200, plus a JSON message that says "match: true" or "match: false".

Edit: #34963

liggitt · 2016-10-19T19:21:59Z

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

 	}

 	return matchedPolicies, nil
 }

+// authorizedForPolicy returns true if info is authorized to perform a "get" on policy.
+func authorizedForPolicy(info user.Info, policy *extensions.PodSecurityPolicy, authz authorizer.Authorizer) bool {
+	if info == nil {


user info is nil when you hit the API via the unsecured port (no auth filter populates user info). would you expect that to return unauthorized for everything or authorized for everything?

I would expect it to return unauthorized if the PSP plugin was enabled. Other opinions?

that means that if there were PSPs present, no one could create pods against the unsecured port, which seems wrong

changed to return true

liggitt · 2016-10-19T19:27:50Z

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

+	attr := authorizer.AttributesRecord{
+		User:            info,
+		Verb:            "use",
+		Namespace:       policy.Namespace,


PSP is cluster-scoped, so policy.Namespace isn't meaningful here. Would it make sense to check if the user is allowed to use the named PSP in the namespace where they are trying to perform the action (the pod namespace)? That would let you grant someone use of a particular PSP in a particular namespace, or cluster-wide.

this is interesting but a fundamental change. I can definitely see the benefit of allowing ns admins to create more restrictive PSPs and granting access there but it would require some other changes and maybe some discussion on whether folks actually want to admin policy this way. Will take a note to discuss

liggitt · 2016-10-19T19:31:29Z

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

 	matchedPolicies := make([]*extensions.PodSecurityPolicy, 0)

 	for _, c := range store.List() {
 		constraint, ok := c.(*extensions.PodSecurityPolicy)
 		if !ok {
 			return nil, errors.NewInternalError(fmt.Errorf("error converting object from store to a pod security policy: %v", c))
 		}
-		matchedPolicies = append(matchedPolicies, constraint)
+
+		if authorizedForPolicy(user, constraint, authz) || authorizedForPolicy(sa, constraint, authz) {


now that user factors into which policies are allowed, a different set could be selected on update than on create. on update, PSPs cannot mutate the pod spec, they simply need to pass/fail the pod as-is. that means splitting the selection/defaulting/validation steps, and skipping the defaulting step on update. can be a follow up, but needs to be done.

be sure to open the follow-up issue to track

liggitt · 2016-10-19T19:37:43Z

examples/podsecuritypolicy/rbac/policies.yaml

+  fsGroup:
+    rule: RunAsAny
+  runAsUser:
+    rule: RunAsAny


doesn't seem very restricted :)

the only difference here was privileged true|false for demo purposes of showing RBAC, not capabilities of the PSP. I can make it a "full" restricted PSP if you think that provides any value.

people copy examples more than you'd think. I would be explicit with privileged: false, limit volumes to the same volumes our restricted scc does, and maybe MustRunAsNonRoot?

liggitt · 2016-10-19T19:39:07Z

examples/podsecuritypolicy/rbac/roles.yaml

+apiVersion: rbac.authorization.k8s.io/v1alpha1
+kind: ClusterRole
+metadata:
+  name: restrictedPSP


restricted-psp-user?

liggitt · 2016-10-19T19:39:15Z

examples/podsecuritypolicy/rbac/roles.yaml

+apiVersion: rbac.authorization.k8s.io/v1alpha1
+kind: ClusterRole
+metadata:
+  name: privilegedPSP


privileged-psp-user?

liggitt · 2016-10-19T19:56:57Z

examples/podsecuritypolicy/rbac/bindings.yaml

+   kind: ClusterRole
+   name: restrictedPSP
+---
+# cluster-writer grants cluster-writer role to system:authenticated.


I'd rather limit the example to psp-related roles/rolebindings
#34729 added admin/edit/view roles to bootstrapping? maybe use those in the examples instead?

edit is exactly what I was shooting for, will rebase and update

changed to bind edit to the system:authenticated

liggitt · 2016-11-02T13:57:54Z

@pweil- status on this for code freeze?

pweil- · 2016-11-02T14:00:10Z

@pweil- status on this for code freeze?

Feedback will be finished up shortly.

erictune · 2016-11-02T15:57:54Z

Should this have 1.5 milestone on it?

pweil- · 2016-11-02T18:52:19Z

@erictune I'd like to get it added to 1.5

@liggitt - feedback addressed. However, in testing I found some issues related to #33966 when rbac is enabled. It appears that all components are now using the secure port. When rbac is enabled there are no bootstrap policies that give the kubelet, controller-manager, and other infra access to the resources they need.

Second commit is a hack to fix it so I could test but that needs resolved before this is merged. I think we should just go ahead and add the policies (cc @deads2k @dims @sttts)

k8s-ci-robot · 2016-11-02T19:10:42Z

Jenkins verification failed for commit 1dc3fa52d7b1fdf1a25b50c3f7fb2ad992d375d0. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

sttts · 2016-11-03T07:05:41Z

@pweil- adding the policies sounds right. Where and how?

deads2k · 2016-11-03T12:33:52Z

@pweil- adding the policies sounds right. Where and how?

We have a bootstrap policy now, like openshift. Here's a pull adding node permissions: #36155 . For the controllers, I think we want to add them at the same time we segment them by controller. For now, the controller effectively wants to be in the system:masters group. You were working on bringing cfssl into the local-up-cluster.sh to mint servering certs right? With that bash tool, we could do client certs too.

pweil- · 2016-11-03T12:49:14Z

removing the local-up-cluster commit based on conversations with @liggitt to unblock this PR. The walk through will have issues until resolved but I think it's still valuable to get in.

pweil- · 2016-11-04T12:12:46Z

added tests for nil user and nil SA

pweil- · 2016-11-04T13:46:28Z

@k8s-bot node e2e test this

hrm, node test is marked as failed and the details show it is still running. Giving it another shot..

pweil- · 2016-11-04T16:37:25Z

@liggitt tests are green if you want to apply milestones/labels

pweil- · 2016-11-07T17:35:10Z

bump, ready for any additional sign off, comments, or having labels applied and being merged.

pweil- · 2016-11-07T20:00:36Z

@derekwaynecarr FYI

deads2k · 2016-11-07T21:04:19Z

bump, ready for any additional sign off, comments, or having labels applied and being merged.

Tagged. Can you fill out the description a little more for people looking up the release note?

pweil- · 2016-11-07T21:22:15Z

updated description

timstclair · 2016-11-07T22:13:22Z

examples/podsecuritypolicy/rbac/README.md

+
+### Roles and bindings
+
+In order to a `PodSecurityPolicy` a user must have the ability to perform the `use` verb on the policy.


What does it mean to "peform the use verb on a policy?

I understand after reading the code, but I think you should explain it here for the uninformed user.

timstclair · 2016-11-07T22:16:53Z

examples/podsecuritypolicy/rbac/README.md

+  volumes:
+  - '*'
+
+```


The docs repo has a syntax for embedding files in docs. Do you know whether that works here?

No, unfortunately I don't think so. I found github/markup#172 (comment) which mentions it is not allowed due to security concerns.

saad-ali · 2016-11-08T06:04:25Z

Release-czar approved post-code freeze merge--This was LGTMed and in the merge-queue at code freeze time for 1.5. Leaving 1.5 milestone to let it gets merged after code freeze.

pweil- · 2016-11-08T13:38:14Z

@timstclair added some more wording about use granting access to use the policy but nothing else and that it is specific to PSP. If you're ok with this please reapply labels. Thanks for the feedback!

k8s-ci-robot · 2016-11-08T13:52:59Z

Jenkins unit/integration failed for commit bbe9c8f. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-08T13:59:11Z

Jenkins GCE Node e2e failed for commit bbe9c8f. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

pweil- · 2016-11-08T13:59:29Z

@k8s-bot unit test this

flake #36422

pweil- · 2016-11-08T13:59:41Z

@k8s-bot node e2e test this

k8s-github-robot · 2016-11-10T04:02:44Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-10T04:39:30Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 20, 2016

k8s-github-robot assigned lavalamp Sep 20, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 20, 2016

lavalamp assigned erictune and unassigned lavalamp Sep 20, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2016

pweil- force-pushed the psp-authorizer branch 2 times, most recently from 19992a8 to 8992e27 Compare October 17, 2016 17:33

pweil- changed the title ~~WIP: Add authz to psp admission~~ Add authz to psp admission Oct 17, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2016

pweil- mentioned this pull request Oct 17, 2016

PSP usability subresources #34963

Closed

liggitt reviewed Oct 20, 2016

View reviewed changes

pweil- force-pushed the psp-authorizer branch from 8992e27 to 1dc3fa5 Compare November 2, 2016 18:43

pweil- force-pushed the psp-authorizer branch from cbbb83c to 7db7924 Compare November 4, 2016 12:09

deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 7, 2016

deads2k added this to the v1.5 milestone Nov 7, 2016

timstclair reviewed Nov 7, 2016

View reviewed changes

add authz checks to allowed policies admission

bbe9c8f

pweil- force-pushed the psp-authorizer branch from 7db7924 to bbe9c8f Compare November 8, 2016 13:36

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

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2016

k8s-github-robot merged commit 5267462 into kubernetes:master Nov 10, 2016

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

This was referenced Jan 23, 2017

Bring PodSecurityPolicy to usable state #23217

Closed

Proposal: Authorizing use of a PodSecurityPolicy via the "Use" verb #17637

Closed

pweil- mentioned this pull request Apr 10, 2017

Update security proposals based on recent discussions kubernetes/community#500

Closed


		### Roles and bindings

		In order to a `PodSecurityPolicy` a user must have the ability to perform the `use` verb on the policy.

Add authz to psp admission #33080

Add authz to psp admission #33080

Conversation

pweil- commented Sep 20, 2016 • edited Loading

erictune commented Sep 22, 2016

erictune commented Sep 22, 2016

liggitt commented Sep 22, 2016 • edited Loading

erictune commented Sep 26, 2016

pweil- commented Oct 17, 2016

pweil- commented Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 20, 2016 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 2, 2016

pweil- commented Nov 2, 2016

erictune commented Nov 2, 2016

pweil- commented Nov 2, 2016 • edited Loading

k8s-ci-robot commented Nov 2, 2016

sttts commented Nov 3, 2016

deads2k commented Nov 3, 2016

pweil- commented Nov 3, 2016

pweil- commented Nov 4, 2016

pweil- commented Nov 4, 2016

pweil- commented Nov 4, 2016

pweil- commented Nov 7, 2016

pweil- commented Nov 7, 2016

deads2k commented Nov 7, 2016

pweil- commented Nov 7, 2016

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

saad-ali commented Nov 8, 2016

pweil- commented Nov 8, 2016

k8s-ci-robot commented Nov 8, 2016

k8s-ci-robot commented Nov 8, 2016

pweil- commented Nov 8, 2016

pweil- commented Nov 8, 2016

k8s-github-robot commented Nov 10, 2016

k8s-github-robot commented Nov 10, 2016

pweil- commented Sep 20, 2016 •

edited

Loading

liggitt commented Sep 22, 2016 •

edited

Loading

pweil- commented Oct 17, 2016 •

edited

Loading

liggitt Oct 20, 2016 •

edited

Loading

pweil- commented Nov 2, 2016 •

edited

Loading