Ignore errors caused by unkonwn scopes where it makes sense to do so. by simo5 · Pull Request #20911 · openshift/origin

simo5 · 2018-09-07T21:07:44Z

This allows us to interop with an OIDC/OAuth server that sends us tokens with additional scopes used for other applications than Openshift

enj · 2018-09-07T21:12:12Z

pkg/authorization/authorizer/scope/converter.go

 func (userEvaluator) Handles(scope string) bool {
-	return strings.HasPrefix(scope, UserIndicator)
+	switch scope {
+	case UserFull, UserInfo, UserAccessCheck, UserListScopedProjects, UserListAllProjects:


Probably just easier to check Validate(scope) == nil since you are basically binding their logic.

@deads2k you probably have an opinion here.

enj · 2018-09-07T21:17:31Z

pkg/authorization/authorizer/scope/converter.go

 			}
 		}
-
-		if !found {


Hmm let us add something like ignoreUnhandledScopes as a bool param and let the caller tell us this is ok to ignore.

all callers will tell us that though ... can do it, but sounds a bit redundant

Seems bad to just drop it. I keep wondering if it would just be better to let the callers ignore all errors. Sigh, I cannot tell what would be best here.

Ok rebased to what it looks like with the boolean ...

enj · 2018-09-07T21:43:38Z

@openshift/sig-security

enj · 2018-09-10T15:54:29Z

pkg/authorization/authorizer/scope/converter.go

 type userEvaluator struct{}

 func (userEvaluator) Handles(scope string) bool {
-	return strings.HasPrefix(scope, UserIndicator)


I was expecting something like return e.Validate(scope) == nil since that forces the logic into one place.

it's the same just makes more semantic sense inside Validate() to check for Handles() rather than in Handles() to call Validate()

simo5 · 2018-09-10T17:00:30Z

/retest

enj · 2018-09-10T17:06:51Z

Will give David/others a chance to look.

Changes implemented

mrogers950 · 2018-09-10T19:42:26Z

/lgtm

deads2k · 2018-09-11T13:21:22Z

If I were trying to consume a different kind of scope from a different provider, I would expect them to sanitize the scopes to indicate which ones they expect our authorizer to enforce and provide a list in a different user.Extra key for all their scopes. That would clearly delineate responsibility of enforcement instead of ignoring.

I accept that this may work, but it does seem to increase the risk of bugs. If you decide to go with this anyway, you need tests ensuring that properly fail closed when scopes are presented and none are recognized.

deads2k · 2018-09-11T13:21:50Z

pkg/authorization/authorizer/scope/converter.go

 // ScopesToVisibleNamespaces returns a list of namespaces that the provided scopes have "get" access to.
 // This exists only to support efficiently list/watch of projects (ACLed namespaces)
-func ScopesToVisibleNamespaces(scopes []string, clusterRoleGetter rbaclisters.ClusterRoleLister) (sets.String, error) {
+func ScopesToVisibleNamespaces(scopes []string, clusterRoleGetter rbaclisters.ClusterRoleLister, ignoreUnhandledScopes bool) (sets.String, error) {


It appears that every caller is setting this to true. Why have the option?

mo requested it ...

I did not want to blanket remove the "unhandled scope" errors...

simo5 · 2018-09-11T13:55:32Z

If I were trying to consume a different kind of scope from a different provider, I would expect them to sanitize the scopes to indicate which ones they expect *our* authorizer to enforce and provide a list in a different user.Extra key for all their scopes.

This is not how an external auth server works, it is safer and more secure for us to properly handle extra scopes than depend on them filtering stuff.

That would clearly delineate responsibility of enforcement instead of ignoring.

We are responsible for enforcing, and we enforce only scopes we understand, the rest we do not care for. There is no risk in ignoring scopes we do not understand as individual scopes allow things they do not restrict, so we fail safe anyway. It *also* allows us to upgrade servers where we need to add a new scope without causing old servers to completely fail all requests, and I think this alone is a big plus.

I accept that this may work, but it does seem to increase the risk of bugs. If you decide to go with this anyway, you need tests ensuring that properly fail closed when scopes are presented and none are recognized.

We have unit tests that show that unknown scopes return an error, and the 2 calls that we depend on are just fine ignoring unknown scopes as there is no way they are going to grant any more access than they do now. Unfortunately creating also an integration test with our own oauth server and unknown scopes is not possible, but we'll definitely test as soon as we have integration tests with Keycloak

simo5 · 2018-09-11T14:36:27Z

Truns out we have a way to test unknown scopes by using the webhook authentication interface, so I'm going to add to the webhook integration test as well

simo5 · 2018-09-11T14:36:33Z

/hold

enj

Some important role: bits, otherwise minor nits.

enj · 2018-09-12T22:06:38Z

test/integration/scopes_test.go

+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	_, haroldClientConfig, err := testutil.GetClientForUser(clusterAdminClientConfig, "harold")


CreateNewProject already gives you a haroldClientConfig

enj · 2018-09-12T22:10:22Z

test/integration/scopes_test.go

+			Scopes: []string{"user:info", "user:garbage", "garbage"},
+		},
+	}
+	rulesReviewWithGarbageObj, err := authzv1client.SelfSubjectRulesReviews(projectName).Create(rulesReviewWithGarbage)


unchecked error?

enj · 2018-09-12T22:12:29Z

test/integration/scopes_test.go

+	}
+	rulesReviewOnlyGarbageObj, err := authzv1client.SelfSubjectRulesReviews(projectName).Create(rulesReviewOnlyGarbage)
+
+	//Check same rules, first we need to convert SSRR result from:


Can you move the conversion to its own function, it is pretty awful.

enj · 2018-09-12T22:18:26Z

test/integration/scopes_test.go

+	// Test with valid scope as baseline
+	rulesReview := &authorizationv1.SelfSubjectRulesReview{
+		Spec: authorizationv1.SelfSubjectRulesReviewSpec{
+			Scopes: []string{"user:info"},


Add a valid role: based scope for admin in the current namespace.

enj · 2018-09-12T22:18:54Z

test/integration/scopes_test.go

+	// Try adding garbage scope and check we have the same perms as baseline
+	rulesReviewWithGarbage := &authorizationv1.SelfSubjectRulesReview{
+		Spec: authorizationv1.SelfSubjectRulesReviewSpec{
+			Scopes: []string{"user:info", "user:garbage", "garbage"},


Also the same valid role: from above and also add garbage role:

enj · 2018-09-12T22:22:17Z

test/integration/scopes_test.go

+		t.Fatalf("unexpected error getting user harold client config: %v", err)
+	}
+
+	authzv1client, err := authorizationv1typedclient.NewForConfig(haroldClientConfig)


Is there a NewForConfigOrDie to remove some more boilerplate?

enj · 2018-09-12T22:26:26Z

test/integration/scopes_test.go

+			Scopes: []string{"user:garbage", "garbage"},
+		},
+	}
+	rulesReviewOnlyGarbageObj, err := authzv1client.SelfSubjectRulesReviews(projectName).Create(rulesReviewOnlyGarbage)


unchecked error?

enj · 2018-09-12T22:26:51Z

test/integration/scopes_test.go

+	// Make sure no rules (beyond baseline) when only garbage scopes are present
+	rulesReviewOnlyGarbage := &authorizationv1.SelfSubjectRulesReview{
+		Spec: authorizationv1.SelfSubjectRulesReviewSpec{
+			Scopes: []string{"user:garbage", "garbage"},


Add garbage role:

enj · 2018-09-12T22:34:30Z

test/integration/scopes_test.go

+	// semantically identical
+	baselineRules := []rbacv1.PolicyRule{authorizationapi.DiscoveryRule}
+
+	covers, unexpectedRules := rbacvalidation.Covers(baselineRules, rbacv1Rules)


An assertEqual helper function would be nice.

simo5 · 2018-09-13T20:39:52Z

Ok all the requests should have been handled now, and I have tests that exercise both ScopesToRules and ScopesToVisibleNamespaces

enj

minor things

enj · 2018-09-13T22:41:17Z

test/integration/scopes_test.go

+			if err != nil {
+				return false, err
+			}
+			if len(projects.Items) > 0 {


return len(projects.Items) > 0, nil

enj · 2018-09-13T22:42:49Z

test/integration/scopes_test.go

+	projectClient := projectclient.NewForConfigOrDie(&impersonatingConfig)
+
+	var projects *projectapiv1.ProjectList
+	err = wait.Poll(100*time.Millisecond, 10*time.Second,


Probably want 30 seconds, CI can be slow and it is not worth the flake.

enj · 2018-09-13T22:45:03Z

test/integration/scopes_test.go

+		&badScopesUserInfo, *clusterAdminClientConfig)
+	badScopesProjectClient := projectclient.NewForConfigOrDie(&badScopesImpersonatingConfig)
+	projects, err = badScopesProjectClient.ProjectV1().Projects().List(metav1.ListOptions{})
+	if !kapierrors.IsForbidden(err) {


Add a check to fail on nil error and then change this to if !kapierrors.IsForbidden(err) || !strings.Contains(err.Error(), "prevent this action, additionally the following non-fatal errors were reported"

Forbidden is the default fall through case so let us make sure the scope message is there.

Added integration tests Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2018-09-14T18:33:04Z

Ok addressed the last nits too.

enj · 2018-09-14T18:35:39Z

/lgtm
/hold cancel

openshift-ci-robot · 2018-09-14T18:36:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, mrogers950, simo5

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/authorization/OWNERS~~ [enj,simo5]
~~pkg/project/OWNERS~~ [enj]
~~test/integration/OWNERS~~ [enj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2018-09-14T19:09:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from enj and ericavonb September 7, 2018 21:07

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 7, 2018

simo5 force-pushed the allowUnknownScopes branch from ce9b412 to cc8d9e7 Compare September 7, 2018 21:29

enj previously requested changes Sep 7, 2018

View reviewed changes

openshift-ci-robot added the sig/security label Sep 7, 2018

simo5 force-pushed the allowUnknownScopes branch 2 times, most recently from ca267a1 to ac4ef34 Compare September 10, 2018 13:23

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2018

simo5 force-pushed the allowUnknownScopes branch from ac4ef34 to dd48375 Compare September 10, 2018 13:29

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 10, 2018

simo5 force-pushed the allowUnknownScopes branch from dd48375 to 3dda4db Compare September 10, 2018 15:25

enj reviewed Sep 10, 2018

View reviewed changes

openshift-ci-robot assigned mrogers950 Sep 10, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2018

deads2k reviewed Sep 11, 2018

View reviewed changes

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2018

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 11, 2018

simo5 force-pushed the allowUnknownScopes branch 3 times, most recently from 0f000b8 to 3ea2d37 Compare September 12, 2018 21:55

enj reviewed Sep 12, 2018

View reviewed changes

simo5 force-pushed the allowUnknownScopes branch 2 times, most recently from 599453b to 6f664ef Compare September 13, 2018 20:38

simo5 force-pushed the allowUnknownScopes branch 4 times, most recently from 22fe81e to e19ea8d Compare September 14, 2018 01:03

enj reviewed Sep 14, 2018

View reviewed changes

Ignore errors caused by unkonwn scopes where it makes sense to do so.

0f2438c

Added integration tests Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the allowUnknownScopes branch from e19ea8d to 0f2438c Compare September 14, 2018 18:32

openshift-ci-robot assigned enj Sep 14, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 14, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2018

openshift-merge-robot merged commit feec27a into openshift:master Sep 14, 2018

Conversation

simo5 commented Sep 7, 2018

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 commented Sep 7, 2018

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!

simo5 commented Sep 10, 2018

Uh oh!

enj commented Sep 10, 2018

Uh oh!

mrogers950 commented Sep 10, 2018

Uh oh!

deads2k commented Sep 11, 2018

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!

simo5 commented Sep 11, 2018 via email

Uh oh!

simo5 commented Sep 11, 2018

Uh oh!

simo5 commented Sep 11, 2018

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!

Choose a reason for hiding this comment

Uh oh!

simo5 commented Sep 13, 2018

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!

simo5 commented Sep 14, 2018

Uh oh!

enj commented Sep 14, 2018

Uh oh!

openshift-ci-robot commented Sep 14, 2018