[auth] Don't panic on transient failures when refreshing JWKS keys - #1634
Conversation
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
This is not exactly what is described in #1632 (comment), but I think that is OK as a first step. To be seen if that warrants a follow-up issue though, notably fro the dynamic adaptation depending on the incoming tokens expiration.
| jwksEndpoint = flag.String("jwks_endpoint", "", "URL pointing to an endpoint serving JWKS") | ||
| jwksKeyIDs = flag.String("jwks_key_ids", "", "IDs of a set of key in a JWKS, separated by commas") | ||
| keyRefreshTimeout = flag.Duration("key_refresh_timeout", 1*time.Minute, "Timeout for refreshing keys for JWT verification") | ||
| jwksMaxCacheAge = flag.Duration("jwks_max_cache_age", 1*time.Hour, "Maximum duration during which keys that could not be refreshed are still used before shutting down the service") |
There was a problem hiding this comment.
jwks_ttlinstead?- mention 1 hour is a sane default due to https://github.com/astm-utm/Protocol/blob/F3548-21/utm.yaml#L92
There was a problem hiding this comment.
#1636 add some general documentation, for the second point would that be enough to put it there (PR may need some improvement for that) ? I would try to avoid lengthy explanations there.
First point done.
Yes, it probably since dynamic adaptation is a bit 'tricky' and is probably worth a dedicated PR |
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
LGTM
I will remove however from the description the fact that it fixes the issue and let @BenjaminPelletier assess what to do with it (close it, leave it open, and/or open a follow-up issue).
2 participants
This PR contributes to #1632 by implementing a retry mechanism for transient JWKS key refresh failures.
The service will still panic when receiving a valid reply where, e.g., the required key is not found.
Implementing it via the
CodeRetryablesystem had the positive side effect of applying the same retry behavior on startup in case of issues - similar to the datastore - which makes sense instead of directly crashing, so I kept it.No documentation changes are included as none currently exist. I will create follow-up PRs for that along with deployment changes.