-
Notifications
You must be signed in to change notification settings - Fork 193
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
Fetch key from JWKS URI if available #133
Conversation
ceda8f5
to
63417c7
Compare
@jessieay @bufferoverflow Could you review this? |
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.
One question for you @stanhu
Also what do you think about adding docs on this / adding a note to CHANGELOG as part of this PR?
key_or_secret | ||
elsif client_options.jwks_uri | ||
fetch_key | ||
end |
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.
Question: should there be an else
option here that returns config.jwks
?
Previous logic would return config.jwks if options.discovery
but also fell back to config.jwks
if key_or_secret
were nil
.
In addition to adding fetch_key
, we are changing the fallback/default behavior here, which could have unintended / backward incompatible consequences.
What do you think?
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.
@jessieay It's a good question, but config
always attempts to call out to the OpenID discovery URL, which should not happen if discovery is disabled.
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.
ah ok that makes sense @stanhu . In other words: the former default return value was problematic and what we are fixing 💯
@Azure Is this correct ? Azure B2C, discovery does not work because the discovery URL does not match the issuer field and therefore being non-standard OpenID Connect? |
@bufferoverflow FYI, the Azure non-compliance has been discussed in other issues: |
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.
LGTM!
In non-standard OpenID Connect providers, such as Azure B2C, discovery does not work because the discovery URL does not match the issuer field. If a JWKS URI is provided when discovery is disabled, we should make an HTTP request for the keys and use the response. Closes #72
63417c7
to
cd28cc2
Compare
In non-standard OpenID Connect providers, such as Azure B2C, discovery
does not work because the discovery URL does not match the issuer field.
If a JWKS URI is provided when discovery is disabled, we should make an
HTTP request for the keys and use the response.
Closes #72
This is part of the effort to upstream changes in the GitLab fork: https://gitlab.com/gitlab-org/ruby/gems/gitlab-omniauth-openid-connect/-/issues/5.