feat: improved JWT Authorizer JWKs fetching #726
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eroznik
changed the title
aeneasr
reviewed
May 27, 2021
credentials/fetcher_default.go
Outdated
| @@ -124,19 +128,21 @@ func (s *FetcherDefault) fetchParallel(ctx context.Context, locations []url.URL) | |||
| select { | |||
| case <-ctx.Done(): | |||
| s.l.Errorf("Ignoring JSON Web Keys from at least one URI because the request timed out waiting for a response.") | |||
| return fetchResult{true} | |||
There was a problem hiding this comment.
Maybe return an error here instead? And then we check for errors in the caller code? That would be a bit more idiomatic go code :)
There was a problem hiding this comment.
Thanks for the honest feedback, I really appreciate comments like this - I am not that proficient with Go yet :)
Changed to return error, is now ok - or should I even improve the implementation with a custom error?
No worries, I understood already from my first Oathkeeper PR that you're quite a busy man :)) |
aeneasr
reviewed
May 28, 2021
…ed error propagation)
aeneasr
approved these changes
May 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue
#203
@aeneasr
Proposed changes
The current implementation is not suitable for deployments in which low response times are required. With this PR I am improving the current implementation with the following changes:
FetcherDefaultfetches new JWKS and the request takes more then allowed, it will use "stale" JWKs for JWT validation. The refresh request will still continue and if successful, it will refresh the cached JWK for the "next" request.FetcherDefaultalready accepts configuration for JWK cache TTL and max await for the refresh(and init!) JWK requests, the problem was that the end user could not change these values. This PR enables the user to change these values with 2 new configs:authenticators.jwt.config.jwks_max_waitandauthenticators.jwt.config.jwks_ttl.Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
I am aware that adding 2 more configuration keys makes stuff a little bit more complex, but I am certain this will help out quite a bit of users down the road. Schemas + docs + reference config updated.