cubeapi: key the rate limiter on the validated identity, not an unvalidated header

[dwin-gharibi]

Closes #1379.

Motivation

The limiter bucketed on the raw X-API-Key header. Because extract_credential prefers
Authorization: Bearer, that header is never validated when a Bearer token is present — but it still
chose the bucket. Rotating it therefore gave a fresh full-quota bucket per request and the limit never
applied. Clients sending no X-API-Key all shared a single "anonymous" bucket, so in callback
(multi-tenant) mode any one tenant could starve the rest. And the keyed state store was never reclaimed.

What this changes

1. middleware/auth.rs — the auth middleware publishes the identity it validated. A new
RateLimitIdentity(String) is inserted into the request extensions once the credential has been accepted,
in both modes:

• simple-key mode: after the key comparison succeeds
• callback mode: after the callback returns 200

The identity is prefixed (bearer: / apikey:) so the two credential kinds cannot collide.

2. middleware/rate_limit.rs — the limiter reads that extension instead of a header, falling back to
a single "unauthenticated" bucket if it is somehow absent. In practice it never is: rate_limit is only
ever layered together with unified_auth, and unified_auth runs first.

3. middleware/auth.rs — the API key comparison is now constant-time. provided != expected_key
short-circuits at the first differing byte, which leaks the key one byte at a time to a patient attacker.
Replaced with a length-check plus an XOR-accumulate loop. CubeOps already does this
(subtle.ConstantTimeCompare), so the two services now agree.

4. state.rs — a background task calls rate_limiter.retain_recent() every 60s, so the DashMap no
longer grows without bound.

No comment changes.

Testing

Re-ran the probe that originally demonstrated the bypass, against the real binary with
CUBE_API_KEY=supersecret --rate-limit-per-sec 3, 30 requests per case:

case	master	this branch
A) Bearer only	26 × 429	27 × 429
B) Bearer + rotating unvalidated X-API-Key	0 × 429	26 × 429
C) valid X-API-Key only	—	27 × 429

Case B is the fix. Case C confirms API-key clients are still limited, and case A that Bearer clients are
too — all three shapes now behave the same.

CubeAPI's own suite:

$ cargo test
test result: ok. 109 passed; 0 failed; 0 ignored

CI gates checked locally:

• cargo fmt --check — clean (fmt-check).
• cargo build — clean.
• cargo clippy --all-targets — no new warnings. The two field_reassign_with_default hits reported in
  auth.rs are the pre-existing ones in that file's test module; their line numbers moved because this
  change inserts code above them.

Behaviour change worth noting

Bearer clients are now limited per token rather than sharing one global bucket. That is the intended
behaviour, but it does mean a deployment that was unknowingly relying on the shared-bucket accounting will
see different 429 patterns. Concretely: a single Bearer client that previously consumed the shared quota
now gets its own, so aggregate throughput across many Bearer clients goes up, while an individual abusive
client is now actually constrained.

Not fixed here

extract_credential matches the Bearer scheme case-sensitively, so a spec-compliant
authorization: bearer <token> is rejected (RFC 7235 §2.1 makes the scheme case-insensitive). I confirmed
it still reproduces on this branch (5/5 requests → 401) and left it alone: it is a separate defect with its
own issue, and mixing an interop fix into a rate-limiting fix would muddy both.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

Stale doc comments now that the key comes from the validated identity, not the header. The module doc above still reads: "Per-API-key token bucket rate limiter middleware. Reads the X-API-Key header and checks the shared governor limiter." That's no longer accurate — this now keys on the RateLimitIdentity extension published by unified_auth (which is itself keyed on the validated credential). Same for AppState::rate_limiter's "Per-API-key rate limiter" field doc. Worth a one-line update so the docs don't mislead a future reader into re-adding header-based keying.

[cubesandboxbot]

Review: cubeapi — key the rate limiter on the validated identity (PR #1380)

AI-generated review. Reviewed against the base branch workspace; the diff was not truncated. I could not run cargo in this environment, so anything compile-related is based on the pinned lockfile (CubeAPI/Cargo.lock → governor 0.6.3) and the surrounding code.

Summary

The core fix is sound and well-motivated: the limiter now buckets on the credential that unified_auth actually validated instead of the unvalidated X-API-Key header, closing the rotation bypass (case B in the PR body), and the identity is set in both auth modes. The constant-time key comparison is a correct improvement, the new tests cover the interesting cases (rotating header, Bearer-only, alternating header styles, per-token separation in callback mode), and the middleware-ordering assumption (unified_auth runs before rate_limit because it is the last-added / outermost layer in with_auth_and_rate_limit) checks out.

There is one blocking issue (the GC call in state.rs does not match the pinned governor API), plus a few lower-severity items.

Blocking

1. retain_recent() arity mismatch against governor 0.6.3 — CubeAPI/src/state.rs:67

limiter.retain_recent() as written will not compile. In governor 0.6.3 (the version pinned in CubeAPI/Cargo.lock, no [patch]), KeyedRateLimiter::retain_recent is pub fn retain_recent(&self, up_to: Instant) -> usize — it requires an Instant argument. The PR body's claim that cargo test / cargo build pass should be re-verified against the pinned lockfile.

Even after adding the argument, watch the semantics: retain_recent(up_to) drops every key whose last update is not after up_to. Passing Instant::now() therefore wipes the whole map on every 60s tick — resetting every client's bucket once a minute and letting a client sitting exactly at the burst boundary get a fresh full bucket right after the sweep. Pass a retention window, e.g. retain_recent(Instant::now() - Duration::from_secs(120)).

Medium / low

2. Simple-key identity embeds the configured secret — CubeAPI/src/middleware/auth.rs:137

RateLimitIdentity(format!("configured-key:{}", expected_key)) stores the plaintext API key as the limiter's DashMap key for the process lifetime. Simple-key mode has exactly one valid credential, so a constant (or a hash) is equivalent and avoids duplicating the secret into a second in-memory structure. Low severity, but easy to fix.

3. Callback-mode identity stores the raw token as the bucket key — CubeAPI/src/middleware/auth.rs:188

identity_of keys the bucket on the raw credential. For Bearer tokens this is usually a multi-KB JWT, so each distinct token becomes a large DashMap entry retained until the GC sweep, and raw credentials stay resident past the request. Hash the token for the key (bearer:<sha256> / apikey:<sha256>) — identity separation is preserved, per-key memory is bounded, and plaintext tokens don't linger. This is the same unbounded-growth class the new GC is meant to contain, so hashing makes the bound effective.

4. Test timing dependence — CubeAPI/src/routes.rs (new tests)

The throttled-count assertions (> 20 of 30 at 3 req/s) are wall-clock dependent. In callback mode each request also makes a real loopback HTTP round-trip to the spawned callback server; if 30 sequential requests take > ~2s on a loaded CI runner, the bucket refills enough to push the throttled count under 20 and the test flakes. The thresholds are tolerantly chosen, so this is likely fine in practice — just flagging the sensitivity.

5. Nit: hand-rolled constant-time compare

constant_time_eq is functionally correct (constant-time in all but length, which is standard), but the PR itself notes CubeOps uses subtle::ConstantTimeCompare. Using subtle (or ring) would be more auditable and keep the two services literally consistent.

Things that look right

• Middleware ordering: with_auth_and_rate_limit adds rate_limit first and unified_auth second; in tower/axum the last-added layer is outermost, so unified_auth runs first and publishes the extension before rate_limit reads it — matches the design and the debug_assert! guards the invariant in debug builds.
• Both auth modes publish the identity only after the credential is accepted (401 paths return before the insert).
• alternating_header_styles_share_one_bucket_in_simple_key_mode correctly pins down that Bearer and X-API-Key forms of the same credential share one bucket.
• The docs/CLI text was updated consistently with the behavioral change, and the "behaviour change worth noting" section is an honest description of the per-token limiting trade-off.

[cubesandboxbot]

This fallback masks a fragile layering invariant. The fix only works because, in with_auth_and_rate_limit (routes.rs:214), unified_auth is the outermost of the two layers (in axum the last .layer() added runs first), so the RateLimitIdentity extension is always populated by the time this runs. If those two .layer() calls are ever reordered, every request silently lands in this single shared "unauthenticated" bucket — re-introducing exactly the multi-tenant starvation this PR fixes — with no error, no log, and no test catching it. Current code is correct, but consider a comment on with_auth_and_rate_limit (or a debug assertion) documenting that unified_auth must run before rate_limit.

[cubesandboxbot]

This call won't compile against the pinned governor 0.6.3 (CubeAPI/Cargo.lock). In 0.6.x, KeyedRateLimiter::retain_recent is pub fn retain_recent(&self, up_to: Instant) -> usize — it takes an Instant argument, so limiter.retain_recent() is an arity error (E0061).

Also check the semantics once you add the argument: retain_recent(up_to) retains only keys whose last update is after up_to. Passing Instant::now() at each tick drops every key (every key's last update is before "now"), wiping the whole map — i.e. every client's bucket resets once a minute. That defeats the limiter for clients that are right at their burst boundary when the tick fires (they get a fresh full bucket). Pass a retention window instead, e.g.

let window = std::time::Instant::now() - std::time::Duration::from_secs(120);
limiter.retain_recent(window);

so recently-active keys (and the burst they've accrued) survive the sweep while abandoned keys are reclaimed.

[cubesandboxbot]

This embeds the configured secret itself into the rate-limiter key (configured-key:<secret>). Simple-key mode has exactly one valid credential, so a constant like "configured-key" is equivalent — the key value adds no information. As written, the plaintext API key is stored as a DashMap key for the process lifetime (the GC never removes this single key), so the secret lingers in the limiter's map in addition to config. A constant (or a hash of the key) keeps the identity stable without duplicating the secret in a second place.

[cubesandboxbot]

The callback-mode identity stores the raw credential as the bucket key. For Bearer tokens this is often a multi-KB JWT, so every distinct token becomes a large DashMap entry that lingers until the GC sweep — and raw credentials stay resident in memory past the request. Since the limiter only needs to distinguish identities (not read the token back), key on a fixed-size hash instead, e.g. format!("bearer:{}", sha256(token)) / format!("apikey:{}", sha256(key)). That preserves per-tenant separation while bounding per-key memory and not retaining plaintext secrets. (This is also the shape of the unbounded-growth attack the GC in state.rs is meant to bound — hashing makes each entry small regardless.)