feat(api): capture client IP via proxy headers & SPIFFE (#358 follow-up)#908
feat(api): capture client IP via proxy headers & SPIFFE (#358 follow-up)#908ymh1874 wants to merge 2 commits into
Conversation
22bd084 to
2a4b898
Compare
gtema
left a comment
There was a problem hiding this comment.
While it is good that you add certain compliance to oslo_middleware and rely on it's configuration, it does not by default implement sufficient security:
Proper model: don't blindly trust leftmost entry from any peer just cuz flag on. Standard practice (nginx, oslo.middleware, Go's httputil, etc):
-
Trusted-proxy allowlist, not just boolean flag.
Config should hold list of trusted proxy CIDRs (trusted_proxies = [...]). Middleware only rewrites ConnectInfo if immediate TCP peer is in that list. Else ignore header entirely — untrusted client hitting listener directly could forge X-Forwarded-For and set whatever IP it wants (bypass IP allowlist, spoof audit logs). -
Walk chain right-to-left, strip trusted hops.
X-Forwarded-For: client, proxy1, proxy2 — proxy2 (closest, direct peer) appended last. Correct algo:
- start from rightmost entry
- if it's a trusted proxy, pop it, check next-left
- first entry NOT in trusted set = real client
- leftmost entry alone (current impl) is trivially spoofable — attacker just prepends fake IP as first hop, no proxy needed to strip it since nothing validates chain length/authenticity of entries left of the real edge proxy.
-
Cap hop count / header length. Guard against unbounded comma list (DoS on parsing) — set max hops parsed (e.g. 5-10).
-
Same treatment for Forwarded header — parse for= chain right-to-left same way, by= optionally cross-checked against known proxy identity.
I would at least extend the oslo_middleware config with the 'trusted_proxies' to do more secure extraction
| #[serde(default)] | ||
| pub mapping: MappingProvider, | ||
|
|
||
| /// `[oslo_middleware]` configuration (proxy header parsing, issue #358). |
There was a problem hiding this comment.
you do not need to refer to the issue here. It make sense only for some critical behavior (vulnerabilities, bugs, etc), but not a regular feature trackers
There was a problem hiding this comment.
Done — removed the issue reference from the doc comment (and the other gratuitous #358 references in the touched comments).
2a4b898 to
45731b5
Compare
|
Thanks — you were right that the boolean flag plus leftmost-
As you noted this correlates with the API-key ingress, so rather than a second near-identical resolver I extracted the logic into a shared |
45731b5 to
93d3e46
Compare
Follow-up to openstack-experimental#842 for the reopened issue openstack-experimental#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<SocketAddr>` 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 (openstack-experimental#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 <ymh1874@gmail.com>
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 <noreply@anthropic.com> Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
93d3e46 to
e64d3a0
Compare
Follow-up to #842 for the reopened #358. #842 captured the raw TCP peer on the public HTTP listener; per the maintainer comment two gaps remained (proxy header parsing, SPIFFE), while UDS is acceptable to skip.
Proxy header parsing (config-gated, off by default)
New
[oslo_middleware] enable_proxy_headers_parsingflag mirroring upstream Python Keystone's oslo.middleware (HTTPProxyToWSGI), off by default. When enabled, a middleware on the public interface parses RFC 7239Forwarded(preferred) thenX-Forwarded-For, takes the leftmost originating client, and overwritesConnectInfo<SocketAddr>. Every downstream consumer (therequesttracing span'sclient.addr, the API-Key IP allowlist, any future IP-based login control) transparently sees the real client.Enabling it asserts the immediate peer is a trusted proxy; left off by default, a deployment not behind such a proxy cannot be tricked into trusting a spoofed header. The layer is wired only on the public interface — never on the internal SPIFFE / admin listeners.
SPIFFE internal interface (
client_infowas not covered)The internal mTLS listener uses
hyper::service::service_fn, which bypasses axum's make-service, so it never populatedConnectInfo. The accepted TCP peer address is now injected by hand alongside the existing SVID /Interfaceextensions, soclient.addris captured on the internal interface too. Extracted into a smallattach_request_contexthelper for testability.The admin UDS interface is intentionally left uncovered — a Unix socket has no meaningful
SocketAddr(acceptable per the issue).Tests
ip:port, bracketed/bare IPv6, RFC 7239 params, obfuscated identifiers, precedence) + in-process middleware tests driving the realinto_make_service_with_connect_infopath.[oslo_middleware]config deserialization test.attach_request_contextunit test assertingConnectInfo/Interface/SVID are attached.cargo fmt --checkandcargo clippy --lib --testsclean; full keystone lib + config suites pass.🤖 Generated with Claude Code