Make openshift authorizer.GetResoruce match upstream by deads2k · Pull Request #13259 · openshift/origin

deads2k · 2017-03-06T19:51:25Z

Builds on #13256 .

This switches us to use our own interface for authorization evaluation and updates our GetResource to match upstreams and adds the GetSubresource to have complete information.

@liggitt fyi
@enj ptal.

deads2k · 2017-03-06T21:00:17Z

[test]

liggitt · 2017-03-08T21:08:34Z

pkg/authorization/authorizer/attributes.go

 // because the authorizer takes that information on the context
 func ToDefaultAuthorizationAttributes(in authorizationapi.Action) Action {
+	// to match the RequestInfoFactory assuming an in.resource of one/two/three, one==resource, two==subresource, three=nothing
+	tokens := strings.Split(in.Resource, "/")


I don't see any code paths where we put more than two segments in the resource field, but if we did, I don't think we should discard extra segments. I'd expect SplitN and let the subresource have the remainder, including any extra slashes.

liggitt · 2017-03-08T21:12:25Z

pkg/authorization/authorizer/attributes.go

+		return allowedResourceTypes.Has(strings.ToLower(a.GetResource()))
+	}
+
+	return allowedResourceTypes.Has(strings.ToLower(a.GetResource() + "/" + a.GetSubresource()))


this is going to get called a lot... should find a way to avoid allocating on every call

this is going to get called a lot... should find a way to avoid allocating on every call

I don't see this as a blocker for merge. The interfaces need to be right for the rebase, the call path optimization only needs to be done by release.

liggitt · 2017-03-08T21:14:18Z

pkg/cmd/server/kubernetes/node_auth.go

 		Verb:         apiVerb,
-		Resource:     "nodes/proxy",
+		Resource:     "nodes",
+		Subresource:  "proxy",


the ones below referenced as constants need changing as well

actually, all the references to the joined resources in synthetic.go need review :-/

heh, just saw you hit that in #13286

actually, all the references to the joined resources in synthetic.go need review :-/

I renamed them to find them. Interestingly enough, the authorization comparison still works because resource is opaque and still compares the entire thing.

liggitt · 2017-03-08T21:17:17Z

pkg/authorization/authorizer/attributes_test.go

+
+func (a authorizationAttributesAdapter) GetSubresource() string {
+	// to match the RequestInfoFactory assuming an in.resource of one/two/three, one==resource, two==subresource, three=nothing
+	tokens := strings.Split(a.attrs.Resource, "/")


same here, I'd expect SplitN

liggitt · 2017-03-08T21:23:33Z

pkg/authorization/authorizer/personal_subjectaccessreview.go

+	if !requestInfo.IsResourceRequest {
+		return requestInfo, nil
+	}
+	if strings.ToLower(requestInfo.Verb) != "create" {


are we getting verbs of mixed case?

are we getting verbs of mixed case?

Not sure. If at some point we want to inspect every possible call path to avoid the tolower, we can see about changing it later.

liggitt · 2017-03-08T21:24:21Z

pkg/authorization/authorizer/personal_subjectaccessreview.go

+	if strings.ToLower(requestInfo.Verb) != "create" {
+		return requestInfo, nil
+	}
+	if len(requestInfo.APIGroup) != 0 {


do the API groupies know the grouped version won't support the special self SubjectAccessReview behavior?

do the API groupies know the grouped version won't support the special self SubjectAccessReview behavior?

One of us merges first. We'll discover it either way.

liggitt · 2017-03-08T21:27:41Z

pkg/cmd/server/origin/master_config.go

 		sets.NewString(bootstrappolicy.AuthenticatedGroup),
 		requestInfoFactory,
 	)
+	personalSARRequestInfoResolveer := authorizer.NewPersonalSARRequestInfoResolver(browserSafeRequestInfoResolver)


typo personalSARRequestInfoResolveer

liggitt · 2017-03-08T21:33:58Z

pkg/authorization/authorizer/attributes.go

 // because the authorizer takes that information on the context
-func ToDefaultAuthorizationAttributes(in authorizationapi.Action) DefaultAuthorizationAttributes {
+func ToDefaultAuthorizationAttributes(in authorizationapi.Action) Action {
 	return DefaultAuthorizationAttributes{


huh... this is a chunky struct to copy around (either as an arg or as a receiver). not new, but worth making a pointer in a follow up.

enj

More nits

enj · 2017-03-08T23:59:10Z

pkg/authorization/authorizer/messages.go

-	messageResolver.addRootScopedForbiddenMessageMaker("update", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot update `+apiGroupIfNotEmpty+`{{.Attributes.GetResource}} at the cluster scope`))
-	messageResolver.addNamespacedForbiddenMessageMaker("delete", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot delete `+apiGroupIfNotEmpty+`{{.Attributes.GetResource}} in project "{{.Namespace}}"`))
-	messageResolver.addRootScopedForbiddenMessageMaker("delete", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot delete `+apiGroupIfNotEmpty+`{{.Attributes.GetResource}} at the cluster scope`))
+	messageResolver.addNamespacedForbiddenMessageMaker("create", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot create `+apiGroupIfNotEmpty+resourceWithSubresourceIfNotEmpty+` in project "{{.Namespace}}"`))


These should be ... resourceWithSubresourceIfNotEmpty+apiGroupIfNotEmpty ...

These should be ... resourceWithSubresourceIfNotEmpty+apiGroupIfNotEmpty

If you want to change the message formats (which I'd agree with), that's a separate thing to do.

enj · 2017-03-09T00:01:29Z

pkg/authorization/authorizer/messages.go

@@ -21,27 +21,28 @@ type ForbiddenMessageResolver struct {

 func NewForbiddenMessageResolver(projectRequestForbiddenTemplate string) *ForbiddenMessageResolver {
 	apiGroupIfNotEmpty := "{{if len .Attributes.GetAPIGroup }}{{.Attributes.GetAPIGroup}}.{{end}}"


Move the . before the group.

The dot is a separator for display. It could be removed entirely and the messages reworked, but it can't move before.

enj · 2017-03-09T00:06:47Z

pkg/authorization/authorizer/remote/authorizer.go

 }

 func getAction(namespace string, attributes authorizer.Action) authzapi.Action {
+	resource := attributes.GetResource()


Do we have tests for round tripping authorizer.Action <-> authzapi.Action. Maybe we don't need them if we completely use kube stuff...

Do we have tests for round tripping authorizer.Action <-> authzapi.Action. Maybe we don't need them if we completely use kube stuff...

We're two pulls (already open from removal). I think this is a gimme.

….k8s.io

deads2k · 2017-03-09T12:52:57Z

Opened #13316 for potential performance related followup.

enj

More nits... once tests pass I think this is good to go.

enj · 2017-03-09T14:53:19Z

pkg/authorization/authorizer/attributes.go

+	resource := ""
+	subresource := ""
+	switch {
+	case len(tokens) >= 2:


case len(tokens) == 2?

enj · 2017-03-09T14:53:50Z

pkg/authorization/authorizer/attributes.go

 // ToDefaultAuthorizationAttributes coerces Action to DefaultAuthorizationAttributes.  Namespace is not included
 // because the authorizer takes that information on the context
 func ToDefaultAuthorizationAttributes(in authorizationapi.Action) Action {
+	// to match the RequestInfoFactory assuming an in.resource of one/two/three, one==resource, two==subresource, three=nothing


Comment is wrong.

deads2k · 2017-03-09T15:05:42Z

More nits... once tests pass I think this is good to go.

comments addressed.

deads2k · 2017-03-09T16:31:20Z

re[test]

deads2k · 2017-03-09T18:50:10Z

re[test]

deads2k · 2017-03-09T19:29:58Z

re[test]

openshift-bot · 2017-03-09T19:37:28Z

Evaluated for origin test up to c3bb1dd

deads2k · 2017-03-09T20:38:13Z

[merge]

openshift-bot · 2017-03-09T20:45:25Z

Evaluated for origin merge up to c3bb1dd

openshift-bot · 2017-03-09T20:53:26Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/82/) (Base Commit: d3a3181)

openshift-bot · 2017-03-10T01:45:31Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/25/) (Base Commit: 18a751b) (Image: devenv-rhel7_6062)

deads2k assigned enj Mar 6, 2017

deads2k force-pushed the auth-05-interface branch from f1e3e39 to d0b1b25 Compare March 6, 2017 20:37

deads2k force-pushed the auth-05-interface branch from d0b1b25 to 37191a4 Compare March 7, 2017 13:39

deads2k mentioned this pull request Mar 7, 2017

switch to kube authorization attributes #13286

Merged

liggitt reviewed Mar 8, 2017

View reviewed changes

enj reviewed Mar 9, 2017

View reviewed changes

deads2k added 2 commits March 9, 2017 07:43

authorize personalSAR based on selfsubjectaccessreviews.authorization…

e3f7aaa

….k8s.io

switch to using interface for authorization matches

a4bdfab

deads2k mentioned this pull request Mar 9, 2017

potential performance cleanup of the authorizer #13316

Closed

deads2k force-pushed the auth-05-interface branch from 37191a4 to 10e2e6c Compare March 9, 2017 12:55

enj reviewed Mar 9, 2017

View reviewed changes

switch meaning to openshift.GetResource to match upstream

c3bb1dd

deads2k force-pushed the auth-05-interface branch from 10e2e6c to c3bb1dd Compare March 9, 2017 15:05

deads2k force-pushed the auth-05-interface branch from 185060d to c3bb1dd Compare March 9, 2017 18:46

openshift-bot merged commit 5602529 into openshift:master Mar 10, 2017

deads2k deleted the auth-05-interface branch August 3, 2017 19:24

simo5 mentioned this pull request Jan 18, 2018

Make Attributesrecord use pointers to reduce copying kubernetes/kubernetes#58356

Closed

		@@ -21,27 +21,28 @@ type ForbiddenMessageResolver struct {

		func NewForbiddenMessageResolver(projectRequestForbiddenTemplate string) *ForbiddenMessageResolver {
		apiGroupIfNotEmpty := "{{if len .Attributes.GetAPIGroup }}{{.Attributes.GetAPIGroup}}.{{end}}"

Conversation

deads2k commented Mar 6, 2017

Uh oh!

deads2k commented Mar 6, 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!

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!

enj left a comment

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!

deads2k commented Mar 9, 2017

Uh oh!

enj left a comment

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!

deads2k commented Mar 9, 2017

Uh oh!

deads2k commented Mar 9, 2017

Uh oh!

deads2k commented Mar 9, 2017

Uh oh!

deads2k commented Mar 9, 2017

Uh oh!

openshift-bot commented Mar 9, 2017

Uh oh!

deads2k commented Mar 9, 2017

Uh oh!

openshift-bot commented Mar 9, 2017

Uh oh!

openshift-bot commented Mar 9, 2017

Uh oh!

openshift-bot commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

openshift-bot commented Mar 10, 2017 •

edited

Loading