Wire in WebhookTokenAutenticators support by simo5 · Pull Request #18868 · openshift/origin

simo5 · 2018-03-06T21:47:21Z

This allows to use an external server for Token authentication.

simo5 · 2018-03-06T21:47:55Z

liggitt · 2018-03-06T21:49:44Z

pkg/cmd/server/apis/config/types.go

 	// OAuthConfig, if present start the /oauth endpoint in this process
 	OAuthConfig *OAuthConfig
+	// WebhookTokenAuthnConfig, if present configures remote token reviewers
+	WebhookTokenAuthenticators []WebhookTokenAuthenticators


probably belongs under MasterAuthConfig

do we have a use case for allowing more than one, or is that just future-proofing?

Maybe seem that it is more similar to OAuthConfig than RequestHeaderAuthenticationOptions as an auth type, why do we not have OAuthConfig under MasterauthConfig ?

OAuth component is distinct from the master API. OAuth config is for the OAuth server. MasterAuthConfig is for the API server.

Maybe seem that it is more similar to OAuthConfig than RequestHeaderAuthenticationOptions as an auth type

RequestHeaderAuthenticationOptions and WebhookTokenAuthenticators both mirror upstream API authentication configuration options. We should group them under MasterAuthConfig.

Ok, makes sense.

liggitt · 2018-03-06T21:50:35Z

pkg/cmd/server/apis/config/types.go

 	AllowRecursiveQueries bool
 }

+type WebhookTokenAuthenticators struct {


type should be singular if it describes a single authenticator

liggitt · 2018-03-06T22:02:26Z

pkg/cmd/server/kubernetes/master/master_config.go

 			},
 		}
 	}
+	// TODO: handle WebhookTokenAuthenticators somehow ?


probably just as an additional condition in if len(config.ServiceAccountConfig.PublicKeyFiles) > 0 || ... token webhooks ... {

yeah I wasn't sure about that I'll do as you suggest

simo5 · 2018-03-07T13:40:17Z

/retest

enj · 2018-03-07T17:15:58Z

pkg/cmd/server/apis/config/v1/types.go

+	WebhookTokenAuthnConfigFile string `json:"webhookTokenAuthnConfigFile"`
+	// WebhookTokenAuthnCacheTTL indicates how long an authentication result should be cached.
+	// It takes a valid time duration string (e.g. "5m").
+	// If empty, you get the default timeout. If zero (e.g. "0m"), caching is disabled


Probably should state what the default timeout is (I see no code that does defaulting in this PR).

There is currently no default, you are required tio state the CacheTTL

Upstream defaults this to 2 minutes. @liggitt any preference?

I've added a 10s default

enj · 2018-03-07T17:17:26Z

pkg/cmd/server/apis/config/validation/master.go

 		validationResults.AddErrors(field.Required(fldPath.Child("requestHeader.extraHeaderPrefixes"), "must be specified for a secure connection"))
 	}

+	for _, wta := range config.WebhookTokenAuthenticators {


enj · 2018-03-07T17:19:05Z

pkg/cmd/server/origin/authenticator.go

 			group.NewTokenGroupAdder(oauthTokenAuthenticator, []string{bootstrappolicy.AuthenticatedOAuthGroup}))
 	}

+	for _, wta := range config.AuthConfig.WebhookTokenAuthenticators {


integration test (which I know you are already working on)

enj · 2018-03-07T17:33:37Z

do we have a use case for allowing more than one, or is that just future-proofing?

@liggitt I specifically asked for this from @simo5. I see no reason not to have it, and I really do not want to try to hack it in later. It is assumed that all such remote token authenticators are allowed to see all tokens - if you do not trust all of them at that level, then you should not put them in your config.

enj · 2018-03-07T17:35:23Z

@openshift/api-review

mrogers950 · 2018-03-07T20:46:56Z

test/integration/webhook_token_auth_test.go

+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// Ex


External / Example / or .. ?

ignore, remnant of my copy/paste from the oidc test

mrogers950 · 2018-03-07T20:50:03Z

pkg/cmd/server/apis/config/types.go

 	EtcdConfig *EtcdConfig
 	// OAuthConfig, if present start the /oauth endpoint in this process
 	OAuthConfig *OAuthConfig
+


unnecessary change

mrogers950 · 2018-03-07T20:50:13Z

pkg/cmd/server/apis/config/v1/types.go

 	EtcdConfig *EtcdConfig `json:"etcdConfig"`
 	// OAuthConfig, if present start the /oauth endpoint in this process
 	OAuthConfig *OAuthConfig `json:"oauthConfig"`
+


same as above

yeah remamnt of a previous incarnation, will remove, thanks for catching

simo5 · 2018-03-08T18:42:47Z

I think we are ready for final reviews
@mrogers950 @enj @liggitt PTAL

deads2k · 2018-03-09T19:59:52Z

api approved.

enj · 2018-03-10T14:04:39Z

pkg/cmd/server/apis/config/validation/master_test.go

+		}
+		errors := ValidateMasterAuthConfig(config, nil)
+		if len(test.expectedErrors) != len(errors.Errors) {
+			t.Errorf("%s: expected %v errors, got %v", test.testName, test.expectedErrors, errors.Errors)


continue after this line or you can panic on the next loop

Ah, true, but then, the test already failed :-)
Will fix

enj · 2018-03-10T14:05:42Z

pkg/cmd/server/apis/config/serialization_test.go


+			for i := range obj.AuthConfig.WebhookTokenAuthenticators {
+				if len(obj.AuthConfig.WebhookTokenAuthenticators[i].ConfigFile) == 0 {
+					continue


I don't understand this continue

Does not default the TTL if there is no file to start with

(config is already wrong)

Sure, but then why do we not want to default? It will just make two errors show up and make the user think that both are required.

ok removing both

enj · 2018-03-10T14:06:06Z

pkg/cmd/server/apis/config/v1/conversions.go

+	// Set defaults Cache TTLs for external Webhook Token Reviewers
+	for i := range obj.AuthConfig.WebhookTokenAuthenticators {
+		if len(obj.AuthConfig.WebhookTokenAuthenticators[i].ConfigFile) == 0 {
+			continue


Same comment

enj · 2018-03-10T14:09:20Z

test/integration/webhook_token_auth_test.go

+	authToken := "Anything-goes!"
+	authTestUser := "testuser"
+	authTestUID := "42"
+	authTestGroups := []string{"testgroup"}


TODO masters group

enj · 2018-03-10T14:12:31Z

test/integration/webhook_token_auth_test.go

+	}
+
+	// Get master config
+	masterOptions, err := testserver.DefaultMasterOptions()


Just cosmetic for increasing readability since you do not need that variable until later.

enj · 2018-03-10T14:16:41Z

test/integration/webhook_token_auth_test.go

+	// Try to authenticate with a token that can be validated only by our
+	// external token reviewer
+	userConfig := &restclient.Config{
+		Host: clientConfig.Host,


Anonymous helper

userConfig := AnonymousClientConfig(...) userConfig.BearerToken = ...

enj · 2018-03-10T14:18:29Z

test/integration/webhook_token_auth_test.go

+	userWhoamiOptions := cmd.WhoAmIOptions{UserInterface: userClient.Users(), Out: ioutil.Discard}
+	retrievedUser, err := userWhoamiOptions.WhoAmI()
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)


Not really needed, other tests do the same

Just because other people write their tests incorrectly is not a valid reason to continue doing the wrong thing.

enj · 2018-03-10T14:54:08Z

test/integration/webhook_token_auth_test.go

+	masterOptions.AuthConfig.WebhookTokenAuthenticators = []configapi.WebhookTokenAuthenticator{
+		{
+			ConfigFile: authConfigFile.Name(),
+			CacheTTL:   "10s",


Remove to check defaulting

defaulting is already checked in uint tests, no need to check it here

Hm I guess serialization tests may be enough.

DEfaulting happens before we override this part of the config anyway, so it wouldn't work :-)

enj · 2018-03-12T14:06:45Z

test/integration/webhook_token_auth_test.go

+		t.Fatalf("unexpected error: %v", err)
+	}
+	err = oauthClient.Oauth().OAuthAccessTokens().Delete("XYZ", &metav1.DeleteOptions{})
+	if !kerrors.IsForbidden(err) {


Needs to check for scope error.

Because we should know we got forbidden via scope and not "the token stopped working for random reason"

Signed-off-by: Monis Khan <mkhan@redhat.com>

All requested changes have been addressed

simo5 · 2018-03-13T17:21:48Z

/retest

enj · 2018-03-13T17:42:33Z

pkg/cmd/server/origin/authenticator.go

+		}
+		webhookTokenAuthenticator, err := webhooktoken.New(wta.ConfigFile, ttl)
+		if err != nil {
+			return nil, nil, fmt.Errorf("Failed to create webhook token authenticator for ConfigFile='%s'", wta.ConfigFile)


Include the error that caused the failure:

return nil, nil, fmt.Errorf("Failed to create webhook token authenticator for ConfigFile=%q: %v", wta.ConfigFile, err)

Also replace '%s' with %q on both messages.

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

enj · 2018-03-13T17:52:08Z

/lgtm
/retest
/refresh

openshift-ci-robot · 2018-03-13T17:52:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, 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:

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

simo5 · 2018-03-13T19:20:51Z

/refresh

simo5 · 2018-03-13T22:00:01Z

/retest

simo5 · 2018-03-14T12:06:31Z

/retest

simo5 · 2018-03-14T15:05:17Z

/retest

simo5 · 2018-03-14T15:27:16Z

/retest

simo5 · 2018-03-14T15:28:36Z

/test extended_conformance_install

simo5 · 2018-03-14T15:56:45Z

/retest

simo5 · 2018-03-14T17:10:16Z

/test extended_networking_minimal

simo5 · 2018-03-14T19:15:43Z

/test extended_conformance_install

simo5 · 2018-03-14T19:41:09Z

Flake: #18982

simo5 · 2018-03-15T14:09:52Z

/retest

simo5 · 2018-03-16T13:54:04Z

/retest

openshift-merge-robot · 2018-03-16T18:34:19Z

Automatic merge from submit-queue.

openshift-ci-robot requested review from csrwng and liggitt March 6, 2018 21:47

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2018

simo5 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2018

openshift-merge-robot added the needs-api-review label Mar 6, 2018

liggitt reviewed Mar 6, 2018

View reviewed changes

simo5 force-pushed the WebhookEnable branch from 7f54d05 to 5986897 Compare March 6, 2018 22:47

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2018

simo5 force-pushed the WebhookEnable branch from 5986897 to 48b61be Compare March 7, 2018 00:17

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 7, 2018

simo5 force-pushed the WebhookEnable branch from 48b61be to 87d354f Compare March 7, 2018 01:07

enj previously requested changes Mar 7, 2018

View reviewed changes

enj removed the request for review from csrwng March 7, 2018 17:31

simo5 force-pushed the WebhookEnable branch from 87d354f to e25c455 Compare March 7, 2018 18:46

mrogers950 reviewed Mar 7, 2018

View reviewed changes

simo5 force-pushed the WebhookEnable branch 3 times, most recently from 5330e97 to b39e082 Compare March 8, 2018 18:41

simo5 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2018

simo5 force-pushed the WebhookEnable branch 2 times, most recently from 13e3e62 to f9af946 Compare March 9, 2018 21:41

enj previously requested changes Mar 13, 2018

View reviewed changes

Include context UID for virtual user ~

9d97317

Signed-off-by: Monis Khan <mkhan@redhat.com>

simo5 force-pushed the WebhookEnable branch from f9af946 to d705a88 Compare March 13, 2018 15:36

enj reviewed Mar 13, 2018

View reviewed changes

Wire in WebhookTokenAutenticator support

963b4b5

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

simo5 force-pushed the WebhookEnable branch from d705a88 to 963b4b5 Compare March 13, 2018 17:47

openshift-ci-robot assigned enj Mar 13, 2018

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

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

openshift-merge-robot merged commit d78a15c into openshift:master Mar 16, 2018

Conversation

simo5 commented Mar 6, 2018

Uh oh!

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

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 Mar 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!

Choose a reason for hiding this comment

Uh oh!

enj commented Mar 7, 2018

Uh oh!

enj commented Mar 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!

simo5 commented Mar 8, 2018

Uh oh!

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

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