From 9020b8526c4ac3027acf92cdaa3d179ad08f79f2 Mon Sep 17 00:00:00 2001 From: archipelago Date: Sat, 4 Jul 2026 15:48:07 -0400 Subject: [PATCH] fix(security): stop trusting client-supplied forwarded headers in rate limiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extract_client_ip took X-Real-IP/X-Forwarded-For from any request, so a client talking to the backend directly (the FIPS peer listener, or any non-proxy path) could rotate a fake IP per request and never trip the login rate limiter. The accept loop now records the TCP peer address in request extensions, and forwarded headers are honored only when the connection itself is from loopback — where nginx overwrites X-Real-IP with the real client address. Direct connections bucket under their socket IP. §C of the 1.8.0 hardening plan; 3 new unit tests cover the loopback/direct/no-header matrix. Co-Authored-By: Claude Fable 5 --- core/archipelago/src/api/rpc/middleware.rs | 84 +++++++++++++++++++++- core/archipelago/src/api/rpc/mod.rs | 7 +- core/archipelago/src/server.rs | 6 +- docs/1.8.0-RELEASE-HARDENING-PLAN.md | 21 ++++-- 4 files changed, 106 insertions(+), 12 deletions(-) diff --git a/core/archipelago/src/api/rpc/middleware.rs b/core/archipelago/src/api/rpc/middleware.rs index 0c9bd6ae..da078543 100644 --- a/core/archipelago/src/api/rpc/middleware.rs +++ b/core/archipelago/src/api/rpc/middleware.rs @@ -179,13 +179,91 @@ pub(super) fn extract_cookie(headers: &hyper::HeaderMap, name: &str) -> Option IpAddr { +/// The TCP peer address of the connection a request arrived on, injected +/// into request extensions by the server accept loop. +#[derive(Debug, Clone, Copy)] +pub struct PeerAddr(pub std::net::SocketAddr); + +/// Extract the client IP for rate limiting. +/// +/// `X-Real-IP`/`X-Forwarded-For` are only honored when the connection +/// itself comes from loopback — i.e. from our local nginx, which sets +/// `X-Real-IP $remote_addr`. On a direct connection (the FIPS peer +/// listener, or anything that isn't the local proxy) the headers are +/// client-supplied, so trusting them let an attacker rotate per-request +/// "IPs" and defeat the login rate limiter; there we use the socket +/// address instead. +pub(super) fn extract_client_ip(parts: &hyper::http::request::Parts) -> IpAddr { + let socket_ip = parts.extensions.get::().map(|p| p.0.ip()); + match socket_ip { + Some(ip) if ip.is_loopback() => forwarded_client_ip(&parts.headers).unwrap_or(ip), + Some(ip) => ip, + // No socket info recorded (shouldn't happen in the server path); + // fall back to the pre-extension behavior. + None => forwarded_client_ip(&parts.headers) + .unwrap_or(IpAddr::V4(std::net::Ipv4Addr::LOCALHOST)), + } +} + +/// The proxy-reported client IP, if a forwarded header carries one. +fn forwarded_client_ip(headers: &hyper::HeaderMap) -> Option { headers .get("x-real-ip") .or_else(|| headers.get("x-forwarded-for")) .and_then(|v| v.to_str().ok()) .and_then(|s| s.split(',').next()) .and_then(|s| s.trim().parse::().ok()) - .unwrap_or(IpAddr::V4(std::net::Ipv4Addr::LOCALHOST)) +} + +#[cfg(test)] +mod client_ip_tests { + use super::*; + use std::net::SocketAddr; + + fn parts_with( + peer: Option<&str>, + real_ip: Option<&str>, + ) -> hyper::http::request::Parts { + let mut builder = hyper::Request::builder().uri("/rpc/v1"); + if let Some(ip) = real_ip { + builder = builder.header("x-real-ip", ip); + } + let (mut parts, _) = builder.body(()).unwrap().into_parts(); + if let Some(addr) = peer { + parts + .extensions + .insert(PeerAddr(addr.parse::().unwrap())); + } + parts + } + + #[test] + fn loopback_connection_trusts_forwarded_header() { + // nginx on loopback forwards the real client IP — use it. + let parts = parts_with(Some("127.0.0.1:44412"), Some("192.168.1.50")); + assert_eq!( + extract_client_ip(&parts), + "192.168.1.50".parse::().unwrap() + ); + } + + #[test] + fn direct_connection_ignores_spoofed_header() { + // A direct (non-proxy) client rotating X-Real-IP per request must + // still bucket under its socket address. + let parts = parts_with(Some("203.0.113.9:9999"), Some("10.0.0.1")); + assert_eq!( + extract_client_ip(&parts), + "203.0.113.9".parse::().unwrap() + ); + } + + #[test] + fn loopback_connection_without_header_uses_socket_ip() { + let parts = parts_with(Some("127.0.0.1:5000"), None); + assert_eq!( + extract_client_ip(&parts), + "127.0.0.1".parse::().unwrap() + ); + } } diff --git a/core/archipelago/src/api/rpc/mod.rs b/core/archipelago/src/api/rpc/mod.rs index e474cbe6..2dc3dd39 100644 --- a/core/archipelago/src/api/rpc/mod.rs +++ b/core/archipelago/src/api/rpc/mod.rs @@ -58,6 +58,7 @@ use middleware::{ derive_csrf_token, extract_client_ip, extract_cookie, sanitize_error_message, CACHEABLE_METHODS, UNAUTHENTICATED_METHODS, }; +pub use middleware::PeerAddr; use response::{cookie_header, json_response, ResponseCache, RpcError, RpcRequest, RpcResponse}; /// Default dev password when no user is set up (matches mock-backend). @@ -369,7 +370,7 @@ impl RpcHandler { // Rate limit login attempts if rpc_req.method == "auth.login" { - let client_ip = extract_client_ip(&parts.headers); + let client_ip = extract_client_ip(&parts); if !self.login_rate_limiter.check(client_ip).await { return Ok(self.rate_limit_response()); } @@ -377,7 +378,7 @@ impl RpcHandler { // Rate limit sensitive endpoints { - let client_ip = extract_client_ip(&parts.headers); + let client_ip = extract_client_ip(&parts); if !self .endpoint_rate_limiter .check(&rpc_req.method, client_ip) @@ -451,7 +452,7 @@ impl RpcHandler { let mut response = json_response(StatusCode::OK, &resp_body); // Post-dispatch: set cookies for auth-related methods - let client_ip = extract_client_ip(&parts.headers); + let client_ip = extract_client_ip(&parts); self.apply_auth_cookies( &rpc_req.method, &mut rpc_resp, diff --git a/core/archipelago/src/server.rs b/core/archipelago/src/server.rs index 807a27f7..4a52851d 100644 --- a/core/archipelago/src/server.rs +++ b/core/archipelago/src/server.rs @@ -1034,9 +1034,13 @@ async fn accept_loop( let permit = active_connections.clone().acquire_owned().await; tokio::spawn(async move { let _permit = permit; - let service = service_fn(move |req: hyper::Request| { + let service = service_fn(move |mut req: hyper::Request| { let handler = handler.clone(); async move { + // Record the TCP peer so rate limiting only trusts + // forwarded headers on loopback (nginx) connections. + req.extensions_mut() + .insert(crate::api::rpc::PeerAddr(peer_addr)); if peer_only && !is_peer_allowed_path(req.uri().path()) { let resp = hyper::Response::builder() .status(hyper::StatusCode::NOT_FOUND) diff --git a/docs/1.8.0-RELEASE-HARDENING-PLAN.md b/docs/1.8.0-RELEASE-HARDENING-PLAN.md index 67a5c887..b223951c 100644 --- a/docs/1.8.0-RELEASE-HARDENING-PLAN.md +++ b/docs/1.8.0-RELEASE-HARDENING-PLAN.md @@ -131,14 +131,25 @@ modules; production request/boot paths are essentially panic-free. The real risk revalidate) instead of blocking on systemctl. `image_verifier.rs` cosign sites have no callers yet — handled with the §A cosign item. Tests: container 155 / transport 29 / config 29 / package 46 all green. -- [ ] 🟡 **Restrict Bitcoin RPC exposure.** `bootstrap.rs:409` writes - `rpcallowip=0.0.0.0/0`. Scope to the container subnet / `127.0.0.1`. +- [ ] 🟡 **Restrict Bitcoin RPC exposure.** INVESTIGATED 2026-07-03 — the fix is NOT + `rpcallowip`: under rootless-podman NAT every forwarded connection reaches bitcoind + with an in-subnet source IP, so scoping `rpcallowip` blocks nothing (and container + consumers use archy-net DNS anyway). The actual exposure is the host-side publish + `8332:8332` (binds 0.0.0.0 → LAN can hit RPC, auth-only barrier; 4 write sites: + `bootstrap.rs:424`, `package/config.rs:694`, `package/install.rs:1333/2450`, plus + the knots manifest). Real fix = `127.0.0.1:8332:8332` host bind (P2P 8333 stays + public; zmq 28332/28333 should get the same look — unauthenticated). ⚠ May break + external wallets pointed straight at nodeIP:8332 — needs a user call + on-node gate + re-run, so NOT changed drive-by from the dev box. - [ ] 🟡 **Move generated secrets from env to file mounts.** `manifest.rs:1208-1226` injects secrets as `-e KEY=value`, readable via `podman inspect` / `/proc//environ`. Prefer bind-mounting the existing `0600` secret file or `podman --secret`. -- [ ] 🟡 **Harden rate-limit IP extraction.** `middleware.rs:120-128` trusts - client-spoofable `X-Real-IP`/`X-Forwarded-For` → per-request bucket rotation defeats the - login limiter. Trust forwarded headers only from a configured proxy; have nginx set them. +- [x] 🟡 **Harden rate-limit IP extraction.** DONE 2026-07-03: the accept loop injects the + TCP `PeerAddr` into request extensions; `extract_client_ip` honors + `X-Real-IP`/`X-Forwarded-For` ONLY when the connection is from loopback (our nginx, + which sets `X-Real-IP $remote_addr`) — direct connections (e.g. the FIPS peer + listener) bucket under their socket IP, so per-request header rotation no longer + defeats the login limiter. 3 unit tests. - [ ] 🟢 **Include `seq` in the mesh signed preimage.** `message_types.rs:245-288` signs `(t,v,ts)` but sets the anti-replay `seq` after signing → a radio MITM can alter ordering without breaking the signature.