Add selfsubjectrulesreview in authorization #48051

xilabao · 2017-06-26T09:59:52Z

What this PR does / why we need it:

Which issue this PR fixes: fixes #47834 #31292

Special notes for your reviewer:

Release note:

Add selfsubjectrulesreview API for allowing users to query which permissions they have in a given namespace.

/cc @deads2k @liggitt

k8s-ci-robot · 2017-06-26T09:59:59Z

Hi @xilabao. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

deads2k · 2017-06-26T13:05:39Z

@ericchiang you were thinking something similar.

ericchiang

Awesome!

Would like to see the kubectl code as a different PR. If it's just @kubernetes/sig-auth-pr-reviews that needs to review then we can probably get this in faster. What do you think?

ericchiang · 2017-06-26T18:04:57Z

pkg/registry/authorization/rest/storage_authorization.go

 )

 type RESTStorageProvider struct {
-	Authorizer authorizer.Authorizer
+	Authorizer                authorizer.Authorizer
+	AuthorizationRuleResolver rbacregistryvalidation.AuthorizationRuleResolver


There are other kind of authorizers. Can we ensure that this isn't actually tied to the RBAC authorizer by introducing another interface like authorizer.Authorizer?

updated. PTAL @ericchiang

ericchiang

You've got some generated code in your first commit.

Also can you explain a bit about what the third commit "create the methods in the generated expansion files" means? Are you creating those manually? Sorry, I'm not super familiar with the generated clientset.

ericchiang · 2017-06-27T22:51:19Z

staging/src/k8s.io/apiserver/pkg/authorization/authorizer/interfaces.go

+// AuthorizationRulesGetter provides a function that get the list of rules that apply to a given user in a given namespace and error.
+// If an error is returned, the slice of rules may not be complete, but it contains all retrievable rules.  This is done because policy
+// rules are purely additive and policy determinations can be made on the basis of those rules that are found.
+type AuthorizationRulesGetter interface {


The equivalent openshift type is called "AuthorizaitonRuleResolver"[1]. I think authorizaiton.RuleResolver would be a much more natural fit.

[1] https://github.com/openshift/origin/blob/496129f10823185674fa9d20a7e11be46ca53512/pkg/authorization/rulevalidation/find_rules.go#L27

I think authorizaiton.RuleResolver would be a much more natural fit.

Done.

ericchiang · 2017-06-27T22:57:04Z

staging/src/k8s.io/apiserver/pkg/authorization/union/union.go

+}
+
+// Authorizes against a chain of authorizer.AuthorizationRulesGetter objects and returns nil if successful and returns error if unsuccessful
+func (authzHandler unionAuthzRulesHandler) RulesFor(user user.Info, namespace string) ([]authorizationapi.ResourceRule, []authorizationapi.NonResourceRule, error) {


I would have expected some sort of logic to de-dup identical rules from different authorizers or when one returns a wildcard rule that encompasses the other rules.

I would have expected some sort of logic to de-dup identical rules from different authorizers or when one returns a wildcard rule that encompasses the other rules.

could we sort in kubectl code?

ericchiang · 2017-06-27T22:58:37Z

pkg/registry/authorization/selfsubjectrulesreview/rest.go

+
+func NewREST(authorizationRulesGetter authorizer.AuthorizationRulesGetter) *REST {
+	return &REST{authorizationRulesGetter}
+	//return &REST{ruleResolver: ruleResolver}


nit: remove commented out return.

ericchiang · 2017-06-27T23:00:54Z

pkg/registry/authorization/selfsubjectrulesreview/rest.go

+	if !ok {
+		return nil, apierrors.NewBadRequest("no user present on request")
+	}
+	namespace, _ := genericapirequest.NamespaceFrom(ctx)


Why are we grabbing a namespace? Isn't the SelfSubjectRulesReview non-namespaced?

Why are we grabbing a namespace? Isn't the SelfSubjectRulesReview non-namespaced?

I think that SelfSubjectRulesReview can get the cluster wide rules and the rules in the specified namespace.

Which specified namespace? The resource isn't namespaced, so I don't see a way for this to ever hold a value.

@ericchiang you are right. SelfSubjectRulesReview should be namespaced. fixed. ptal

SelfSubjectRulesReview should be namespaced.

No, it definitely should definitely be cluster wide or you'd never be able to get information about cluster level resources. e.g. we need this to help determine what namespaces users have access too.

This still doesn't look right... I'd expect the types to be cluster-scoped, and there to be a spec that allows indicating the namespace to evaluate at (see SelfSubjectAccessReview.Spec.ResourceAttributes.Namespace as a parallel)

This still doesn't look right... I'd expect the types to be cluster-scoped, a

It seems weird to have a cluster scoped resource that is only valid in the context of a namespace. We can't reasonably evaluate permissions across all namespaces because of fanout. We could give strictly cluster-scoped powers, but that's of limited utility. You're just trying for parity with SAR?

ericchiang · 2017-06-27T23:07:38Z

staging/src/k8s.io/apiserver/pkg/authorization/authorizer/interfaces.go

+// If an error is returned, the slice of rules may not be complete, but it contains all retrievable rules.  This is done because policy
+// rules are purely additive and policy determinations can be made on the basis of those rules that are found.
+type AuthorizationRulesGetter interface {
+	RulesFor(user user.Info, namespace string) ([]authorizationapi.ResourceRule, []authorizationapi.NonResourceRule, error)


Need to articulate how this method works for cluster wide (non-namespaced) rule evaluation. Particularly since this is only being used to fill out a non-namespaced resource.

Need to articulate how this method works for cluster wide (non-namespaced) rule evaluation. Particularly since this is only being used to fill out a non-namespaced resource.

comments added

ericchiang · 2017-06-27T23:08:30Z

staging/src/k8s.io/apiserver/pkg/authorization/authorizer/interfaces.go

@@ -20,6 +20,7 @@ import (
 	"net/http"

 	"k8s.io/apiserver/pkg/authentication/user"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"


Don't know the implications of having this start to import an API package since before it was nicely self contained. cc @deads2k any thoughts here?

Don't know the implications of having this start to import an API package since before it was nicely self contained.

Done. use "k8s.io/api/authorization/v1" instead of "k8s.io/kubernetes/pkg/apis/authorization"

xilabao · 2017-06-28T08:34:10Z

Also can you explain a bit about what the third commit "create the methods in the generated expansion files" means? Are you creating those manually?

I create those methods manually. I ref to the discussion at #31271 (comment)

ericchiang · 2017-06-28T16:52:23Z

/ok-to-test

ericchiang · 2017-08-31T22:20:01Z

This is working and I don't see any other outstanding concerns about the API

$ curl -k -X POST \
>     --key /var/run/kubernetes/client-admin.key \
>     --cert /var/run/kubernetes/client-admin.crt \
>     -H 'Content-Type: application/json;stream=watch' \
>     --data '{"spec":{"namespace":"kube-system"}}' \
>     https://localhost:6443/apis/authorization.k8s.io/v1/selfsubjectrulesreviews
{
  "kind": "SelfSubjectRulesReview",
  "apiVersion": "authorization.k8s.io/v1",
  "metadata": {
    "creationTimestamp": null
  },
  "spec": {},
  "status": {
    "resourceRules": [
      {
        "verbs": [
          "*"
        ],
        "apiGroups": [
          "*"
        ],
        "resources": [
          "*"
        ]
      },
      {
        "verbs": [
          "create"
        ],
        "apiGroups": [
          "authorization.k8s.io"
        ],
        "resources": [
          "selfsubjectaccessreviews"
        ]
      }
    ],
    "nonResourceRules": [
      {
        "verbs": [
          "*"
        ],
        "nonResourceURLs": [
          "*"
        ]
      },
      {
        "verbs": [
          "get"
        ],
        "nonResourceURLs": [
          "/api",
          "/api/*",
          "/apis",
          "/apis/*",
          "/healthz",
          "/swaggerapi",
          "/swaggerapi/*",
          "/version"
        ]
      }
    ],
    "incomplete": false
  }
}

/lgtm

calebamiles · 2017-09-01T00:04:07Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-bazel

xilabao · 2017-09-01T13:08:27Z

/test pull-kubernetes-bazel
/test pull-kubernetes-kubemark-e2e-gce
/test pull-kubernetes-e2e-gce-etcd3

xilabao · 2017-09-01T14:09:56Z

/test pull-kubernetes-kubemark-e2e-gce

deads2k · 2017-09-01T14:26:44Z

/approve

xilabao · 2017-09-01T15:00:07Z

/test pull-kubernetes-kubemark-e2e-gce

ericchiang · 2017-09-01T15:39:02Z

/lgtm

/assign @smarterclayton

smarterclayton · 2017-09-01T15:47:31Z

/approve

k8s-github-robot · 2017-09-01T15:48:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ericchiang, smarterclayton, xilabao

Associated issue: 47834

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

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

~~api/OWNERS~~ [smarterclayton]
~~cmd/kube-apiserver/OWNERS~~ [deads2k,smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~federation/cmd/federation-apiserver/OWNERS~~ [deads2k,smarterclayton]
~~pkg/apis/OWNERS~~ [smarterclayton]
~~pkg/auth/OWNERS~~ [deads2k,smarterclayton]
~~pkg/client/OWNERS~~ [deads2k,smarterclayton]
~~pkg/controller/garbagecollector/OWNERS~~ [deads2k,smarterclayton]
~~pkg/kubeapiserver/OWNERS~~ [deads2k,smarterclayton]
~~pkg/master/OWNERS~~ [deads2k,smarterclayton]
~~pkg/registry/OWNERS~~ [deads2k,smarterclayton]
~~plugin/pkg/auth/OWNERS~~ [deads2k,smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k,smarterclayton]
~~test/OWNERS~~ [deads2k,smarterclayton]

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

ericchiang · 2017-09-01T16:54:57Z

/retest

ericchiang · 2017-09-01T18:52:31Z

/test pull-kubernetes-kubemark-e2e-gce

fejta-bot · 2017-09-02T12:51:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

CaoShuFeng · 2017-09-02T14:33:41Z

/test pull-kubernetes-verify

liggitt · 2017-09-02T14:59:18Z

/retest

ericchiang · 2017-09-02T17:29:49Z

/retest

k8s-github-robot · 2017-09-02T19:00:39Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-02T19:11:03Z

Automatic merge from submit-queue (batch tested with PRs 45724, 48051, 46444, 51056, 51605)

k8s-ci-robot · 2017-09-02T20:19:51Z

@xilabao: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	921a0566f15b961131e6001966b655535ae64aca	link	`/test pull-kubernetes-bazel`
pull-kubernetes-e2e-gce-etcd3	`790374d`	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-e2e-kops-aws	`790374d`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot requested review from liggitt and deads2k June 26, 2017 09:59

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 26, 2017

k8s-github-robot assigned mikedanese and erictune Jun 26, 2017

ericchiang reviewed Jun 26, 2017

View reviewed changes

xilabao force-pushed the add-selfsubjectrulesreview-api branch from dedc6db to ebc53b5 Compare June 27, 2017 10:03

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2017

xilabao force-pushed the add-selfsubjectrulesreview-api branch from ebc53b5 to 6c4e095 Compare June 27, 2017 10:05

ericchiang reviewed Jun 27, 2017

View reviewed changes

xilabao force-pushed the add-selfsubjectrulesreview-api branch 2 times, most recently from cfcc972 to 7fb4c2f Compare June 28, 2017 08:12

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 28, 2017

xilabao force-pushed the add-selfsubjectrulesreview-api branch 7 times, most recently from 9eaaa68 to 5f4deda Compare June 29, 2017 08:42

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

xilabao added 3 commits September 1, 2017 19:09

add selfsubjectrulesreview api

f14c138

generated

ed8adf6

create the methods in the generated expansion files

790374d

xilabao force-pushed the add-selfsubjectrulesreview-api branch from ae322d7 to 790374d Compare September 1, 2017 11:16

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

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

k8s-ci-robot assigned smarterclayton Sep 1, 2017

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

ericchiang mentioned this pull request Sep 1, 2017

sig-auth release notes: add advanced audit to action required, add selfsubjectrulesreview kubernetes/enhancements#412

Merged

k8s-github-robot merged commit c84b313 into kubernetes:master Sep 2, 2017

tombentley mentioned this pull request Feb 15, 2018

Work accross namespaces strimzi/strimzi-kafka-operator#125

Closed

Add selfsubjectrulesreview in authorization #48051

Add selfsubjectrulesreview in authorization #48051

Conversation

xilabao commented Jun 26, 2017 • edited by ericchiang Loading

k8s-ci-robot commented Jun 26, 2017

deads2k commented Jun 26, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang left a 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

Choose a reason for hiding this comment

xilabao commented Jun 28, 2017

ericchiang commented Jun 28, 2017

ericchiang commented Aug 31, 2017

calebamiles commented Sep 1, 2017

xilabao commented Sep 1, 2017

xilabao commented Sep 1, 2017

deads2k commented Sep 1, 2017

xilabao commented Sep 1, 2017

ericchiang commented Sep 1, 2017

smarterclayton commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

ericchiang commented Sep 1, 2017

ericchiang commented Sep 1, 2017

fejta-bot commented Sep 2, 2017

CaoShuFeng commented Sep 2, 2017

liggitt commented Sep 2, 2017

ericchiang commented Sep 2, 2017

k8s-github-robot commented Sep 2, 2017

k8s-github-robot commented Sep 2, 2017

k8s-ci-robot commented Sep 2, 2017

xilabao commented Jun 26, 2017 •

edited by ericchiang

Loading