diff --git a/docs/configuration.md b/docs/configuration.md index 8359b00..ef51140 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -57,7 +57,7 @@ CIDR ranges controlling which hosts sessions can connect to. All default to loca | Key | Default | Description | |-----|---------|-------------| -| `trusted_proxies` | `[]` | CIDRs of reverse proxies whose X-Forwarded-For to trust | +| `trusted_proxies` | `[]` | CIDRs of reverse proxies whose X-Forwarded-For to trust (read right to left, skipping trusted hops) | | `rate_limit` | `false` | Enable API rate limiting. Not needed when behind a rate-limiting reverse proxy. | | `session_history_retention_days` | `90` | Days to keep session history in the database. 0 = keep forever. | diff --git a/docs/reverse-proxies.md b/docs/reverse-proxies.md index 4aea219..5fc65aa 100644 --- a/docs/reverse-proxies.md +++ b/docs/reverse-proxies.md @@ -60,7 +60,9 @@ server { proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + # Overwrite rather than append ($proxy_add_x_forwarded_for), so a + # client cannot pre-load the header with an address of its choosing. + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; # WebSocket support for session streams. @@ -203,7 +205,12 @@ Whichever proxy you use, set `trusted_proxies` in `config.toml` to match the source IP your proxy connects from (usually `127.0.0.1/32` on same-host deployments). rustguac then honours `X-Forwarded-For` from that source, so client IPs appear correctly in audit logs, session history, -and rate-limit decisions. +IP allowlists and rate-limit decisions. + +rustguac reads `X-Forwarded-For` from the right: the client is the +rightmost address that is not itself a trusted proxy, so entries a client +forges on the left are ignored. List every proxy in the chain if requests +pass through more than one. ```toml trusted_proxies = ["127.0.0.1/32"] diff --git a/docs/security.md b/docs/security.md index f3db60a..b143f4b 100644 --- a/docs/security.md +++ b/docs/security.md @@ -117,7 +117,7 @@ When a user API token authenticates, the effective role is computed as `min(user |--------|-----------| | Token theft / leakage | `rgu_` prefix enables automated secret scanning; short token lifetime recommended; tokens can be revoked immediately | | Privilege escalation via token | effective role is always `min(user_role, max_role)` — demoting the user restricts all their tokens | -| Brute-force token guessing | 240 bits of entropy (60 hex chars); rate limiting at 2 req/sec per IP | +| Brute-force token guessing | 240 bits of entropy (60 hex chars), which makes guessing infeasible regardless of rate limiting | | Token abuse after user offboarding | user deletion cascade-deletes all tokens; disabling a user blocks all their tokens | | Lateral movement from stolen token | tokens inherit the user's identity — all actions are logged with the user's email and client IP | | Audit evasion | all token create/revoke/use events are logged in `token_audit_log` with IP addresses | @@ -133,15 +133,17 @@ Audit logs are retained for 90 days and cleaned up hourly. Admins can view the l ## Rate limiting -Per-IP rate limiting is applied to all endpoints using `tower_governor`: +The OIDC login and callback routes are always rate limited, at 1 request per second per client IP with a burst of 20. + +Setting `rate_limit = true` adds per-IP limits to the rest of the application. It is off by default, since deployments behind a reverse proxy usually limit there: | Endpoint group | Rate | Burst | |---------------|------|-------| -| API routes | 2/sec | 10 | -| Session creation | 1/sec | 5 | -| WebSocket connections | 2/sec | 20 | +| API routes (including requests that fail authentication) | 20/sec | 100 | +| Session creation | 2/sec | 10 | +| WebSocket connections | 5/sec | 50 | -Rate limiting uses the resolved client IP (honoring `trusted_proxies` for X-Forwarded-For). +Every limiter keys on the resolved client IP. Forwarded headers are honoured only from `trusted_proxies`, so a client cannot get a fresh allowance by sending a different `X-Forwarded-For` on each request. Behind a proxy, set `trusted_proxies`, or every user shares the proxy's allowance. ## Security headers @@ -255,7 +257,9 @@ HTTP request bodies are limited to 64KB to prevent memory exhaustion attacks. ## Trusted proxy support -When `trusted_proxies` is configured, rustguac extracts the real client IP from the `X-Forwarded-For` header for connections originating from trusted proxy CIDRs. This ensures correct IP-based rate limiting and audit logging behind reverse proxies. +When `trusted_proxies` is configured, rustguac extracts the real client IP from the `X-Forwarded-For` header for connections originating from trusted proxy CIDRs. This ensures correct IP allowlists, rate limiting and audit logging behind reverse proxies. + +The header is read from the right, skipping addresses that are themselves trusted proxies. The leftmost entry is never trusted on its own, because a proxy that appends to an incoming header passes along whatever the client put there. ```toml trusted_proxies = ["127.0.0.1/32"] diff --git a/src/auth.rs b/src/auth.rs index 440c63b..9d3d317 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -135,24 +135,112 @@ pub fn compute_effective_role(user_role: &str, max_role: &Option) -> Str /// Extract the real client IP, honouring X-Forwarded-For when the socket /// address belongs to a trusted proxy CIDR. pub fn client_ip(headers: &HeaderMap, socket_addr: IpAddr, trusted_proxies: &[String]) -> IpAddr { - if !trusted_proxies.is_empty() { - let networks: Vec = trusted_proxies - .iter() - .filter_map(|s| s.parse::().ok()) - .collect(); + client_ip_from( + headers, + socket_addr, + &parse_trusted_proxies(trusted_proxies), + ) +} - if networks.iter().any(|net| net.contains(socket_addr)) { - if let Some(xff) = headers.get("x-forwarded-for").and_then(|v| v.to_str().ok()) { - // First IP in X-Forwarded-For is the original client - if let Some(first) = xff.split(',').next() { - if let Ok(ip) = first.trim().parse::() { - return ip; - } - } - } +/// Parse `trusted_proxies` CIDRs, dropping entries that do not parse +/// (startup warns about those). +pub fn parse_trusted_proxies(trusted_proxies: &[String]) -> Vec { + trusted_proxies + .iter() + .filter_map(|s| s.parse::().ok()) + .collect() +} + +/// Resolve the client address behind a chain of trusted proxies. +/// +/// X-Forwarded-For is walked from the RIGHT. Each proxy appends the address +/// it received the request from, so the right-hand entries are the only ones +/// written by infrastructure we trust; anything to their left came from the +/// client and may be forged. The client is the first entry, reading +/// leftwards, that is not itself a trusted proxy. Taking the leftmost entry +/// instead would let anyone choose their own address (and so walk through an +/// IP allowlist) behind any proxy that appends to an incoming header, which +/// includes nginx's `$proxy_add_x_forwarded_for` and Apache's mod_proxy. +/// +/// Every X-Forwarded-For header line is read, in order, since some proxies +/// add a new line rather than appending to the existing one. Entries are +/// split on raw bytes so a non-UTF-8 value smuggled in on the left cannot +/// hide the valid address a proxy appended after it. +pub fn client_ip_from(headers: &HeaderMap, socket_addr: IpAddr, trusted: &[IpNetwork]) -> IpAddr { + let is_trusted = |ip: IpAddr| trusted.iter().any(|net| net.contains(ip)); + if !is_trusted(socket_addr) { + return socket_addr; + } + let hops: Vec<&[u8]> = headers + .get_all("x-forwarded-for") + .iter() + .flat_map(|v| v.as_bytes().split(|&b| b == b',')) + .collect(); + let mut client = socket_addr; + for hop in hops.iter().rev() { + // A malformed entry ends the walk: nothing to its left can be + // attributed to a trusted proxy. + let Some(ip) = parse_forwarded_ip(hop) else { + break; + }; + client = ip; + if !is_trusted(ip) { + break; } } - socket_addr + client +} + +/// Rate-limit key: the client address as resolved by [`client_ip_from`]. +/// +/// Replaces tower_governor's `SmartIpKeyExtractor`, which believes +/// X-Forwarded-For (and Forwarded / X-Real-IP) from ANY peer. With that, +/// a client sending a different forged address per request got a fresh +/// bucket every time, so no limiter, including the always-on OIDC one, +/// ever engaged. Forwarded headers are only honoured from `trusted_proxies` +/// here, exactly as for IP allowlists and audit logs. +#[derive(Clone)] +pub struct ClientIpKeyExtractor { + trusted: Arc>, +} + +impl ClientIpKeyExtractor { + pub fn new(trusted_proxies: &[String]) -> Self { + Self { + trusted: Arc::new(parse_trusted_proxies(trusted_proxies)), + } + } +} + +impl tower_governor::key_extractor::KeyExtractor for ClientIpKeyExtractor { + type Key = IpAddr; + + fn extract( + &self, + req: &axum::http::Request, + ) -> Result { + let peer = req + .extensions() + .get::>() + .map(|ConnectInfo(addr)| addr.ip()) + .ok_or(tower_governor::GovernorError::UnableToExtractKey)?; + Ok(client_ip_from(req.headers(), peer, &self.trusted)) + } +} + +/// Parse one X-Forwarded-For entry: a bare address, or one with a port +/// (`203.0.113.5:4711`, `[2001:db8::1]:4711`) as some proxies write it. +fn parse_forwarded_ip(raw: &[u8]) -> Option { + let s = std::str::from_utf8(raw).ok()?.trim(); + if let Ok(ip) = s.parse::() { + return Some(ip); + } + if let Ok(sa) = s.parse::() { + return Some(sa.ip()); + } + s.strip_prefix('[') + .and_then(|r| r.strip_suffix(']')) + .and_then(|r| r.parse::().ok()) } /// Extract session cookie value from the Cookie header. @@ -548,6 +636,83 @@ mod tests { assert_eq!(ip.to_string(), "192.168.1.1"); } + fn xff(values: &[&str]) -> HeaderMap { + let mut headers = HeaderMap::new(); + for v in values { + headers.append("x-forwarded-for", v.parse().unwrap()); + } + headers + } + + /// H2: behind a proxy that appends (nginx `$proxy_add_x_forwarded_for`), + /// a client-supplied left entry must not be believed. + #[test] + fn test_client_ip_ignores_forged_left_entry() { + let proxies = vec!["10.0.0.0/8".into()]; + // Client 198.51.100.7 sent "X-Forwarded-For: 192.168.1.10"; nginx + // appended the real peer. + let headers = xff(&["192.168.1.10, 198.51.100.7"]); + let ip = client_ip(&headers, "10.0.0.1".parse().unwrap(), &proxies); + assert_eq!(ip.to_string(), "198.51.100.7"); + } + + #[test] + fn test_client_ip_walks_through_proxy_chain() { + let proxies = vec!["10.0.0.0/8".into()]; + let headers = xff(&["1.1.1.1, 203.0.113.50, 10.0.0.9, 10.0.0.8"]); + let ip = client_ip(&headers, "10.0.0.1".parse().unwrap(), &proxies); + assert_eq!(ip.to_string(), "203.0.113.50"); + } + + #[test] + fn test_client_ip_reads_every_header_line() { + // A proxy that adds its own line rather than appending. + let proxies = vec!["10.0.0.0/8".into()]; + let headers = xff(&["192.168.1.10", "198.51.100.7"]); + let ip = client_ip(&headers, "10.0.0.1".parse().unwrap(), &proxies); + assert_eq!(ip.to_string(), "198.51.100.7"); + } + + #[test] + fn test_client_ip_non_utf8_left_entry_does_not_hide_peer() { + let proxies = vec!["10.0.0.0/8".into()]; + let mut headers = HeaderMap::new(); + headers.insert( + "x-forwarded-for", + axum::http::HeaderValue::from_bytes(b"\xff\xfe, 198.51.100.7").unwrap(), + ); + let ip = client_ip(&headers, "10.0.0.1".parse().unwrap(), &proxies); + assert_eq!(ip.to_string(), "198.51.100.7"); + } + + #[test] + fn test_client_ip_entry_with_port() { + let proxies = vec!["10.0.0.0/8".into()]; + let ip = client_ip( + &xff(&["198.51.100.7:4711"]), + "10.0.0.1".parse().unwrap(), + &proxies, + ); + assert_eq!(ip.to_string(), "198.51.100.7"); + let ip = client_ip( + &xff(&["[2001:db8::1]:4711"]), + "10.0.0.1".parse().unwrap(), + &proxies, + ); + assert_eq!(ip.to_string(), "2001:db8::1"); + } + + #[test] + fn test_client_ip_all_hops_trusted_returns_leftmost() { + let proxies = vec!["10.0.0.0/8".into()]; + let ip = client_ip( + &xff(&["10.1.2.3, 10.0.0.9"]), + "10.0.0.1".parse().unwrap(), + &proxies, + ); + assert_eq!(ip.to_string(), "10.1.2.3"); + } + #[test] fn test_client_ip_xff_invalid_ip() { let mut headers = HeaderMap::new(); diff --git a/src/config.rs b/src/config.rs index 5385028..ba89df9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -418,7 +418,8 @@ pub struct Config { pub rate_limit: bool, /// Trusted proxy CIDRs. When the connecting IP matches one of these, - /// the first address in X-Forwarded-For is used as the real client IP. + /// X-Forwarded-For is read from the right and the first address that + /// is not itself a trusted proxy is used as the real client IP. #[serde(default)] pub trusted_proxies: Vec, diff --git a/src/main.rs b/src/main.rs index ca5832f..d9e57ab 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,9 +32,7 @@ use axum::{middleware, Extension, Router}; use clap::{Parser, Subcommand}; use std::net::SocketAddr; use std::sync::Arc; -use tower_governor::{ - governor::GovernorConfigBuilder, key_extractor::SmartIpKeyExtractor, GovernorLayer, -}; +use tower_governor::{governor::GovernorConfigBuilder, GovernorLayer}; use tower_http::services::ServeDir; use tracing_subscriber::EnvFilter; @@ -963,6 +961,7 @@ async fn run_server(config: Config, database: Db) { // Build TLS connector for guacd if configured let guacd_tls = build_guacd_tls(&config); let rate_limit_enabled = config.rate_limit; + let trusted_proxy_cidrs = config.trusted_proxies.clone(); // Create session manager let manager: AppState = Arc::new(SessionManager::new_with_db( @@ -1109,6 +1108,14 @@ async fn run_server(config: Config, database: Db) { if rate_limit_enabled { tracing::info!("API rate limiting enabled"); } + // Every limiter keys on the client address resolved through + // trusted_proxies, never on forwarded headers from arbitrary peers. + let rate_key = auth::ClientIpKeyExtractor::new(&trusted_proxy_cidrs); + for cidr in &trusted_proxy_cidrs { + if cidr.parse::().is_err() { + tracing::warn!(entry = %cidr, "trusted_proxies entry is not a valid IP or CIDR and is ignored"); + } + } // WebSocket ticket store (single-use tokens to keep API keys out of WS URLs) let ws_ticket_store = auth::WsTicketStore::new(); @@ -1121,7 +1128,7 @@ async fn run_server(config: Config, database: Db) { let conf = GovernorConfigBuilder::default() .per_second(2) .burst_size(10) - .key_extractor(SmartIpKeyExtractor) + .key_extractor(rate_key.clone()) .finish() .expect("Failed to build session creation rate limit config"); session_create_route = session_create_route.layer(GovernorLayer::new(conf)); @@ -1246,41 +1253,43 @@ async fn run_server(config: Config, database: Db) { ) .route("/api/ssh/probe-host-key", post(api::ssh_probe_host_key)) .merge(session_create_route) - .with_state(manager.clone()); - if rate_limit_enabled { - let conf = GovernorConfigBuilder::default() - .per_second(20) - .burst_size(100) - .key_extractor(SmartIpKeyExtractor) - .finish() - .expect("Failed to build API rate limit config"); - api_routes = api_routes.layer(GovernorLayer::new(conf)); - } - let api_routes = api_routes + .with_state(manager.clone()) .layer(middleware::from_fn(auth::require_auth)) .layer(Extension(ws_ticket_store.clone())) .layer(Extension(vault_client.clone())) .layer(Extension(vault_configured.clone())) .layer(Extension(credential_default_scope.clone())) .layer(Extension(database.clone())); + // Applied OUTSIDE require_auth so requests with bad credentials are + // counted too: a flood of bogus keys is throttled before each one + // queues on the database lock to be looked up. + if rate_limit_enabled { + let conf = GovernorConfigBuilder::default() + .per_second(20) + .burst_size(100) + .key_extractor(rate_key.clone()) + .finish() + .expect("Failed to build API rate limit config"); + api_routes = api_routes.layer(GovernorLayer::new(conf)); + } // WebSocket route with optional auth let mut ws_route = Router::new() .route("/ws/{session_id}", get(websocket::ws_handler)) - .with_state(manager.clone()); + .with_state(manager.clone()) + .layer(middleware::from_fn(auth::optional_auth)) + .layer(Extension(ws_ticket_store.clone())) + .layer(Extension(database.clone())); + // Outside optional_auth, as for the API routes above. if rate_limit_enabled { let conf = GovernorConfigBuilder::default() .per_second(5) .burst_size(50) - .key_extractor(SmartIpKeyExtractor) + .key_extractor(rate_key.clone()) .finish() .expect("Failed to build WebSocket rate limit config"); ws_route = ws_route.layer(GovernorLayer::new(conf)); } - let ws_route = ws_route - .layer(middleware::from_fn(auth::optional_auth)) - .layer(Extension(ws_ticket_store.clone())) - .layer(Extension(database.clone())); // Quick-connect route with optional auth (handles its own redirect-to-login) let connect_route = Router::new() @@ -1328,10 +1337,12 @@ async fn run_server(config: Config, database: Db) { // Add OIDC routes if configured (always rate-limited to prevent brute-force) if let Some(ref oidc_st) = oidc_state { + // Burst sized for an office of users behind one NAT address + // logging in at once (each login is two requests). let auth_rate_conf = GovernorConfigBuilder::default() .per_second(1) - .burst_size(5) - .key_extractor(SmartIpKeyExtractor) + .burst_size(20) + .key_extractor(rate_key.clone()) .finish() .expect("Failed to build auth rate limit config"); let oidc_routes = Router::new() @@ -1586,45 +1597,93 @@ mod tests { // refactor that drops `.layer(GovernorLayer::new(...))` from a route // group without noticing. - #[tokio::test] - async fn rate_limit_layer_returns_429_on_burst() { - use axum::{body::Body, http::Request, routing::get, Router}; - use tower::ServiceExt; - - // Same settings family as the OIDC auth-rate-limit path in main.rs. + fn probe_app(burst: u32) -> axum::Router { + use axum::routing::get; let conf = GovernorConfigBuilder::default() .per_second(1) - .burst_size(3) - .key_extractor(SmartIpKeyExtractor) + .burst_size(burst) + .key_extractor(auth::ClientIpKeyExtractor::new(&["10.0.0.0/8".into()])) .finish() .expect("governor config"); - - let app: Router = Router::new() + axum::Router::new() .route("/probe", get(|| async { axum::http::StatusCode::OK })) - .layer(GovernorLayer::new(conf)); + .layer(GovernorLayer::new(conf)) + } - // Fire 10 requests back-to-back from the same source IP. With - // burst=3 per_second=1, at least one must be throttled (429). - // SmartIpKeyExtractor honours X-Forwarded-For when present. + fn probe_req(peer: &str, xff: Option<&str>) -> axum::http::Request { + let mut b = axum::http::Request::builder().uri("/probe"); + if let Some(x) = xff { + b = b.header("x-forwarded-for", x); + } + let mut req = b.body(axum::body::Body::empty()).unwrap(); + let addr: std::net::SocketAddr = format!("{peer}:40000").parse().unwrap(); + req.extensions_mut() + .insert(axum::extract::ConnectInfo(addr)); + req + } + + #[tokio::test] + async fn rate_limit_layer_returns_429_on_burst() { + use tower::ServiceExt; + let app = probe_app(3); let mut saw_ok = false; let mut saw_429 = false; for _ in 0..10 { - let req = Request::builder() - .uri("/probe") - .header("x-forwarded-for", "203.0.113.99") - .body(Body::empty()) + let resp = app + .clone() + .oneshot(probe_req("203.0.113.99", None)) + .await .unwrap(); - let resp = app.clone().oneshot(req).await.unwrap(); match resp.status().as_u16() { 200 => saw_ok = true, 429 => saw_429 = true, - other => panic!("unexpected status {other} — governor misconfigured"), + other => panic!("unexpected status {other}: governor misconfigured"), } } - assert!(saw_ok, "no requests succeeded — layer misconfigured"); + assert!(saw_ok, "no requests succeeded: layer misconfigured"); assert!( saw_429, - "no requests were throttled — rate-limit not applied" + "no requests were throttled: rate-limit not applied" ); } + + /// H1: a client that forges a different X-Forwarded-For on every + /// request must still share one bucket when it is not a trusted proxy. + #[tokio::test] + async fn rate_limit_ignores_forged_xff_from_untrusted_peer() { + use tower::ServiceExt; + let app = probe_app(3); + let mut throttled = 0; + for i in 0..10 { + let forged = format!("198.51.100.{i}"); + let resp = app + .clone() + .oneshot(probe_req("203.0.113.99", Some(&forged))) + .await + .unwrap(); + if resp.status() == axum::http::StatusCode::TOO_MANY_REQUESTS { + throttled += 1; + } + } + assert!( + throttled >= 6, + "forged XFF escaped the limiter ({throttled} throttled)" + ); + } + + /// Behind a trusted proxy, clients get their own buckets. + #[tokio::test] + async fn rate_limit_keys_on_client_behind_trusted_proxy() { + use tower::ServiceExt; + let app = probe_app(3); + for i in 0..10 { + let client = format!("198.51.100.{i}"); + let resp = app + .clone() + .oneshot(probe_req("10.0.0.1", Some(&client))) + .await + .unwrap(); + assert_eq!(resp.status(), axum::http::StatusCode::OK); + } + } }