Skip to content
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 auth: make the OIDC claims prefix configurable #50875

Merged
merged 1 commit into from
Sep 3, 2017

Conversation

ericchiang
Copy link
Contributor

@ericchiang ericchiang commented Aug 18, 2017

Add the following flags to control the prefixing of usernames and
groups authenticated using OpenID Connect tokens.

--oidc-username-prefix
--oidc-groups-prefix
The OpenID Connect authenticator can now use a custom prefix, or omit the default prefix, for username and groups claims through the --oidc-username-prefix and --oidc-groups-prefix flags. For example, the authenticator can map a user with the username "jane" to "google:jane" by supplying the "google:" username prefix.

Closes #50408
Ref #31380

cc @grillz @kubernetes/sig-auth-pr-reviews @thomastaylor312 @gtaylor

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 18, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 18, 2017
}
}

if a.usernamePrefix != "" && a.usernamePrefix != "-" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we should define - as a const. or i have to go to the flag help string to figure out why - is special.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do


u, ok, err := o.parseTokenClaims(test.claims)
if err != nil {
if !test.wantErr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test.wantErr is also a check, not a pre condition. probably should not fatal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return in the next statement anyway, but yeah I can change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@thomastaylor312
Copy link
Contributor

I had no idea someone was working on this! Thanks @ericchiang!

Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this mostly from a user perspective and it looks good. From what I can see, the code is good too

Copy link
Contributor

@gtaylor gtaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had one question about the difference in behavior of username prefixing vs group claim prefixing. Looks great!


fs.StringVar(&s.OIDC.GroupsPrefix, "oidc-groups-prefix", "", ""+
"If provided, all groups will be prefixed with this value to prevent conflicts with "+
"other authentication strategies.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we just less concerned about collisions for the group claim vs the username claim?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, the prefixing wasn't done when group support was added, and this is preserving compatibility with existing behavior

@pbarker
Copy link
Contributor

pbarker commented Aug 18, 2017

LGTM, thank you @ericchiang 👍

default:
// For all other cases, use issuerURL + claim as the user name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually hoist the specified/unspecified/-/email/not-email logic up to newAuthenticatorFromOIDCIssuerURL so from this authenticator's perspective, it just prepends the specified prefixes. There's no reason for this authenticator to be aware of special flag values like -

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Will update.

Add the following flags to control the prefixing of usernames and
groups authenticated using OpenID Connect tokens.

	--oidc-username-prefix
	--oidc-groups-prefix
@ericchiang
Copy link
Contributor Author

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/50875/pull-kubernetes-e2e-gce-etcd3/48166/#sig-network-services-should-serve-a-basic-endpoint-from-pods-conformance

[sig-network] Services should serve a basic endpoint from pods [Conformance] 1m25s
go run hack/e2e.go -v -test --test_args='--ginkgo.focus=\[sig\-network\]\sServices\sshould\sserve\sa\sbasic\sendpoint\sfrom\spods\s\[Conformance\]$'
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/network/service.go:130
Aug 18 17:26:23.297: Timed out waiting for service endpoint-test2 in namespace e2e-tests-services-mmffq to expose endpoints map[pod1:[80] pod2:[80]] (1m0s elapsed)
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/framework/service_util.go:1175

/retest

@ericchiang
Copy link
Contributor Author

/retest

Adding to the 1.8 milestone since there aren't any outstanding concerns.

@ericchiang ericchiang added this to the v1.8 milestone Aug 30, 2017
@liggitt
Copy link
Member

liggitt commented Sep 1, 2017

/lgtm
please update the release note with the flag names to use

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 50408

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

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017
@ericchiang
Copy link
Contributor Author

please update the release note with the flag names to use

done

@liggitt
Copy link
Member

liggitt commented Sep 1, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 50579, 50875, 51797, 51807, 51803)

@k8s-github-robot k8s-github-robot merged commit d970eb8 into kubernetes:master Sep 3, 2017
@ericchiang ericchiang deleted the oidc-claims-prefix branch October 12, 2017 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC server auth: add option to specify the claim prefix
10 participants