oidc client plugin: reduce round trips and fix scopes requested #45317

ericchiang · 2017-05-03T23:53:35Z

This PR attempts to simplify the OpenID Connect client plugin to
reduce round trips. The steps taken by the client are now:

If ID Token isn't expired:
- Do nothing.
If ID Token is expired:
- Query /.well-known discovery URL to find token_endpoint.
- Use an OAuth2 client and refresh token to request new ID token.

This avoids the previous pattern of always initializing a client,
which would hit the /.well-known endpoint several times.

The client no longer does token validation since the server already
does this. As a result, this code no longer imports
github.com/coreos/go-oidc, instead just using golang.org/x/oauth2
for refreshing.

Overall reduction in tests because we're not verify as many things
on the client side. For example, we're no longer validating the
id_token signature (again, because it's being done on the server
side).

This has been manually tested against dex, and I hope to continue
to test this over the 1.7 release cycle.

cc @mlbiam @frodenas @curtisallen @jsloyer @rithujohn191 @philips @kubernetes/sig-auth-pr-reviews

NONE

Updates #42654
Closes #37875
Closes #37874

k8s-reviewable · 2017-05-03T23:53:45Z

This change is

ericchiang · 2017-05-04T00:40:35Z

@k8s-bot unit test this

ericchiang · 2017-05-10T17:51:27Z

rebased with #45529

ericchiang · 2017-05-10T18:41:29Z

@k8s-bot kops aws e2e test this

caesarxuchao · 2017-05-10T20:19:48Z

Reassign to @liggitt to triage.

liggitt · 2017-05-11T15:25:09Z

if they have an unexpired but invalid token, this change would make them go through more manual steps to obtain a valid token again, right?

liggitt · 2017-05-11T15:26:27Z

for example, if the keys used to sign id_tokens got rotated, so their current id_token was not valid, but they could have used the refresh token to obtain a new valid id_token

liggitt · 2017-05-11T15:26:48Z

I don't think this is release-note-worthy

lpabon

I'm not an auth expert, but the code looks sound and the tests look great. Nice job @ericchiang .

ericchiang · 2017-05-15T15:05:38Z

if they have an unexpired but invalid token, this change would make them go through more manual steps to obtain a valid token again, right?

@liggitt yeah, this PR stopped validating the following things on the kubectl side that the API server does check.

JWT signatures
Sub claim (which holds the client id)

When we've had problems with signature validation, it's usually because of provider incompatibilities, not something that changes with a new id_token. I don't know if refreshing will help this situation.

Client ID claims are hard to validate because we don't know which client ID the API server has been configured with.

The rest of the API server validations are:

Malformed JWT (loosely checked client side)
Malformed or missing username or group claim (same as client ID, hard to check client side because claim names are dynamic)

liggitt · 2017-05-15T15:16:33Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/oidc/oidc.go

 	cfgIDToken                  = "id-token"
 	cfgRefreshToken             = "refresh-token"
+
+	// Unused. Scopes aren't sent during refreshing.
+	cfgExtraScopes = "extra-scopes"


worth a release note, and maybe worth outputting a low-verbosity info message, since they're ignored now?

Yeah, I can add a warning message.

liggitt · 2017-05-15T15:17:04Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/oidc/oidc.go

-	mu    sync.RWMutex
+	mu sync.RWMutex
+	// Each oidcAuthProvider holds a mutex to ensure a set of credentials only refresh
+	// their token once.


does this assume short-lived processes? refreshing a token multiple times could be valid

It tries to ensure that concurrent calls to the round tripper, when the credentials expire, only trigger a single refresh. E.g. if I've configure a client and do something like:

go client.ListNodes() go client.ListNodes()

Only one of those calls should trigger a refresh event.

removed this comment because it's addressed where the actual mutex is defined on oidcAuthProvider

liggitt · 2017-05-15T15:23:32Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/oidc/oidc.go

+		return false, fmt.Errorf("parsing claims: %v", err)
+	}
+
+	return time.Now().Add(expiryDelta).Before(time.Time(claims.Expiry)), nil


pass in now() for testing?

liggitt · 2017-05-15T15:24:27Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/oidc/oidc.go

+// jsonTime is a json.Unmarshaler that parses a unix timestamp.
+// Because JSON numbers don't differentiate between ints and floats,
+// we want to ensure we can parse either.
+type jsonTime time.Time


please tell me you haven't seen a floating point expiration time in the wild.

An old version of dex used to do this because of handling claims as map[string]interface{} :( We've "fixed" it for all relevant but I've always done this defensively so if other providers do it I wouldn't know. golang.org/x/oauth2 does something similar for numeric values (expires_in)[1].

[1] https://github.com/golang/oauth2/blob/ad516a297a9f2a74ecc244861b298c94bdd28b9d/internal/token.go#L62-L63

liggitt · 2017-05-15T15:26:53Z

When we've had problems with signature validation, it's usually because of provider incompatibilities, not something that changes with a new id_token. I don't know if refreshing will help this situation.

I can buy that. A few questions, LGTM overall

ericchiang · 2017-05-16T01:27:34Z

@k8s-bot kops aws e2e test this

ericchiang · 2017-05-19T16:44:43Z

bump @liggitt

liggitt · 2017-05-19T19:50:49Z

/lgtm

curtisallen · 2017-05-20T04:42:06Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

curtisallen · 2017-05-20T04:42:36Z

@k8s-bot kops aws e2e test this

ericchiang · 2017-05-22T16:58:33Z

Federation test (https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/45317/pull-kubernetes-federation-e2e-gce/5836/)

I0519 22:03:12.461] Extracting kubernetes-test.tar.gz into /workspace/kubernetes
W0519 22:03:19.063] 2017/05/19 22:03:19 util.go:131: Step './get-kube.sh' finished in 12.426052885s
W0519 22:03:19.063] 2017/05/19 22:03:19 util.go:129: Running: ./federation/cluster/federation-down.sh
W0519 22:03:19.115] Project: k8s-jkns-pr-bldr-e2e-gce-fdrtn
W0519 22:03:19.116] Zone: us-central1-f
W0519 22:03:19.318] error: context "e2e-f8n-agent-pr-93-0" does not exist
I0519 22:03:19.419] Cleaning Federation control plane objects
W0519 22:03:49.518] Unable to connect to the server: dial tcp 35.188.69.68:443: i/o timeout
W0519 22:04:19.720] Unable to connect to the server: dial tcp 35.188.69.68:443: i/o timeout
W0519 22:04:49.902] Unable to connect to the server: dial tcp 35.188.69.68:443: i/o timeout

Kops test (https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/45317/pull-kubernetes-e2e-kops-aws/26025/)

<testsuite failures="1" tests="4" time="1324.691737505">
<testcase classname="e2e.go" name="TearDown Previous" time="8.444205226"/>
<testcase classname="e2e.go" name="Up" time="1226.128770805">
<failure>waiting for nodes timed out</failure>
</testcase>
<testcase classname="e2e.go" name="DumpClusterLogs" time="17.527676872"/>
<testcase classname="e2e.go" name="Deferred TearDown" time="72.590966347"/>
</testsuite>

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-bot kops aws e2e test this

If these don't pass will rebase and re-request LGTM.

This PR attempts to simplify the OpenID Connect client plugin to reduce round trips. The steps taken by the client are now: * If ID Token isn't expired: * Do nothing. * If ID Token is expired: * Query /.well-known discovery URL to find token_endpoint. * Use an OAuth2 client and refresh token to request new ID token. This avoids the previous pattern of always initializing a client, which would hit the /.well-known endpoint several times. The client no longer does token validation since the server already does this. As a result, this code no longer imports github.com/coreos/go-oidc, instead just using golang.org/x/oauth2 for refreshing.

ericchiang · 2017-05-23T18:55:10Z

Federation test flake is #45978 Everything else is green.

liggitt · 2017-05-24T22:57:00Z

/lgtm

k8s-github-robot · 2017-05-24T22:57:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, liggitt

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

~~staging/src/k8s.io/client-go/plugin/pkg/client/auth/oidc/OWNERS~~ [ericchiang]

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

k8s-ci-robot · 2017-05-25T02:29:31Z

@ericchiang: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins kops AWS e2e	96edba25700e08e6215a4bc260eaa5ff042e0781	link	`@k8s-bot kops aws e2e test this`
pull-kubernetes-federation-e2e-gce	`6915f85`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

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.

k8s-github-robot · 2017-05-25T02:49:25Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 3, 2017

k8s-github-robot assigned piosz and gmarek May 3, 2017

gmarek assigned caesarxuchao and unassigned gmarek and piosz May 4, 2017

ericchiang changed the title ~~oidc client plugin: reduce round trips and switch to golang.org/x/oauth2~~ oidc client plugin: reduce round trips and fix scopes requested May 9, 2017

ericchiang force-pushed the oidc-client-update branch from c21fe5b to 1bc434c Compare May 10, 2017 17:50

caesarxuchao assigned liggitt and unassigned caesarxuchao May 10, 2017

caesarxuchao added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label May 10, 2017

lpabon approved these changes May 12, 2017

View reviewed changes

liggitt reviewed May 15, 2017

View reviewed changes

ericchiang force-pushed the oidc-client-update branch from 1bc434c to 96edba2 Compare May 16, 2017 00:23

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

ericchiang force-pushed the oidc-client-update branch from 96edba2 to 6915f85 Compare May 22, 2017 17:31

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2017

k8s-github-robot merged commit 9812856 into kubernetes:master May 25, 2017

ericchiang deleted the oidc-client-update branch May 25, 2017 15:30

xiangpengzhao mentioned this pull request Jul 1, 2017

k8s-merge-robot labels PR "release-note" even when the release note is "NONE" kubernetes/test-infra#3295

Closed

ericchiang mentioned this pull request Oct 12, 2017

oidc: document oidc prefix flags and add note about Okta kubernetes/website#5884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oidc client plugin: reduce round trips and fix scopes requested #45317

oidc client plugin: reduce round trips and fix scopes requested #45317

ericchiang commented May 3, 2017 •

edited

Loading

k8s-reviewable commented May 3, 2017

ericchiang commented May 4, 2017

ericchiang commented May 10, 2017

ericchiang commented May 10, 2017

caesarxuchao commented May 10, 2017

liggitt commented May 11, 2017

liggitt commented May 11, 2017

liggitt commented May 11, 2017

lpabon left a comment

ericchiang commented May 15, 2017

liggitt May 15, 2017

ericchiang May 15, 2017

liggitt May 15, 2017

ericchiang May 15, 2017

ericchiang May 16, 2017

liggitt May 15, 2017

liggitt May 15, 2017

ericchiang May 15, 2017

liggitt commented May 15, 2017

ericchiang commented May 16, 2017

ericchiang commented May 19, 2017

liggitt commented May 19, 2017

curtisallen commented May 20, 2017

curtisallen commented May 20, 2017

ericchiang commented May 22, 2017

ericchiang commented May 23, 2017

liggitt commented May 24, 2017

k8s-github-robot commented May 24, 2017

k8s-ci-robot commented May 25, 2017 •

edited

Loading

k8s-github-robot commented May 25, 2017

oidc client plugin: reduce round trips and fix scopes requested #45317

oidc client plugin: reduce round trips and fix scopes requested #45317

Conversation

ericchiang commented May 3, 2017 • edited Loading

k8s-reviewable commented May 3, 2017

ericchiang commented May 4, 2017

ericchiang commented May 10, 2017

ericchiang commented May 10, 2017

caesarxuchao commented May 10, 2017

liggitt commented May 11, 2017

liggitt commented May 11, 2017

liggitt commented May 11, 2017

lpabon left a comment

Choose a reason for hiding this comment

ericchiang commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 15, 2017

ericchiang commented May 16, 2017

ericchiang commented May 19, 2017

liggitt commented May 19, 2017

curtisallen commented May 20, 2017

curtisallen commented May 20, 2017

ericchiang commented May 22, 2017

ericchiang commented May 23, 2017

liggitt commented May 24, 2017

k8s-github-robot commented May 24, 2017

k8s-ci-robot commented May 25, 2017 • edited Loading

k8s-github-robot commented May 25, 2017

ericchiang commented May 3, 2017 •

edited

Loading

k8s-ci-robot commented May 25, 2017 •

edited

Loading