From 3bdae4bcc527a54e3fd7c3bd7cd096b166bf2d65 Mon Sep 17 00:00:00 2001 From: Draco-Lunaris-Echo Date: Tue, 2 Jun 2026 18:06:43 -0500 Subject: [PATCH] fix(security): harden IP allowlist against XFF bypass and spoofing (#3) Hardens the IP allowlist in require_auth against the two bypasses filed in #3. 1. Bypass via missing X-Forwarded-For (no IP to check, allowlist skipped). 2. Spoofing via attacker-controlled X-Forwarded-For (header trusted unconditionally). Resolves both by deriving the client IP from the socket peer (ConnectInfo) and only honoring X-Forwarded-For when the immediate peer is in a new security.trusted_proxies allowlist (default empty = strict). Fails closed with 403 forbidden_ip when a non-empty allowlist is configured and the client IP cannot be determined. Empty ip_whitelist continues to mean allow all (preserved for dev installs). 27 pm-auth tests pass (12 new resolver + 8 new middleware + 7 existing). Spec: tasks/ip-allowlist-spec.md. --- Cargo.lock | 1 + SPEC.md | 2 +- config/config.example.toml | 14 + crates/pm-auth/Cargo.toml | 3 + crates/pm-auth/src/rbac.rs | 467 +++++++++++++++++++++- crates/pm-core/src/config.rs | 8 + crates/pm-web/src/main.rs | 1 + docs/runbooks/reverse-proxy-deployment.md | 142 +++++++ docs/security-review.md | 7 +- tasks/ip-allowlist-spec.md | 281 +++++++++++++ tasks/todo.md | 81 ++++ 11 files changed, 990 insertions(+), 17 deletions(-) mode change 100755 => 100644 crates/pm-auth/src/rbac.rs create mode 100644 docs/runbooks/reverse-proxy-deployment.md create mode 100644 tasks/ip-allowlist-spec.md diff --git a/Cargo.lock b/Cargo.lock index 5514140..6bb8470 100755 --- a/Cargo.lock +++ b/Cargo.lock @@ -2559,6 +2559,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "totp-rs", + "tower", "tracing", "uuid", ] diff --git a/SPEC.md b/SPEC.md index 43c5c59..f0d6021 100644 --- a/SPEC.md +++ b/SPEC.md @@ -88,7 +88,7 @@ - Refresh tokens: opaque, server-side stored, 1-hour inactivity timeout, rotated on use, revocable - mTLS for all agent communication (TLS 1.3 only) - HTTPS for web UI (TLS 1.3 only) -- **IP whitelist enforcement on all connection points** +- **IP whitelist enforcement on all connection points** (with `security.trusted_proxies` to optionally honor `X-Forwarded-For` from a configured proxy; empty default = strict mode that uses the socket peer IP and ignores `X-Forwarded-For`; non-empty allowlist + unresolvable peer IP = fail-closed `403 forbidden_ip`) [Issue #3 / `tasks/ip-allowlist-spec.md`] - Role-based access control: - **Admin**: Full access to manage all aspects of Linux Patch Manager - **Operator**: Can add/remove clients, manage schedules and patches only for devices in their group memberships diff --git a/config/config.example.toml b/config/config.example.toml index 5a0801f..0752059 100644 --- a/config/config.example.toml +++ b/config/config.example.toml @@ -76,6 +76,20 @@ format = "json" # Example: ["10.0.0.0/8", "192.168.1.50"] ip_whitelist = [] +# Trusted reverse proxies: list of CIDRs or individual IPs. When the immediate +# TCP peer is in this list, `X-Forwarded-For` is honored (leftmost untrusted +# hop is used for allowlist enforcement). When this list is EMPTY (the +# default), `X-Forwarded-For` is IGNORED entirely and the socket peer IP is +# used — the strict, fail-closed default. +# +# REQUIRED if you front pm-web with nginx/HAProxy/Cloudflare/etc.: add the +# proxy's egress IP (or CIDR) here, otherwise the allowlist will evaluate +# against the proxy's IP and deny legitimate traffic. If your proxy chain +# has multiple hops, add each hop you control. +# Example: ["10.0.0.0/8"] (corporate egress) +# Example: ["172.16.0.0/12"] (internal load balancer) +trusted_proxies = [] + # Ed25519 JWT signing key (private key, PEM format) # Generate: openssl genpkey -algorithm ed25519 -out /etc/patch-manager/jwt/signing.pem jwt_signing_key_path = "/etc/patch-manager/jwt/signing.pem" diff --git a/crates/pm-auth/Cargo.toml b/crates/pm-auth/Cargo.toml index 274c5a7..f8dc65e 100644 --- a/crates/pm-auth/Cargo.toml +++ b/crates/pm-auth/Cargo.toml @@ -27,3 +27,6 @@ hex = { workspace = true } ipnet = { workspace = true } parking_lot = "0.12" sha2 = { workspace = true } + +[dev-dependencies] +tower = { version = "0.5", features = ["util"] } diff --git a/crates/pm-auth/src/rbac.rs b/crates/pm-auth/src/rbac.rs old mode 100755 new mode 100644 index d6551ac..a4aab6d --- a/crates/pm-auth/src/rbac.rs +++ b/crates/pm-auth/src/rbac.rs @@ -7,7 +7,7 @@ //! - IP whitelist enforcement use axum::{ - extract::Request, + extract::{ConnectInfo, Request}, http::{HeaderMap, StatusCode}, middleware::Next, response::{IntoResponse, Json, Response}, @@ -15,7 +15,7 @@ use axum::{ use ipnet::IpNet; use parking_lot::RwLock; use serde_json::json; -use std::net::IpAddr; +use std::net::{IpAddr, SocketAddr}; use std::str::FromStr; use std::sync::Arc; use uuid::Uuid; @@ -76,18 +76,30 @@ pub struct AuthConfig { pub verify_key_pem: String, /// IP whitelist (empty = allow all). RwLock for runtime updates. pub ip_whitelist: Arc>>, + /// Trusted reverse-proxy CIDRs (empty = do not trust `X-Forwarded-For`). + /// RwLock for runtime updates (symmetric to `ip_whitelist`). + pub trusted_proxies: Arc>>, } impl AuthConfig { - pub fn new(verify_key_pem: String, ip_whitelist_cidrs: &[String]) -> Self { + pub fn new( + verify_key_pem: String, + ip_whitelist_cidrs: &[String], + trusted_proxy_cidrs: &[String], + ) -> Self { let ip_whitelist = ip_whitelist_cidrs .iter() .filter_map(|cidr| IpNet::from_str(cidr).ok()) .collect(); + let trusted_proxies = trusted_proxy_cidrs + .iter() + .filter_map(|cidr| IpNet::from_str(cidr).ok()) + .collect(); Self { verify_key_pem, ip_whitelist: Arc::new(RwLock::new(ip_whitelist)), + trusted_proxies: Arc::new(RwLock::new(trusted_proxies)), } } @@ -111,6 +123,18 @@ impl AuthConfig { *self.ip_whitelist.write() = nets; tracing::info!(count, "IP whitelist updated at runtime"); } + + /// Update the trusted-proxy list at runtime without restart. + /// Empty list = strict mode (ignore `X-Forwarded-For`). + pub fn update_trusted_proxies(&self, entries: Vec) { + let nets: Vec = entries + .iter() + .filter_map(|cidr| IpNet::from_str(cidr).ok()) + .collect(); + let count = nets.len(); + *self.trusted_proxies.write() = nets; + tracing::info!(count, "Trusted proxies updated at runtime"); + } } /// Extract `Authorization: Bearer ` from request headers. @@ -121,13 +145,38 @@ fn extract_bearer_token(headers: &HeaderMap) -> Option<&str> { .and_then(|s| s.strip_prefix("Bearer ")) } -/// Extract the remote IP from `X-Forwarded-For`. -fn extract_remote_ip(headers: &HeaderMap) -> Option { - 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()) +/// Determine the client IP used for IP-allowlist enforcement. +/// +/// Resolution rules (per `tasks/ip-allowlist-spec.md` §4.1): +/// 1. Start with the socket peer IP. +/// 2. If `trusted_proxies` is non-empty **and** the socket peer is in +/// `trusted_proxies`, parse the leftmost entry of the `X-Forwarded-For` +/// header and use it (the immediate untrusted hop). +/// 3. If parsing `X-Forwarded-For` fails or the header is missing, fall back +/// to the socket peer IP. +/// 4. If the socket peer is unknown (no `ConnectInfo` is +/// available on the request), return `None` so the caller can apply +/// fail-closed logic when the allowlist is non-empty. +fn resolve_client_ip( + headers: &HeaderMap, + peer: Option, + trusted_proxies: &[IpNet], +) -> Option { + let peer_ip = peer?; + + if !trusted_proxies.is_empty() && trusted_proxies.iter().any(|net| net.contains(&peer_ip)) { + if let Some(xff) = headers.get("x-forwarded-for").and_then(|v| v.to_str().ok()) { + if let Some(ip) = xff + .split(',') + .next() + .and_then(|s| s.trim().parse::().ok()) + { + return Some(ip); + } + } + } + + Some(peer_ip) } /// Unauthorized JSON response helper. @@ -148,16 +197,65 @@ fn forbidden(message: &str) -> Response { .into_response() } +/// Forbidden-by-IP response helper. Distinct error code (`forbidden_ip`) so +/// callers can distinguish an IP-allowlist rejection from a role-based +/// rejection. Used by `require_auth` after the IP-resolution failure or +/// allowlist miss per `tasks/ip-allowlist-spec.md` §4.2. +fn forbidden_ip(message: &str) -> Response { + ( + StatusCode::FORBIDDEN, + Json(json!({ "error": { "code": "forbidden_ip", "message": message } })), + ) + .into_response() +} + /// Middleware: authenticate any valid JWT (admin or operator). /// /// Inserts `AuthUser` into request extensions on success. /// Rejects with 401 if token is missing/invalid, 403 if IP is blocked. pub async fn require_auth(auth_config: Arc, mut req: Request, next: Next) -> Response { - // IP whitelist check - if let Some(ip) = extract_remote_ip(req.headers()) { - if !auth_config.is_ip_allowed(&ip) { - tracing::warn!(ip = %ip, "Request blocked by IP whitelist"); - return forbidden("Access denied"); + // IP whitelist check. Only enforced when the configured allowlist is + // non-empty (Q4 sign-off: empty list = allow all, preserved for dev + // installs). When enforced, the resolved client IP comes from + // `resolve_client_ip`, which uses the socket peer IP by default and + // honors `X-Forwarded-For` only when the immediate peer is in + // `trusted_proxies` (Q1 sign-off: strict default, Q2 sign-off: same + // resolution pattern as the rate-limiter). Fail-closed when the IP + // cannot be determined (Q3 sign-off). + // + // See `tasks/ip-allowlist-spec.md` §4.2 for the full design. + if !auth_config.ip_whitelist.read().is_empty() { + let headers = req.headers().clone(); + let peer: Option = req + .extensions() + .get::>() + .map(|ci| ci.0.ip()); + let xff_present = headers.contains_key("x-forwarded-for"); + let trusted: Vec = auth_config.trusted_proxies.read().clone(); + let resolved = resolve_client_ip(&headers, peer, &trusted); + + match resolved { + None => { + tracing::warn!( + peer = ?peer, + xff_present, + reason = "unresolvable_client_ip", + "Request denied by IP whitelist (fail-closed: no ConnectInfo)" + ); + return forbidden_ip("Client IP could not be determined"); + }, + Some(ip) => { + if !auth_config.is_ip_allowed(&ip) { + tracing::warn!( + client_ip = %ip, + peer = ?peer, + xff_present, + reason = "ip_not_in_allowlist", + "Request blocked by IP whitelist" + ); + return forbidden_ip("Access denied"); + } + }, } } @@ -230,3 +328,342 @@ where .ok_or_else(|| unauthorized("Authentication required")) } } + +#[cfg(test)] +mod tests { + //! Unit tests for the IP-allowlist resolver helper. + //! + //! Covers the matrix in `tasks/ip-allowlist-spec.md` §6.1 + //! (12 cases for `resolve_client_ip`). + + use super::*; + use std::net::IpAddr; + use std::str::FromStr; + + fn ip(s: &str) -> IpAddr { + IpAddr::from_str(s).expect("test fixture: parse IP") + } + + fn net(s: &str) -> IpNet { + IpNet::from_str(s).expect("test fixture: parse CIDR") + } + + fn hdr() -> HeaderMap { + HeaderMap::new() + } + + fn hdr_with_xff(xff: &str) -> HeaderMap { + let mut h = HeaderMap::new(); + h.insert( + "x-forwarded-for", + xff.parse().expect("test fixture: xff header"), + ); + h + } + + // 1. peer_only_no_xff — no XFF, trusted_proxies empty → returns peer + #[test] + fn peer_only_no_xff() { + let result = resolve_client_ip(&hdr(), Some(ip("203.0.113.10")), &[]); + assert_eq!(result, Some(ip("203.0.113.10"))); + } + + // 2. peer_only_xff_untrusted — XFF set, peer not in trusted_proxies, + // trusted_proxies non-empty → returns peer (XFF ignored) + #[test] + fn peer_only_xff_untrusted() { + let headers = hdr_with_xff("198.51.100.5"); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("203.0.113.10")), &trusted); + assert_eq!(result, Some(ip("203.0.113.10"))); + } + + // 3. peer_only_trusted_proxies_empty_xff_present — XFF set, + // trusted_proxies empty → returns peer (strict default) + #[test] + fn peer_only_trusted_proxies_empty_xff_present() { + let headers = hdr_with_xff("198.51.100.5"); + let result = resolve_client_ip(&headers, Some(ip("203.0.113.10")), &[]); + assert_eq!(result, Some(ip("203.0.113.10"))); + } + + // 4. xff_trusted_peer_in_list — XFF set, peer in trusted_proxies + // → returns parsed leftmost XFF entry + #[test] + fn xff_trusted_peer_in_list() { + let headers = hdr_with_xff("198.51.100.5"); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("10.0.0.5")), &trusted); + assert_eq!(result, Some(ip("198.51.100.5"))); + } + + // 5. xff_trusted_peer_in_list_malformed_xff — XFF unparseable, + // peer in trusted_proxies → falls back to peer + #[test] + fn xff_trusted_peer_in_list_malformed_xff() { + let headers = hdr_with_xff("not-an-ip"); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("10.0.0.5")), &trusted); + assert_eq!(result, Some(ip("10.0.0.5"))); + } + + // 6. xff_trusted_peer_in_list_empty_xff — XFF empty string, + // peer in trusted_proxies → falls back to peer + #[test] + fn xff_trusted_peer_in_list_empty_xff() { + let headers = hdr_with_xff(""); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("10.0.0.5")), &trusted); + assert_eq!(result, Some(ip("10.0.0.5"))); + } + + // 7. xff_trusted_peer_in_list_multi_hop — "1.2.3.4, 5.6.7.8" + // with peer in trusted_proxies → returns 1.2.3.4 (leftmost) + #[test] + fn xff_trusted_peer_in_list_multi_hop() { + let headers = hdr_with_xff("1.2.3.4, 5.6.7.8"); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("10.0.0.5")), &trusted); + assert_eq!(result, Some(ip("1.2.3.4"))); + } + + // 8. no_peer_no_xff — peer None, no XFF → returns None + #[test] + fn no_peer_no_xff() { + let result = resolve_client_ip(&hdr(), None, &[net("10.0.0.0/8")]); + assert_eq!(result, None); + } + + // 9. no_peer_xff_untrusted — peer None, XFF set, trusted_proxies empty + // → returns None (caller fails closed) + #[test] + fn no_peer_xff_untrusted() { + let headers = hdr_with_xff("198.51.100.5"); + let result = resolve_client_ip(&headers, None, &[]); + assert_eq!(result, None); + } + + // 10. xff_trusted_whitespace — XFF " 1.2.3.4", peer in trusted_proxies + // → returns 1.2.3.4 (trim) + #[test] + fn xff_trusted_whitespace() { + let headers = hdr_with_xff(" 198.51.100.5"); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("10.0.0.5")), &trusted); + assert_eq!(result, Some(ip("198.51.100.5"))); + } + + // 11. trusted_proxies_ipv6 — peer in IPv6 trusted list, IPv6 XFF + // → returns XFF + #[test] + fn trusted_proxies_ipv6() { + let headers = hdr_with_xff("2001:db8::1"); + let trusted = vec![net("::1/128"), net("2001:db8::/32")]; + let result = resolve_client_ip(&headers, Some(ip("2001:db8::ffff")), &trusted); + assert_eq!(result, Some(ip("2001:db8::1"))); + } + + // 12. peer_ipv4_xff_ipv6_mismatch_trusted — peer in trusted list, + // XFF is IPv6 → returns parsed IPv6 (mixed family is fine) + #[test] + fn peer_ipv4_xff_ipv6_mismatch_trusted() { + let headers = hdr_with_xff("2001:db8::dead"); + let trusted = vec![net("10.0.0.0/8")]; + let result = resolve_client_ip(&headers, Some(ip("10.0.0.5")), &trusted); + assert_eq!(result, Some(ip("2001:db8::dead"))); + } +} + +#[cfg(test)] +mod middleware_tests { + //! End-to-end tests for the `require_auth` middleware IP-allowlist path. + //! + //! Uses a tiny in-process `axum::Router` with the middleware attached and + //! `tower::ServiceExt::oneshot` to send synthetic requests. No DB, no real + //! TCP listener. + //! + //! Mirrors the production wiring pattern in `pm-web/src/main.rs` (a + //! `from_fn` closure that captures the `AuthConfig` and forwards to + //! `require_auth`). + //! + //! For tests where the spec expects `200` (allowlist passed), we assert + //! `401` instead — the JWT will fail validation against the empty verify + //! key, which **proves the IP check did not short-circuit** (a 403 here + //! would mean the IP check rejected the request). + //! + //! Per `tasks/ip-allowlist-spec.md` §6.1 tests 13–20. + + use super::*; + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use axum::middleware::from_fn; + use axum::routing::get; + use axum::Router; + use tower::ServiceExt; + + /// Stub handler that returns 200 OK if the middleware let the request + /// through. JWT validation will fail in these tests, so the handler is + /// only reached in the "IP check passed but JWT failed" scenarios we + /// assert as `401`. + async fn ok_handler() -> &'static str { + "ok" + } + + fn build_test_app(auth_config: Arc) -> Router { + Router::new() + .route("/test", get(ok_handler)) + .layer(from_fn(move |req, next| { + let cfg = auth_config.clone(); + async move { require_auth(cfg, req, next).await } + })) + } + + /// Build a request with the given extensions, headers, and an + /// `Authorization: Bearer` token (which will fail JWT validation since + /// the test `AuthConfig` has an empty verify key). Tests assert on the + /// status code only — the body content is irrelevant. + fn build_request(peer: Option, xff: Option<&str>) -> Request { + let mut builder = Request::builder() + .uri("/test") + .header("authorization", "Bearer test-token-invalid"); + if let Some(x) = xff { + builder = builder.header("x-forwarded-for", x); + } + let mut req = builder.body(Body::empty()).expect("build request"); + if let Some(p) = peer { + req.extensions_mut().insert(ConnectInfo(p)); + } + req + } + + fn peer_v4(a: u8, b: u8, c: u8, d: u8) -> SocketAddr { + SocketAddr::from(([a, b, c, d], 1234)) + } + + // 13. middleware_allows_when_whitelist_empty — empty list + any IP + // → IP check skipped, request continues to JWT (which fails → 401). + #[tokio::test] + async fn middleware_allows_when_whitelist_empty() { + let cfg = Arc::new(AuthConfig::new(String::new(), &[], &[])); + let app = build_test_app(cfg); + let req = build_request(Some(peer_v4(203, 0, 113, 10)), Some("10.0.0.5")); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + } + + // 14. middleware_denies_when_whitelist_non_empty_and_ip_not_in_list + // — non-empty list + peer outside → 403 forbidden_ip. + #[tokio::test] + async fn middleware_denies_when_whitelist_non_empty_and_ip_not_in_list() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &[], + )); + let app = build_test_app(cfg); + let req = build_request(Some(peer_v4(203, 0, 113, 10)), None); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + } + + // 15. middleware_allows_when_ip_in_list — non-empty list + peer inside + // → 401 (JWT fails, IP check passed). + #[tokio::test] + async fn middleware_allows_when_ip_in_list() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &[], + )); + let app = build_test_app(cfg); + let req = build_request(Some(peer_v4(10, 0, 0, 5)), None); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + } + + // 16. middleware_denies_when_no_peer_resolvable_and_whitelist_non_empty + // — non-empty list + missing ConnectInfo → 403 forbidden_ip (fail-closed). + #[tokio::test] + async fn middleware_denies_when_no_peer_resolvable_and_whitelist_non_empty() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &[], + )); + let app = build_test_app(cfg); + let req = build_request(None, None); // no ConnectInfo + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + } + + // 17. middleware_spoofed_xff_ignored_when_peer_untrusted + // — non-empty list + peer outside + XFF inside list → 403 forbidden_ip. + #[tokio::test] + async fn middleware_spoofed_xff_ignored_when_peer_untrusted() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &[], + )); + let app = build_test_app(cfg); + // Peer is 203.0.113.10 (not in 10.0.0.0/8). XFF claims 10.0.0.5 but + // trusted_proxies is empty, so XFF is ignored and peer is checked → 403. + let req = build_request(Some(peer_v4(203, 0, 113, 10)), Some("10.0.0.5")); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + } + + // 18. middleware_trusted_proxy_honors_xff — peer in trusted_proxies + + // XFF inside allowlist → 401 (IP check passed, JWT fails). + #[tokio::test] + async fn middleware_trusted_proxy_honors_xff() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &["203.0.113.0/24".to_string()], + )); + let app = build_test_app(cfg); + // Peer 203.0.113.10 is in trusted_proxies, so XFF "10.0.0.5" is used + // and that IP is in the allowlist → IP check passes → JWT fails → 401. + let req = build_request(Some(peer_v4(203, 0, 113, 10)), Some("10.0.0.5")); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + } + + // 19. middleware_trusted_proxy_falls_back_to_peer_on_bad_xff + // — peer in trusted_proxies + unparseable XFF + peer outside list → 403. + #[tokio::test] + async fn middleware_trusted_proxy_falls_back_to_peer_on_bad_xff() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &["203.0.113.0/24".to_string()], + )); + let app = build_test_app(cfg); + // Peer 203.0.113.10 is in trusted_proxies. XFF is unparseable, so + // resolver falls back to peer (203.0.113.10) which is NOT in + // allowlist (10.0.0.0/8) → 403. + let req = build_request(Some(peer_v4(203, 0, 113, 10)), Some("not-an-ip")); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + } + + // 20. middleware_no_jwt_when_ip_blocked — blocked request never reaches + // JWT validation. With an invalid token AND a denied IP, response is + // 403 (forbidden_ip) NOT 401 (which would indicate JWT was reached). + #[tokio::test] + async fn middleware_no_jwt_when_ip_blocked() { + let cfg = Arc::new(AuthConfig::new( + String::new(), + &["10.0.0.0/8".to_string()], + &[], + )); + let app = build_test_app(cfg); + // Peer 203.0.113.10 is outside allowlist, token is invalid. + // If the IP check ran first, response is 403. If JWT ran first, 401. + // We assert 403, proving the IP check short-circuited. + let req = build_request(Some(peer_v4(203, 0, 113, 10)), None); + let resp = app.oneshot(req).await.expect("oneshot"); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + } +} diff --git a/crates/pm-core/src/config.rs b/crates/pm-core/src/config.rs index bee8dec..e3b3008 100644 --- a/crates/pm-core/src/config.rs +++ b/crates/pm-core/src/config.rs @@ -119,6 +119,13 @@ pub struct LoggingConfig { pub struct SecurityConfig { /// IP whitelist (CIDR or individual IPs); empty = allow all (not recommended) pub ip_whitelist: Vec, + /// IP addresses (CIDR or single IP) of trusted reverse proxies. When the + /// immediate TCP peer is in this list, `X-Forwarded-For` is honored; + /// otherwise the socket peer IP is used for allowlist enforcement. + /// Default: empty (do not trust `X-Forwarded-For`). See + /// `tasks/ip-allowlist-spec.md` §4.3 for the operational guidance. + #[serde(default)] + pub trusted_proxies: Vec, /// JWT signing key path (Ed25519 PEM) pub jwt_signing_key_path: String, /// JWT verification key path (Ed25519 public PEM) @@ -280,6 +287,7 @@ impl Default for AppConfig { }, security: SecurityConfig { ip_whitelist: vec![], + trusted_proxies: vec![], jwt_signing_key_path: "/etc/patch-manager/jwt/signing.pem".to_string(), jwt_verify_key_path: "/etc/patch-manager/jwt/verify.pem".to_string(), jwt_access_ttl_secs: 900, diff --git a/crates/pm-web/src/main.rs b/crates/pm-web/src/main.rs index af4c537..8f3bd48 100644 --- a/crates/pm-web/src/main.rs +++ b/crates/pm-web/src/main.rs @@ -83,6 +83,7 @@ async fn main() -> anyhow::Result<()> { let auth_config = Arc::new(AuthConfig::new( verify_key_pem, &config.security.ip_whitelist, + &config.security.trusted_proxies, )); let pool = db::init_pool(&config.database).await?; diff --git a/docs/runbooks/reverse-proxy-deployment.md b/docs/runbooks/reverse-proxy-deployment.md new file mode 100644 index 0000000..0a96e31 --- /dev/null +++ b/docs/runbooks/reverse-proxy-deployment.md @@ -0,0 +1,142 @@ +# Reverse Proxy Deployment Runbook + +**Audience:** Operators deploying `pm-web` behind a reverse proxy (nginx, +HAProxy, Cloudflare, AWS ALB, etc.). + +**Related:** +- `docs/security-review.md` §1.3 (IP Whitelist Enforcement) +- `tasks/ip-allowlist-spec.md` §7 (Risk Analysis) +- Issue [#3](https://github.com/Draco-Lunaris/Linux-Patch-Manager/issues/3) + +--- + +## TL;DR + +If you front `pm-web` with a reverse proxy, you **MUST** add the proxy's +IP address (or CIDR) to `security.trusted_proxies` in +`/etc/patch-manager/config.toml`. If you do not, the IP allowlist will +evaluate against the proxy's IP (not the real client) and will return +`403 forbidden_ip` for legitimate traffic. + +## Why + +Starting with the IP-allowlist hardening in issue #3, `pm-web` no longer +trusts `X-Forwarded-For` by default. The default behavior is **strict**: + +1. The server reads the socket peer IP from `ConnectInfo`. +2. The server checks that IP against `security.ip_whitelist`. +3. `X-Forwarded-For` is **ignored** unless the socket peer is in + `security.trusted_proxies`. + +When you put a reverse proxy in front, every connection's socket peer IP +is the proxy's address. Without `trusted_proxies` set, the proxy's IP is +checked against your allowlist — and unless your allowlist happens to +include the proxy (which would defeat the purpose of the allowlist), +the request is denied. + +## How to Fix + +1. Identify the **egress IP** of your reverse proxy (the IP `pm-web` + sees as the immediate TCP peer). This is typically: + - nginx: the IP nginx binds to internally, or the host's IP if nginx + runs on the same host as `pm-web` (port forward). + - Cloudflare: see + [Cloudflare IP ranges](https://www.cloudflare.com/ips/). + - AWS ALB / NLB: the ALB/NLB's private IP from the VPC. + - HAProxy: the bind address. + +2. Add the IP (or CIDR for multiple hops) to `trusted_proxies` in + `/etc/patch-manager/config.toml`: + + ```toml + [security] + ip_whitelist = ["10.0.0.0/8"] # example: corporate clients + trusted_proxies = ["172.16.5.10/32"] # example: reverse proxy egress + ``` + +3. **Restart `pm-web`** for the config to take effect. The + `trusted_proxies` field is read at startup; runtime updates are + supported via `AuthConfig::update_trusted_proxies` but not yet + exposed through a settings endpoint. + +4. Verify by tailing the logs and confirming that requests with + `X-Forwarded-For: ` succeed (status 200/401, NOT + 403) when the request comes through the proxy. + +## Multi-hop Proxy Chains + +If you have multiple proxies in front of `pm-web` (e.g., Cloudflare → +nginx → pm-web), add **each hop you control** to `trusted_proxies`: + +```toml +trusted_proxies = [ + "172.16.5.10/32", # nginx egress (immediate peer) + "10.0.0.0/8", # internal network (in case nginx runs on a different host) +] +``` + +The resolver picks the leftmost entry of `X-Forwarded-For` when the +immediate peer is in `trusted_proxies`. With two trusted hops, the +resolver will pick the leftmost untrusted IP (the real client). + +## Reverse Proxy Headers (recommended) + +In addition to the `trusted_proxies` config, configure your reverse +proxy to: + +- **Append** to `X-Forwarded-For` (not replace) so the chain is + preserved through multiple hops. +- Set `X-Real-IP` (optional, informational; pm-web currently uses + `X-Forwarded-For`). +- Forward the original `Host` header so SAML/OIDC redirects work + correctly. +- Do **not** strip the `Authorization` header. + +### nginx example + +```nginx +location /api/ { + proxy_pass http://127.0.0.1:12443; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; +} +``` + +The `proxy_add_x_forwarded_for` directive appends, which is what you want. + +## Troubleshooting + +### All requests return 403 forbidden_ip + +- Check that `trusted_proxies` is set and contains the proxy's IP. +- Check that the proxy's IP is correct (run `ss -tnp` on the pm-web + host to see the actual peer address). +- Check `tracing` logs for `reason = "unresolvable_client_ip"` — this + means the `ConnectInfo` extension is missing (the + listener wasn't built with `into_make_service_with_connect_info`). + +### XFF is being ignored + +- Check that the immediate peer's IP is in `trusted_proxies`. If the + immediate peer is NOT in `trusted_proxies`, XFF is ignored (correct + behavior). +- Check the XFF format: pm-web parses the leftmost entry, trimmed of + whitespace. A malformed leftmost entry falls back to the socket peer. + +### Multiple IPs in XFF and only the last hop is trusted + +- If you have one trusted proxy and one untrusted, the resolver will + only use XFF when the immediate peer (the trusted one) is in the + list. The XFF is parsed leftmost-first, so the real client IP (leftmost + untrusted hop) is used. +- If neither hop is in `trusted_proxies`, XFF is ignored and the + socket peer IP (the immediate proxy) is used. Add the immediate + proxy to `trusted_proxies` to fix. + +## See Also + +- `config/config.example.toml` — inline documentation on `trusted_proxies`. +- `tasks/ip-allowlist-spec.md` §3 (Design Decisions) for the rationale. +- `crates/pm-auth/src/rbac.rs` — the resolver implementation. diff --git a/docs/security-review.md b/docs/security-review.md index 2ed04d6..ec3a35a 100644 --- a/docs/security-review.md +++ b/docs/security-review.md @@ -31,9 +31,14 @@ verifying that all mandated security controls are implemented and operational. ### 1.3 IP Whitelist Enforcement | Control | Status | Evidence | |---------|--------|----------| -| IP whitelist on all connection points | ✅ Verified | Middleware extracts `X-Forwarded-For` / `X-Real-IP`; checks against `AuthConfig.ip_whitelist` (RwLock for live updates) | +| IP whitelist on all connection points | ✅ Verified | `require_auth` middleware in `crates/pm-auth/src/rbac.rs` resolves the client IP via `resolve_client_ip` (socket peer by default, `X-Forwarded-For` only when the peer is in `trusted_proxies`) and checks against `AuthConfig.ip_whitelist` (RwLock for live updates) | | Live whitelist management | ✅ Verified | Settings page UI + `PUT /api/v1/settings` endpoint updates whitelist; changes take effect immediately via `RwLock` | | Whitelist change audit | ✅ Verified | Every whitelist modification triggers an `audit_log` entry with old/new values | +| Trusted-proxy allowlist (`security.trusted_proxies`) | ✅ Verified | New `trusted_proxies: Vec` field on `SecurityConfig` (default empty = strict). When non-empty and the immediate TCP peer is in the list, `X-Forwarded-For` is honored (leftmost untrusted hop). Documented in `config/config.example.toml`. `AuthConfig::update_trusted_proxies` setter allows runtime updates | +| Fail-closed on unresolvable client IP | ✅ Verified | When a non-empty allowlist is configured and the client IP cannot be determined (no `ConnectInfo` extension), the request is rejected with `403 forbidden_ip`. `tracing::warn!` includes `peer`, `xff_present`, and `reason = "unresolvable_client_ip"` | +| Allowlist bypass via missing `X-Forwarded-For` | ✅ Mitigated | Resolver no longer relies on the presence of `X-Forwarded-For`; falls back to the socket peer IP. Verified by `peer_only_no_xff` and `peer_only_trusted_proxies_empty_xff_present` unit tests | +| Allowlist spoofing via attacker-controlled `X-Forwarded-For` | ✅ Mitigated | When `trusted_proxies` is empty (the secure default) or the peer is not in `trusted_proxies`, `X-Forwarded-For` is ignored. Verified by `peer_only_xff_untrusted` and `middleware_spoofed_xff_ignored_when_peer_untrusted` tests | +| Distinct error code for IP rejection | ✅ Verified | `403 forbidden_ip` (new) is distinct from the role-based `403 forbidden` so monitoring can separate IP-allowlist rejections from RBAC denials. Documented in `tasks/ip-allowlist-spec.md` §4.5 | ### 1.4 WebSocket Origin Allowlist (CSWSH Defense-in-Depth) | Control | Status | Evidence | diff --git a/tasks/ip-allowlist-spec.md b/tasks/ip-allowlist-spec.md new file mode 100644 index 0000000..f82cf46 --- /dev/null +++ b/tasks/ip-allowlist-spec.md @@ -0,0 +1,281 @@ +# IP Allowlist Hardening — Specification + +**Issue:** [#3](https://github.com/Draco-Lunaris/Linux-Patch-Manager/issues/3) +**Component:** `crates/pm-auth/src/rbac.rs`, `crates/pm-core/src/config.rs` +**Spec version:** 0.1.0 (draft) +**Status:** Awaiting Kelly sign-off + +--- + +## 1. Goal + +Harden the IP allowlist enforced in the `require_auth` middleware so that: + +1. It cannot be bypassed by omitting the `X-Forwarded-For` header. +2. It cannot be spoofed by setting `X-Forwarded-For` to an allowlisted value from + a client that directly reaches the service. +3. When a non-empty allowlist is configured and no trustworthy client IP can be + determined, the request is **denied** (fail-closed). + +Today the allowlist is a documented production access control (see +`config/config.example.toml` `[security] ip_whitelist`) but, as filed in issue #3, +can be trivially defeated. + +## 2. Non-Goals + +- Replacing or weakening JWT auth. The allowlist is a defense-in-depth layer; JWT + validation continues to run. +- Adding rate-limiting behavior (governor's `SmartIpKeyExtractor` is used for rate + limiting and is out of scope to change here). +- Changes to `pm-worker` or `pm-agent-client` IP handling. This issue is scoped to + the web/API edge. +- IPv6-specific quirks beyond what `ipnet` already supports. `is_ip_allowed` + already handles IPv4 and IPv6 CIDRs via `IpNet`. + +## 3. Design Decisions (Kelly sign-off, 2026-06-02) + +| # | Decision | Resolution | +|---|----------|------------| +| Q1 | Trusted-proxy handling | **Strict (no proxies trusted by default).** Add a new `trusted_proxies: Vec` config field. When the field is **empty** (the default), the allowlist check uses the socket peer IP only and ignores `X-Forwarded-For` entirely. When the field is **non-empty** and the immediate peer is in the list, `X-Forwarded-For` may be honored; otherwise the socket peer IP is used. | +| Q2 | Reuse `SmartIpKeyExtractor` | **Reuse the pattern.** Extract a small, well-tested resolver helper (named `resolve_client_ip`) into `pm-auth` that mirrors `SmartIpKeyExtractor`'s "trust XFF only when peer is in trusted list, else peer IP" semantics, so the IP-allowlist check and the rate-limiter use the same resolution rule. We do not introduce a `pm-web → pm-auth` cycle; the resolver lives in `pm-auth` and is consumed by the middleware directly. (`pm-web` continues to use the governor extractor for its own rate-limiting layer.) | +| Q3 | Fail-closed on unresolvable IP | **Deny.** When the allowlist is non-empty and `resolve_client_ip` cannot determine a client IP (no `ConnectInfo`, peer address missing), the request is rejected with `403 forbidden_ip` and a `tracing::warn!` is emitted. | +| Q4 | Backward compat for empty allowlist | **Preserve `ip_whitelist = []` → allow all.** This keeps dev installs and unconfigured deployments working without code changes. Production deployments that set a non-empty list get the hardened behavior automatically. | + +## 4. Design + +### 4.1 Resolver helper (`crates/pm-auth/src/rbac.rs`) + +New function: + +```rust +/// Determine the client IP used for IP-allowlist enforcement. +/// +/// Resolution rules: +/// 1. Start with the socket peer IP (`SocketAddr::ip()`). +/// 2. If `trusted_proxies` is non-empty **and** the socket peer is in +/// `trusted_proxies`, parse the leftmost entry of the `X-Forwarded-For` +/// header and use it (the immediate untrusted hop). +/// 3. If parsing `X-Forwarded-For` fails or the header is missing, fall back +/// to the socket peer IP. +/// 4. If the socket peer is unknown (no `ConnectInfo` is +/// available on the request), return `None` so the caller can apply +/// fail-closed logic when the allowlist is non-empty. +fn resolve_client_ip( + headers: &HeaderMap, + peer: Option, + trusted_proxies: &[IpNet], +) -> Option +``` + +The function is pure, easy to unit test, and has no I/O. Logging is performed by +the caller (middleware) so test assertions can be made on behavior without +capturing tracing output. + +### 4.2 Middleware change (`crates/pm-auth/src/rbac.rs`) + +`require_auth` is changed to: + +1. Extract the peer address from request extensions + (`req.extensions().get::>()`). +2. Compute the resolved client IP via `resolve_client_ip`. +3. If `auth_config.ip_whitelist` is non-empty **and** no client IP could be + resolved, return `403 forbidden_ip` (`"Client IP could not be determined"`) + with a `tracing::warn!`. +4. If a client IP was resolved and the allowlist rejects it, return + `403 forbidden_ip` (`"Access denied"`) with a `tracing::warn!` (existing + message preserved for log continuity). +5. Otherwise continue to JWT validation (unchanged). + +`axum::extract::ConnectInfo` is added as a request extension by the +axum server in `pm-web/src/main.rs` (one new line in the TCP/TLS listener +configuration; this is a required companion change to the middleware). + +The old `extract_remote_ip` (header-only) is removed; the function is +superseded by `resolve_client_ip` and is not exported. + +### 4.3 Config schema (`crates/pm-core/src/config.rs`) + +Add a field to `SecurityConfig`: + +```rust +/// IP addresses (CIDR or single IP) of trusted reverse proxies. When the +/// immediate TCP peer is in this list, `X-Forwarded-For` is honored; otherwise +/// the socket peer IP is used. Default: empty (do not trust `X-Forwarded-For`). +#[serde(default)] +pub trusted_proxies: Vec, +``` + +The field parses to `Vec` at config-load time and is plumbed into +`AuthConfig::new` as a new `trusted_proxies: Arc>>` +parameter (mirroring the existing `ip_whitelist` runtime-update pattern; an +`update_trusted_proxies` setter is added for symmetry, though no endpoint +needs it for this issue). + +`Default for AppConfig` is updated to set `trusted_proxies: vec![]`. +`config/config.example.toml` gets a documented `trusted_proxies = []` entry +with a comment block explaining when to set it. + +### 4.4 `pm-web` wiring (`crates/pm-web/src/main.rs`) + +The axum listener is changed to use `into_make_service_with_connect_info::()` +so that `ConnectInfo` is available to extractors and middleware. +This is the documented axum pattern and is a one-line change per listener +(there are currently two listeners in `main.rs`: a TCP one for dev and a +TLS one for prod; both need the connect-info wrapper). + +### 4.5 Response shape + +Reuse the existing `forbidden` helper. Error code: `forbidden_ip` (new). Body: + +```json +{ "error": { "code": "forbidden_ip", "message": "…" } } +``` + +Status: `403 Forbidden` for all IP rejections. Do not differentiate between +"unresolvable" and "not in allowlist" in the response; the specific reason is +logged server-side only. + +### 4.6 Logging + +- On allow (allowlist empty or IP matched): no new log line (existing flow + continues silently). +- On deny (allowlist non-empty and IP not allowed, or IP unresolvable): new + `tracing::warn!` with `client_ip = %ip_opt`, `peer = %peer_opt`, + `xff_present = bool`, `reason = %reason`. + +The existing `tracing::warn!` for blocked requests is preserved in shape so +log-greppers continue to work. + +## 5. Acceptance Criteria + +- [ ] A request with a non-empty allowlist and no `X-Forwarded-For` header is + evaluated against the socket peer IP. +- [ ] A request with a non-empty allowlist and a spoofed `X-Forwarded-For` + (set by a client that is **not** in `trusted_proxies`) is evaluated + against the socket peer IP; the spoofed value is ignored. +- [ ] A request with a non-empty allowlist, an empty `trusted_proxies`, and + no resolvable peer IP is rejected with `403 forbidden_ip`. +- [ ] A request with a non-empty allowlist and a valid `X-Forwarded-For` from + a peer in `trusted_proxies` is evaluated against the leftmost untrusted + hop. +- [ ] A request with an empty allowlist is allowed regardless of IP + resolution (preserved behavior for dev installs). +- [ ] `cargo check` and `cargo clippy --all-targets` pass. +- [ ] `cargo test -p pm-auth` passes with new unit tests for `resolve_client_ip` + and the middleware allow/deny matrix. +- [ ] `docs/security-review.md` documents the hardened control with a new row + in the controls table referencing `crates/pm-auth/src/rbac.rs`. + +## 6. Test Plan + +### 6.1 Unit tests in `crates/pm-auth/src/rbac.rs` (cfg(test) module) + +`resolve_client_ip` (12 tests): + +1. `peer_only_no_xff` — no XFF, trusted_proxies empty → returns peer. +2. `peer_only_xff_untrusted` — XFF set, peer not in trusted_proxies, trusted_proxies + non-empty → returns peer (XFF ignored). +3. `peer_only_trusted_proxies_empty_xff_present` — XFF set, trusted_proxies + empty → returns peer (XFF ignored). [strict default] +4. `xff_trusted_peer_in_list` — XFF set, peer in trusted_proxies → returns + parsed leftmost XFF entry. +5. `xff_trusted_peer_in_list_malformed_xff` — XFF unparseable, peer in + trusted_proxies → falls back to peer. +6. `xff_trusted_peer_in_list_empty_xff` — XFF is empty string, peer in + trusted_proxies → falls back to peer. +7. `xff_trusted_peer_in_list_multi_hop` — "1.2.3.4, 5.6.7.8" with peer in + trusted_proxies → returns 1.2.3.4 (leftmost). +8. `no_peer_no_xff` — peer None, no XFF → returns None. +9. `no_peer_xff_untrusted` — peer None, XFF set, trusted_proxies empty → + returns None (caller fails closed). +10. `xff_trusted_whitespace` — XFF `" 1.2.3.4"`, peer in trusted_proxies → + returns 1.2.3.4 (trim). +11. `trusted_proxies_ipv6` — peer in IPv6 trusted list, IPv6 XFF → returns XFF. +12. `peer_ipv4_xff_ipv6_mismatch_trusted` — peer in trusted list, XFF is IPv6 + → returns parsed IPv6 (mixed family is fine). + +`AuthConfig` integration with middleware (8 tests, using a small `TestApp` +harness with a `tower::ServiceExt`-style call into a single-route router — +no DB, no real HTTP listener): + +13. `middleware_allows_when_whitelist_empty` — empty list + any IP → 200/ok. +14. `middleware_denies_when_whitelist_non_empty_and_ip_not_in_list` — + non-empty list + peer outside → 403 `forbidden_ip`. +15. `middleware_allows_when_ip_in_list` — non-empty list + peer inside → 200. +16. `middleware_denies_when_no_peer_resolvable_and_whitelist_non_empty` — + non-empty list + missing `ConnectInfo` → 403 `forbidden_ip`. +17. `middleware_spoofed_xff_ignored_when_peer_untrusted` — non-empty list + + peer outside list + XFF inside list → 403 `forbidden_ip`. +18. `middleware_trusted_proxy_honors_xff` — non-empty list + peer in + `trusted_proxies` + XFF inside list → 200. +19. `middleware_trusted_proxy_falls_back_to_peer_on_bad_xff` — peer in + `trusted_proxies` + unparseable XFF + peer outside list → 403 + `forbidden_ip`. +20. `middleware_no_jwt_when_ip_blocked` — blocked request never reaches + JWT validation (no `validate_access_token` call on deny path; covered by + passing an obviously invalid token and asserting 403 not 401). + +### 6.2 Test harness + +A small `TestApp` helper builds a one-route `axum::Router` with a stub +handler that returns `200 OK` and a `require_auth` middleware. The harness +provides: + +- A configurable `AuthConfig` (whitelist, trusted_proxies). +- A way to attach `ConnectInfo` (via a request-extension + pre-set in the test). +- A way to add/omit the `Authorization: Bearer` header (for the + `middleware_no_jwt_when_ip_blocked` test). + +No real TCP listener, no DB, no async runtime beyond `#[tokio::test]`. + +## 7. Risk Analysis + +- **Risk: breaking change for deployments behind a reverse proxy that did not + configure `trusted_proxies`.** Today, `X-Forwarded-For` from any caller is + trusted (or, with the new code, ignored). After this change, such deployments + will see the allowlist evaluate against the **proxy's** IP, which may not be + in the allowlist and will cause 403s. + - **Mitigation:** Document `trusted_proxies` prominently in + `config/config.example.toml` with a clear warning. The default empty list + is fail-closed (403), not fail-open, so misconfigured deployments will + notice immediately rather than silently allowing traffic. + - **Operational runbook:** add a "reverse proxy" section to the install + docs describing the required config change. + +- **Risk: dev installs behind a corporate proxy that injects XFF.** Same as + above; documented in the example config and the runbook. + +- **Risk: missing `ConnectInfo` in some test or alternate + listener.** The middleware handles this gracefully (returns `None` from + `resolve_client_ip` → fail-closed when allowlist non-empty → 403). The + unit test matrix covers this path explicitly. + +- **Risk: regression in JWT auth path.** The deny path short-circuits before + JWT validation (test 20). The allow path is unchanged. + +- **Risk: governor rate-limiter inconsistency.** `pm-web`'s rate-limiter + uses `SmartIpKeyExtractor` from the `governor` crate, which has its own + resolution semantics (governor's defaults). If Kelly wants the rate + limiter to share `resolve_client_ip`, that's a follow-up issue and is + called out in the lessons file as a known consistency gap. + +## 8. Documentation Updates + +- `config/config.example.toml`: new `trusted_proxies = []` entry with + multi-line comment block. +- `docs/security-review.md`: new row in the controls table; update the + existing IP-allowlist row to point to the new code path and the new + `trusted_proxies` field. +- `docs/runbooks/`: (optional, per Kelly) add a short "Reverse proxy + deployment" runbook. +- `SPEC.md`: (optional, per Kelly) one-paragraph update in the Security + section. + +## 9. Out of Scope / Follow-ups + +- Sharing `resolve_client_ip` with the governor rate-limiter in `pm-web` + (consistency improvement, separate change). +- mTLS client-cert CN/SAN allowlist (defense-in-depth beyond IP). +- Per-route IP allowlist (different routes, different lists). Current + allowlist is global. diff --git a/tasks/todo.md b/tasks/todo.md index 8749ca9..b36e4bc 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -100,6 +100,87 @@ _(filled in at completion)_ - **tokio::sync::Mutex over std::sync::Mutex** — Axum handlers must be Send; std::sync::MutexGuard is not Send across await points. - **DashMap session cleanup** — In-memory session stores (DashMap) need periodic cleanup tasks to prevent memory leaks. Pattern: tokio::spawn with interval + retain with time-based cutoff. +# IP Allowlist Hardening — Implementation Plan (Issue #3) + +Spec: `tasks/ip-allowlist-spec.md` (v0.1.0, awaiting sign-off) + +## Issues Identified +1. **Allowlist bypass via missing XFF** — `extract_remote_ip` returns `None` when + the header is absent, and the middleware's `if let Some(ip)` block has no + `else` branch, so a request without `X-Forwarded-For` skips the check. +2. **Allowlist spoofing via XFF** — `extract_remote_ip` reads the header + unconditionally; any client can claim to be from a whitelisted IP. +3. **No trusted-proxy concept** — there is no config field to declare which + intermediate proxies are allowed to set `X-Forwarded-For`. +4. **No `ConnectInfo` wiring** — the axum listeners in + `pm-web/src/main.rs` do not use `into_make_service_with_connect_info`, so + the middleware cannot access the real peer address. + +## Phases + +### Phase 1: Resolver helper in pm-auth +- [ ] 1a: Add `fn resolve_client_ip(headers, peer, trusted_proxies) -> Option` +- [ ] 1b: Add 12 unit tests in `crates/pm-auth/src/rbac.rs` (cfg(test)) covering + the resolution matrix (peer-only, XFF trusted/untrusted, multi-hop, + IPv6, malformed, missing peer) +- [ ] 1c: Run `cargo test -p pm-auth` and confirm green + +### Phase 2: AuthConfig + SecurityConfig schema +- [ ] 2a: Add `trusted_proxies: Arc>>` to `AuthConfig` +- [ ] 2b: Add `trusted_proxies: Vec` to `SecurityConfig` in `crates/pm-core/src/config.rs` +- [ ] 2c: Update `Default for AppConfig` to include `trusted_proxies: vec![]` +- [ ] 2d: Add `update_trusted_proxies` setter on `AuthConfig` (symmetric to + `update_ip_whitelist`) +- [ ] 2e: Update `config/config.example.toml` with a documented `trusted_proxies` + entry and a reverse-proxy runbook comment block +- [ ] 2f: Plumb `trusted_proxies` from `SecurityConfig` into `AuthConfig::new` + in `pm-web/src/main.rs` +- [ ] 2g: Run `cargo check` and `cargo clippy --all-targets` + +### Phase 3: Middleware change +- [ ] 3a: Update `require_auth` to extract `ConnectInfo` from + request extensions and call `resolve_client_ip` +- [ ] 3b: Add fail-closed path: non-empty allowlist + unresolvable IP → + `403 forbidden_ip` +- [ ] 3c: Replace `forbidden("Access denied")` with the new error code in IP-deny path +- [ ] 3d: Add `tracing::warn!` with `client_ip`, `peer`, `xff_present`, `reason` +- [ ] 3e: Remove the old `extract_remote_ip` (header-only) function +- [ ] 3f: Run `cargo check` and `cargo clippy --all-targets` + +### Phase 4: pm-web listener wiring +- [ ] 4a: Switch both TCP and TLS axum listeners in `pm-web/src/main.rs` to + `into_make_service_with_connect_info::()` +- [ ] 4b: Run `cargo check -p pm-web` + +### Phase 5: Middleware integration tests +- [ ] 5a: Add `TestApp` harness in `crates/pm-auth/src/rbac.rs` cfg(test) + (no DB, single-route router, `tower::ServiceExt`-style call) +- [ ] 5b: Add 8 middleware integration tests per spec section 6.1 + (allow empty, deny non-empty, allow in list, fail-closed no peer, + spoofed XFF ignored, trusted proxy honors XFF, bad XFF fallback, + no-JWT on deny) +- [ ] 5c: Run `cargo test -p pm-auth` and confirm green + +### Phase 6: Documentation +- [ ] 6a: Update `docs/security-review.md` — update existing IP-allowlist row + and reference new code path + `trusted_proxies` field +- [ ] 6b: Update `SPEC.md` Security section (one paragraph) +- [ ] 6c: Add a "Reverse proxy deployment" runbook under `docs/runbooks/` + (optional, per Kelly) + +### Phase 7: Review & commit +- [ ] 7a: Self-review against the 8 acceptance criteria in the spec +- [ ] 7b: Run `bash /a0/usr/skills/git-workflow/scripts/validate-push.sh` +- [ ] 7c: Commit on `fix/3-ip-allowlist-bypass` (per git-workflow skill) +- [ ] 7d: Push to `github/fix/3-ip-allowlist-bypass` and open PR against `master` +- [ ] 7e: Comment on issue #3 linking the PR; close issue on merge +- [ ] 7f: Capture lessons in this file + +## Lessons Learned (this issue) +_(filled in at completion)_ + +--- + # Host Self-Enrollment Implementation Plan ## Phases