Forwarded-trust hardening: X-Real-IP fallback, and no warning for a trusting listener with no match_local_ip #64

New issue

Open

opened 2026-10-26 15:39:08 +00:00 by james.oates · 0 comments

james.oates commented

2026-10-26 15:39:08 +00:00

Owner

From the v2.0.0 security audit. Two related gaps in trust_forwarded_for, both hardening rather than defects — the trust gate itself now holds on both the believe and send sides after f02af79.

1. The `X-Real-IP` fallback assumes the intermediary overwrites it

access::effective_client_ip honours CF-Connecting-IP and falls back to X-Real-IP. That is only sound if the trusted intermediary overwrites or strips X-Real-IP. Cloudflare and cloudflared set CF-Connecting-IP, which wins, so the documented deployment is fine.

But README presents nginx, HAProxy and NLB front-ends as supported shapes, and nginx does not set X-Real-IP unless configured to. An operator with HAProxy in front, a trust_forwarded_for listener and allow_ips = ["100.64.0.0/10"] is then bypassed by a client sending X-Real-IP: 100.64.0.1.

The code path is certain; the exposure depends on the front-end, and no fleet deployment is known to hit it.

Fix: either gate the fallback behind an explicit per-listener choice of which header to trust, or document that a trusted intermediary MUST overwrite both names. The explicit choice is better — "which header carries the client IP" is a property of the intermediary, and guessing a priority order is what created this.

2. Nothing warns about the dangerous listener shape

There is no code notion of "directly reachable" — the invariant is structural and operator-asserted via the parent binding's address plus match_local_ip. Zero-config is safe: all three synthesised-listener branches hard-set trust_forwarded_for: false.

But trust_forwarded_for = true with no match_local_ip, on a binding bound to 0.0.0.0, starts silently and lets every direct client spoof the client IP. That is the one configuration that breaks the invariant, and it is accepted without comment.

Fix: a startup warning naming the listener. Cheap, and it is the only signal an operator would get.

3. Bare `split(':')` on the admin Host header

src/middleware.rs:102 and :141 parse the Host with h.split(':').next() rather than proxy::authority_host, which CLAUDE.md names as a landmine. An IPv6-literal admin host ([fd00::1]:8443) yields "[", so admin_host_gate never matches and the admin API becomes unreachable on that host.

No bypass — admin_ip_middleware uses the identical parse, and proxy_handler uses authority_host correctly and only serves dashboard HTML after its own access check. Availability bug plus a deviation from a documented rule.

From the v2.0.0 security audit. Two related gaps in `trust_forwarded_for`, both hardening rather than defects — the trust gate itself now holds on both the believe and send sides after `f02af79`. ## 1. The `X-Real-IP` fallback assumes the intermediary overwrites it `access::effective_client_ip` honours `CF-Connecting-IP` and falls back to `X-Real-IP`. That is only sound if the trusted intermediary overwrites or strips `X-Real-IP`. Cloudflare and cloudflared set `CF-Connecting-IP`, which wins, so the documented deployment is fine. But README presents nginx, HAProxy and NLB front-ends as supported shapes, and nginx does not set `X-Real-IP` unless configured to. An operator with HAProxy in front, a `trust_forwarded_for` listener and `allow_ips = ["100.64.0.0/10"]` is then bypassed by a client sending `X-Real-IP: 100.64.0.1`. The code path is certain; the exposure depends on the front-end, and no fleet deployment is known to hit it. Fix: either gate the fallback behind an explicit per-listener choice of which header to trust, or document that a trusted intermediary MUST overwrite both names. The explicit choice is better — "which header carries the client IP" is a property of the intermediary, and guessing a priority order is what created this. ## 2. Nothing warns about the dangerous listener shape There is no code notion of "directly reachable" — the invariant is structural and operator-asserted via the parent binding's address plus `match_local_ip`. Zero-config is safe: all three synthesised-listener branches hard-set `trust_forwarded_for: false`. But `trust_forwarded_for = true` with no `match_local_ip`, on a binding bound to `0.0.0.0`, starts silently and lets every direct client spoof the client IP. That is the one configuration that breaks the invariant, and it is accepted without comment. Fix: a startup warning naming the listener. Cheap, and it is the only signal an operator would get. ## 3. Bare `split(':')` on the admin Host header `src/middleware.rs:102` and `:141` parse the Host with `h.split(':').next()` rather than `proxy::authority_host`, which CLAUDE.md names as a landmine. An IPv6-literal admin host (`[fd00::1]:8443`) yields `"["`, so `admin_host_gate` never matches and the admin API becomes unreachable on that host. No bypass — `admin_ip_middleware` uses the identical parse, and `proxy_handler` uses `authority_host` correctly and only serves dashboard HTML after its own access check. Availability bug plus a deviation from a documented rule.

james.oates added this to the v2.1 milestone

2026-10-26 17:53:31 +00:00

james.oates referenced this issue

2026-10-26 17:53:55 +00:00

The WebSocket path has no connect timeout, no idle timeout and no connection cap #62

james.oates referenced this issue

2026-10-27 14:53:23 +00:00

Make the forwarded client-IP header an explicit per-listener choice #73