fix: tokenreview caching improvements - #596
Draft
wseaton wants to merge 5 commits into
Draft
Conversation
wseaton
changed the title
The rejection cache is keyed on the raw token, so a caller cycling unique invalid tokens sent one TokenReview to the apiserver per token. Reviews run under the server's own ServiceAccount, so the resulting load lands in APF's global-default priority level and throttles the server against legitimate callers. The 10k-entry caches also shared a size, letting rejected traffic evict verified entries. Reject anything over 8 KiB, not shaped like a JWT, or whose unverified sub claim is not an allowlisted service account, before contacting the apiserver. The claim is caller-controlled, so this only filters: the TokenReview result stays the sole authority on identity, and the post-review allowlist check is unchanged. Bound what survives the filter with a global pool of 8 concurrent reviews and 4 per claimed caller. Those come from APF's default share for global-default: roughly 6% of the apiserver's total concurrency, shared cluster-wide, so 8 is about a fifth of the level and still serves ~1.6k reviews/sec at a 5ms review. Permits are taken per-caller first so a flooding caller cannot hold global permits while queued on its own pool. Acquisition waits up to 250ms before shedding, because a cold-start rollout is a legitimate burst of misses and instant rejection turns it into a retry storm; saturation returns UNAVAILABLE rather than UNAUTHENTICATED so a legitimate client retries instead of treating its credentials as bad, and is never cached. Denials log at debug rather than warn. Bounding apiserver calls while leaving a synchronous log write per request just moves the amplification from the apiserver to the log pipeline. An attacker who forges the sub claim of an allowlisted caller can still occupy that caller's 4 permits, so the per-caller split is worth little with a single-entry allowlist. The global cap holds regardless. Refs: ai-dynamo#530 Signed-off-by: Will Eaton <weaton@redhat.com>
Both caches held the bearer token as their key, so entry size was whatever a caller chose to send and every cached credential sat in memory in the clear. Key on SHA-256 instead: entries are 32 bytes regardless of input and no raw token is retained. The apiserver's own cachedTokenAuthenticator uses an HMAC here, citing precomputation, length extension, and hash collisions. None of those apply to a lookup key that is only ever compared for equality: computing the digest requires already holding the token, and a SHA-256 collision against a valid token is not a threat model. A bare digest gets the size bound and the secret hygiene without a keyed-MAC dependency. Signed-off-by: Will Eaton <weaton@redhat.com>
A flood and a rollout look identical once denials stop being logged per request. Count reviews issued, tokens rejected, requests shed for want of a permit, and backend errors, and report them on a tokio interval so the window reported is the window covered. Four counters is the minimum that says which bound is doing the work; finer attribution belongs in Prometheus metrics rather than a log line. Backend errors warn only on the first occurrence per window, since an apiserver outage would otherwise produce a warning per cache miss. The reporter holds a Weak reference to the state and exits when the server drops it, so it needs no shutdown plumbing. Signed-off-by: Will Eaton <weaton@redhat.com>
Drives real gRPC against an AuthLayer-wrapped server whose fake apiserver reports its own in-flight concurrency, so the assertions are about what a flood costs the apiserver rather than what the code appears to do: 20k unique junk tokens produce no reviews and do not evict a verified caller, and 300 forged-claim tokens never exceed the permit pools while a second allowlisted caller keeps authenticating. Verified the tests fail without the fix. With the prefilter removed and both pools raised, the junk flood produces 500 reviews against an expected 0 and peak concurrency reaches 254 against a pool of 8; the 20k-token case stops finishing at all. Server-boot and token fixtures move to tests/common so both auth test files share them. Signed-off-by: Will Eaton <weaton@redhat.com>
The step named only in_process_server, so auth_e2e never ran in CI despite being gated behind the same feature, and any test target added later would have been silently skipped too. Run the whole set; it costs about 5 seconds. Signed-off-by: Will Eaton <weaton@redhat.com>
wseaton
force-pushed
the
weaton/auth-tokenreview-bounds
branch
from
August 7, 2026 17:08
8d18ee6 to
b98cecf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.