New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oidc authentication: switch to v2 of coreos/go-oidc #58544

Merged
merged 3 commits into from Feb 21, 2018

Conversation

@ericchiang
Member

ericchiang commented Jan 19, 2018

Switch to v2 of coreos/go-oidc, which uses square/go-jose to verify tokens and supports more signing algorithms.

Most of this PR removes dependencies used by the older version of github.com/coreos/go-oidc, and updates vendor files.

This PR has been tested against tokens issued by Okta, Google, and CoreOS's dex.

Closes #57806

kube-apiserver: the OpenID Connect authenticator can now verify ID Tokens signed with JOSE algorithms other than RS256 through the --oidc-signing-algs flag.
kube-apiserver: the OpenID Connect authenticator no longer accepts tokens from the Google v3 token APIs, users must switch to the "https://www.googleapis.com/oauth2/v4/token" endpoint.

cc @rithujohn191 @liggitt
cc @kubernetes/sig-auth-pr-reviews

@thockin

Godep parts look OK. ping me when LGTM'ed and I can approve.

@rithujohn191

the lint check and staging-godeps check seems to have failed

var groups []string
if err := c.unmarshalClaim(a.groupsClaim, &groups); err != nil {
var group string
if err := c.unmarshalClaim(a.groupsClaim, &group); err != nil {

This comment has been minimized.

@rithujohn191

rithujohn191 Jan 20, 2018

Contributor

A comment about why you are trying to decode it as string again as opposed to an array of strings would be helpful

This comment has been minimized.

@ericchiang

ericchiang Jan 20, 2018

Member

Added a link to #33290

This comment has been minimized.

@mikedanese

mikedanese Jan 26, 2018

Member

alternatively create a:

type stringOrArray []string

with a UnmarshalJSON func that does the right thing.

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 20, 2018

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/58544/pull-kubernetes-unit/76002/

Failure on

k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/test/integration TestPatch 0.82s

/test pull-kubernetes-unit

@@ -0,0 +1,27 @@
#!/bin/bash -e

This comment has been minimized.

@liggitt

liggitt Jan 20, 2018

Member

can we move this under testdata? makes it clearer it is only for test

}
// whilelist of signing algorithms to ensure users don't mistakenly pass something
// goofy.

This comment has been minimized.

@liggitt

liggitt Jan 20, 2018

Member

don't want to enable none? :)

This comment has been minimized.

@ericchiang

ericchiang Jan 22, 2018

Member

I swear I remember someone accidentally using tokens with HS256.

if client := a.oidcClient.Load(); client != nil {
return client.(*oidc.Client), nil
}
client := &http.Client{Transport: tr, Timeout: 15 * time.Second}

This comment has been minimized.

@liggitt

liggitt Jan 20, 2018

Member

this is the client used in the background to fetch discovery docs and public keys? what was the timeout before?

edit: found it. there was no timeout previously. 15 seconds sounds a little aggressive, especially for a background process.

This comment has been minimized.

@ericchiang

ericchiang Jan 22, 2018

Member

Bumped to 30 seconds

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 22, 2018

/test pull-kubernetes-e2e-kops-aws

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 22, 2018

/test pull-kubernetes-e2e-kops-aws

edit: flaking on #58578

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 23, 2018

/test pull-kubernetes-e2e-kops-aws

2 similar comments
@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 24, 2018

/test pull-kubernetes-e2e-kops-aws

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 24, 2018

/test pull-kubernetes-e2e-kops-aws

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 25, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Jan 26, 2018

+	// Check issuer.
+	if t.Issuer != v.issuer {
+		// Google sometimes returns "accounts.google.com" as the issuer claim instead of
+		// the required "https://accounts.google.com". Detect this case and allow it only
+		// for Google.
+		//
+		// We will not add hooks to let other providers go off spec like this.
+		if !(v.issuer == issuerGoogleAccounts && t.Issuer == issuerGoogleAccountsNoScheme) {
+			return nil, fmt.Errorf("oidc: id token issued by a different provider, expected %q got %q", v.issuer, t.Issuer)
+		}
+	}

really?

if !ok {
return fmt.Errorf("claim not present")
}
return json.Unmarshal([]byte(val), v)

This comment has been minimized.

@liggitt

liggitt Jan 26, 2018

Member

does this echo the value in errors? we weren't returning claim values in error messages before, just type info

This comment has been minimized.

@ericchiang

ericchiang Jan 26, 2018

Member

It's doesn't appear to return the value: https://play.golang.org/p/Ed2vi-1gDEK

I can add a test to make sure that behavior doesn't change in future releases of Go.

This comment has been minimized.

@ericchiang

ericchiang Jan 26, 2018

Member

Test added.

return nil, false, err
}
claims, err := jwt.Claims()
idToken, err := verifier.Verify(ctx, token)

This comment has been minimized.

@liggitt

liggitt Jan 26, 2018

Member

can be a follow-up, but might be good to consider doing something similar to #58791 to avoid noise in logs when this is combined with other token auth methods

@liggitt

This comment has been minimized.

Member

liggitt commented Jan 26, 2018

a couple questions, LGTM overall. would like a second review on the authorize path. @tallclair can you review or pick a delegate?

@liggitt liggitt assigned liggitt and mikedanese and unassigned dchen1107 Jan 26, 2018

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 26, 2018

@ericchiang

This comment has been minimized.

Member

ericchiang commented Jan 26, 2018

Authenticator now implements the same strategy as #58791, throwing out tokens that aren't issued by the OIDC provider.

}
return client, nil
if claims.Issuer != iss {

This comment has been minimized.

@ericchiang

ericchiang Jan 26, 2018

Member

Right after I committed, I realized that this of course won't work with Google ID tokens (iss can be "accounts.google.com" instead of "https://accounts.google.com"). I'm inclined to add the same check we have in the oidc library.

Edit: added

Edit #2: Google returns the right issuer with newer endpoints.

@mikedanese

It would be pretty costly to fix at this point unfortunately.

}
type OIDCAuthenticator struct {
type Authenticator struct {

This comment has been minimized.

@mikedanese

mikedanese Jan 26, 2018

Member

If you rename this, might as well rename OIDCOptions to Options.

// idTokenVerifier method.
verifier atomic.Value
ctx context.Context

This comment has been minimized.

@mikedanese

mikedanese Jan 26, 2018

Member

This looks weird. From the godoc:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx

var groups []string
if err := c.unmarshalClaim(a.groupsClaim, &groups); err != nil {
var group string
if err := c.unmarshalClaim(a.groupsClaim, &group); err != nil {

This comment has been minimized.

@mikedanese

mikedanese Jan 26, 2018

Member

alternatively create a:

type stringOrArray []string

with a UnmarshalJSON func that does the right thing.

// in its ID tokens. Allow this case.
//
// https://developers.google.com/identity/protocols/OpenIDConnect#obtainuserinfo
if iss == "https://accounts.google.com" && claims.Issuer == "accounts.google.com" {

This comment has been minimized.

@liggitt

liggitt Jan 27, 2018

Member

I don't think we should embed vendor-specific non-spec behavior like this. It's hard to justify writing to the spec (e.g. our position on use of email_verified) when combined with exceptions. @mikedanese, do you know under what circumstances the issuer does not match the published value in https://accounts.google.com/.well-known/openid-configuration?

This comment has been minimized.

@ericchiang

ericchiang Jan 31, 2018

Member

go-oidc has always had this behavior to support Google. It'd be a breaking change to merge this change without this, though I agree with the sentiment.

This comment has been minimized.

@mikedanese

mikedanese Feb 1, 2018

Member

@mdietz is source of truth on this

This comment has been minimized.

@mdietz

mdietz Feb 1, 2018

The issuer returned in a Google ID Token depends on the token endpoint used. The accounts.google.com and googleapis.com/oauth2/v3 token endpoints have a pre-finalized OIDC spec implementation that elides the https:// in the issuer field for issued ID tokens. The googleapis.com/oauth2/v4 token endpoint is spec compliant. See http://openid.net/specs/openid-connect-core-1_0.html#GoogleIss.

This comment has been minimized.

@liggitt

liggitt Feb 1, 2018

Member

Sounds like if we use the URLs in the discovery doc, which we do, we should be able to rely on correct issuers now.

This comment has been minimized.

@liggitt

liggitt Feb 7, 2018

Member

@ericchiang ping on removing the google issuer spec exception, since the published token endpoint is now spec compliant

This comment has been minimized.

@ericchiang

ericchiang Feb 7, 2018

Member

Sorry wanted to verify this manually. Tested and can confirm @mdietz's comments.

@liggitt

This comment has been minimized.

Member

liggitt commented Jan 27, 2018

/unassign
/assign @deads2k

@k8s-ci-robot k8s-ci-robot assigned deads2k and unassigned liggitt Jan 27, 2018

@thockin

This comment has been minimized.

Member

thockin commented Feb 6, 2018

I have no context on this - please assign to me if you need some approval.

@thockin thockin removed their assignment Feb 6, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 10, 2018

/lgtm

would like a second from another @kubernetes/sig-auth-pr-reviews reviewer, then can get top-level approval for godep change

@mikedanese

/lgtm

})
}
// whilelist of signing algorithms to ensure users don't mistakenly pass something

This comment has been minimized.

@mikedanese

mikedanese Feb 16, 2018

Member

typo: whitelist

ericchiang added some commits Jan 19, 2018

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 16, 2018

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 16, 2018

@ericchiang

This comment has been minimized.

Member

ericchiang commented Feb 16, 2018

/assign @thockin

For final approval

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Feb 21, 2018

/approve

for godep

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 21, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 21, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 21, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit cdbc4fb into kubernetes:master Feb 21, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation ericchiang authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ericchiang ericchiang deleted the ericchiang:oidc-v2 branch Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment