-
Notifications
You must be signed in to change notification settings - Fork 39k
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: email_verified claim is not required for JWT validation #61508
oidc authentication: email_verified claim is not required for JWT validation #61508
Conversation
/ok-to-test cc @kubernetes/sig-auth-pr-reviews any objections here? There was an comment a while back that this case should require an additional flag, but that seems like overkill. |
Any thoughts/ objections on this PR? |
if !emailVerified { | ||
return nil, false, fmt.Errorf("oidc: email not verified") | ||
// If the email_verified claim is not present we do not have to verify it. | ||
if err != nil && err != errClaimNotFound { |
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.
it seems like it would be clearer to check for presence of the claim before calling unmarshalClaim, rather than comparing literal errors
if _, hasEmailVerifiedClaim := c["email_verified"]; hasEmailVerifiedClaim {
...
}
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
nit on checking for presence rather than comparing errors. no objections overall |
f700614
to
1f25319
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ericchiang, rithujohn191 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 |
Automatic merge from submit-queue (batch tested with PRs 61806, 61508, 62075, 62079, 62052). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Currently the "email_verified" claim is required by the API server to verify an OIDC token. Many OIDC providers do not support the "email_verified" claim. We want to be able to allow their OIDC tokens as valid.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #59496
Release note:
/sig auth
/kind feature
/assign @ericchiang
CC: @sreetummidi