-
Notifications
You must be signed in to change notification settings - Fork 40k
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
oidc auth: make the OIDC claims prefix configurable #50875
Conversation
} | ||
} | ||
|
||
if a.usernamePrefix != "" && a.usernamePrefix != "-" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I had no idea someone was working on this! Thanks @ericchiang! |
There was a problem hiding this 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
There was a problem hiding this 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.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
LGTM, thank you @ericchiang 👍 |
default: | ||
// For all other cases, use issuerURL + claim as the user name. |
There was a problem hiding this comment.
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 -
There was a problem hiding this comment.
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
1f723a9
to
1f8ee7f
Compare
/retest |
/retest Adding to the 1.8 milestone since there aren't any outstanding concerns. |
/lgtm |
[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 |
done |
/retest |
Automatic merge from submit-queue (batch tested with PRs 50579, 50875, 51797, 51807, 51803) |
Add the following flags to control the prefixing of usernames and
groups authenticated using OpenID Connect tokens.
Closes #50408
Ref #31380
cc @grillz @kubernetes/sig-auth-pr-reviews @thomastaylor312 @gtaylor