-
Notifications
You must be signed in to change notification settings - Fork 19
Add support for multiple JWKS urls
#128
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
Conversation
jwt/jwt.go
Outdated
|
||
func (m *multiValidator) Validate(ctx context.Context, token string, expected Expected) (map[string]interface{}, error) { | ||
for _, v := range m.validators { | ||
claims, err := v.Validate(ctx, token, expected) |
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 think that we can optimize this if for each keySet
we expose some way to get a []string
which contains all kid
for the keys in the set.
Then rather than attempting each validator, we can define candidates as the validators which contain the kid
for the incoming JWT in their keySet
. I think we would still probably want to check all sets rather than short circuit on the first hit in case of a collision.
Let me know what you think @austingebauer
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.
That seems like a nice optimization. Can you elaborate on the collision case you were thinking of? I'd be open to trying something like this in a future PR 👍
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.
For the collision case: since the JWKS at each jwks_url
are independent it would be possible that there are two keys in different JWKS that have the same key id kid
.
If we were to do something like the following
// An incoming JWT with a `kid` claim for the key id that corresponds to it
var neededKID := ourJWT.kidClaim // making up the syntax here
// jwks_url -> set of key ids in the set
var knownKeys map[string][]string
// assume some other routine keeps the known keys up to date - can probably tie into caching
for jwks_url, keys := range knownKeys {
if slices.Contains(keys, neededKID) {
// Try the actual verification using this keyset since we think it could have the right key
...
}
}
If we were to break from the loop over the knownKeys
early there would be a small chance that there is another keyset which contains a key with the same identifier and we could falsely claim that we failed to verify the JWT.
I think that this could save us on CPU over the current approach since we don't need to attempt to do the verification with each keyset. Only the ones that are known to have a key with a matching key id.
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.
Thanks, @johnlanda! LGTM after the changes.
This PR is intended as a building block to add support for multiple JWKS URLs in a single auth method.
Main changes:
Validator
to take multiple key sets.