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

use jwks caching feature of openid_connect gem #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nov
Copy link

@nov nov commented Sep 23, 2022

OpenIDConnect::ResponseObject::IdToken.decode now accepts OpenIDConnect::Discovery::Provider::Config::Response instead of key.
https://github.com/nov/openid_connect/blob/master/spec/openid_connect/response_object/id_token_spec.rb#L254-L300

then fetch JWK specified by the ID Token kid header from jwks_uri using JSON::JWK::Set::Fetcher.
https://github.com/nov/openid_connect/blob/master/lib/openid_connect/response_object/id_token.rb#L70-L73
https://github.com/nov/openid_connect/blob/master/lib/openid_connect/discovery/provider/config/response.rb#L90-L93

and JSON::JWK::Set::Fetcher has JWKS caching feature.
https://github.com/nov/json-jwt/wiki/JWK-Set#fetching

so, once omniauth_openid_connect gem users specify like this, this gem start caching JWKS by kid.

JSON::JWK::Set::Fetcher.cache = Rails.cache

ps.
you might want to support caching option via omniauth config?

config.omniauth :openid_connect, {
  issuer: 'https://idp.example.com',
  discovery: true,
  jwks_cacher: Rails.cache,
  client_options: {..}
}

Copy link

@timpeat timpeat left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -231,7 +233,7 @@ def access_token
end

def decode_id_token(id_token)
::OpenIDConnect::ResponseObject::IdToken.decode(id_token, public_key)
::OpenIDConnect::ResponseObject::IdToken.decode(id_token, public_key_or_config)
Copy link
Contributor

Choose a reason for hiding this comment

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

@nov In #134, I'm fixing how this gem handles HS256 signatures. I'm wondering if you see any issues with the approach there.

I was actually just looking at the changes made in the openid-connect gem in https://github.com/omniauth/omniauth_openid_connect/pull/134/files#r1027569274. I think that change assumes the JWT includes the kid when HS256 is used. It's been a while since I've played with Keycloak, but if I recall correctly it may not be included. I'll have to try this again.

@paulh-bb
Copy link

I wanted to check on the status of this. I know the PR is a bit out of date. Is there a plan to incorporate something like this? Caching the public keys with a configurable cache so we don't hit our jwks_uri on every verification would be very helpful for a project I am working on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants