DEV Community

polycratia
polycratia

Posted on Originally published at polycratia.com

A JWKS cache keyed on the token, not on a timer

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)
}
Enter fullscreen mode Exit fullscreen mode

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
}
Enter fullscreen mode Exit fullscreen mode

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)
    }
}
Enter fullscreen mode Exit fullscreen mode

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
}
Enter fullscreen mode Exit fullscreen mode

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
}
Enter fullscreen mode Exit fullscreen mode

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)

Collapse
 
robertpaschal profile image
Odinaka Robert Nnamani •

kid first, clock second. a TTL answers how old the copy is, not whether it has the key.

Collapse
 
polycratia profile image
polycratia •

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.

Collapse
 
robertpaschal profile image
Odinaka Robert Nnamani •

Good catch on removal. For rootmail in beta, a withdrawn key should fail loudly, since mail is where a silent miss costs the most.

Thread Thread
 
polycratia profile image
polycratia •

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.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

It lands on one line in refresh. The last statement there today is j.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 between parseKeySet returning and that line, and any kid present in j.keys but absent from the new keys is a withdrawal.

One thing I want the signal to carry: a kid can leave keys without the issuer having removed it. If the issuer republishes the same kid with, say, a 1024-bit modulus, parseKeySet routes it into skipped with 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), and skipped already 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. JWKS has no hook on it today, and anything you add gets called with j.mu held, 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.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

That skipped distinction matters, and the j.mu warning rules out anything slow on that path.

Thread Thread
 
polycratia profile image
polycratia •

What I would land on keeps the callback off the lock entirely instead of asking it to be fast. Key takes j.mu with a defer, so anything refresh reaches 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, and Key can 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.keys is 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 the skipped check staying inside the diff so a republished-but-refused kid never reports as withdrawn.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

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. minRefresh is 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.get returns the error, refresh returns it, and the assignment to j.keys never happens, so you have nothing to diff. And j.attempted = now is recorded above j.get, so the abandoned attempt still spends the interval, and the next request can be refused its refresh for up to MinRefresh.

So the field's doc comment carries more than its signature does. A func(kids []string) sitting next to Now tells 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.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

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, after Key has let go of j.mu, and it is not handed that caller's ctx. 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. MinRefresh currently says it is the shortest gap between two fetches, which you read as fetches that actually happened. But j.attempted = now sits above j.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.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

Those are the same wording problem in two places, and the second one has a tail I had not followed all the way out. retryAfter in guard.go is a const "30", so a cold-cache failure answers 503 with Retry-After: 30 no matter what MinRefresh is set to. Set MinRefresh to 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 retryAfter stops being a const in guard.go and the wait comes from the key set: refresh already knows now.Sub(j.attempted) against minRefresh() when it refuses, and that remainder is probably the only honest number to put in the header.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

The remainder needs to travel from refresh out to writeChallenge, and right now it can't: Key wraps the cold-cache failure as fmt.Errorf("%w: ...", ErrUnavailable, err), so the number ends up buried in a string. I'd make that a typed error carrying the wait, let writeChallenge pull it out with errors.As, and keep retryAfter as the fallback only, for a failure that carries no interval.

The arithmetic comes out the same whichever way the refresh failed. j.attempted = now sits above j.get, so after a fetch that died in flight the remaining wait is minRefresh() - now.Sub(j.attempted), exactly as it is after a floor refusal. One detail: Retry-After is 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.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

Two separate things here. Knocking cannot push the deadline back: the floor check in refresh returns before j.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 in writeChallenge, with the retryAfter const left as the fallback for a failure that carries no interval.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

Worth being precise about how narrow that fallback actually gets. ErrUnavailable leaves Key on exactly one path: a refresh that failed with no set held. And j.attempted = now sits above j.get, so every failure on that path has a remainder to report, a floor refusal and a fetch that died in flight alike. So inside jwks.go the const has no live caller left once the typed error lands.

I am keeping it anyway, because ErrUnavailable is 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 on retryAfter in guard.go saying it is reached only when the error carries no wait, which the key set never does.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

The typed error depends on something the const fallback does not: the wait has to survive the trip. errors.As in writeChallenge only reaches it if every hop between JWKS.Key and there wraps with %w, and the hop that matters is JWTVerifier.key, because it puts every key-set failure behind ErrBadSignature. Put a %v on 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.go carrying the inner error as %w too, double-wrapped so ErrBadSignature and the wait both come out the other end. I will pin it with a test that pulls the wait with errors.As through the verifier, not straight off Key. Testing at Key is probably the spot where the chain looks fine and is still broken one layer up...

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

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.

Thread Thread
 
polycratia profile image
polycratia •

I had the verb backwards: a %v collapses the wait into a substring, and %w is what keeps it reachable. The hop was wrong too. In the code as it stands, JWTVerifier.key tests errors.Is(err, ErrUnavailable) and returns that error untouched, and only the other key-set failures go behind ErrBadSignature. The wait rides ErrUnavailable and nothing else, so there is no double wrap to add in jwt.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 Verify rather than off Key, asserting that errors.As finds the typed error and the wait on what the verifier returns. Then the day someone tidies that branch into the ErrBadSignature wrap, it fails in the test instead of in a Retry-After nobody reads.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

pinning it through Verify is the right guard. the test catches the tidy-up before users do.

Thread Thread
 
polycratia profile image
polycratia •

That test needs one more thing beyond the wait, or it misses the exact tidy-up it exists to catch. Say you fold the ErrUnavailable branch into the ErrBadSignature wrap with %w: errors.As still 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 a Retry-After instead of the 503 the cold-cache path is supposed to produce.

So I'd assert two things about what Verify returns rather than one: errors.As finds the typed error with a positive wait, and errors.Is(err, ErrBadSignature) is false. The second one is what fails on the tidy-up. It goes in jwt_test.go beside the existing ErrUnavailable case.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

yeah, folding it into the bad-signature wrap hides the real failure. 401 vs 503 is the whole point of the distinction.

Thread Thread
 
polycratia profile image
polycratia •

The whole split sits in one function. writeChallenge in guard.go checks ErrUnavailable before it builds anything: it sets Retry-After, answers 503, and sends no WWW-Authenticate at all, and only the fall-through past that assembles the 401 with error="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.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

A critic that only reports fits that split. Approval power is what would pull it back into the blast radius.

Thread Thread
 
polycratia profile image
polycratia •

Right, and JWKS.Key already draws the line by what it can return: a key, or an error. Nothing it hands back means accept. The verdict is verifySignature against whatever key came out, so even the availability branch, the cached key you get back when refresh fails, 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, and writeChallenge turns that into the 503. Report-only is the invariant, and the fallback policy just sits on top of it.

Thread Thread
 
robertpaschal profile image
Odinaka Robert Nnamani •

report-only as the invariant, then. an outage that withholds a key is the safe failure.

Thread Thread
 
polycratia profile image
polycratia •

Yes, and Key is picky about which failure it calls an outage. A failed refresh with no set held at all comes back as ErrUnavailable. A failed refresh with a set in memory that doesn't name the kid comes back as a plain unknown-kid error, gated on len(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.

Collapse
 
_firelinks profile image
Mike Dabydeen •

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 MinRefresh of 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 with Retry-After set 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.

Collapse
 
polycratia profile image
polycratia •

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.

Collapse
 
emilywoodsnyc profile image
Emily Woods •

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.

Collapse
 
polycratia profile image
polycratia •

The two timestamps are already kept apart in the code: refresh sets j.attempted before j.get, and j.fetched moves only after parseKeySet succeeds, so crossing CacheFor in stale() 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 to jwks_test.go: advance the clock past CacheFor with the issuer failing, call Key twice inside MinRefresh, 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. refresh swaps j.keys and j.skipped wholesale, so the only freshness boundary left to pin is stale(), which compares with >= against cacheFor. I'll pin it exactly at fetched plus CacheFor, same file.

Collapse
 
kreout profile image
Andrei Ovcharenko •

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.

Collapse
 
polycratia profile image
polycratia •

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 exp values, and the known-kid branch in Key hands back the cached key for as long as refresh keeps failing, with no ceiling on it at all.

The timestamp that ceiling needs is already sitting there: j.fetched advances only after a successful parse in refresh, and j.attempted soaks up the failed attempts, so a down issuer can't push it forward. Your constraint holds for free.

So I'll add MaxStale to JWKS and check it in that fallback branch: past the ceiling the cached key isn't returned, and the error is ErrUnavailable rather than ErrBadSignature, 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 into jwks_test.go the way you describe it.

Collapse
 
alexshev profile image
Alex Shev •

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.

Collapse
 
polycratia profile image
polycratia •

Rotation, invented kids, a dead issuer: each one already has a path in Key. The unknown-kid refetch, the MinRefresh floor in refresh with j.attempted written before j.get, and ErrUnavailable when no set is held against the cached key when one is. Concurrency under refresh pressure is the case with nothing behind it. j.mu is 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 in jwks_test.go that fires parallel Verify calls on an unknown kid and fails unless the issuer was asked once.

Issuer changes I would not call a cache boundary at all: a JWKS holds one URL, so a different issuer is configuration plus an Issuers entry, 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.

Collapse
 
mona_d_4222dda374567263b profile image
Amber D. •

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.

Collapse
 
polycratia profile image
polycratia • • Edited

You're right about the fast path, and you name the cost correctly. Key takes j.mu before it looks at j.keys at all, so a known kid in a fresh set waits out whatever j.get is doing, bounded by the ten second default client timeout and by maxKeySetBytes. 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. refresh replaces j.keys and j.skipped wholesale, 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 refresh synchronously, 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-Control had not occurred to me: j.get reads the status code and the body and nothing else off the response. The problem is that max-age only moves cacheFor, 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 advertising max-age=0 makes every lookup stale and sends Key into refresh every time, with MinRefresh the only thing standing between that and a fetch per token.