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 publishe...
For further actions, you may consider blocking this person and/or reporting abuse
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.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 negative cache stops repeats, but the sequence I described never repeats a kid. Each request carries a fresh random string, which costs the attacker nothing, so the per-kid cache never gets a hit and the shared attempt still goes to junk every 30 seconds. It's worth keeping for clients that loop on one bad token, just not for this attack.
What takes the attacker out of it is a refresh they can't trigger. Run a background fetch on the cache's own timer, every few minutes or on the issuer's Cache-Control max-age, separate from the unknown-kid floor. If the issuer publishes rsa-2 before it starts signing with it, that fetch picks the key up before any token carries it, and the floor only matters for issuers that rotate without notice. The test fits next to the other one: keep the floor busy with invented kids, publish rsa-2 ahead of use, let the timer fire, and a real rsa-2 token passes on its first try.
You're right about the negative cache: a random kid on every request never touches it, so it only pays off against a client looping on one bad token.
The background fetch has to dodge one thing or it inherits the problem. The floor sits inside
refresh, which checksj.attemptedfirst, so a timer callingrefreshgets refused by the attempt the attacker just burned. So the background path skips that check and recordsj.attempteditself: its rate comes from the ticker, not from callers. The cost is thatJWKSstops working as a zero value. It gets aStartthat returns a stop func, andCacheForturns into the tick interval instead of a staleness check on the request path. Your test goes in with the tick driven directly, since an injectedNowcan't fire a real ticker.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.