10000 Add support for multiple JWKS urls by johnlanda · Pull Request #128 · hashicorp/cap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Add support for multiple JWKS urls #128

merged 4 commits into from
Feb 6, 2024

Conversation

johnlanda
Copy link
Contributor
@johnlanda johnlanda commented Feb 1, 2024

This PR is intended as a building block to add support for multiple JWKS URLs in a single auth method.

Main changes:

  • Update the Validator to take multiple key sets.
  • Update the validation logic to loop over keysets and attempt to verify JWT with any available

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)
Copy link
Contributor Author

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

Copy link
Contributor
@austingebauer austingebauer Feb 5, 2024

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 👍

Copy link
Contributor Author

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.

@johnlanda johnlanda marked this pull request as ready for review February 2, 2024 19:09
Copy link
Contributor
@austingebauer austingebauer left a 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.

@johnlanda johnlanda merged commit c782f1d into main Feb 6, 2024
@johnlanda johnlanda deleted the johnlanda/vault-22973 branch February 6, 2024 00:27
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.

2 participants
0