A key set cached for fifteen minutes stays correct right up to the moment the issuer rotates. An issuer signs with a new key as soon as it publishes it, so a resource server waiting out its TTL turns away every token minted in the gap: good signature, trusted issuer, correct audience, 401. What the cache needs is a second trigger with nothing to do with age, a token naming a key id the cache does not hold, and once you add that trigger the hard decisions move somewhere new.
I have been verifying bearer tokens since 2018, in production systems where money moves, and this is the failure that never turns up in staging. Rotations are rare, on the issuer's side they are instant, and the symptom is a few minutes of 401s that stop by themselves before anyone has finished reading the logs. The code below comes from mcpaudience, a small Go package that does the resource-server half of MCP authorization, and the key set lives in jwks.go.
The timer answers a question nobody asked
A TTL answers one question: how old is my copy of the key set. The question a verifier actually has is whether that copy contains the key this token names. Those two questions agree nearly all of the time and come apart during exactly one event, a rotation. The issuer publishes rsa-2 and starts signing with it right away, and my cached copy, eight minutes old and perfectly fresh by its own accounting, holds only rsa-1.
So this is not a TTL cache. The lookup takes the kid first and the clock second:
func (j *JWKS) Key(ctx context.Context, kid string) (crypto.PublicKey, error) {
if err := j.checkURL(); err != nil {
return nil, err
}
// The lock is held across the fetch, so a burst of requests arriving on a
// rotation makes one request at the issuer rather than one each.
j.mu.Lock()
defer j.mu.Unlock()
cached, known := j.keys[kid]
if known && !j.stale() {
return cached, nil
}
if err := j.refresh(ctx); err != nil {
if known {
// An issuer being unreachable is not evidence that the key it
// published stopped being its key. Tokens still expire on their own.
return cached, nil
}
return nil, fmt.Errorf("no key for kid %q: %v", kid, err)
}
if key, ok := j.keys[kid]; ok {
return key, nil
}
if reason, ok := j.skipped[kid]; ok {
return nil, fmt.Errorf("the key set lists kid %q but it cannot be used: %s", kid, reason)
}
return nil, fmt.Errorf("no key for kid %q in the key set at %s", kid, j.URL)
}
The fast path is those first four lines: a kid I hold, in a set that is not stale, answered out of memory with no network at all. Everything after that is the rotation path. CacheFor is still there and still forces a refetch at fifteen minutes by default, but its job has shrunk to housekeeping, eventually noticing keys the issuer took away. It is not what makes rotation work any more.
About the mutex: it is held across an HTTP request, not only around the map. That serializes verification for the length of one fetch, which sounds expensive until you price the alternative: a rotation throws off a burst of unknown-kid lookups that all arrive together, and with no lock each one of them opens its own connection to the issuer. One bounded wait is cheaper than a herd I caused myself. Bounded is doing real work in that sentence, because the default client carries a ten second timeout and the response body is read through io.LimitReader up to maxKeySetBytes, so a hanging or endless issuer cannot sit on the lock forever.
A fetch the caller can trigger needs a floor
This is the part that makes the unknown-kid trigger more than a one-line change. The kid comes out of the token header, and the token header is attacker-controlled. If an unknown kid means a fetch, then a stream of tokens carrying invented kids means a fetch each, and my resource server turns into a small load generator aimed at my own authorization server. That server is shared infrastructure: the amplification takes out the login flow, not just my service.
So refresh refuses to run twice inside MinRefresh, thirty seconds by default, and it records the attempt before it tries:
func (j *JWKS) refresh(ctx context.Context) error {
now := j.now()
if since := now.Sub(j.attempted); !j.attempted.IsZero() && since < j.minRefresh() {
return fmt.Errorf("the key set was fetched %s ago and is not fetched again more than once every %s",
since.Round(time.Second), j.minRefresh())
}
j.attempted = now
body, err := j.get(ctx)
if err != nil {
return err
}
keys, skipped, err := parseKeySet(body)
if err != nil {
return fmt.Errorf("%s: %v", j.URL, err)
}
j.keys, j.skipped, j.fetched = keys, skipped, now
return nil
}
func (j *JWKS) minRefresh() time.Duration {
if j.MinRefresh > 0 {
return j.MinRefresh
}
return 30 * time.Second
}
j.attempted = now sits above j.get, and that is the line I would get wrong if I were writing this in a hurry. Record the attempt only on success and a down issuer means the floor never engages, so every single request retries: the floor would hold exactly when nothing needed holding, then disappear during the outage, when hammering a sick issuer is at its worst. Note too that j.fetched moves forward only on success, so a failed attempt limits retries without making a stale set look fresh. Two timestamps doing two different jobs, and collapsing them into one is tempting and wrong.
The behaviour is cheap to pin down in a test, and this is the one I would write first:
// Refetching on an unknown kid is free traffic at the issuer unless it is rate
// limited: invented key ids must not become one request each.
func TestAnUnknownKeyIDIsNotAFetchPerToken(t *testing.T) {
s := newSigner(t)
issuer := newKeyServer(t, rsaJWK("rsa-1", &s.rsaKey.PublicKey))
v := keySetVerifier(&JWKS{URL: issuer.url, MinRefresh: time.Minute, Now: func() time.Time { return now }})
for i := 0; i < 5; i++ {
_, err := v.Verify(context.Background(), s.sign(t, "RS256", "invented", goodClaims()))
if !errors.Is(err, ErrBadSignature) {
t.Fatalf("err = %v, want ErrBadSignature", err)
}
}
if got := issuer.count(); got != 1 {
t.Errorf("the issuer was asked %d times for a key id it never published, want once", got)
}
}
The injected Now is what makes this testable without sleeping. A key set cache is almost all clock-dependent logic, and a clock you cannot set is a cache you cannot test.
Unknown is three answers, not one
After a successful refresh there are three distinct reasons the kid is still not usable, and the tail of Key keeps them apart. The key is present: return it. The kid is in skipped, meaning the issuer did publish it but parseKeySet dropped it and remembered why: an HMAC or none alg, an RSA modulus under 2048 bits, a curve that is not P-256, a kty of oct. That refusal says so, with the reason attached. Only a kid the issuer has genuinely never published comes back as "unknown".
Two lines of distinction there save an hour of somebody's day. "Unknown kid" sends an operator off to the issuer to check whether the key was published, and the issuer answers yes, it is right there in the document. "The key set lists kid rsa-3 but it cannot be used: RSA modulus is 1024 bits" sends them straight to the right place. A set with no usable key in it is an error rather than an empty cache, and the previous keys stay put, since a malformed publish should not take verification down.
The fetch is a trust boundary too, and the cheapest guard sits at configuration time:
func (j *JWKS) checkURL() error {
raw := strings.TrimSpace(j.URL)
if raw == "" {
return fmt.Errorf("no key set URL configured")
}
parsed, err := url.Parse(raw)
if err != nil || !parsed.IsAbs() || parsed.Host == "" {
return fmt.Errorf("key set URL %q is not an absolute URL", j.URL)
}
if parsed.Scheme != "https" && parsed.Hostname() != "localhost" && parsed.Hostname() != "127.0.0.1" {
return fmt.Errorf("key set URL %q must use https: a key set an attacker on the path can replace verifies the attacker's tokens", j.URL)
}
return nil
}
The error message is the argument. Nothing else in a verifier counts for much if the document that supplies the public keys can be swapped in transit.
Availability over freshness, stated out loud
The last branch is the one worth arguing about. When the refresh fails and the kid was already held, the cached key verifies the token anyway. My reasoning: an issuer being unreachable is not evidence that the key it published stopped being its key, and the tokens it signed expire on their own. Refusing every request while the issuer is down converts a brief issuer blip into a total outage of my service, and it buys no security, since nothing about an HTTP 503 says anything about a public key.
The cost is real and I will name it. A key the issuer deliberately removed keeps verifying tokens until the next successful fetch. If a signing key leaks and the issuer pulls it from the document, my server honours it for as long as the issuer stays unreachable. That is the trade, and I think it is only acceptable because a key set was never a revocation mechanism: pulling a key does not invalidate the already-issued tokens signed with it either. If you need revocation that takes effect now, you need token introspection, and then you are making a request per token with its own failure policy instead of pretending a cached document can do that job.
Note that the fallback applies only to a kid already held. An unknown kid during an outage gets the error, because there is nothing to fall back to and guessing would mean picking some other key out of the set.
One more detail decides what the client sees. The verifier resolves a kid against its configured Keys first and the fetched KeySet second, so a fetched key never overrides a pinned one, and every key-set failure comes back wrapped in ErrBadSignature:
func (v *JWTVerifier) key(ctx context.Context, kid string) (crypto.PublicKey, error) {
if key, ok := v.Keys[kid]; ok {
return key, nil
}
if v.KeySet == nil {
return nil, fmt.Errorf("%w: no key for kid %q", ErrBadSignature, kid)
}
if strings.TrimSpace(kid) == "" {
// Without a kid there is nothing to look up and nothing to rotate; a
// verifier that guesses picks the wrong key the first time one is added.
return nil, fmt.Errorf("%w: the token names no kid, so no key can be chosen", ErrBadSignature)
}
key, err := v.KeySet.Key(ctx, kid)
if err != nil {
return nil, fmt.Errorf("%w: %v", ErrBadSignature, err)
}
return key, nil
}
The wrapping is a deliberate answer to a question that comes up in review: should "I could not reach the issuer" be a 500? It should not. The client's situation is identical either way, the token it presented did not verify and its move is to go get a new one, and a 500 invites it to retry that same token against a server it has just been told is broken. A token with no kid at all is refused for the same reason it cannot be rotated: there is nothing to look up, and a verifier that guesses picks wrong the first time the issuer adds a second key.
What I would do differently
The rate limit is global, not per kid. An attacker spraying invented kids can burn the one allowed attempt in an interval and delay the refetch that a genuine rotation needs by up to MinRefresh. With a thirty second default that window is small and the failure heals itself, so it is a defensible shape for a small package. In a system where I controlled both ends, I would keep a per-kid negative cache next to the floor, so a kid that was already refused and is already absent from a freshly fetched set does not spend the shared attempt at all.
I would also not leave the outage recovery passive. As it stands, the last good set returns to normal only when some request happens to arrive with a kid that is unknown or a set that has gone stale. A background refresh attempt during a known-failed state would shorten the window in which a removed key stays live, which is the one cost of the availability trade.
And I would instrument skipped and attempted. Both are facts about the issuer that nothing else in the system knows: a key published but unusable, and a refetch declined by the floor. They stay invisible until someone reads a 401 and starts guessing...
The shape that matters generalizes past JWKS. When a cache's correctness depends on an external event rather than on elapsed time, the trigger belongs with the event and not with the clock, and the moment that trigger becomes reachable by an untrusted input, it needs a floor that counts failures.
Originally published on polycratia.com — where I write about payment systems, crypto rails and marketplace backends.
Top comments (42)
kid first, clock second. a TTL answers how old the copy is, not whether it has the key.
That's the whole post in two lines, and better than my title. The one thing I'd add in the clock's defence: removal. A cache keyed on kid never learns that the issuer pulled a key, because nothing arriving asks about it. So the TTL stays, but demoted to housekeeping, a ceiling on how long a withdrawn key keeps working rather than the thing that makes rotation work. Curious how you handle that in rootmail, since mail is one of the places where a key quietly disappearing hurts the most.
Good catch on removal. For rootmail in beta, a withdrawn key should fail loudly, since mail is where a silent miss costs the most.
Agreed, and for mail I'd go one step further than my own code does. The fallback I defend only covers the issuer being unreachable; a successful fetch that no longer lists a kid I was holding is a different event, and that's the one to make loud. Diff the key set on every refresh, drop the vanished kid right there, and emit it as its own signal, not as a 401 that looks like every other 401. Then a withdrawn key costs you one visible event instead of a quiet week of accepted mail.
That diff on every refresh is the part I want to keep. A vanished kid getting its own signal beats sorting it out from a pile of identical 401s.
It lands on one line in
refresh. The last statement there today isj.keys, j.skipped, j.fetched = keys, skipped, now, and that replaces the old set wholesale, so you can only compare before that assignment: after it there is nothing left to compare against. The diff sits betweenparseKeySetreturning and that line, and any kid present inj.keysbut absent from the newkeysis a withdrawal.One thing I want the signal to carry: a kid can leave
keyswithout the issuer having removed it. If the issuer republishes the same kid with, say, a 1024-bit modulus,parseKeySetroutes it intoskippedwith the reason attached, so the kid is still in the document and just unusable. For an operator those are two different events (the key is gone, versus the key is there and I refuse it), andskippedalready holds the reason that tells them apart, so the diff should check it before calling anything withdrawn.The part I would be careful about is where the signal goes.
JWKShas no hook on it today, and anything you add gets called withj.muheld, the lock that stays held across the fetch. A slow callback there stalls every verification queued behind it, so whatever it ends up being has to be a counter bump or a buffered send, nothing that talks to the network. It goes on the list.That skipped distinction matters, and the j.mu warning rules out anything slow on that path.
What I would land on keeps the callback off the lock entirely instead of asking it to be fast.
Keytakesj.muwith adefer, so anythingrefreshreaches fires while the lock is still held, and one slow callback stalls every verification queued behind it. Move the locked body into an inner method that returns the key, the error and the list of withdrawn kids, andKeycan release the mutex before emitting: a callback doing something unwise then costs the one request that tripped the refresh, nothing else.The diff needs no special case for a cold start, and that is the part I expected to have to handle.
j.keysis nil until the first successful fetch, so the first set compares against an empty map and reports nothing withdrawn. A key I never held cannot have been pulled.The other end of this is who runs the callback. It is whichever request happened to trigger the refresh, so even off the lock it sits on a verification path: a log line or a counter is fine there, an outbound call is not. I would hang it on the list as a field next to
Now, same injectable style, nil meaning no signal, with theskippedcheck staying inside the diff so a republished-but-refused kid never reports as withdrawn.releasing the mutex before emitting is the clean move. that part about whoever triggered the refresh running the callback is the bit people will miss.
That cost is already the shape of the fetch, which is probably why it slides past you. The ten second timeout on the default client in
client()gets paid by whichever request showed up with an unknown kid, so that one request underwrites the refresh for everyone queued behind it, before any callback exists: adding the emission just puts a second line on the same bill.minRefreshis what keeps it bounded. At most one fetch per thirty seconds by default means at most one emission per thirty seconds, so it is not a hot path, only an occasionally unlucky request.What I would not do is hand that callback the caller's
ctx. A withdrawn kid is a fact about the issuer's document, not about the request that happened to notice it, and binding it to that request's cancellation makes the signal disappear at the least convenient moment. That failure mode is live already, callback or not: if the caller goes away mid-fetch,j.getreturns the error,refreshreturns it, and the assignment toj.keysnever happens, so you have nothing to diff. Andj.attempted = nowis recorded abovej.get, so the abandoned attempt still spends the interval, and the next request can be refused its refresh for up toMinRefresh.So the field's doc comment carries more than its signature does. A
func(kids []string)sitting next toNowtells you nothing about running on a verification path, off the lock, in whichever goroutine tripped the refresh. The comment goes on the list along with the field.The abandoned attempt spending the interval is the part I had not priced in. A doc comment saying whose goroutine runs it would save the next reader a guess.
The doc comment is the cheaper half here. I would hang it on the callback field itself, right next to
Now: it runs on the goroutine of whatever request tripped the refresh, afterKeyhas let go ofj.mu, and it is not handed that caller'sctx. The field is what someone is looking at when they decide what to put in it, so the constraint belongs there and not in prose sitting further away.The abandoned attempt needs its own line too, and not where you would look for it.
MinRefreshcurrently says it is the shortest gap between two fetches, which you read as fetches that actually happened. Butj.attempted = nowsits abovej.get, so a fetch that died in flight burns the interval exactly like a completed one does, and I will say so on that field.Putting the note on the callback field is the stronger move, since that is where someone decides what to pass in. The MinRefresh point is easy to miss too: if j.attempted sits above j.get, a dead fetch still spends the interval.
Those are the same wording problem in two places, and the second one has a tail I had not followed all the way out.
retryAfterin guard.go is a const "30", so a cold-cache failure answers 503 with Retry-After: 30 no matter whatMinRefreshis set to. SetMinRefreshto a minute and a client that obeys the header comes back at thirty seconds into a floor that will not let the refetch happen: it gets the same 503, and the round trip bought nothing. The two numbers agree only because they share a default.So the 503 should carry what is actually left of the interval instead of a constant. That means
retryAfterstops being a const in guard.go and the wait comes from the key set:refreshalready knowsnow.Sub(j.attempted)againstminRefresh()when it refuses, and that remainder is probably the only honest number to put in the header.the 30 versus a minute example makes the mismatch concrete. carrying the remainder from refresh instead of a constant keeps the header saying what the client will actually wait for.
The remainder needs to travel from
refreshout towriteChallenge, and right now it can't:Keywraps the cold-cache failure asfmt.Errorf("%w: ...", ErrUnavailable, err), so the number ends up buried in a string. I'd make that a typed error carrying the wait, letwriteChallengepull it out witherrors.As, and keepretryAfteras the fallback only, for a failure that carries no interval.The arithmetic comes out the same whichever way the refresh failed.
j.attempted = nowsits abovej.get, so after a fetch that died in flight the remaining wait isminRefresh() - now.Sub(j.attempted), exactly as it is after a floor refusal. One detail:Retry-Afteris whole seconds, so you have to round up. Round down and the client is back at the door just before the floor lifts, which is the thing being fixed.The rounding up point is the one I would have missed. A client returning a fraction early and hitting the same floor again is a neat way to turn a fix into a loop.
Two separate things here. Knocking cannot push the deadline back: the floor check in
refreshreturns beforej.attempted = now, so a refused refresh leaves that timestamp wherever the last real attempt put it, and the interval still expires at a fixed instant. An early arrival costs you a round trip, not a moving target.The rounding is what would actually loop, and the bad value is zero, not a fraction. A remainder of 400ms truncated to whole seconds gives
Retry-After: 0, which sends the client straight back into a floor that still has 400ms left on it. Ceiling division makes any positive remainder at least 1, so that is how I will compute the header value inwriteChallenge, with theretryAfterconst left as the fallback for a failure that carries no interval.The zero remainder case makes the loop concrete in a way the fraction case never did. Keeping the const only for failures with no interval reads like the right fallback.
Worth being precise about how narrow that fallback actually gets.
ErrUnavailableleavesKeyon exactly one path: a refresh that failed with no set held. Andj.attempted = nowsits abovej.get, so every failure on that path has a remainder to report, a floor refusal and a fetch that died in flight alike. So insidejwks.gothe const has no live caller left once the typed error lands.I am keeping it anyway, because
ErrUnavailableis exported and any key source you hand a verifier can return it bare, with no interval behind it. You cannot see that reason at the declaration, so I will put it there: a comment onretryAfteringuard.gosaying it is reached only when the error carries no wait, which the key set never does.That narrow fallback makes sense. Keeping the const for a bare exported error is a clean way to cover a key source outside this set.
The typed error depends on something the const fallback does not: the wait has to survive the trip.
errors.AsinwriteChallengeonly reaches it if every hop betweenJWKS.Keyand there wraps with%w, and the hop that matters isJWTVerifier.key, because it puts every key-set failure behindErrBadSignature. Put a%von the inner error there and the wait is a substring again: the branch falls through to the const, and the symptom looks exactly like the thing being fixed.So the typed error lands with that wrap in
jwt.gocarrying the inner error as%wtoo, double-wrapped soErrBadSignatureand the wait both come out the other end. I will pin it with a test that pulls the wait witherrors.Asthrough the verifier, not straight offKey. Testing atKeyis probably the spot where the chain looks fine and is still broken one layer up...That JWTVerifier.key catch is the sharp part. A %v on the inner error there keeps the wait from collapsing back to a substring, and testing through the verifier is the right place to pin it.
I had the verb backwards: a
%vcollapses the wait into a substring, and%wis what keeps it reachable. The hop was wrong too. In the code as it stands,JWTVerifier.keytestserrors.Is(err, ErrUnavailable)and returns that error untouched, and only the other key-set failures go behindErrBadSignature. The wait ridesErrUnavailableand nothing else, so there is no double wrap to add injwt.go: the pass-through already carries it. I will leave that branch returning the error unwrapped.So the test becomes the whole guard. I will pin it through
Verifyrather than offKey, asserting thaterrors.Asfinds the typed error and the wait on what the verifier returns. Then the day someone tidies that branch into theErrBadSignaturewrap, it fails in the test instead of in aRetry-Afternobody reads.pinning it through Verify is the right guard. the test catches the tidy-up before users do.
That test needs one more thing beyond the wait, or it misses the exact tidy-up it exists to catch. Say you fold the
ErrUnavailablebranch into theErrBadSignaturewrap with%w:errors.Asstill finds the typed error and the wait still comes out the other end, so an assertion on the wait alone goes green.What breaks is the classification. The error starts satisfying
errors.Is(err, ErrBadSignature), so the challenge turns into a 401 that happens to carry aRetry-Afterinstead of the 503 the cold-cache path is supposed to produce.So I'd assert two things about what
Verifyreturns rather than one:errors.Asfinds the typed error with a positive wait, anderrors.Is(err, ErrBadSignature)is false. The second one is what fails on the tidy-up. It goes injwt_test.gobeside the existingErrUnavailablecase.yeah, folding it into the bad-signature wrap hides the real failure. 401 vs 503 is the whole point of the distinction.
The whole split sits in one function.
writeChallengein guard.go checksErrUnavailablebefore it builds anything: it setsRetry-After, answers 503, and sends noWWW-Authenticateat all, and only the fall-through past that assembles the 401 witherror="invalid_token". So the fold would not touch a status code you could see in a diff. What it changes is which branch of that chain the error matches, three hops from the code it ends up producing.jwks_test.go already pins the response end of it: the cold-cache case asserts 503, a non-empty
Retry-After, and an empty challenge header. The thing nothing pins yet is the shape of the error the verifier hands up, and that's the gap the new assertion closes.A critic that only reports fits that split. Approval power is what would pull it back into the blast radius.
Right, and
JWKS.Keyalready draws the line by what it can return: a key, or an error. Nothing it hands back means accept. The verdict isverifySignatureagainst whatever key came out, so even the availability branch, the cached key you get back whenrefreshfails, only carries a token as far as a signature check it still has to pass. Give the key set approval power and an unreachable issuer becomes a path to acceptance. With report-only the worst an outage does is withhold a key, andwriteChallengeturns that into the 503. Report-only is the invariant, and the fallback policy just sits on top of it.report-only as the invariant, then. an outage that withholds a key is the safe failure.
Yes, and
Keyis picky about which failure it calls an outage. A failed refresh with no set held at all comes back asErrUnavailable. A failed refresh with a set in memory that doesn't name the kid comes back as a plain unknown-kid error, gated onlen(j.keys) == 0.Same failed fetch, two answers: a held set that never listed that kid already answers the question without the issuer. So the 503 is yours on a cold cache only, and a withheld key from a warm one stays a 401.
The refresh floor is shared, so an attacker can choose when it gets spent. Send an invented kid every 30 seconds and every fetch slot goes to junk. When the issuer really rotates, tokens carrying rsa-2 get turned away until the floor runs out. That's at most
MinRefreshof 401s, a fair price for protecting the issuer, but it's the rotation gap the post set out to close, now on a timer the attacker sets.I'd make it visible rather than try to remove it. The test I'd add next to yours: invented kid at t=0, issuer publishes rsa-2 at t=1, a real token at t=2 fails, and the same token at t=31 passes. Then I'd stop wrapping the floor refusal in
ErrBadSignature. A refusal from the floor says nothing about the token, so a 503 withRetry-Afterset to the time left on the floor lets a client retry once. A 401 tells it to throw the token away, and a fresh one signed with rsa-2 gets refused for the same reason.You're right, and you've put a number on something I only waved at: the floor is shared, so the attacker decides when it gets spent. The article admits the rate limit is global, but your sequence is the honest version of that admission, so I'm taking the test more or less as written. Invented kid at t=0, rsa-2 published at t=1, a real token refused at t=2 and accepted at t=31. It documents the cost instead of hiding it in a paragraph.
On the status code I agree with the diagnosis and would move the fix one layer up. A floor refusal says nothing about the token, and wrapping it in ErrBadSignature throws that fact away. So the verifier should return a distinct error, something like ErrKeySetCoolingDown with the time left on the floor, and leave the HTTP mapping to the handler. In an API I'd map it to 503 with Retry-After, as you say. In a library I'd rather not pick HTTP codes for the caller, but I'd make it impossible for the caller to confuse "your token is bad" with "I can't look right now".
The other half is a per-kid negative cache. Once an invented kid has been looked up and refused, it remembers its own refusal, and the shared attempt is only spent on kids nobody has seen before. The attacker then has to invent a new kid every thirty seconds to keep the floor busy, which is still possible but no longer free.
Thanks for reading it that closely. This is the review the code should have had before it shipped.
The injected clock is the detail I’d borrow first. I’d start with a small TTL cache exercise and pin the exact expiry boundary, then test CacheFor and MinRefresh independently here. Advancing past freshness shouldn’t accidentally erase the record of a failed refresh. Two timestamps can look redundant until one of those boundary tests fails.
The two timestamps are already kept apart in the code:
refreshsetsj.attemptedbeforej.get, andj.fetchedmoves only afterparseKeySetsucceeds, so crossingCacheForinstale()tells you nothing about whether a failed refresh got recorded. What I don't have is a test that pins that. I'll add one tojwks_test.go: advance the clock pastCacheForwith the issuer failing, callKeytwice insideMinRefresh, assert the issuer was asked once.One spot where the TTL exercise doesn't map onto this cache: there is no per-key expiry, and nothing gets evicted.
refreshswapsj.keysandj.skippedwholesale, so the only freshness boundary left to pin isstale(), which compares with>=againstcacheFor. I'll pin it exactly atfetchedplusCacheFor, same file.One limit I'd add to the availability fallback is an explicit maximum stale age. Token expiry alone doesn't bound exposure after a private-key compromise: whoever holds that key can keep minting tokens with future exp values. If refresh keeps failing, the known-kid branch can keep accepting those fresh forgeries. A max-stale policy measured from the last successful fetch would put a ceiling on that trade-off; failed refresh attempts must not reset it. The useful boundary test is an outage that lasts past that ceiling, with a newly minted token still carrying the cached kid.
Expiry doesn't bound this, you're right. A key the issuer retired on schedule leaves behind tokens that expire on their own. A key an attacker holds keeps producing fresh
expvalues, and the known-kid branch inKeyhands back the cached key for as long asrefreshkeeps failing, with no ceiling on it at all.The timestamp that ceiling needs is already sitting there:
j.fetchedadvances only after a successful parse inrefresh, andj.attemptedsoaks up the failed attempts, so a down issuer can't push it forward. Your constraint holds for free.So I'll add
MaxStaletoJWKSand check it in that fallback branch: past the ceiling the cached key isn't returned, and the error isErrUnavailablerather thanErrBadSignature, since the signature was never checked and a 401 would just send the client off for a replacement that fails the same way. Your boundary test goes intojwks_test.gothe way you describe it.Authentication caches need a threat model as well as a performance model. The useful tests here are key rotation, unknown key IDs, issuer changes, and concurrent verification under refresh pressure. A cache that is fast on the happy path but stale at those boundaries quietly changes the security contract.
Rotation, invented kids, a dead issuer: each one already has a path in
Key. The unknown-kid refetch, theMinRefreshfloor inrefreshwithj.attemptedwritten beforej.get, andErrUnavailablewhen no set is held against the cached key when one is. Concurrency under refresh pressure is the case with nothing behind it.j.muis held across the fetch, so the claim is that a burst of unknown kids turns into one request at the issuer, and no test asserts that. I will add a test injwks_test.gothat fires parallelVerifycalls on an unknown kid and fails unless the issuer was asked once.Issuer changes I would not call a cache boundary at all: a
JWKSholds oneURL, so a different issuer is configuration plus anIssuersentry, not anything the cache can observe. The single branch where staleness moves the security contract is the held-key fallback after a failed refresh, and that is a trade I took on purpose rather than a hole.Good write-up. The point that stuck with me is that a TTL tells you how old your copy is, not whether it holds the key this token needs. Making the kid the trigger and the clock the housekeeping is a cleaner mental model than most JWKS libraries give you.
Two details I liked a lot. Setting j.attempted before the fetch, with j.fetched moving only on success, is easy to get backwards, and the failure it prevents (the floor disappearing during an issuer outage) is exactly the kind that never shows up in staging. And separating “unknown” from “listed but unusable” in the error is the kind of small thing that saves an on-call engineer an hour.
One thing I’d poke at: because Key takes the mutex before checking the cache, the “fast path” still queues behind an in-flight refresh. During a slow issuer, known-kid verifications could wait up to the 10s client timeout even though they never needed the network. An RWMutex for the hit path, or singleflight for the refresh with stale-serve for held keys, would keep the herd protection without that coupling. Is the serialization a deliberate simplicity trade for a small package?
I’d also be curious whether you considered honoring the issuer’s Cache-Control: max-age on the JWKS response instead of a fixed CacheFor. It could shrink the window where a removed key stays live without adding a background refresher.
Your closing point generalizes well: when correctness depends on an external event, tie the trigger to the event, and once an untrusted input can reach that trigger, put a floor on it. Thanks for sharing the failure-mode reasoning along with the code.
You're right about the fast path, and you name the cost correctly.
Keytakesj.mubefore it looks atj.keysat all, so a known kid in a fresh set waits out whateverj.getis doing, bounded by the ten second default client timeout and bymaxKeySetBytes. In the article I called that a bounded wait and priced it only for the requests that caused the fetch. The requests that never needed the network pay the same bill, and that is coupling rather than a subtlety.The serialization was a trade I made for the herd: the hit path was collateral, not something I weighed. The shape I'd take is close to your first suggestion: an RLock around the
known && !j.stale()check, the fetch behind its own mutex, and a re-check under the write lock so the second arrival sees the set the first one installed.refreshreplacesj.keysandj.skippedwholesale, so a read path does not see a half-updated map. It goes on the list, next to the per-kid negative cache.Stale-serve for held keys is a bigger change. Today a known kid in a stale set goes through
refreshsynchronously, which means the housekeeping path charges a live request for evicting keys it isn't using. Returning the held key first and refreshing after needs a goroutine and a context that outlives the request, and the package has neither right now.Cache-Controlhad not occurred to me:j.getreads the status code and the body and nothing else off the response. The problem is thatmax-ageonly movescacheFor, which is housekeeping now, so it doesn't shrink the window you mean. A removed key stops verifying on the next successful fetch, and during an outage there's no response header to tell me anything. It would also need clamping both ways, since an issuer advertisingmax-age=0makes every lookup stale and sendsKeyintorefreshevery time, withMinRefreshthe only thing standing between that and a fetch per token.