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.
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.
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.
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.
austingebauer
left a comment
There was a problem hiding this comment.
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:
Validatorto take multiple key sets.