feat(proxy): fall back to TCP peer addr in extract_client_ip #33

Merged

sameh-farouk merged 1 commit from feat/peer-addr-fallback into development

2026-04-26 17:34:41 +00:00

sameh-farouk commented

2026-04-26 15:54:34 +00:00

Member

Closes #31. Surfaces #32 (pre-existing X-Forwarded-For spoof gate, separate issue).

Summary

Adds the missing third tier to extract_client_ip: TCP peer address from the kernel, used when neither X-Real-Ip nor X-Forwarded-For is present. Without this, hero_proxy as the edge can never identify direct browser connections — which silently breaks the IP-auto-login feature configured via user_networks.

Changes

File	Change
`crates/hero_proxy_server/src/proxy.rs`	`extract_client_ip` signature gains `Option<&SocketAddr>` parameter and returns `peer_addr.map(
`crates/hero_proxy_server/src/main.rs`	Three `into_make_service()` call sites (HTTP `axum::serve`, HTTPS-ACME `axum_server::bind`, HTTPS-self-signed `axum_server::bind_rustls`) → `into_make_service_with_connect_info::<SocketAddr>()` so the ConnectInfo extractor can populate the request extension.

Net: +20 lines, -9 lines.

Why

extract_client_ip was missing the standard third tier of every edge HTTP server's client-IP lookup chain (axum-extra ClientIp, tower-http RealIp, nginx real_ip_module, actix ConnectionInfo — all use this pattern):

1. X-Real-Ip       (single IP from a trusted upstream proxy)
2. X-Forwarded-For (chain from CDN / multi-hop)
3. TCP peer addr   (← was missing — required when proxy IS the edge)

Since hero_proxy is designed as the edge (terminates TLS, owns auth, is the public listener), step 3 is what's needed for its actual deployment shape. Putting another reverse proxy in front of hero_proxy would defeat its purpose.

Concrete consequence today: an operator can add a user_networks row mapping 127.0.0.1/32 → user 1 via the admin UI, set auto_accept = 1, and find_user_for_ip will still never trigger for browser connections to :9997 because extract_client_ip returns None. The feature is dead from the deployment shape it was built for.

Test plan

cargo build --release -p hero_proxy_server clean
Pre-fix: curl -X POST http://127.0.0.1:9997/hero_collab/rpc/rpc -d '{"method":"user.me",...}' returns {authenticated:false, user:null} despite user_networks row matching loopback
Post-fix: same call returns the auto-resolved user
Header-bearing requests unchanged: curl -H "X-Real-Ip: 10.0.0.5" ... still resolves to 10.0.0.5
No regression for deployments behind nginx/caddy: header path takes precedence over peer-addr path
Reviewer to validate: axum_server::bind_rustls + into_make_service_with_connect_info::<SocketAddr> works end-to-end with TLS (compiles fine; runtime test would require ACME or self-signed cert provisioning that isn't part of CI today)

Pre-existing trust gap (NOT fixed by this PR — see #32)

This patch inherits the existing trust gap: extract_client_ip honors X-Forwarded-For from any peer, with no validation that the immediate TCP peer is a trusted upstream proxy. With IP auto-login now actually working via this PR, that gap becomes operationally exploitable — an attacker reaching the listener directly can spoof X-Forwarded-For: 127.0.0.1 and impersonate the loopback-mapped user.

The patch does not introduce that gap; it inherits it. Filed as #32. Recommended fix shape: add trusted_proxies: Vec<IpNetwork> setting; only honor X-Real-Ip / X-Forwarded-For when the immediate peer is in that list; default empty (= edge mode = ignore those headers).

The dev-mode behaviour question (what happens when the operator wants the picker even with user_networks configured) is handled separately on the hero_collab side: that service's --auth-mode=dev will be updated to ignore upstream identity entirely, making the auth contract honest in both modes. No coupling needed in this PR.

Notes

The header precedence is preserved exactly — deployments behind a CDN see no behaviour change.
axum_server (used for both HTTPS branches) supports into_make_service_with_connect_info natively; the underlying TCP peer addr is what gets exposed (correct — you want the real client IP, not "TLS").
ConnectInfo<SocketAddr> is infallible after the make_service wiring — no error paths added to the handler.

Closes #31. Surfaces #32 (pre-existing X-Forwarded-For spoof gate, separate issue). ## Summary Adds the missing third tier to `extract_client_ip`: TCP peer address from the kernel, used when neither `X-Real-Ip` nor `X-Forwarded-For` is present. Without this, hero_proxy as the edge can never identify direct browser connections — which silently breaks the IP-auto-login feature configured via `user_networks`. ## Changes | File | Change | |---|---| | `crates/hero_proxy_server/src/proxy.rs` | `extract_client_ip` signature gains `Option<&SocketAddr>` parameter and returns `peer_addr.map(|a| a.ip().to_string())` after the existing header lookups. `proxy_handler` extracts `axum::extract::ConnectInfo<SocketAddr>` and passes it through. Header precedence unchanged: X-Real-Ip → X-Forwarded-For → peer addr. | | `crates/hero_proxy_server/src/main.rs` | Three `into_make_service()` call sites (HTTP `axum::serve`, HTTPS-ACME `axum_server::bind`, HTTPS-self-signed `axum_server::bind_rustls`) → `into_make_service_with_connect_info::<SocketAddr>()` so the ConnectInfo extractor can populate the request extension. | Net: +20 lines, -9 lines. ## Why `extract_client_ip` was missing the standard third tier of every edge HTTP server's client-IP lookup chain (axum-extra `ClientIp`, tower-http `RealIp`, nginx `real_ip_module`, actix `ConnectionInfo` — all use this pattern): ``` 1. X-Real-Ip (single IP from a trusted upstream proxy) 2. X-Forwarded-For (chain from CDN / multi-hop) 3. TCP peer addr (← was missing — required when proxy IS the edge) ``` Since hero_proxy is designed as the edge (terminates TLS, owns auth, is the public listener), step 3 is what's needed for its actual deployment shape. Putting another reverse proxy in front of hero_proxy would defeat its purpose. Concrete consequence today: an operator can add a `user_networks` row mapping `127.0.0.1/32 → user 1` via the admin UI, set `auto_accept = 1`, and `find_user_for_ip` will still never trigger for browser connections to `:9997` because `extract_client_ip` returns `None`. The feature is dead from the deployment shape it was built for. ## Test plan - [x] `cargo build --release -p hero_proxy_server` clean - [x] Pre-fix: `curl -X POST http://127.0.0.1:9997/hero_collab/rpc/rpc -d '{"method":"user.me",...}'` returns `{authenticated:false, user:null}` despite `user_networks` row matching loopback - [x] Post-fix: same call returns the auto-resolved user - [x] Header-bearing requests unchanged: `curl -H "X-Real-Ip: 10.0.0.5" ...` still resolves to 10.0.0.5 - [x] No regression for deployments behind nginx/caddy: header path takes precedence over peer-addr path - [ ] Reviewer to validate: `axum_server::bind_rustls` + `into_make_service_with_connect_info::<SocketAddr>` works end-to-end with TLS (compiles fine; runtime test would require ACME or self-signed cert provisioning that isn't part of CI today) ## Pre-existing trust gap (NOT fixed by this PR — see #32) This patch inherits the existing trust gap: `extract_client_ip` honors `X-Forwarded-For` from any peer, with no validation that the immediate TCP peer is a trusted upstream proxy. With IP auto-login now actually working via this PR, that gap becomes operationally exploitable — an attacker reaching the listener directly can spoof `X-Forwarded-For: 127.0.0.1` and impersonate the loopback-mapped user. The patch does not introduce that gap; it inherits it. Filed as #32. Recommended fix shape: add `trusted_proxies: Vec<IpNetwork>` setting; only honor `X-Real-Ip` / `X-Forwarded-For` when the immediate peer is in that list; default empty (= edge mode = ignore those headers). The dev-mode behaviour question (what happens when the operator wants the picker even with `user_networks` configured) is handled separately on the hero_collab side: that service's `--auth-mode=dev` will be updated to ignore upstream identity entirely, making the auth contract honest in both modes. No coupling needed in this PR. ## Notes - The header precedence is preserved exactly — deployments behind a CDN see no behaviour change. - `axum_server` (used for both HTTPS branches) supports `into_make_service_with_connect_info` natively; the underlying TCP peer addr is what gets exposed (correct — you want the real client IP, not "TLS"). - `ConnectInfo<SocketAddr>` is infallible after the make_service wiring — no error paths added to the handler.

sameh-farouk added 1 commit

2026-04-26 15:54:34 +00:00

feat(proxy): fall back to TCP peer addr in extract_client_ip

Build & Test / check (pull_request) Successful in 2m44s

Details

55620487bd

`extract_client_ip` only read `X-Real-Ip` and `X-Forwarded-For`. Both
are upstream-proxy headers — neither is set by a browser hitting
hero_proxy directly. Result: `client_ip = None` → `find_user_for_ip`
never matches → no `X-Hero-User` injected via IP auto-login →
downstream apps in proxy mode fail-closed with 'No hero_collab
account for "..."' even though the operator has a `user_networks`
row matching their loopback IP.

The pattern hero_proxy was missing is the same one every edge HTTP
server uses: trust X-Forwarded-For when present (CDN deployments),
trust X-Real-Ip when present (nginx/caddy fronts), otherwise read
the kernel-supplied TCP peer address. This patch adds the third
fallback.

  * `extract_client_ip` signature gains an `Option<&SocketAddr>`
    parameter and returns `peer_addr.map(|a| a.ip().to_string())`
    after the existing header lookups.
  * `proxy_handler` extracts `axum::extract::ConnectInfo<SocketAddr>`
    and passes it to `extract_client_ip`.
  * Listener wiring in `main.rs` switches three call sites from
    `into_make_service()` to
    `into_make_service_with_connect_info::<SocketAddr>()` — required
    for the ConnectInfo extractor to populate the request extension.

Known follow-up (filed separately): the existing trust model honors
X-Forwarded-For from any source. Anyone hitting `:9997` directly with
a spoofed X-Forwarded-For gets the spoofed IP's auto-login access.
This patch does not introduce that gap; it just inherits it. Fix
shape: add `trusted_proxies: Vec<IpNetwork>` setting; only honor
X-Real-Ip / X-Forwarded-For when peer is in that list.

sameh-farouk force-pushed feat/peer-addr-fallback from 55620487bd

Build & Test / check (pull_request) Successful in 2m44s

Details

to 54b0694a67

Build & Test / check (pull_request) Successful in 1m34s

Details

2026-04-26 17:33:47 +00:00

Compare