RBAC Migration Followup #4: Use upstream subject locator directly #16034

abstractj · 2017-08-29T15:07:25Z

This PR attempts to fix the follow up #4 from RBAC migration.

Remaining task:

Merge my changes with @tiran's code to convert []rbac.Subject -> users, groups
[]string

openshift-merge-robot · 2017-08-29T15:07:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abstractj
We suggest the following additional approver: liggitt

Assign the PR to them by writing /assign @liggitt in a comment when ready.

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

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

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

abstractj · 2017-08-29T15:15:26Z

/unassign @liggitt

/assign @enj

abstractj · 2017-08-29T18:59:19Z

/retest

liggitt · 2017-08-29T19:07:39Z

pkg/authorization/registry/resourceaccessreview/rest.go

+	groups := sets.String{}
+	for _, subject := range subjects {
+		switch subject.Kind {
+		case rbac.UserKind:


need to handle ServiceAccount and map to a username

@liggitt I was wondering if we should take the code from here https://github.com/openshift/origin/pull/16034/files#diff-5654e020d459e05c8fcca57d5675ece4R59 and move to an utility function. In this way would be possible to reuse here and also at reviewer.go
Does it make sense? If yes, which place would be the best for it?

sure, a utility function to go from subjects to users/groups is fine

liggitt · 2017-08-29T19:07:56Z

pkg/project/auth/reviewer.go

+	users := sets.String{}
+	groups := sets.String{}
+	for _, subject := range subjects {
+		switch subject.Kind {


liggitt · 2017-08-29T19:19:47Z

pkg/cmd/server/origin/openshift_apiserver.go

@@ -62,6 +61,7 @@ import (
 	securityapiv1 "github.com/openshift/origin/pkg/security/apis/security/v1"
 	templateapiv1 "github.com/openshift/origin/pkg/template/apis/template/v1"
 	userapiv1 "github.com/openshift/origin/pkg/user/apis/user/v1"
+	"k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac"


group with kube imports

enj · 2017-08-29T19:29:27Z

pkg/cmd/server/origin/master_config.go

 	scopeLimitedAuthorizer := scope.NewAuthorizer(roleBasedAuthorizer, clusterRoleGetter, messageMaker)

 	authorizer := authorizerunion.New(
 		authorizerfactory.NewPrivilegedGroups(user.SystemPrivilegedGroup), // authorizes system:masters to do anything, just like upstream
 		scopeLimitedAuthorizer)

-	return authorizer, subjectLocator
+	return authorizer, kubeSubjectLocator


This should no longer take or return the kubeSubjectLocator

enj · 2017-08-29T19:37:05Z

pkg/authorization/authorizer/authorizer.go

+//	users := sets.String{}
+//	groups := sets.String{}
+//	namespace := attributes.GetNamespace()
+//	subjects, err := a.delegate.AllowedSubjects(attributes)


@abstractj sync with @tiran to use this as the foundation for two public functions:

[]rbac.Subject -> users, groups []string

users, groups []string -> []rbac.Subject

Put those in a new file at pkg/authorization/registry/util/subject.go for now.

[]rbac.Subject, defaultServiceAccountNamespace -> users, groups []string

do we need the reverse?

https://github.com/openshift/origin/pull/16033/files#diff-9cbb0b75c1c78e77c6ee106a6ac6fdd8R121

abstractj · 2017-08-30T10:09:44Z

/retest

dobbymoodge · 2017-08-30T18:30:01Z

/retest

abstractj · 2017-08-31T06:28:45Z

/retest

abstractj · 2017-08-31T12:13:54Z

/retest

enj · 2017-08-31T14:23:45Z

@abstractj you broke it 😄

coverage: 61.6% of statements
ok  	github.com/openshift/origin/vendor/k8s.io/kubernetes/vendor/k8s.io/kube-gen/cmd/openapi-gen/generators	1.549s	coverage: 61.6% of statements
[WARNING] `go test` had the following output to stderr:
# github.com/openshift/origin/pkg/authorization/registry/localresourceaccessreview
pkg/authorization/registry/localresourceaccessreview/rest_test.go:75: cannot use authorizer (type *testAuthorizer) as type "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator in argument to resourceaccessreview.NewREST:
	*testAuthorizer does not implement "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator (missing AllowedSubjects method)
pkg/authorization/registry/localresourceaccessreview/rest_test.go:123: cannot use r.authorizer (type *testAuthorizer) as type "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator in argument to resourceaccessreview.NewREST:
	*testAuthorizer does not implement "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator (missing AllowedSubjects method)
# github.com/openshift/origin/pkg/authorization/registry/resourceaccessreview
pkg/authorization/registry/resourceaccessreview/rest_test.go:108: cannot use r.authorizer (type *testAuthorizer) as type "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator in field value:
	*testAuthorizer does not implement "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator (missing AllowedSubjects method)
[INFO] No compiled `junitreport` binary was found. Attempting to build one using:
[INFO]   $ hack/build-go.sh tools/junitreport
++ Building go targets for linux/amd64: tools/junitreport
[INFO] hack/build-go.sh exited with code 0 after 00h 00m 17s
[INFO] jUnit XML report placed at _output/scripts/test-go/artifacts/gotest_report_4HKfT.xml
Of 6084 tests executed in 2616.257s, 6065 succeeded, 0 failed, and 19 were skipped.
In suite "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/cloudstack", test case "TestNewCSCloud" was skipped:
=== RUN   TestNewCSCloud
--- SKIP: TestNewCSCloud (0.00s)
	cloudstack_test.go:90: No config found in environment
In suite "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/cloudstack", test case "TestLoadBalancer" was skipped:
=== RUN   TestLoadBalancer
--- SKIP: TestLoadBalancer (0.00s)

abstractj · 2017-10-02T12:52:39Z

@enj I just did a rebase, but I've been banging my head against the wall with rest_test.go. I would appreciate if have some minutes of your life to give me a help.

Now that I changed the test I get the following output running rest_test.go:

	rest_test.go:186: diff (
		
		A: authorizer.AttributesRecord){User:(user.Info)<nil> Verb:(string)get Namespace:(string)unittest APIGroup:(string) APIVersion:(string) Resource:(string)pods Subresource:(string) Name:(string) ResourceRequest:(bool)true Path:(string)}
		
		B: interface {})<nil>
		
	rest_test.go:175: diff (*authorization.ResourceAccessReviewResponse){TypeMeta:(v1.TypeMeta){Kind:(string) APIVersion:(string)} Namespace:(string)unittest Users:(sets.String)map[
		
		A: one:{} two:{}] Groups:(sets.String)map[three:{} four:{}] EvaluationError:(string)}
		
		B: ] Groups:(sets.String)map[] EvaluationError:(string)}
		
	rest_test.go:186: diff (
		
		A: authorizer.AttributesRecord){User:(user.Info)<nil> Verb:(string)delete Namespace:(string)unittest APIGroup:(string) APIVersion:(string) Resource:(string)deploymentConfig Subresource:(string) Name:(string) ResourceRequest:(bool)true Path:(string)}
		
		B: interface {})<nil>
		

Process finished with exit code 1

Which I believe, the cause is exclusively my ignorance about it.

abstractj · 2017-10-04T00:27:47Z

/retest

abstractj · 2017-10-04T01:18:34Z

/retest

All the changes requested were already included, I believe.

abstractj · 2017-10-04T03:00:16Z

@simo5 could you please give it a try and let me know what do you think? Now rest_test.go is passing, just not sure what's wrong in ci/openshift-jenkins/unit job

simo5 · 2017-10-04T12:30:49Z

This is the error:

pkg/authorization/registry/resourceaccessreview/rest_test.go:108: cannot use r.authorizer (type *testAuthorizer) as type "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator in field value:
	*testAuthorizer does not implement "github.com/openshift/origin/vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac".SubjectLocator (missing AllowedSubjects method)```

abstractj · 2017-10-05T18:55:21Z

/retest

abstractj · 2017-10-05T21:03:13Z

/retest

abstractj · 2017-10-05T22:43:29Z

/retest

abstractj · 2017-10-06T02:31:26Z

/retest

0xmichalis · 2017-10-06T09:38:40Z

/retest

abstractj · 2017-10-06T16:45:27Z

/retest

abstractj · 2017-10-09T22:26:48Z

/retest

enj · 2017-10-10T14:56:19Z

/hold

This is not a bug, docs or tests so cannot be merged until 3.8.

I still need to review it though.

enj · 2017-10-10T15:16:04Z

pkg/authorization/registry/localresourceaccessreview/rest_test.go

-		return a.users, a.groups, nil
-	}
-	return a.users, a.groups, errors.New(a.err)
+	subjects := authorizationutil.BuildRBACSubjects(a.users.List(), a.groups.List())


This is fine if the goal is to keep the diff small. But in reality the test authorizer should just return RBAC subject directly.

@enj I rebased/updated this PR with the changes you request. But this is the only part of your review which I'm not following.

The goal of this change was to keep the diff small as possible, plus I couldn't find any examples of usage to include RBAC subject directly. But I will be more than happy to look at this if you have any pointers of snippets.

enj · 2017-10-10T15:16:35Z

pkg/authorization/registry/resourceaccessreview/rest.go

@@ -4,29 +4,30 @@ import (
 	"errors"
 	"fmt"

+	authorizationutil "github.com/openshift/origin/pkg/authorization/util"


Move import to openshfit block.

enj · 2017-10-10T15:33:32Z

pkg/authorization/registry/resourceaccessreview/rest_test.go

-		return a.users, a.groups, nil
-	}
-	return a.users, a.groups, errors.New(a.err)
+	subjects := authorizationutil.BuildRBACSubjects(a.users.List(), a.groups.List())


Same comment as earlier.

This is fine if the goal is to keep the diff small. But in reality the test authorizer should just return RBAC subject directly.

enj · 2017-10-10T15:33:55Z

pkg/authorization/util/subject.go

@@ -23,3 +25,35 @@ func BuildRBACSubjects(users, groups []string) []rbac.Subject {

 	return subjects
 }
+
+func ExpandSubjects(namespace string, subjects []rbac.Subject) (sets.String, sets.String, error) {
+	var err error


list of errors

enj · 2017-10-10T15:34:06Z

pkg/authorization/util/subject.go

+				users.Insert(name)
+			}
+		default:
+			err = fmt.Errorf("Unrecognized kinds for subjects: %s. "+


append to list of errors

enj · 2017-10-10T15:34:35Z

pkg/authorization/util/subject.go

+		}
+
+	}
+	return users, groups, err


NewAggregate on the errors

enj · 2017-10-10T15:37:17Z

pkg/project/auth/reviewer.go

+	users, groups, expandSubjectError := authorizationutil.ExpandSubjects(namespaceName, subjects)
+
+	if expandSubjectError != nil {
+		return nil, expandSubjectError


This should not be fatal, it should add to review.evaluationError

enj · 2017-10-10T15:42:44Z

pkg/project/auth/reviewer.go

@@ -73,7 +74,9 @@ func (r *authorizerReviewer) Review(namespaceName string) (Review, error) {
 		ResourceRequest: true,
 	}

-	users, groups, err := r.policyChecker.GetAllowedSubjects(attributes)
+	subjects, err := r.policyChecker.AllowedSubjects(attributes)


Something like this:

func (r *authorizerReviewer) Review(namespaceName string) (Review, error) { attributes := kauthorizer.AttributesRecord{ Verb: "get", Namespace: namespaceName, Resource: "namespaces", Name: namespaceName, ResourceRequest: true, } // err is non fatal and partial success is possible subjects, err := r.policyChecker.AllowedSubjects(attributes) // expandSubjectError is non fatal and partial success is possible users, groups, expandSubjectError := authorizationutil.ExpandSubjects(namespaceName, subjects) review := &defaultReview{ users: users.List(), groups: groups.List(), } if err != nil { review.evaluationError = err.Error() } if expandSubjectError != nil { review.evaluationError += " " + expandSubjectError.Error() } return review, nil }

enj · 2017-10-10T15:48:22Z

pkg/authorization/registry/resourceaccessreview/rest.go

@@ -63,7 +63,9 @@ func (r *REST) Create(ctx apirequest.Context, obj runtime.Object, _ bool) (runti
 	}

 	attributes := util.ToDefaultAuthorizationAttributes(nil, resourceAccessReview.Action.Namespace, resourceAccessReview.Action)
-	users, groups, err := r.subjectLocator.GetAllowedSubjects(attributes)
+	subjects, err := r.subjectLocator.AllowedSubjects(attributes)


Something like:

func (r *REST) Create(ctx apirequest.Context, obj runtime.Object, _ bool) (runtime.Object, error) { resourceAccessReview, ok := obj.(*authorizationapi.ResourceAccessReview) if !ok { return nil, kapierrors.NewBadRequest(fmt.Sprintf("not a resourceAccessReview: %#v", obj)) } if errs := authorizationvalidation.ValidateResourceAccessReview(resourceAccessReview); len(errs) > 0 { return nil, kapierrors.NewInvalid(authorizationapi.Kind(resourceAccessReview.Kind), "", errs) } user, ok := apirequest.UserFrom(ctx) if !ok { return nil, kapierrors.NewInternalError(errors.New("missing user on request")) } // if a namespace is present on the request, then the namespace on the on the RAR is overwritten. // This is to support backwards compatibility. To have gotten here in this state, it means that // the authorizer decided that a user could run an RAR against this namespace if namespace := apirequest.NamespaceValue(ctx); len(namespace) > 0 { resourceAccessReview.Action.Namespace = namespace } else if err := r.isAllowed(user, resourceAccessReview); err != nil { // this check is mutually exclusive to the condition above. localSAR and localRAR both clear the namespace before delegating their calls // We only need to check if the RAR is allowed **again** if the authorizer didn't already approve the request for a legacy call. return nil, err } attributes := util.ToDefaultAuthorizationAttributes(nil, resourceAccessReview.Action.Namespace, resourceAccessReview.Action) // err is non fatal and partial success is possible subjects, err := r.subjectLocator.AllowedSubjects(attributes) // expandSubjectError is non fatal and partial success is possible users, groups, expandSubjectError := authorizationutil.ExpandSubjects(resourceAccessReview.Action.Namespace, subjects) response := &authorizationapi.ResourceAccessReviewResponse{ Namespace: resourceAccessReview.Action.Namespace, Users: users, Groups: groups, } if err != nil { response.EvaluationError = err.Error() } if err != nil { response.EvaluationError += " " + expandSubjectError.Error() } return response, nil }

abstractj · 2017-11-09T11:01:48Z

Fixing the build....hold on

abstractj · 2017-11-09T13:57:42Z

/retest

abstractj · 2017-11-10T12:02:12Z

/hold cancel

enj · 2017-11-10T13:54:52Z

/hold

Needs to wait until Monday at the earliest.

openshift-ci-robot · 2017-11-13T22:51:06Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/origin/verify	`9fbc0c3`	link	`/test origin-verify`
ci/openshift-jenkins/experimental/integration	`e5955e7`	link	`/test origin-it`
ci/openshift-jenkins/extended_conformance_install_update	`566de13`	link	`/test extended_conformance_install_update`
ci/openshift-jenkins/extended_conformance_gce	`566de13`	link	`/test extended_conformance_gce`

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.

openshift-merge-robot · 2017-11-29T08:15:11Z

@abstractj PR needs rebase

enj · 2018-01-18T01:20:41Z

Superseded by #18152

abstractj requested a review from enj August 29, 2017 15:07

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 29, 2017

openshift-merge-robot assigned liggitt and enj Aug 29, 2017

openshift-ci-robot unassigned liggitt Aug 29, 2017

liggitt reviewed Aug 29, 2017

View reviewed changes

enj previously requested changes Aug 29, 2017

View reviewed changes

enj mentioned this pull request Aug 29, 2017

Drop double conversion in boostrap policy #16033

Merged

abstractj requested review from enj and liggitt August 31, 2017 12:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2017

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2017

abstractj changed the title ~~[DO NOT MERGE] - RBAC Migration Followup #4: Use upstream subject locator directly~~ RBAC Migration Followup #4: Use upstream subject locator directly Oct 4, 2017

abstractj requested a review from simo5 October 4, 2017 02:48

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2017

abstractj mentioned this pull request Oct 6, 2017

Swagger spec is causing failures on ci/openshift-jenkins/origin/verify job #16720

Closed

simo5 approved these changes Oct 10, 2017

View reviewed changes

enj added this to the 3.8.0 milestone Oct 10, 2017

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2017

enj suggested changes Oct 10, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2017

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2017

abstractj requested a review from enj November 9, 2017 01:41

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2017

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2017

RBAC Migration Followup #4: Use upstream subject locator directly

566de13

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2017

enj mentioned this pull request Jan 18, 2018

remove subjectlocator shim and divide responsibility #18152

Merged

enj closed this Jan 18, 2018

RBAC Migration Followup #4: Use upstream subject locator directly #16034

RBAC Migration Followup #4: Use upstream subject locator directly #16034

Uh oh!

Conversation

abstractj commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-robot commented Aug 29, 2017

Uh oh!

abstractj commented Aug 29, 2017

Uh oh!

abstractj commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abstractj commented Aug 30, 2017

Uh oh!

dobbymoodge commented Aug 30, 2017

Uh oh!

abstractj commented Aug 31, 2017

Uh oh!

abstractj commented Aug 31, 2017

Uh oh!

enj commented Aug 31, 2017

Uh oh!

abstractj commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abstractj commented Oct 4, 2017

Uh oh!

abstractj commented Oct 4, 2017

Uh oh!

abstractj commented Oct 4, 2017

Uh oh!

simo5 commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abstractj commented Oct 5, 2017

Uh oh!

abstractj commented Oct 5, 2017

Uh oh!

abstractj commented Oct 5, 2017

Uh oh!

abstractj commented Oct 6, 2017

Uh oh!

0xmichalis commented Oct 6, 2017

Uh oh!

abstractj commented Oct 6, 2017

Uh oh!

abstractj commented Oct 9, 2017

Uh oh!

enj commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

abstractj commented Aug 29, 2017 •

edited

Loading

abstractj commented Oct 2, 2017 •

edited

Loading

simo5 commented Oct 4, 2017 •

edited

Loading

enj Oct 10, 2017 •

edited

Loading

openshift-ci-robot commented Nov 13, 2017 •

edited

Loading