From 9eac724d3acb44a6ba08563e692d73a8c43bb804 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Fri, 3 Jul 2026 14:49:21 +0300 Subject: [PATCH 1/2] feat(api): Capture client IP via proxy & SPIFFE Follow-up to #842 for the reopened issue #358. That PR captured the raw TCP peer on the public HTTP listener; two gaps remained. Proxy header parsing (config-gated, off by default): add an `[oslo_middleware] enable_proxy_headers_parsing` flag mirroring upstream Python Keystone's oslo.middleware. When enabled, a middleware on the public interface parses RFC 7239 `Forwarded` (preferred) and `X-Forwarded-For`, takes the leftmost originating client, and overwrites `ConnectInfo` so every downstream consumer (the request span, the API-Key IP allowlist, future IP-based login control) transparently sees the real client. Left off by default so a deployment not behind a trusted proxy cannot be tricked into trusting a spoofed header. SPIFFE internal interface: the mTLS listener bypasses axum's make-service, so it never populated `ConnectInfo`. Inject the accepted TCP peer address by hand alongside the existing SVID/interface extensions, so `client.addr` is captured on the internal interface too. The admin UDS interface is intentionally left uncovered (no meaningful `SocketAddr`). Tests: proxy-header parsing and middleware units; a composed public-listener test proving proxy rewrite + trailing-slash normalization (#734) + connect-info compose; a `[oslo_middleware]` config test; and a SPIFFE `attach_request_context` unit test. Note: This commit was done with the help of AI. Signed-off-by: Yousef Hussein --- crates/config/src/lib.rs | 6 + crates/config/src/oslo_middleware.rs | 64 +++++ crates/keystone/src/api/mod.rs | 50 ++++ crates/keystone/src/bin/keystone.rs | 31 +- crates/keystone/src/server.rs | 1 + .../src/server/listener/spiffe_tls.rs | 83 +++++- crates/keystone/src/server/proxy_headers.rs | 267 ++++++++++++++++++ 7 files changed, 489 insertions(+), 13 deletions(-) create mode 100644 crates/config/src/oslo_middleware.rs create mode 100644 crates/keystone/src/server/proxy_headers.rs diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index d7b3c49af..04d9d643f 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -77,6 +77,7 @@ mod interface; mod k8s_auth; mod listener; mod mapping; +mod oslo_middleware; mod policy; mod resource; mod revoke; @@ -108,6 +109,7 @@ pub use interface::*; pub use k8s_auth::*; pub use listener::*; pub use mapping::*; +pub use oslo_middleware::*; pub use policy::*; pub use resource::*; pub use revoke::*; @@ -202,6 +204,10 @@ pub struct Config { #[serde(default)] pub mapping: MappingProvider, + /// `[oslo_middleware]` configuration (proxy header parsing, issue #358). + #[serde(default)] + pub oslo_middleware: OsloMiddleware, + /// Server listener configuration for the internal interface. #[serde(rename = "interface_internal", default)] pub interface_internal: Option, diff --git a/crates/config/src/oslo_middleware.rs b/crates/config/src/oslo_middleware.rs new file mode 100644 index 000000000..b4ecfb637 --- /dev/null +++ b/crates/config/src/oslo_middleware.rs @@ -0,0 +1,64 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 +//! # `[oslo_middleware]` configuration +//! +//! Mirrors the subset of upstream Python Keystone's `[oslo_middleware]` +//! section that is relevant to client-address capture (issue #358). + +use serde::Deserialize; + +/// `[oslo_middleware]` section. +#[derive(Debug, Deserialize, Clone, Default)] +pub struct OsloMiddleware { + /// Whether to parse proxy forwarding headers (`Forwarded` per RFC 7239, + /// falling back to `X-Forwarded-For`) on the public interface to recover + /// the originating client address, overwriting the raw TCP peer captured + /// in `ConnectInfo`. + /// + /// The name and default (**off**) match upstream oslo.middleware's + /// `HTTPProxyToWSGI`. It MUST only be enabled when Keystone sits behind a + /// trusted reverse proxy / load balancer: with it on, the immediate peer is + /// trusted to have set these headers honestly, so a client able to reach + /// the listener directly could otherwise spoof its apparent address. Off by + /// default, a deployment that is not actually behind a trusted proxy cannot + /// be tricked into trusting a forged header. + #[serde(default)] + pub enable_proxy_headers_parsing: bool, +} + +#[cfg(test)] +mod tests { + use config::{Config, File, FileFormat}; + + use super::*; + + #[test] + fn defaults_to_disabled() { + let sot = OsloMiddleware::default(); + assert!(!sot.enable_proxy_headers_parsing); + } + + #[test] + fn parses_enabled_flag_from_ini() { + let c = Config::builder() + .add_source(File::from_str( + "enable_proxy_headers_parsing = true", + FileFormat::Ini, + )) + .build() + .unwrap(); + let sot: OsloMiddleware = c.try_deserialize().unwrap(); + assert!(sot.enable_proxy_headers_parsing); + } +} diff --git a/crates/keystone/src/api/mod.rs b/crates/keystone/src/api/mod.rs index a3bcde006..9fba1cdea 100644 --- a/crates/keystone/src/api/mod.rs +++ b/crates/keystone/src/api/mod.rs @@ -214,4 +214,54 @@ pub(crate) mod tests { .unwrap(); assert_eq!(&body[..], b"192.0.2.4:5555"); } + + /// Issue #358 follow-up: when proxy-header parsing is enabled (config-gated, + /// off by default), the public listener wraps the router with the + /// `rewrite_client_addr` layer, exactly as `spawn_public_listener` does: + /// the layer sits on the outer `Router`, *outside* the #734 + /// `NormalizePathLayer` fallback. This drives that full production + /// composition in-process and asserts that a `/echo/` request carrying + /// `X-Forwarded-For` (a) still normalizes the trailing slash, (b) reaches + /// the handler, and (c) delivers the *proxy-resolved* client address — not + /// the raw TCP peer — proving the layer runs before routing/normalization + /// and rewrites `ConnectInfo` end to end. + #[tokio::test] + async fn proxy_headers_rewrite_client_addr_and_normalize() { + async fn echo_addr(ConnectInfo(addr): ConnectInfo) -> String { + addr.ip().to_string() + } + + let router = Router::new().route("/echo", get(echo_addr)); + // Mirror `build_router`: the API service is `NormalizePath`-wrapped and + // mounted as the outer router's fallback. + let normalized = NormalizePathLayer::trim_trailing_slash().layer(router); + let app = Router::new() + .fallback_service(normalized) + .layer(axum::middleware::from_fn( + crate::server::proxy_headers::rewrite_client_addr, + )); + + let make = + AxumServiceExt::>::into_make_service_with_connect_info::(app); + // Raw TCP peer is the reverse proxy; the header carries the real client. + let peer: SocketAddr = "10.0.0.9:5555".parse().unwrap(); + let svc = make.oneshot(peer).await.unwrap(); + + let response = svc + .oneshot( + Request::builder() + .uri("/echo/") + .header("x-forwarded-for", "203.0.113.7, 10.0.0.9") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!(&body[..], b"203.0.113.7"); + } } diff --git a/crates/keystone/src/bin/keystone.rs b/crates/keystone/src/bin/keystone.rs index 590711191..cda184ee0 100644 --- a/crates/keystone/src/bin/keystone.rs +++ b/crates/keystone/src/bin/keystone.rs @@ -74,6 +74,7 @@ use openstack_keystone::revoke::RevokeHook; use openstack_keystone::role::RoleHook; use openstack_keystone::scim; use openstack_keystone::server::listener::{raft_grpc, spiffe_tls, spiffe_tls_uds}; +use openstack_keystone::server::proxy_headers; use openstack_keystone::token::TokenHook; use openstack_keystone::trust::TrustHook; use openstack_keystone::webauthn; @@ -613,11 +614,16 @@ async fn build_router( .layer( TraceLayer::new(common::KeystoneResponseClassifier) .make_span_with(|request: &Request<_>| { - // Raw TCP peer address captured by - // `into_make_service_with_connect_info` on the public - // listener (the keystone-ng analogue of Python Keystone's - // WSGI REMOTE_ADDR / flask.request.remote_addr). `None` for - // the SPIFFE interfaces, which do not populate ConnectInfo. + // Client address captured into `ConnectInfo` + // (the keystone-ng analogue of Python Keystone's WSGI + // REMOTE_ADDR / flask.request.remote_addr): the raw TCP peer + // on the public listener via + // `into_make_service_with_connect_info`, or the mTLS peer on + // the internal SPIFFE-TLS listener (injected by hand, see + // issue #358). When `enable_proxy_headers_parsing` is on, the + // public value has been overwritten with the proxy-resolved + // client address. `None` on the admin UDS interface, which + // has no meaningful `SocketAddr`. let client_addr = request .extensions() .get::>() @@ -783,7 +789,20 @@ async fn spawn_public_listener( info!("Starting Rest API at {}", cfg.interface_public.tcp_address); let listener = TcpListener::bind(&cfg.interface_public.tcp_address).await?; let rest_cancel_token = token.clone(); - let rest_app = app; + // When operating behind a trusted reverse proxy (config-gated, + // off by default), parse `Forwarded`/`X-Forwarded-For` and rewrite + // the raw-peer `ConnectInfo` with the originating client address + // *before* the tracing span and handlers read it (issue #358). The + // layer is added only on this public interface — never on the + // internal SPIFFE/admin listeners, whose peers are the mTLS mesh. + let rest_app = if cfg.oslo_middleware.enable_proxy_headers_parsing { + info!("Proxy header parsing enabled on the public interface"); + app.layer(axum::middleware::from_fn( + proxy_headers::rewrite_client_addr, + )) + } else { + app + }; handles.spawn(async move { // `rest_app` is a `Router` whose fallback is the // `NormalizePath`-wrapped API service (issue #734, #1467); use diff --git a/crates/keystone/src/server.rs b/crates/keystone/src/server.rs index 12f3d3b72..9d4c7d1fa 100644 --- a/crates/keystone/src/server.rs +++ b/crates/keystone/src/server.rs @@ -13,3 +13,4 @@ // SPDX-License-Identifier: Apache-2.0 //! # Keystone server listeners pub mod listener; +pub mod proxy_headers; diff --git a/crates/keystone/src/server/listener/spiffe_tls.rs b/crates/keystone/src/server/listener/spiffe_tls.rs index 4ee270062..79036acbf 100644 --- a/crates/keystone/src/server/listener/spiffe_tls.rs +++ b/crates/keystone/src/server/listener/spiffe_tls.rs @@ -14,6 +14,7 @@ //! # TLS server listener with the SPIFFE integration use axum::Router; +use axum::extract::ConnectInfo; use color_eyre::eyre::{Report, Result}; use hyper_util::rt::{TokioExecutor, TokioIo}; use hyper_util::server::conn::auto::Builder; @@ -75,13 +76,12 @@ pub async fn start_axum_app( let interface_clone = interface_clone.clone(); async move { let mut req = req; - if let Some(spiffe_id) = spiffe_id { - // Move the client TLS certificate into the request extensions - tracing::debug!("The client supplied certificate for spiffe_id: {:?}", spiffe_id); - - req.extensions_mut().insert(spiffe_id); - } - req.extensions_mut().insert(interface_clone); + attach_request_context( + req.extensions_mut(), + spiffe_id, + peer_addr, + interface_clone, + ); // Call Axum and explicitly wrap the result app.call(req).await } @@ -120,3 +120,72 @@ pub async fn start_axum_app( Ok(()) } + +/// Attach the per-request context derived from an mTLS connection onto the +/// request extensions before it is handed to the axum app. +/// +/// Because `hyper::service::service_fn` bypasses axum's make-service, the +/// context the public HTTP path gets for free must be injected by hand here: +/// * the peer's validated [`CoreSpiffeId`], when present; +/// * the raw TCP peer address in the same [`ConnectInfo`] +/// extension the public listener populates via +/// `into_make_service_with_connect_info` (issue #358), so `client.addr` +/// is captured on the internal interface too; +/// * the [`Interface`] the request arrived on. +fn attach_request_context( + extensions: &mut axum::http::Extensions, + spiffe_id: Option, + peer_addr: std::net::SocketAddr, + interface: Interface, +) { + if let Some(spiffe_id) = spiffe_id { + // Move the client TLS certificate into the request extensions + tracing::debug!( + "The client supplied certificate for spiffe_id: {:?}", + spiffe_id + ); + extensions.insert(spiffe_id); + } + extensions.insert(ConnectInfo(peer_addr)); + extensions.insert(interface); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn attaches_connect_info_and_interface_without_spiffe_id() { + let mut ext = axum::http::Extensions::new(); + let peer: std::net::SocketAddr = "203.0.113.7:4711".parse().unwrap(); + + attach_request_context(&mut ext, None, peer, Interface::Internal); + + // Issue #358: the mTLS peer address is captured in the same extension + // type the public listener uses, so `client.addr` is populated here. + assert_eq!( + ext.get::>() + .map(|ci| ci.0), + Some(peer) + ); + assert_eq!(ext.get::(), Some(&Interface::Internal)); + // No SVID presented → no SpiffeId extension. + assert!(ext.get::().is_none()); + } + + #[test] + fn attaches_spiffe_id_when_present() { + let mut ext = axum::http::Extensions::new(); + let peer: std::net::SocketAddr = "[2001:db8::1]:8443".parse().unwrap(); + let spiffe_id = CoreSpiffeId::new("spiffe://example.org/workload").unwrap(); + + attach_request_context(&mut ext, Some(spiffe_id.clone()), peer, Interface::Internal); + + assert_eq!(ext.get::(), Some(&spiffe_id)); + assert_eq!( + ext.get::>() + .map(|ci| ci.0), + Some(peer) + ); + } +} diff --git a/crates/keystone/src/server/proxy_headers.rs b/crates/keystone/src/server/proxy_headers.rs new file mode 100644 index 000000000..e4f6c5d73 --- /dev/null +++ b/crates/keystone/src/server/proxy_headers.rs @@ -0,0 +1,267 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 +//! # Proxy forwarded-header parsing (issue #358) +//! +//! When Keystone runs behind a trusted reverse proxy / load balancer, the raw +//! TCP peer captured in [`ConnectInfo`] is the proxy's address, not +//! the real client's. This middleware recovers the originating client address +//! from the standard forwarding headers and overwrites `ConnectInfo` with it, +//! so every downstream consumer (the `request` tracing span, the API-Key IP +//! allowlist, any future IP-based login control) transparently observes the +//! real client. +//! +//! It mirrors upstream Python Keystone's `[oslo_middleware] +//! enable_proxy_headers_parsing` (oslo.middleware `HTTPProxyToWSGI`): the +//! [`rewrite_client_addr`] layer is only wired onto the **public** listener +//! when that flag is enabled, and it is **off by default**. Enabling it asserts +//! that the immediate peer is a trusted proxy; a deployment not behind such a +//! proxy leaves it off and cannot be tricked into trusting a spoofed header. +//! +//! [RFC 7239] `Forwarded` is honoured first; `X-Forwarded-For` is the fallback. +//! In both the originating client is the **leftmost** entry (each proxy appends +//! the peer it received the request from), matching the trusted-proxy model. +//! +//! [RFC 7239]: https://datatracker.ietf.org/doc/html/rfc7239 + +use std::net::{IpAddr, Ipv6Addr, SocketAddr}; + +use axum::extract::ConnectInfo; +use axum::extract::Request; +use axum::http::HeaderMap; +use axum::middleware::Next; +use axum::response::Response; + +/// Axum middleware that overwrites the request's [`ConnectInfo`] +/// with the proxy-resolved client address when a forwarding header is present. +/// +/// Wired onto the public listener only, and only when +/// `[oslo_middleware] enable_proxy_headers_parsing` is on — so its mere +/// presence in the stack already means the operator has opted in to trusting +/// the peer's forwarding headers. The recovered address has no meaningful +/// source port, so port `0` is used. +pub async fn rewrite_client_addr(mut req: Request, next: Next) -> Response { + if let Some(ip) = resolve_forwarded_ip(req.headers()) { + req.extensions_mut() + .insert(ConnectInfo(SocketAddr::new(ip, 0))); + } + next.run(req).await +} + +/// Resolve the originating client IP from forwarding headers, preferring +/// RFC 7239 `Forwarded` over `X-Forwarded-For`. Returns `None` when neither is +/// present or parseable (e.g. an obfuscated `for=_hidden` identifier), leaving +/// the raw peer address untouched. +fn resolve_forwarded_ip(headers: &HeaderMap) -> Option { + if let Some(ip) = headers + .get("forwarded") + .and_then(|v| v.to_str().ok()) + .and_then(parse_forwarded_client) + { + return Some(ip); + } + + headers + .get("x-forwarded-for") + .and_then(|v| v.to_str().ok()) + .and_then(|s| s.split(',').next()) + .and_then(|s| parse_ip_maybe_port(s.trim())) +} + +/// Extract the client IP from the leftmost element's `for=` parameter of an +/// RFC 7239 `Forwarded` header value. +fn parse_forwarded_client(value: &str) -> Option { + let first = value.split(',').next()?; + for param in first.split(';') { + let mut kv = param.trim().splitn(2, '='); + let key = kv.next()?.trim(); + if key.eq_ignore_ascii_case("for") { + let raw = kv.next()?.trim().trim_matches('"'); + return parse_ip_maybe_port(raw); + } + } + None +} + +/// Parse an address token that may be a bare IP, an `ip:port`, or a bracketed +/// IPv6 (`[::1]` / `[::1]:443`), returning just the IP. Obfuscated or malformed +/// tokens yield `None`. +fn parse_ip_maybe_port(s: &str) -> Option { + let s = s.trim(); + if s.is_empty() { + return None; + } + // Bracketed IPv6 (RFC 7239 form), with an optional trailing port. + if let Some(rest) = s.strip_prefix('[') { + let end = rest.find(']')?; + return rest[..end].parse::().ok().map(IpAddr::V6); + } + // `ip:port` (only unambiguous for IPv4; a bare IPv6 has many colons and is + // handled by the bare-IP branch below). + if let Ok(sa) = s.parse::() { + return Some(sa.ip()); + } + // Bare IPv4 or IPv6, no port. + s.parse::().ok() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn headers(pairs: &[(&str, &str)]) -> HeaderMap { + let mut h = HeaderMap::new(); + for (k, v) in pairs { + h.insert( + axum::http::HeaderName::from_bytes(k.as_bytes()).unwrap(), + v.parse().unwrap(), + ); + } + h + } + + fn ip(s: &str) -> IpAddr { + s.parse().unwrap() + } + + #[test] + fn no_headers_yields_none() { + assert_eq!(resolve_forwarded_ip(&HeaderMap::new()), None); + } + + #[test] + fn xff_takes_leftmost_originating_client() { + // client, proxy1, proxy2 — leftmost is the origin. + let h = headers(&[("x-forwarded-for", "203.0.113.7, 10.0.0.1, 10.0.0.2")]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.7"))); + } + + #[test] + fn forwarded_header_takes_precedence_over_xff() { + let h = headers(&[ + ("forwarded", "for=203.0.113.9"), + ("x-forwarded-for", "198.51.100.1"), + ]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.9"))); + } + + #[test] + fn forwarded_parses_leftmost_for_with_other_params() { + let h = headers(&[( + "forwarded", + "for=203.0.113.9;proto=https;by=203.0.113.43, for=198.51.100.17", + )]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.9"))); + } + + #[test] + fn forwarded_parses_quoted_ipv6_with_port() { + let h = headers(&[("forwarded", r#"for="[2001:db8:cafe::17]:4711""#)]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("2001:db8:cafe::17"))); + } + + #[test] + fn forwarded_obfuscated_identifier_is_ignored() { + let h = headers(&[("forwarded", "for=_hidden")]); + assert_eq!(resolve_forwarded_ip(&h), None); + } + + #[test] + fn forwarded_case_insensitive_for_key() { + let h = headers(&[("forwarded", "For=203.0.113.9")]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.9"))); + } + + #[test] + fn xff_ipv4_with_port_strips_port() { + let h = headers(&[("x-forwarded-for", "203.0.113.7:5555")]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.7"))); + } + + #[test] + fn xff_bare_ipv6_is_parsed() { + let h = headers(&[("x-forwarded-for", "2001:db8::1, 10.0.0.1")]); + assert_eq!(resolve_forwarded_ip(&h), Some(ip("2001:db8::1"))); + } + + #[test] + fn garbage_xff_yields_none() { + let h = headers(&[("x-forwarded-for", "not-an-ip")]); + assert_eq!(resolve_forwarded_ip(&h), None); + } + + #[tokio::test] + async fn middleware_overwrites_connect_info_when_forwarded_present() { + use axum::body::Body; + use axum::routing::get; + use axum::{Router, ServiceExt as AxumServiceExt}; + use tower::ServiceExt as _; + + async fn echo(ConnectInfo(addr): ConnectInfo) -> String { + addr.ip().to_string() + } + + let app = Router::new() + .route("/echo", get(echo)) + .layer(axum::middleware::from_fn(rewrite_client_addr)); + let make = + AxumServiceExt::::into_make_service_with_connect_info::(app); + // Raw TCP peer is the proxy (10.0.0.9); the header carries the client. + let peer: SocketAddr = "10.0.0.9:1111".parse().unwrap(); + let svc = make.oneshot(peer).await.unwrap(); + + let resp = svc + .oneshot( + Request::builder() + .uri("/echo") + .header("x-forwarded-for", "203.0.113.7") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!(&body[..], b"203.0.113.7"); + } + + #[tokio::test] + async fn middleware_preserves_peer_when_no_forwarded_header() { + use axum::body::Body; + use axum::routing::get; + use axum::{Router, ServiceExt as AxumServiceExt}; + use tower::ServiceExt as _; + + async fn echo(ConnectInfo(addr): ConnectInfo) -> String { + addr.ip().to_string() + } + + let app = Router::new() + .route("/echo", get(echo)) + .layer(axum::middleware::from_fn(rewrite_client_addr)); + let make = + AxumServiceExt::::into_make_service_with_connect_info::(app); + let peer: SocketAddr = "203.0.113.50:2222".parse().unwrap(); + let svc = make.oneshot(peer).await.unwrap(); + + let resp = svc + .oneshot(Request::builder().uri("/echo").body(Body::empty()).unwrap()) + .await + .unwrap(); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!(&body[..], b"203.0.113.50"); + } +} From e64d3a0882e559fcde5f677d1e14cd4ea463aa2e Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Sat, 4 Jul 2026 01:12:26 +0300 Subject: [PATCH 2/2] feat(api): Trusted-proxy header extraction Address review feedback: the previous middleware trusted any peer once the flag was on and took the leftmost `X-Forwarded-For` entry, which a direct client can trivially spoof. Extraction now follows the trusted-proxy model already used by the API-key ingress: - Add `[oslo_middleware] trusted_proxies`, a CIDR allowlist. The header is honoured only when the immediate TCP peer falls within it; an empty list (the default) trusts no one, so the raw peer is always kept. - Resolve the client by walking the chain right-to-left and returning the first address that is not itself a trusted proxy, instead of blindly taking the leftmost entry. - Cap the number of parsed hops to guard against an unbounded comma-list (parsing DoS). - Apply the same right-to-left walk to the RFC 7239 `Forwarded` header. The resolution logic is extracted into a shared `core::api::forwarded` module so the SCIM API-key ingress and this middleware use one implementation rather than two near-identical copies; the shared version also gains `Forwarded` support and the hop cap. `ConnectInfo` is only overwritten when a different upstream client is recovered, so a direct client's source port is preserved. Also drop the issue-tracker reference from the `[oslo_middleware]` config doc comment. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Yousef Hussein --- crates/config/src/lib.rs | 2 +- crates/config/src/oslo_middleware.rs | 44 ++- crates/core/src/api.rs | 1 + crates/core/src/api/api_key_auth.rs | 118 +------- crates/core/src/api/forwarded.rs | 261 +++++++++++++++++ crates/keystone/src/api/mod.rs | 15 +- crates/keystone/src/bin/keystone.rs | 37 ++- crates/keystone/src/server/proxy_headers.rs | 306 +++++++------------- 8 files changed, 432 insertions(+), 352 deletions(-) create mode 100644 crates/core/src/api/forwarded.rs diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 04d9d643f..ae389e40a 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -204,7 +204,7 @@ pub struct Config { #[serde(default)] pub mapping: MappingProvider, - /// `[oslo_middleware]` configuration (proxy header parsing, issue #358). + /// `[oslo_middleware]` configuration (proxy header parsing). #[serde(default)] pub oslo_middleware: OsloMiddleware, diff --git a/crates/config/src/oslo_middleware.rs b/crates/config/src/oslo_middleware.rs index b4ecfb637..7db8c88bf 100644 --- a/crates/config/src/oslo_middleware.rs +++ b/crates/config/src/oslo_middleware.rs @@ -14,10 +14,12 @@ //! # `[oslo_middleware]` configuration //! //! Mirrors the subset of upstream Python Keystone's `[oslo_middleware]` -//! section that is relevant to client-address capture (issue #358). +//! section that is relevant to client-address capture. use serde::Deserialize; +use crate::common::csv; + /// `[oslo_middleware]` section. #[derive(Debug, Deserialize, Clone, Default)] pub struct OsloMiddleware { @@ -27,14 +29,21 @@ pub struct OsloMiddleware { /// in `ConnectInfo`. /// /// The name and default (**off**) match upstream oslo.middleware's - /// `HTTPProxyToWSGI`. It MUST only be enabled when Keystone sits behind a - /// trusted reverse proxy / load balancer: with it on, the immediate peer is - /// trusted to have set these headers honestly, so a client able to reach - /// the listener directly could otherwise spoof its apparent address. Off by - /// default, a deployment that is not actually behind a trusted proxy cannot - /// be tricked into trusting a forged header. + /// `HTTPProxyToWSGI`. Even when enabled, a header is only honoured when the + /// immediate TCP peer matches [`trusted_proxies`](Self::trusted_proxies), so + /// a client reaching the listener directly cannot spoof its address. #[serde(default)] pub enable_proxy_headers_parsing: bool, + + /// CIDR blocks of reverse proxies trusted to set the forwarding headers + /// (e.g. `10.0.0.0/8, 192.168.0.0/16`). The client address is recovered + /// only when the immediate TCP peer falls within one of these ranges; the + /// effective client is then the rightmost address in the forwarding chain + /// that is not itself a trusted proxy. Empty (the default) means no proxy + /// is trusted, so the raw peer address is always used even when parsing is + /// enabled. + #[serde(default, deserialize_with = "csv")] + pub trusted_proxies: Vec, } #[cfg(test)] @@ -61,4 +70,25 @@ mod tests { let sot: OsloMiddleware = c.try_deserialize().unwrap(); assert!(sot.enable_proxy_headers_parsing); } + + #[test] + fn defaults_to_no_trusted_proxies() { + assert!(OsloMiddleware::default().trusted_proxies.is_empty()); + } + + #[test] + fn parses_trusted_proxies_csv_from_ini() { + let c = Config::builder() + .add_source(File::from_str( + "trusted_proxies = 10.0.0.0/8,192.168.0.0/16", + FileFormat::Ini, + )) + .build() + .unwrap(); + let sot: OsloMiddleware = c.try_deserialize().unwrap(); + assert_eq!( + sot.trusted_proxies, + vec!["10.0.0.0/8".to_string(), "192.168.0.0/16".to_string()] + ); + } } diff --git a/crates/core/src/api.rs b/crates/core/src/api.rs index b10d92a46..bb8a5b1cc 100644 --- a/crates/core/src/api.rs +++ b/crates/core/src/api.rs @@ -15,6 +15,7 @@ pub mod api_key_auth; pub mod auth; pub mod common; +pub mod forwarded; pub mod v3; pub mod v4; diff --git a/crates/core/src/api/api_key_auth.rs b/crates/core/src/api/api_key_auth.rs index 663c0b508..09b951b2c 100644 --- a/crates/core/src/api/api_key_auth.rs +++ b/crates/core/src/api/api_key_auth.rs @@ -34,6 +34,7 @@ use openstack_keystone_core_types::mapping::resolution::IdentitySource; use openstack_keystone_core_types::mapping::virtual_user::MatchResult; use crate::api::KeystoneApiError; +use crate::api::forwarded::resolve_client_ip; use crate::api_key::{crypto, token}; use crate::auth::{ExecutionContext, ValidatedSecurityContext}; use crate::keystone::ServiceState; @@ -214,51 +215,6 @@ fn spawn_last_used_update(state: &ServiceState, resource: &ApiClientResource, no }); } -/// Compute the effective client IP using the rightmost-non-trusted-proxy -/// algorithm (ADR 0021 §3 Step 2, §6.E, Invariant 4): append the raw TCP -/// peer to the right of the `X-Forwarded-For` chain, then walk right to -/// left, returning the first address not in `trusted_proxies`. If the raw -/// TCP peer itself is not trusted, it is used directly without consulting -/// XFF at all. -fn resolve_client_ip( - headers: &axum::http::HeaderMap, - peer_ip: Option, - trusted_proxies: &[String], -) -> Option { - let trusted: Vec = trusted_proxies - .iter() - .filter_map(|c| c.parse::().ok()) - .collect(); - - let is_trusted = |ip: &IpAddr| trusted.iter().any(|net| net.contains(ip)); - - let peer = peer_ip?; - if !is_trusted(&peer) { - return Some(peer); - } - - let xff_chain: Vec = headers - .get(axum::http::header::HeaderName::from_static( - "x-forwarded-for", - )) - .and_then(|h| h.to_str().ok()) - .map(|h| { - h.split(',') - .filter_map(|s| s.trim().parse::().ok()) - .collect() - }) - .unwrap_or_default(); - - let mut chain = xff_chain; - chain.push(peer); - - chain - .into_iter() - .rev() - .find(|ip| !is_trusted(ip)) - .or(Some(peer)) -} - /// Whether `client_ip` satisfies the key's `allowed_ips` CIDR allowlist. /// `None` (missing field) means no restriction applies (ADR 0021 Invariant /// 5); a missing field and `Some(vec![])` are treated identically. @@ -394,83 +350,11 @@ async fn hydrate_ephemeral_context( #[cfg(test)] mod tests { use super::*; - use axum::http::HeaderMap; - - fn headers_with_xff(xff: &str) -> HeaderMap { - let mut headers = HeaderMap::new(); - headers.insert( - axum::http::header::HeaderName::from_static("x-forwarded-for"), - xff.parse().unwrap(), - ); - headers - } fn ip(s: &str) -> IpAddr { s.parse().unwrap() } - // --------------------------------------------------------------------- - // resolve_client_ip (ADR 0021 §3 Step 2, §6.E, Invariant 4) - // --------------------------------------------------------------------- - - #[test] - fn untrusted_peer_ignores_xff_entirely() { - // Peer itself is not a trusted proxy: XFF must not be consulted at - // all, even if present (prevents spoofing via an untrusted hop). - let headers = headers_with_xff("1.2.3.4"); - let peer = Some(ip("203.0.113.5")); - let trusted = vec!["10.0.0.0/8".to_string()]; - assert_eq!( - resolve_client_ip(&headers, peer, &trusted), - Some(ip("203.0.113.5")) - ); - } - - #[test] - fn trusted_peer_walks_xff_rightmost_non_trusted() { - // Chain (left to right): 1.2.3.4 (attacker-controlled), 10.0.0.5 - // (trusted intermediate hop), peer 10.0.0.1 (trusted, terminal - // proxy). Effective IP must be 10.0.0.5's predecessor scanning - // right-to-left: append peer, walk right-to-left, first non-trusted. - let headers = headers_with_xff("1.2.3.4, 10.0.0.5"); - let peer = Some(ip("10.0.0.1")); - let trusted = vec!["10.0.0.0/8".to_string()]; - assert_eq!( - resolve_client_ip(&headers, peer, &trusted), - Some(ip("1.2.3.4")) - ); - } - - #[test] - fn trusted_peer_all_hops_trusted_falls_back_to_peer() { - let headers = headers_with_xff("10.0.0.9"); - let peer = Some(ip("10.0.0.1")); - let trusted = vec!["10.0.0.0/8".to_string()]; - assert_eq!( - resolve_client_ip(&headers, peer, &trusted), - Some(ip("10.0.0.1")) - ); - } - - #[test] - fn leftmost_xff_entry_is_never_trusted_blindly() { - // Regression guard for the leftmost-take vulnerability (ADR 0021 F2): - // an attacker prepending a spoofed IP as the leftmost XFF entry must - // not be accepted just because it's present. - let headers = headers_with_xff("203.0.113.99, 1.2.3.4, 10.0.0.5"); - let peer = Some(ip("10.0.0.1")); - let trusted = vec!["10.0.0.0/8".to_string()]; - let effective = resolve_client_ip(&headers, peer, &trusted); - assert_ne!(effective, Some(ip("203.0.113.99"))); - assert_eq!(effective, Some(ip("1.2.3.4"))); - } - - #[test] - fn no_peer_ip_resolves_to_none() { - let headers = HeaderMap::new(); - assert_eq!(resolve_client_ip(&headers, None, &[]), None); - } - // --------------------------------------------------------------------- // ip_allowed (ADR 0021 Invariant 5) // --------------------------------------------------------------------- diff --git a/crates/core/src/api/forwarded.rs b/crates/core/src/api/forwarded.rs new file mode 100644 index 000000000..ea9de812d --- /dev/null +++ b/crates/core/src/api/forwarded.rs @@ -0,0 +1,261 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 +//! # Client-IP resolution from proxy forwarding headers +//! +//! Shared by the API-key ingress (ADR 0021 §3) and the public-listener proxy +//! header middleware. It implements the trusted-proxy model: forwarding headers +//! are consulted **only** when the immediate TCP peer is a configured trusted +//! proxy, and the effective client is the rightmost address in the chain that +//! is not itself a trusted proxy. A client able to reach the listener directly +//! therefore cannot spoof its apparent address by prepending an +//! `X-Forwarded-For`/`Forwarded` entry. + +use std::net::{IpAddr, Ipv6Addr, SocketAddr}; + +use axum::http::HeaderMap; +use ipnet::IpNet; + +/// Upper bound on forwarding-chain entries parsed, guarding against an +/// unbounded comma list (parsing DoS). Only the rightmost hops are relevant to +/// the rightmost-non-trusted walk, so any excess left-hand entries are ignored. +const MAX_FORWARDED_HOPS: usize = 10; + +/// Resolve the effective client IP behind trusted reverse proxies. +/// +/// The raw TCP `peer_ip` is appended to the right of the forwarding chain +/// (RFC 7239 `Forwarded` preferred, else `X-Forwarded-For`); the chain is then +/// walked right to left, returning the first address that is not a member of +/// `trusted_proxies`. If the immediate peer is itself not trusted, the headers +/// are ignored entirely and the peer is returned unchanged. +/// +/// `trusted_proxies` is a list of CIDR blocks (e.g. `10.0.0.0/8`); unparsable +/// entries are skipped. Returns `None` only when no peer address is available. +pub fn resolve_client_ip( + headers: &HeaderMap, + peer_ip: Option, + trusted_proxies: &[String], +) -> Option { + let trusted: Vec = trusted_proxies + .iter() + .filter_map(|c| c.trim().parse::().ok()) + .collect(); + let is_trusted = |ip: &IpAddr| trusted.iter().any(|net| net.contains(ip)); + + let peer = peer_ip?; + // The immediate peer must be a trusted proxy before any header is honoured. + if !is_trusted(&peer) { + return Some(peer); + } + + let mut chain = forwarded_chain(headers); + chain.push(peer); + chain + .into_iter() + .rev() + .find(|ip| !is_trusted(ip)) + .or(Some(peer)) +} + +/// Parse the forwarding chain in left-to-right order, capped to the rightmost +/// [`MAX_FORWARDED_HOPS`] entries. Prefers the RFC 7239 `Forwarded` header, +/// falling back to `X-Forwarded-For`. +fn forwarded_chain(headers: &HeaderMap) -> Vec { + if let Some(value) = headers.get("forwarded").and_then(|v| v.to_str().ok()) { + return capped_chain(value, forwarded_element_ip); + } + if let Some(value) = headers.get("x-forwarded-for").and_then(|v| v.to_str().ok()) { + return capped_chain(value, |s| parse_ip_maybe_port(s.trim())); + } + Vec::new() +} + +/// Take the rightmost [`MAX_FORWARDED_HOPS`] comma-separated elements of a +/// header value, parse each with `parse`, and return them in left-to-right +/// order. Bounding the split before parsing caps the work an attacker can force. +fn capped_chain(value: &str, parse: impl Fn(&str) -> Option) -> Vec { + let mut ips: Vec = value + .rsplit(',') + .take(MAX_FORWARDED_HOPS) + .filter_map(parse) + .collect(); + ips.reverse(); + ips +} + +/// Extract the `for=` IP from a single RFC 7239 `Forwarded` list element such as +/// `for=192.0.2.60;proto=http;by=203.0.113.43`. Obfuscated identifiers +/// (`for=_hidden`) yield `None`. +fn forwarded_element_ip(element: &str) -> Option { + for param in element.split(';') { + let mut kv = param.trim().splitn(2, '='); + let key = kv.next()?.trim(); + if key.eq_ignore_ascii_case("for") { + let raw = kv.next()?.trim().trim_matches('"'); + return parse_ip_maybe_port(raw); + } + } + None +} + +/// Parse an address token that may be a bare IP, an `ip:port`, or a bracketed +/// IPv6 (`[::1]` / `[::1]:443`), returning just the IP. Obfuscated or malformed +/// tokens yield `None`. +fn parse_ip_maybe_port(s: &str) -> Option { + let s = s.trim(); + if s.is_empty() { + return None; + } + // Bracketed IPv6 (RFC 7239 form), with an optional trailing port. + if let Some(rest) = s.strip_prefix('[') { + let end = rest.find(']')?; + return rest[..end].parse::().ok().map(IpAddr::V6); + } + // `ip:port` (only unambiguous for IPv4; a bare IPv6 has many colons and is + // handled by the bare-IP branch below). + if let Ok(sa) = s.parse::() { + return Some(sa.ip()); + } + // Bare IPv4 or IPv6, no port. + s.parse::().ok() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn headers(pairs: &[(&str, &str)]) -> HeaderMap { + let mut h = HeaderMap::new(); + for (k, v) in pairs { + h.insert( + axum::http::HeaderName::from_bytes(k.as_bytes()).unwrap(), + v.parse().unwrap(), + ); + } + h + } + + fn ip(s: &str) -> IpAddr { + s.parse().unwrap() + } + + const TRUSTED: &[&str] = &["10.0.0.0/8"]; + + fn trusted() -> Vec { + TRUSTED.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn no_peer_ip_resolves_to_none() { + assert_eq!(resolve_client_ip(&HeaderMap::new(), None, &trusted()), None); + } + + #[test] + fn untrusted_peer_ignores_headers_entirely() { + // Peer is not a trusted proxy: the header must not be consulted at all, + // even when present (prevents spoofing via a direct connection). + let h = headers(&[("x-forwarded-for", "1.2.3.4")]); + let peer = Some(ip("203.0.113.5")); + assert_eq!( + resolve_client_ip(&h, peer, &trusted()), + Some(ip("203.0.113.5")) + ); + } + + #[test] + fn trusted_peer_walks_xff_rightmost_non_trusted() { + let h = headers(&[("x-forwarded-for", "1.2.3.4, 10.0.0.5")]); + let peer = Some(ip("10.0.0.1")); + assert_eq!(resolve_client_ip(&h, peer, &trusted()), Some(ip("1.2.3.4"))); + } + + #[test] + fn leftmost_xff_entry_is_never_trusted_blindly() { + // Attacker prepends a spoofed leftmost entry; it must not be returned. + let h = headers(&[("x-forwarded-for", "203.0.113.99, 1.2.3.4, 10.0.0.5")]); + let peer = Some(ip("10.0.0.1")); + let effective = resolve_client_ip(&h, peer, &trusted()); + assert_ne!(effective, Some(ip("203.0.113.99"))); + assert_eq!(effective, Some(ip("1.2.3.4"))); + } + + #[test] + fn all_hops_trusted_falls_back_to_peer() { + let h = headers(&[("x-forwarded-for", "10.0.0.9")]); + let peer = Some(ip("10.0.0.1")); + assert_eq!( + resolve_client_ip(&h, peer, &trusted()), + Some(ip("10.0.0.1")) + ); + } + + #[test] + fn forwarded_header_is_preferred_and_walked_right_to_left() { + let h = headers(&[ + ( + "forwarded", + "for=203.0.113.7;proto=https, for=10.0.0.5;proto=https", + ), + ("x-forwarded-for", "198.51.100.1"), + ]); + let peer = Some(ip("10.0.0.1")); + assert_eq!( + resolve_client_ip(&h, peer, &trusted()), + Some(ip("203.0.113.7")) + ); + } + + #[test] + fn forwarded_quoted_ipv6_with_port_is_parsed() { + let h = headers(&[("forwarded", r#"for="[2001:db8:cafe::17]:4711""#)]); + let peer = Some(ip("10.0.0.1")); + assert_eq!( + resolve_client_ip(&h, peer, &trusted()), + Some(ip("2001:db8:cafe::17")) + ); + } + + #[test] + fn forwarded_obfuscated_identifier_falls_back_to_peer() { + let h = headers(&[("forwarded", "for=_hidden")]); + let peer = Some(ip("10.0.0.1")); + assert_eq!( + resolve_client_ip(&h, peer, &trusted()), + Some(ip("10.0.0.1")) + ); + } + + #[test] + fn xff_ipv4_with_port_strips_port() { + let h = headers(&[("x-forwarded-for", "203.0.113.7:5555")]); + let peer = Some(ip("10.0.0.1")); + assert_eq!( + resolve_client_ip(&h, peer, &trusted()), + Some(ip("203.0.113.7")) + ); + } + + #[test] + fn hop_count_is_capped_against_long_chains() { + // 12 spoofed public entries followed by two trusted hops and the peer. + // The cap keeps only the rightmost 10 parsed hops, but the real client + // (first non-trusted from the right) is still resolved correctly. + let spoof = std::iter::repeat_n("203.0.113.1", 12) + .collect::>() + .join(", "); + let value = format!("{spoof}, 8.8.8.8, 10.0.0.5"); + let h = headers(&[("x-forwarded-for", value.as_str())]); + let peer = Some(ip("10.0.0.1")); + assert_eq!(resolve_client_ip(&h, peer, &trusted()), Some(ip("8.8.8.8"))); + } +} diff --git a/crates/keystone/src/api/mod.rs b/crates/keystone/src/api/mod.rs index 9fba1cdea..c47e0ccd6 100644 --- a/crates/keystone/src/api/mod.rs +++ b/crates/keystone/src/api/mod.rs @@ -235,11 +235,16 @@ pub(crate) mod tests { // Mirror `build_router`: the API service is `NormalizePath`-wrapped and // mounted as the outer router's fallback. let normalized = NormalizePathLayer::trim_trailing_slash().layer(router); - let app = Router::new() - .fallback_service(normalized) - .layer(axum::middleware::from_fn( - crate::server::proxy_headers::rewrite_client_addr, - )); + // The raw peer (10.0.0.9) must be a trusted proxy for the header to be + // honoured, mirroring the `[oslo_middleware] trusted_proxies` allowlist. + let trusted = std::sync::Arc::new(vec!["10.0.0.0/8".to_string()]); + let app = + Router::new() + .fallback_service(normalized) + .layer(axum::middleware::from_fn_with_state( + trusted, + crate::server::proxy_headers::rewrite_client_addr, + )); let make = AxumServiceExt::>::into_make_service_with_connect_info::(app); diff --git a/crates/keystone/src/bin/keystone.rs b/crates/keystone/src/bin/keystone.rs index cda184ee0..9135e3513 100644 --- a/crates/keystone/src/bin/keystone.rs +++ b/crates/keystone/src/bin/keystone.rs @@ -619,11 +619,11 @@ async fn build_router( // REMOTE_ADDR / flask.request.remote_addr): the raw TCP peer // on the public listener via // `into_make_service_with_connect_info`, or the mTLS peer on - // the internal SPIFFE-TLS listener (injected by hand, see - // issue #358). When `enable_proxy_headers_parsing` is on, the - // public value has been overwritten with the proxy-resolved - // client address. `None` on the admin UDS interface, which - // has no meaningful `SocketAddr`. + // the internal SPIFFE-TLS listener (injected by hand). When + // `enable_proxy_headers_parsing` is on, the public value has + // been overwritten with the proxy-resolved client address. + // `None` on the admin UDS interface, which has no meaningful + // `SocketAddr`. let client_addr = request .extensions() .get::>() @@ -792,12 +792,19 @@ async fn spawn_public_listener( // When operating behind a trusted reverse proxy (config-gated, // off by default), parse `Forwarded`/`X-Forwarded-For` and rewrite // the raw-peer `ConnectInfo` with the originating client address - // *before* the tracing span and handlers read it (issue #358). The - // layer is added only on this public interface — never on the - // internal SPIFFE/admin listeners, whose peers are the mTLS mesh. + // *before* the tracing span and handlers read it. The header is + // honoured only when the immediate peer matches `trusted_proxies`, + // so an empty allowlist keeps the raw peer. The layer is added only + // on this public interface — never on the internal SPIFFE/admin + // listeners, whose peers are the mTLS mesh. let rest_app = if cfg.oslo_middleware.enable_proxy_headers_parsing { - info!("Proxy header parsing enabled on the public interface"); - app.layer(axum::middleware::from_fn( + info!( + trusted_proxies = cfg.oslo_middleware.trusted_proxies.len(), + "Proxy header parsing enabled on the public interface" + ); + let trusted = Arc::new(cfg.oslo_middleware.trusted_proxies.clone()); + app.layer(axum::middleware::from_fn_with_state( + trusted, proxy_headers::rewrite_client_addr, )) } else { @@ -812,11 +819,11 @@ async fn spawn_public_listener( // `into_make_service_with_connect_info::` stores the // raw TCP peer address in a `ConnectInfo` request // extension (the analogue of Python Keystone's WSGI REMOTE_ADDR). - // This is the *raw* peer, not proxy-resolved: behind a reverse - // proxy/LB it is the proxy's address. A trusted forwarded-header - // layer (mirroring oslo_middleware's `enable_proxy_headers_parsing`, - // off by default) is a deliberate follow-up before this is used - // for any IP-based login control. See issue #358. + // Behind a reverse proxy/LB this is the proxy's address; the + // `rewrite_client_addr` layer wired above (when + // `enable_proxy_headers_parsing` is on) overwrites it with the + // trusted-proxy-resolved client address before any consumer + // reads it. let cancel_token = rest_cancel_token.clone(); if let Err(e) = axum::serve( listener, diff --git a/crates/keystone/src/server/proxy_headers.rs b/crates/keystone/src/server/proxy_headers.rs index e4f6c5d73..36c445026 100644 --- a/crates/keystone/src/server/proxy_headers.rs +++ b/crates/keystone/src/server/proxy_headers.rs @@ -11,7 +11,7 @@ // limitations under the License. // // SPDX-License-Identifier: Apache-2.0 -//! # Proxy forwarded-header parsing (issue #358) +//! # Proxy forwarded-header parsing //! //! When Keystone runs behind a trusted reverse proxy / load balancer, the raw //! TCP peer captured in [`ConnectInfo`] is the proxy's address, not @@ -22,246 +22,138 @@ //! real client. //! //! It mirrors upstream Python Keystone's `[oslo_middleware] -//! enable_proxy_headers_parsing` (oslo.middleware `HTTPProxyToWSGI`): the -//! [`rewrite_client_addr`] layer is only wired onto the **public** listener -//! when that flag is enabled, and it is **off by default**. Enabling it asserts -//! that the immediate peer is a trusted proxy; a deployment not behind such a -//! proxy leaves it off and cannot be tricked into trusting a spoofed header. +//! enable_proxy_headers_parsing`: the [`rewrite_client_addr`] layer is only +//! wired onto the **public** listener when that flag is enabled, and it is +//! **off by default**. //! -//! [RFC 7239] `Forwarded` is honoured first; `X-Forwarded-For` is the fallback. -//! In both the originating client is the **leftmost** entry (each proxy appends -//! the peer it received the request from), matching the trusted-proxy model. -//! -//! [RFC 7239]: https://datatracker.ietf.org/doc/html/rfc7239 +//! Extraction is not blind. The header is honoured only when the immediate TCP +//! peer matches the operator-configured `trusted_proxies` allowlist, and the +//! effective client is the rightmost address in the chain that is not itself a +//! trusted proxy — the same rightmost-non-trusted-proxy algorithm the API-Key +//! ingress uses ([`openstack_keystone_core::api::forwarded`]). A client able to +//! reach the listener directly therefore cannot spoof its apparent address, and +//! an empty allowlist trusts no one (the raw peer is always kept). -use std::net::{IpAddr, Ipv6Addr, SocketAddr}; +use std::net::SocketAddr; +use std::sync::Arc; use axum::extract::ConnectInfo; use axum::extract::Request; -use axum::http::HeaderMap; +use axum::extract::State; use axum::middleware::Next; use axum::response::Response; +use openstack_keystone_core::api::forwarded::resolve_client_ip; + /// Axum middleware that overwrites the request's [`ConnectInfo`] -/// with the proxy-resolved client address when a forwarding header is present. +/// with the proxy-resolved client address when the immediate peer is a trusted +/// proxy and a forwarding header carries a different upstream client. /// -/// Wired onto the public listener only, and only when -/// `[oslo_middleware] enable_proxy_headers_parsing` is on — so its mere -/// presence in the stack already means the operator has opted in to trusting -/// the peer's forwarding headers. The recovered address has no meaningful -/// source port, so port `0` is used. -pub async fn rewrite_client_addr(mut req: Request, next: Next) -> Response { - if let Some(ip) = resolve_forwarded_ip(req.headers()) { +/// `trusted_proxies` is the operator-configured CIDR allowlist (`[oslo_middleware] +/// trusted_proxies`). When the resolved client equals the raw peer (direct +/// client, untrusted peer, or an all-trusted chain), `ConnectInfo` is left +/// untouched so the real source port is preserved. The recovered address has no +/// meaningful source port, so port `0` is used. +pub async fn rewrite_client_addr( + State(trusted_proxies): State>>, + mut req: Request, + next: Next, +) -> Response { + let peer_ip = req + .extensions() + .get::>() + .map(|ConnectInfo(addr)| addr.ip()); + + if let Some(client_ip) = resolve_client_ip(req.headers(), peer_ip, &trusted_proxies) + && Some(client_ip) != peer_ip + { req.extensions_mut() - .insert(ConnectInfo(SocketAddr::new(ip, 0))); + .insert(ConnectInfo(SocketAddr::new(client_ip, 0))); } next.run(req).await } -/// Resolve the originating client IP from forwarding headers, preferring -/// RFC 7239 `Forwarded` over `X-Forwarded-For`. Returns `None` when neither is -/// present or parseable (e.g. an obfuscated `for=_hidden` identifier), leaving -/// the raw peer address untouched. -fn resolve_forwarded_ip(headers: &HeaderMap) -> Option { - if let Some(ip) = headers - .get("forwarded") - .and_then(|v| v.to_str().ok()) - .and_then(parse_forwarded_client) - { - return Some(ip); - } - - headers - .get("x-forwarded-for") - .and_then(|v| v.to_str().ok()) - .and_then(|s| s.split(',').next()) - .and_then(|s| parse_ip_maybe_port(s.trim())) -} - -/// Extract the client IP from the leftmost element's `for=` parameter of an -/// RFC 7239 `Forwarded` header value. -fn parse_forwarded_client(value: &str) -> Option { - let first = value.split(',').next()?; - for param in first.split(';') { - let mut kv = param.trim().splitn(2, '='); - let key = kv.next()?.trim(); - if key.eq_ignore_ascii_case("for") { - let raw = kv.next()?.trim().trim_matches('"'); - return parse_ip_maybe_port(raw); - } - } - None -} - -/// Parse an address token that may be a bare IP, an `ip:port`, or a bracketed -/// IPv6 (`[::1]` / `[::1]:443`), returning just the IP. Obfuscated or malformed -/// tokens yield `None`. -fn parse_ip_maybe_port(s: &str) -> Option { - let s = s.trim(); - if s.is_empty() { - return None; - } - // Bracketed IPv6 (RFC 7239 form), with an optional trailing port. - if let Some(rest) = s.strip_prefix('[') { - let end = rest.find(']')?; - return rest[..end].parse::().ok().map(IpAddr::V6); - } - // `ip:port` (only unambiguous for IPv4; a bare IPv6 has many colons and is - // handled by the bare-IP branch below). - if let Ok(sa) = s.parse::() { - return Some(sa.ip()); - } - // Bare IPv4 or IPv6, no port. - s.parse::().ok() -} - #[cfg(test)] mod tests { use super::*; - fn headers(pairs: &[(&str, &str)]) -> HeaderMap { - let mut h = HeaderMap::new(); - for (k, v) in pairs { - h.insert( - axum::http::HeaderName::from_bytes(k.as_bytes()).unwrap(), - v.parse().unwrap(), - ); - } - h - } - - fn ip(s: &str) -> IpAddr { - s.parse().unwrap() - } - - #[test] - fn no_headers_yields_none() { - assert_eq!(resolve_forwarded_ip(&HeaderMap::new()), None); - } - - #[test] - fn xff_takes_leftmost_originating_client() { - // client, proxy1, proxy2 — leftmost is the origin. - let h = headers(&[("x-forwarded-for", "203.0.113.7, 10.0.0.1, 10.0.0.2")]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.7"))); - } - - #[test] - fn forwarded_header_takes_precedence_over_xff() { - let h = headers(&[ - ("forwarded", "for=203.0.113.9"), - ("x-forwarded-for", "198.51.100.1"), - ]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.9"))); - } - - #[test] - fn forwarded_parses_leftmost_for_with_other_params() { - let h = headers(&[( - "forwarded", - "for=203.0.113.9;proto=https;by=203.0.113.43, for=198.51.100.17", - )]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.9"))); - } - - #[test] - fn forwarded_parses_quoted_ipv6_with_port() { - let h = headers(&[("forwarded", r#"for="[2001:db8:cafe::17]:4711""#)]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("2001:db8:cafe::17"))); - } - - #[test] - fn forwarded_obfuscated_identifier_is_ignored() { - let h = headers(&[("forwarded", "for=_hidden")]); - assert_eq!(resolve_forwarded_ip(&h), None); - } - - #[test] - fn forwarded_case_insensitive_for_key() { - let h = headers(&[("forwarded", "For=203.0.113.9")]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.9"))); - } - - #[test] - fn xff_ipv4_with_port_strips_port() { - let h = headers(&[("x-forwarded-for", "203.0.113.7:5555")]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("203.0.113.7"))); - } - - #[test] - fn xff_bare_ipv6_is_parsed() { - let h = headers(&[("x-forwarded-for", "2001:db8::1, 10.0.0.1")]); - assert_eq!(resolve_forwarded_ip(&h), Some(ip("2001:db8::1"))); - } - - #[test] - fn garbage_xff_yields_none() { - let h = headers(&[("x-forwarded-for", "not-an-ip")]); - assert_eq!(resolve_forwarded_ip(&h), None); - } - - #[tokio::test] - async fn middleware_overwrites_connect_info_when_forwarded_present() { - use axum::body::Body; - use axum::routing::get; - use axum::{Router, ServiceExt as AxumServiceExt}; - use tower::ServiceExt as _; - - async fn echo(ConnectInfo(addr): ConnectInfo) -> String { - addr.ip().to_string() - } - - let app = Router::new() - .route("/echo", get(echo)) - .layer(axum::middleware::from_fn(rewrite_client_addr)); + use axum::body::Body; + use axum::routing::get; + use axum::{Router, ServiceExt as AxumServiceExt}; + use tower::ServiceExt as _; + + async fn echo(ConnectInfo(addr): ConnectInfo) -> String { + addr.to_string() + } + + /// Build the app with the middleware carrying a fixed trusted-proxy list, + /// drive it with `peer` as the raw TCP peer, and return the address the + /// handler observed. + async fn observed_addr(trusted: &[&str], peer: &str, headers: &[(&str, &str)]) -> String { + let trusted = Arc::new(trusted.iter().map(|s| s.to_string()).collect::>()); + let app = + Router::new() + .route("/echo", get(echo)) + .layer(axum::middleware::from_fn_with_state( + trusted, + rewrite_client_addr, + )); let make = AxumServiceExt::::into_make_service_with_connect_info::(app); - // Raw TCP peer is the proxy (10.0.0.9); the header carries the client. - let peer: SocketAddr = "10.0.0.9:1111".parse().unwrap(); + let peer: SocketAddr = peer.parse().unwrap(); let svc = make.oneshot(peer).await.unwrap(); + let mut builder = Request::builder().uri("/echo"); + for (k, v) in headers { + builder = builder.header(*k, *v); + } let resp = svc - .oneshot( - Request::builder() - .uri("/echo") - .header("x-forwarded-for", "203.0.113.7") - .body(Body::empty()) - .unwrap(), - ) + .oneshot(builder.body(Body::empty()).unwrap()) .await .unwrap(); let body = axum::body::to_bytes(resp.into_body(), usize::MAX) .await .unwrap(); - assert_eq!(&body[..], b"203.0.113.7"); + String::from_utf8(body.to_vec()).unwrap() } #[tokio::test] - async fn middleware_preserves_peer_when_no_forwarded_header() { - use axum::body::Body; - use axum::routing::get; - use axum::{Router, ServiceExt as AxumServiceExt}; - use tower::ServiceExt as _; + async fn rewrites_to_client_when_peer_is_trusted() { + // Raw peer 10.0.0.9 is a trusted proxy; the XFF chain carries the client. + let addr = observed_addr( + &["10.0.0.0/8"], + "10.0.0.9:1111", + &[("x-forwarded-for", "203.0.113.7, 10.0.0.9")], + ) + .await; + // Rewritten to the client IP (with the conventional port 0). + assert_eq!(addr, "203.0.113.7:0"); + } - async fn echo(ConnectInfo(addr): ConnectInfo) -> String { - addr.ip().to_string() - } + #[tokio::test] + async fn ignores_header_when_peer_is_untrusted() { + // A direct (untrusted) client cannot spoof its address via the header; + // the raw peer address — including its real port — is preserved. + let addr = observed_addr( + &["10.0.0.0/8"], + "203.0.113.50:2222", + &[("x-forwarded-for", "1.2.3.4")], + ) + .await; + assert_eq!(addr, "203.0.113.50:2222"); + } - let app = Router::new() - .route("/echo", get(echo)) - .layer(axum::middleware::from_fn(rewrite_client_addr)); - let make = - AxumServiceExt::::into_make_service_with_connect_info::(app); - let peer: SocketAddr = "203.0.113.50:2222".parse().unwrap(); - let svc = make.oneshot(peer).await.unwrap(); + #[tokio::test] + async fn preserves_peer_when_no_forwarded_header() { + let addr = observed_addr(&["10.0.0.0/8"], "10.0.0.9:3333", &[]).await; + // Peer is trusted but there is no header, so it stays the peer. + assert_eq!(addr, "10.0.0.9:3333"); + } - let resp = svc - .oneshot(Request::builder().uri("/echo").body(Body::empty()).unwrap()) - .await - .unwrap(); - let body = axum::body::to_bytes(resp.into_body(), usize::MAX) - .await - .unwrap(); - assert_eq!(&body[..], b"203.0.113.50"); + #[tokio::test] + async fn empty_allowlist_trusts_no_one() { + // With no trusted proxies configured, the header is never honoured. + let addr = observed_addr(&[], "10.0.0.9:4444", &[("x-forwarded-for", "203.0.113.7")]).await; + assert_eq!(addr, "10.0.0.9:4444"); } }