diff --git a/crates/manager-core/src/auth_api.rs b/crates/manager-core/src/auth_api.rs index ffb7963..b6a4bc8 100644 --- a/crates/manager-core/src/auth_api.rs +++ b/crates/manager-core/src/auth_api.rs @@ -83,8 +83,9 @@ async fn login( ) -> Response { // Audit 2026-06-11 H-B1 — rate limit BEFORE any DB read or // Argon2 verify so an attacker can't even queue. Keying is - // (X-Forwarded-For first hop, username) + (username), so a single - // hot user and a credential-stuffing flurry both saturate. + // (real client IP, username) + (username), so a single hot user and + // a credential-stuffing flurry both saturate. The IP is the last + // X-Forwarded-For hop (Caddy-appended); see `extract_remote_ip`. let remote_ip = extract_remote_ip(&headers); if let LoginRateOutcome::Denied { retry_after } = state .login_rate_limiter @@ -245,16 +246,27 @@ fn extract_token_for_logout(req: &Request) -> Option { Some(token.to_string()) } -/// Pull the originating client IP from the request. Caddy is the only -/// trusted proxy in front of picloud; X-Forwarded-For carries the -/// client. We take the first entry (closest to the original client). -/// Returns `"unknown"` if the header is absent/malformed so the -/// per-user bucket still gates without an IP qualifier. +/// Pull the originating client IP from the request for rate-limiting. +/// +/// Audit 2026-06-11 (H-B1 follow-up) — Caddy is the single trusted +/// proxy in front of picloud and *appends* the immediate peer to +/// `X-Forwarded-For`, so the **last** entry is the address Caddy +/// actually saw. Every earlier entry is client-supplied: an attacker +/// can send `X-Forwarded-For: ` and Caddy forwards +/// `, `. Taking the first entry (the previous +/// behavior) therefore let an attacker rotate the per-IP bucket key on +/// every request and evade the per-IP limit. Take the last entry so the +/// qualifier reflects the real client. Returns `"unknown"` when the +/// header is absent/malformed so the per-username bucket still gates. +/// +/// (This assumes exactly one trusted proxy hop, which is the documented +/// single-node topology. A multi-proxy deployment would need a +/// configured trusted-hop count; tracked for the cluster work.) fn extract_remote_ip(headers: &HeaderMap) -> String { if let Some(xff) = headers.get("x-forwarded-for") { if let Ok(s) = xff.to_str() { - if let Some(first) = s.split(',').next() { - let trimmed = first.trim(); + if let Some(last) = s.split(',').next_back() { + let trimmed = last.trim(); if !trimmed.is_empty() { return trimmed.to_string(); } @@ -298,3 +310,42 @@ fn internal_error() -> Response { ) .into_response() } + +#[cfg(test)] +mod tests { + use super::extract_remote_ip; + use axum::http::{HeaderMap, HeaderValue}; + + fn xff(value: &str) -> HeaderMap { + let mut h = HeaderMap::new(); + h.insert("x-forwarded-for", HeaderValue::from_str(value).unwrap()); + h + } + + #[test] + fn takes_last_xff_entry_caddy_appended_peer() { + // Audit 2026-06-11 (H-B1 follow-up) — the real client is the + // last hop Caddy appended; earlier entries are client-spoofable. + assert_eq!(extract_remote_ip(&xff("203.0.113.9")), "203.0.113.9"); + assert_eq!( + extract_remote_ip(&xff("9.9.9.9, 203.0.113.9")), + "203.0.113.9" + ); + } + + #[test] + fn spoofed_leading_entry_does_not_change_the_key() { + // Attacker prepends junk; Caddy still appends the real peer last, + // so the rate-limit key is stable regardless of the spoof. + let a = extract_remote_ip(&xff("evil-1, 203.0.113.9")); + let b = extract_remote_ip(&xff("evil-2, 203.0.113.9")); + assert_eq!(a, b); + assert_eq!(a, "203.0.113.9"); + } + + #[test] + fn missing_or_empty_header_is_unknown() { + assert_eq!(extract_remote_ip(&HeaderMap::new()), "unknown"); + assert_eq!(extract_remote_ip(&xff(" ")), "unknown"); + } +}