feat(relay): add local auth-failure throttle#273
Conversation
Add a disabled-by-default, in-process, coarse throttle on repeated authentication failures from one client address (auth.failure_throttle). When enabled, an address that reaches max_failures failed auth attempts within window_seconds gets a stable 429 (auth.rate_limited, with Retry-After) before the auth provider runs, for every request from that address until the window rolls over; successful auth neither counts nor resets. Ingress rate limiting remains the primary control against high-volume abuse; this is a local backstop for deployments without a gateway in front of them, or defense in depth behind one. It does not throttle other expensive routes for authenticated callers, which is deliberately deferred. The throttle keys on the same trust-proxy-aware client address the audit record's remote_addr reports, extracted from audit/mod.rs into a shared crate::net::resolve_remote_addr so both subsystems agree on which address a request came from and untrusted X-Forwarded-For values cannot be used to evade the throttle or split it across fake keys. The map is bounded (capped entries, oldest-window eviction) so it cannot grow without bound under a flood of spoofed source addresses. Disabled by default: when auth.failure_throttle.enabled is false (the default), no throttle is constructed and the auth middleware's behavior is unchanged from before this feature. Part of #78 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ef8e150a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(e) => return Error::from(e).into_response(), | ||
| Err(e) => { | ||
| if let (Some(throttle), Some(key)) = (&state.throttle, &throttle_key) { | ||
| throttle.record_failure(key); |
There was a problem hiding this comment.
This records every provider error as a per-client failure. In OIDC deployments, AuthError::JwksUnavailable is the IdP/JWKS outage path that returns 503, not a bad credential; with the throttle enabled, max_failures requests during an outage poison that address and subsequent valid tokens get 429 until the window expires even after JWKS recovers. Please skip non-client-caused provider errors when incrementing the throttle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Fixed in 775b6b8: AuthError::JwksUnavailable and already-rate-limited requests no longer increment the client failure throttle. Added an OIDC/JWKS outage flow test showing a recovered valid token is not poisoned by the outage.
| let failure_throttle = | ||
| crate::auth::failure_throttle::AuthFailureThrottle::new(&config.auth.failure_throttle) | ||
| .map(Arc::new); |
There was a problem hiding this comment.
Because this AuthFailureThrottle is captured when the router is built, it is not refreshed by governed config apply. I checked classify_live_config_change: a signed client_access_change/client_credential_rotation can be accepted when auth.api_keys changes, and equivalent_except_auth ignores the rest of the auth block, so a bundle that also toggles auth.failure_throttle updates the active config snapshot while requests keep using the old throttle state until restart. Please either reject throttle edits on live auth applies or move the throttle into swappable runtime state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Fixed in 775b6b8: live auth applies now require the candidate auth.failure_throttle config to match the active one, so throttle edits are restart-required instead of silently updating config state while the router keeps the old throttle. Added api_key_auth_changed_requires_same_failure_throttle_config.
| // instead of taking down the auth path. | ||
| poisoned.into_inner() | ||
| }); | ||
| prune_expired(&mut counters, now, self.window); |
There was a problem hiding this comment.
With the throttle enabled, every protected request calls check() before authentication, and this line scans/retains the entire counter map under the single mutex. Once a burst of distinct client addresses fills the bounded map, even valid requests from unrelated clients pay an O(10k) scan and serialize behind the mutex, so the backstop can become a CPU/latency amplifier under exactly the spoofed/distributed-address scenario it is meant to survive. Please prune opportunistically or on a schedule instead of on each check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Fixed in 775b6b8: throttle checks remove only the current expired key on the hot path, and full-map pruning happens only under capacity pressure before oldest-entry eviction. Added focused failure-throttle coverage; reviewer reran the eviction test.
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Behind a proxy or load balancer with server.trust_proxy left disabled (the default), the throttle keys on the proxy's own socket address, so every client shares one bucket and combined failures from anyone can 429 all clients until the window rolls. Emit a config.validation_warning finding at boot so operators catch the misconfiguration instead of discovering it as a self-inflicted outage. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0ddd69f01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| return Err(ConfigError::ValidationError); | ||
| } | ||
| if !config.server.trust_proxy.enabled { |
There was a problem hiding this comment.
When auth.failure_throttle.enabled is true and an operator sets server.trust_proxy.enabled: true but leaves trusted_proxies empty, this warning is skipped even though runtime address resolution still falls back to the proxy peer: resolve_remote_addr only honors X-Forwarded-For when trusted_proxy_contains(peer, trusted_proxies) matches, and an empty list never matches. In proxied deployments that means all clients still share one throttle bucket and can 429 each other after combined failures, the same scenario this validation warning is meant to catch.
Useful? React with 👍 / 👎.
| /// `auth.failure_throttle.enabled` is true. `retry_after_seconds` is | ||
| /// surfaced as the `Retry-After` response header. | ||
| #[error("authentication rate limited")] | ||
| RateLimited { retry_after_seconds: u64 }, |
There was a problem hiding this comment.
Adding auth.rate_limited makes protected Relay operations return 429 when the throttle is enabled, but the OpenAPI generator and committed crates/registry-relay/openapi/registry-relay.openapi.json still advertise no 429 responses (I checked the generated artifact: 57 operations have 401 and 0 have 429). Clients generated from the contract will not know to handle this new Problem Details response, so the generator and regenerated artifact should include the rate-limit response on protected operations.
Useful? React with 👍 / 👎.
|
Superseded by the integration meta-PR #292 — this branch is merged there unchanged (with cross-PR conflicts resolved once). Branch retained. |
1 participant
Summary
Adds a local, in-process, coarse throttle on repeated authentication
failures from one client address to Registry Relay (
auth.failure_throttle).Disabled by default. When enabled: once a client address accumulates
max_failuresauthentication failures withinwindow_seconds, furtherrequests from that address get a stable
429/auth.rate_limitedresponse (with a
Retry-Afterheader) before the auth provider runs,until the window rolls over. Successful authentication neither counts
toward the limit nor resets it.
Part of #78.
Design rationale
AuthFailureThrottle::newreturnsNoneunlessauth.failure_throttle.enabledistrue, and every branch of the middleware that touches the throttle is gated onOption::is_some(). Local/demo configs that don't opt in see no behavior change.remote_addrfield reports. That resolution logic (X-Forwarded-Forwalk againstserver.trust_proxy.trusted_proxies) was extracted fromaudit/mod.rsinto a newcrate::net::resolve_remote_addr, used by both subsystems, so the two agree on "who is this request from" and an untrusted proxy hop can't be used to spoof a key.Mutex<HashMap<String, Counter>>capped at 10,000 tracked addresses, evicting the oldest window on overflow, so a flood of spoofed source addresses can't grow it without bound.deployment.evidence.ingress_rate_limitgate andrelay.ingress.rate_limit_missingfinding are unchanged. Expensive-route throttling for authenticated callers is explicitly out of scope and deferred.Review notes (security-sensitive: authentication path + audit)
AuthError::RateLimitedvariant maps to a stable429/auth.rate_limited, added to the error taxonomy table anderror_taxonomy.rs's exhaustive variant/status snapshot tests.remote_addr; an untrustedX-Forwarded-Forvalue is ignored in favor of the actual socket peer, exactly as it is for audit. This means an attacker behind an untrusted hop can't pick an arbitrary throttle key to frame another address, and (per the design) can't evade the throttle by rotating a forwarded-for value through a trusted proxy either, since the resolved client is what's keyed on, not the raw header.error_code: auth.rate_limited/status_code: 429, so a sustained lockout is visible in the audit stream, not silent.auth::middleware::remote_addr()'s doc comment claims upstream tower-http layers resolve trust-proxy addressing before this function runs, which isn't accurate (that resolution actually happens in the newcrate::net::resolve_remote_addr, used separately by the throttle and by audit;remote_addr()itself is unrelated, feeding the auth provider'sremote_addrargument, not throttle/audit keying). Left as-is per scope discipline; flagging here rather than fixing inline since it's unrelated to this change.server.rs; flagging in case reviewers want a single shared instance instead.Verification
cargo fmt --check— cleancargo check --locked -p registry-relay --all-targets— cleancargo clippy --locked -p registry-relay --all-targets -- -D warnings— cleancargo test --locked -p registry-relay— all green (311 tests in the lib crate's own suite plus integration test binaries; no failures)just openapi-contract(fromcrates/registry-relay) — exits 0, no diff. The newauth.rate_limited(429) is a cross-cutting auth-middleware error, like the pre-existingauth.jwks_unavailable(503) from the same choke point, and per existing precedent those are not enumerated per-route in the committed OpenAPI document, so no regeneration was needed.New tests
auth::failure_throttle), including window expiry, per-address isolation, and the bounded-map eviction behavior under a flood of distinct addresses.tests/auth_flow.rs: disabled config never trips 429; failures under the limit pass through as 401 and the failure that reaches the limit returns 429 withauth.rate_limitedand a numericRetry-After; an over-limit address is throttled even with a valid credential; a different address is unaffected; successful auth doesn't count toward the throttle.tests/audit_record.rs: the throttle keys on the resolvedX-Forwarded-Forclient rather than the proxy peer; the audit record captureserror_code: auth.rate_limited/status_code: 429for the throttled short-circuit.config/validate.rsmirroring the existingBreakGlassRateLimitvalidation pattern.