switch to kube authorization attributes #13286

deads2k · 2017-03-07T18:44:03Z

Builds on #13259.

This pull removes our existing attributes and uses the upstream kube attributes. At this point our interfaces should overlap. I'm planning one more to remove our authorizer interface altogether.

@liggitt @enj

deads2k · 2017-03-07T18:44:15Z

[test]

ncdc · 2017-03-08T19:10:38Z

pkg/authorization/authorizer/attributes_builder.go

@@ -16,28 +17,36 @@ func NewAuthorizationAttributeBuilder(contextMapper kapi.RequestContextMapper, i
 	return &openshiftAuthorizationAttributeBuilder{contextMapper, infoFactory}
 }

-func (a *openshiftAuthorizationAttributeBuilder) GetAttributes(req *http.Request) (Action, error) {
+func (a *openshiftAuthorizationAttributeBuilder) GetAttributes(req *http.Request) (authorizer.Attributes, error) {
+	attribs := authorizer.AttributesRecord{}


Move this down before line 32?

ncdc · 2017-03-08T19:21:58Z

pkg/authorization/authorizer/cache/authorizer.go

-		keyData["namespace"] = namespace
-	}
-	if user, ok := kapi.UserFrom(ctx); ok {
+	keyData["namespace"] = a.GetNamespace()


Move up into the map declaration?

liggitt · 2017-03-08T21:45:44Z

pkg/authorization/authorizer/cache/authorizer_test.go

+				Resource:        "r",
+				Subresource:     "sub",
+				Name:            "rn",
+				ResourceRequest: true,


doesn't really matter, but NonResourceURL: true is the opposite of ResourceRequest: true

doesn't really matter, but NonResourceURL: true is the opposite of ResourceRequest: true

Didn't matter in the test. I thought it better to test something other than zero values.

liggitt · 2017-03-08T21:46:50Z

pkg/authorization/authorizer/cache/authorizer_test.go

@@ -59,7 +56,7 @@ func TestCacheKey(t *testing.T) {
 }

 func TestCacheKeyFields(t *testing.T) {
-	keyJSON, err := cacheKey(kapi.NewContext(), &authorizer.DefaultAuthorizationAttributes{})
+	keyJSON, err := cacheKey(authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "me"}})


why was the default user needed?

oh, to exercise all the cache keys

enj

Still reviewing...

enj · 2017-03-09T00:23:31Z

pkg/authorization/authorizer/attributes.go

-	NonResourceURL bool
-	URL            string
-}
-
 // ToDefaultAuthorizationAttributes coerces Action to DefaultAuthorizationAttributes.  Namespace is not included


Out of date comment.

enj · 2017-03-09T01:23:35Z

pkg/authorization/registry/subjectaccessreview/rest.go

+	var localSARAttributes kauthorizer.AttributesRecord
+	// if they are running a personalSAR, create synthentic check for selfSAR
+	if authorizer.IsPersonalAccessReviewFromSAR(sar) {
+		localSARAttributes = kauthorizer.AttributesRecord{


This and every other AttributesRecord that is manually built is incorrect. ResourceRequest needs to be explicitly set to true.

I0309 01:03:37.003518 13555 messages.go:126] User "developer" cannot "create" on "" <nil> authorizer.AttributesRecord{User:(*user.DefaultInfo)(0xc4277905c0), Verb:"create", Namespace:"", APIGroup:"authorization.k8s.io", APIVersion:"", Resource:"selfsubjectaccessreviews", Subresource:"", Name:"", ResourceRequest:false, Path:""}

Well. That's a painful default.

Well. That's a painful default.

Yeah, guess I missed a couple.

Also, glad our tests exercise many of these. Go us :)

enj · 2017-03-09T03:39:39Z

pkg/authorization/authorizer/interfaces.go

-	// GetURL returns the URL path being requested, including the leading '/'
-	GetURL() string
-}
-
 // ForbiddenMessageMaker creates a forbidden message from a MessageContext
 type ForbiddenMessageMaker interface {
 	MakeMessage(ctx MessageContext) (string, error)
 }

 // MessageContext contains sufficient information to create a forbidden message.  It is bundled in this one object to make it easy and obvious how to build a golang template
 type MessageContext struct {


Drop MessageContext and use authorizer.Attributes directly?

Drop MessageContext and use authorizer.Attributes directly?

If you want to do it as a followup, sure. Not needed to collapse interfaces ahead of the rebase.

enj · 2017-03-09T03:41:28Z

pkg/authorization/authorizer/interfaces.go

 	"k8s.io/kubernetes/pkg/util/sets"
 )

 type Authorizer interface {
-	Authorize(ctx kapi.Context, a Action) (allowed bool, reason string, err error)
-	GetAllowedSubjects(ctx kapi.Context, attributes Action) (sets.String, sets.String, error)
+	Authorize(a authorizer.Attributes) (allowed bool, reason string, err error)


a -> attributes

….k8s.io

deads2k · 2017-03-09T13:17:41Z

Oh, the cool one was the subtle namespace problem. We hacked the namespace into the context in a filter for projects. When I eliminated using context, we lost that value for project authorization which made everything go haywire.

deads2k · 2017-03-09T13:23:34Z

comments addressed.

ncdc · 2017-03-09T14:49:58Z

These look like they might be caused by this PR?

09:22:16 === BEGIN TEST CASE ===
09:22:16 test/cmd/images_tests.sh:204: executing 'oc tag cmd-images-old-policy/mysql:5.5 newrepo:latest' expecting success
09:22:16 
FAILURE after 0.722s: test/cmd/images_tests.sh:204: executing 'oc tag cmd-images-old-policy/mysql:5.5 newrepo:latest' expecting success: the command returned the wrong error code
09:22:16 There was no output from the command.
09:22:16 Standard error from the command:
09:22:16 The ImageStream "newrepo" is invalid: []: Internal error: imagestreams "newrepo" is invalid: spec.tags[latest].from: Forbidden: cmd-images-old-policy/mysql
09:22:16 === END TEST CASE ===
09:22:16 In suite "github.com/openshift/origin/test/cmd/policy", test case "test/cmd/policy.sh:113: executing 'oc policy scc-subject-review -f ${OS_ROOT}/test/testdata/job.yaml -o=jsonpath={.status.AllowedBy.name}' expecting success and text 'restricted'" failed:
09:22:16 === BEGIN TEST CASE ===
09:22:16 test/cmd/policy.sh:113: executing 'oc policy scc-subject-review -f ${OS_ROOT}/test/testdata/job.yaml -o=jsonpath={.status.AllowedBy.name}' expecting success and text 'restricted'
09:22:16 FAILURE after 0.194s: test/cmd/policy.sh:113: executing 'oc policy scc-subject-review -f ${OS_ROOT}/test/testdata/job.yaml -o=jsonpath={.status.AllowedBy.name}' expecting success and text 'restricted': the command returned the wrong error code; the output content test failed
09:22:16 There was no output from the command.
09:22:16 Standard error from the command:
09:22:16 error: unable to compute Pod Security Policy Subject Review for "hello": User "bob" cannot create podsecuritypolicyselfsubjectreviews in project "bob"
09:22:16 === END TEST CASE ===

deads2k · 2017-03-09T14:57:32Z

These look like they might be caused by this PR?

Yeah, I'll make it through.

enj · 2017-03-09T15:49:19Z

pkg/cmd/server/origin/master_config.go

@@ -827,8 +827,9 @@ func newAuthorizationAttributeBuilder(requestContextMapper kapi.RequestContextMa
 		requestInfoFactory,
 	)
 	personalSARRequestInfoResolver := authorizer.NewPersonalSARRequestInfoResolver(browserSafeRequestInfoResolver)
+	projectRequestInfoResolver := authorizer.NewProjectRequestInfoResolver(personalSARRequestInfoResolver)


final := chainFunc( browserSafeRequestInfoResolver, // base info authorizer.NewPersonalSARRequestInfoResolver, // wrapper funcs authorizer.NewProjectRequestInfoResolver, )

final := chainFunc(
browserSafeRequestInfoResolver, // base info
authorizer.NewPersonalSARRequestInfoResolver, // wrapper funcs
authorizer.NewProjectRequestInfoResolver,
)

No real disagreement, but I suspect this gets reswizzled in the rebase. You want the related updates now?

After rebase sounds good to me.

enj · 2017-03-09T16:17:15Z

LGTM if tests pass.

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

re[test]

deads2k · 2017-03-09T20:37:14Z

re[test]

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

[merge]

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

Evaluated for origin merge up to 52e4858

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

Evaluated for origin test up to 52e4858

openshift-bot · 2017-03-09T21:57:55Z

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

openshift-bot · 2017-03-10T04:53:15Z

continuous-integration/openshift-jenkins/merge ABORTED (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/26/) (Base Commit: 5602529)

deads2k assigned enj Mar 7, 2017

deads2k mentioned this pull request Mar 7, 2017

Split openshift authorizer interface to match upstream #13287

Merged

deads2k force-pushed the auth-06-collapse branch from 4b54e2b to 9139921 Compare March 8, 2017 13:53

ncdc reviewed Mar 8, 2017

View reviewed changes

liggitt mentioned this pull request Mar 8, 2017

Make openshift authorizer.GetResoruce match upstream #13259

Merged

liggitt reviewed Mar 8, 2017

View reviewed changes

enj suggested changes 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 force-pushed the auth-06-collapse branch from 9139921 to e38baf1 Compare March 9, 2017 13:23

switch meaning to openshift.GetResource to match upstream

c3bb1dd

enj mentioned this pull request Mar 9, 2017

Incorrect human readable messages from authorizer with non-empty group #13155

Closed

deads2k force-pushed the auth-06-collapse branch from e38baf1 to 35173c8 Compare March 9, 2017 15:27

enj reviewed Mar 9, 2017

View reviewed changes

switch to kube authorization attributes

52e4858

deads2k force-pushed the auth-06-collapse branch from 35173c8 to 52e4858 Compare March 9, 2017 16:56

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

enj mentioned this pull request Mar 15, 2017

Add conversions from RBAC resources to origin resources #13334

Merged

deads2k deleted the auth-06-collapse branch August 3, 2017 19:24

switch to kube authorization attributes #13286

switch to kube authorization attributes #13286

Uh oh!

Conversation

deads2k commented Mar 7, 2017

Uh oh!

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

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!

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!

ncdc commented Mar 9, 2017

Uh oh!

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

openshift-bot commented Mar 9, 2017

Uh oh!

openshift-bot commented Mar 9, 2017

Uh oh!

openshift-bot commented Mar 10, 2017

Uh oh!

Uh oh!