fix(nginx): stop the sidecar forwarding a client-supplied X-Forwarded-For - #135
fix(nginx): stop the sidecar forwarding a client-supplied X-Forwarded-For#135omarsy wants to merge 1 commit into
Conversation
…d X-Forwarded-For
The sidecar set X-Forwarded-For from $proxy_add_x_forwarded_for, which APPENDS
the peer to whatever the client sent. Consumers read the leftmost entry, so any
unauthenticated client could choose the address an app records against a request.
This is not theoretical. Against the Keycloak greffon now on catalog main, a
single `curl -H 'X-Forwarded-For: 203.0.113.77'` on a failed login wrote
ipAddress=203.0.113.77 into its admin event log. On an identity provider that log
is a security control, and a forgeable address is worse than an unhelpful one: it
can frame a third party and poison anything built on it, such as a fail2ban rule.
The Keycloak entry currently ships a comment telling operators to treat that field
as a hint rather than evidence, and points here for the fix.
This sidecar is the trust boundary. It terminates TLS and is the first hop we
control, so there is no upstream value worth preserving, and $remote_addr is the
correct source. X-Real-IP already used it; this makes the two agree.
What each mode yields, checked rather than assumed:
proxy mode - the real client address. The browser connects to the sidecar's
published port, and Linux docker publishing preserves the source
address through DNAT.
tunnel mode - the tunnel sidecar's address. rathole is a plain TCP tunnel: it
adds no HTTP headers and no PROXY protocol, so the client's
address is already lost at the transport before nginx sees it.
Constant and unhelpful, but true. Recovering per-client
attribution there needs PROXY protocol on both rathole ends plus
real_ip_header proxy_protocol here, which is a bigger change than
this one and is NOT worth faking with the append form.
Verified end to end by running the real rendered config against a
header-echoing upstream: a request carrying
`X-Forwarded-For: 203.0.113.77, 198.51.100.9` reached the app as
`X-Forwarded-For: 192.168.65.1`, the actual peer, with the forged chain gone and
X-Forwarded-Proto: https unaffected. That peer is the Docker Desktop gateway
because the probe ran on macOS, where Desktop NATs through its VM; on a Linux
greffer the same path yields the client's own address.
Adds a regression test that asserts the directive uses $remote_addr and that the
append form appears in no directive (only in the comment naming it as the thing
not to use). Mutation-checked: restoring $proxy_add_x_forwarded_for fails it.
Suites touching rendered nginx content pass (test_nginx_conf, l4_network_exposure:
35 tests). The full suite was not run to completion: this machine sits at load
~150 and starves it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 583a488971
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # needed through the tunnel it takes PROXY protocol on both | ||
| # rathole ends plus `real_ip_header proxy_protocol` here; | ||
| # do NOT reintroduce the append form to fake it. | ||
| proxy_set_header X-Forwarded-For $remote_addr; |
There was a problem hiding this comment.
In deployments using the supported manager/public-proxy path, nginx's peer is the edge proxy rather than the browser—the manager-supplied ports[].url flow is explicitly documented in apps/utils/docker/compose.py as having a public proxy in front. Unconditionally replacing X-Forwarded-For with $remote_addr therefore makes every request appear to originate from that proxy, breaking per-client audit logs, bans, and rate limits. Prevent spoofing by accepting forwarded addresses only from configured trusted proxy ranges (for example with nginx real-IP settings) while overwriting the header for direct, untrusted peers.
Useful? React with 👍 / 👎.
1 participant
The per-instance nginx sidecar set
X-Forwarded-Forfrom$proxy_add_x_forwarded_for, which appends the peer to whatever the client sent. Consumers read the leftmost entry, so any unauthenticated client could choose the address an app records against a request.Not theoretical
Against the Keycloak greffon now on catalog
main, one request:On an identity provider that log is a security control. A forgeable address is worse than an unhelpful one, because it can frame a third party and poison anything built on it, such as a fail2ban rule. The Keycloak entry currently ships a comment telling operators to treat that field as a hint rather than evidence, and points here for the fix.
Why
$remote_addris the right sourceThis sidecar is the trust boundary: it terminates TLS and is the first hop we control, so there is no upstream value worth preserving.
X-Real-IPalready used$remote_addr; this makes the two agree.What each mode yields, checked rather than assumed:
X-Forwarded-ForbecomesTunnel mode deserves the explicit note: rathole is a plain TCP tunnel, adding no HTTP headers and no PROXY protocol, so the client's address is already lost at the transport before nginx sees it. Constant and unhelpful, but true. Recovering per-client attribution there needs PROXY protocol on both rathole ends plus
real_ip_header proxy_protocolhere. That is a larger change, and it is not worth faking with the append form in the meantime.Verification
Ran the real rendered config against a header-echoing upstream. Client sent
X-Forwarded-For: 203.0.113.77, 198.51.100.9; the app received:Forged chain gone,
X-Forwarded-Protounaffected. That peer is the Docker Desktop gateway because the probe ran on macOS, where Desktop NATs through its VM; on a Linux greffer the same path yields the client's own address.Adds a regression test asserting the directive uses
$remote_addrand that the append form appears in no directive (it survives only in the comment naming it as the thing not to use). Mutation-checked: restoring$proxy_add_x_forwarded_forfails it.Scope
test_nginx_confandtest_l4_network_exposure, the two suites that assert on rendered nginx content, pass (35 tests). The full suite was not run to completion: this machine sits at load ~150 and starves it, so CI is the real check.Apps reading
X-Real-IPsee no change. Apps readingX-Forwarded-Fornow get one unforgeable entry instead of a client-controlled chain; anything parsing it as a list still works, since a single address is a valid list.