Fall back to still-valid cached keys when a JWKS refresh fails - #217
Open
djw8605 wants to merge 2 commits into
Open
Fall back to still-valid cached keys when a JWKS refresh fails#217djw8605 wants to merge 2 commits into
djw8605 wants to merge 2 commits into
Conversation
When cached keys pass next_update, verification kicks off a refresh and sets m_ignore_error, intending to tolerate a refresh failure since the cached keys remain valid until their (much later) expiry. That flag was broken twice over: it was set on the old status object which was immediately replaced by get_public_keys_from_web()'s new status, and nothing ever read it. The catch block only covered a synchronous failure in perform_start(); once the refresh continued asynchronously, any failure (issuer briefly down, timeout, bad response) propagated out of verify() and failed the validation despite valid cached keys -- defeating the stale-key tolerance the keycache is designed for (refresh every 10 minutes, keys valid for 4 days). Carry the flag and the still-valid cached keys onto the refresh status, and on an asynchronous fetch failure fall back to those keys (recording a failed refresh in the monitoring stats) instead of rethrowing. The keycache entry is untouched, so the next validation past next_update retries the refresh. The regression test stores keys for an unreachable issuer (https://127.0.0.1:1) with a 1-second update interval: the refresh fails asynchronously and validation must still succeed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or 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
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.
Problem
When cached keys pass
next_update, verification starts a refresh and setsm_ignore_error = true, intending to tolerate refresh failure — the cached keys remain valid until their much later expiry (default: refresh every 600 s, keys valid 4 days).That flag is broken twice over:
get_public_keys_from_web()'s freshly-constructed status — so the flag is lost.m_ignore_erroranyway (grepconfirms the only reference is the assignment).The existing
catchonly covers a synchronous failure insideperform_start(). Once the refresh continues asynchronously (the normal case for network I/O), any failure — issuer briefly down, timeout, bad response — propagates out ofverify()and fails the validation despite valid cached keys, defeating the stale-key tolerance the keycache is designed for.Fix
m_fallback_keys).get_public_key_pem_continue, save the fallback state in locals before calling into the fetch (the status object is destroyed during unwinding if it throws), and on failure fall back to the cached keys — recordingfailed_refreshesin the monitoring stats — instead of rethrowing.next_updateretries the refresh.Testing
SerializeTest.StaleKeyFallbackTest: stores valid keys for an unreachable issuer (https://127.0.0.1:1, connection refused — no external network) with a 1-second update interval, so verification triggers an asynchronously failing refresh. Verified the test fails without the fix and passes with it.ctestunit, env_config, and monitoring suites pass.🤖 Generated with Claude Code