Believe forwarded client addresses only from trusted proxies - #16
Merged
Conversation
The anonymous rate limit and the upload audit trail are both keyed on the
client address, so an address the client can choose is an address that resets
the budget and writes fiction into the audit trail.
Two layers were each trusting the header on their own:
- app/fingerprint.py read X-Forwarded-For whenever TRUST_FORWARDED_FOR was
set, and took split(",")[0] — the client-controlled end of the chain, since
proxies append.
- the container ran uvicorn with --proxy-headers --forwarded-allow-ips "*",
which rewrote the peer address from the same header for any caller, before
the application saw it. TRUST_FORWARDED_FOR=false did not prevent this; the
documented off switch was not an off switch.
The app is not published to a host port, so this is not reachable from the
internet — the front replaces the header on the way through. It is reachable
from the four other containers that share the idp_proxy network: a POST
straight to beckham-share-app:8000 with a forged X-Forwarded-For was recorded
verbatim in upload_events.
Now uvicorn reports the socket peer and the application makes the decision in
one place. Forwarded headers are honoured only when the request arrived from a
peer in the new TRUSTED_PROXIES setting (addresses, CIDR ranges, or hostnames
resolved at runtime, since Docker assigns container addresses), and the chain
is read right to left so hops appended by the client are discarded.
TRUST_FORWARDED_FOR is replaced by TRUSTED_PROXIES; nothing set it.
The browser fingerprint is deliberately left as it is. It is client-supplied
by nature, and it is OR-ed with the address rather than substituted for it, so
rotating it cannot buy a fresh budget while the address key still matches. Its
job is to catch one device rotating addresses. Documented in docs/SECURITY.md
so the distinction is not mistaken for an oversight.
Verified on a staging container: an untrusted neighbour's forged header is now
ignored and the request is attributed to the address it came from; a trusted
peer's header is still honoured; a chain of "forged, real" resolves to the
real hop. Eight unit tests cover the same cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
The anonymous rate limit and the upload audit trail are both keyed on the client
address. An address the client can choose is an address that resets the budget
and writes fiction into the audit trail.
Two layers were each trusting
X-Forwarded-Foron their own:app/fingerprint.pyread the header wheneverTRUST_FORWARDED_FORwas set,and took
split(",")[0]— the client-controlled end of the chain, sinceproxies append.
--proxy-headers --forwarded-allow-ips "*",which rewrote the peer address from the same header for any caller,
before the application saw it.
Consequence of (2):
TRUST_FORWARDED_FOR=falsewas not an off switch. Withit set to
false, a forged header still landed in the audit trail, because theserver had already rewritten the peer address.
Is it reachable?
Tested rather than assumed.
replaces
X-Forwarded-Foron the way through. A forged header sent tohttps://share.beckham.ai/api/anon-uploadwas discarded; the row recorded thereal address.
idp_proxywith this app. A
POSTstraight tobeckham-share-app:8000carryingX-Forwarded-For: 198.51.100.7was recorded verbatim inupload_events.So the exposure is "any container on the shared proxy network", not "anyone on
the internet" — but that is exactly the layer the in-app control exists to
backstop.
The change
uvicorn now reports the socket peer, and the application makes the decision in
one place:
TRUSTED_PROXIESsetting — comma-separated addresses, CIDR ranges, orhostnames, resolved at runtime because Docker assigns container addresses.
Defaults to
idp-caddy, which resolves to172.23.0.5from inside the appcontainer.
those peers. Everything else is attributed to the address it came from,
whatever headers it carries.
untrusted address is the closest one a trusted proxy actually observed.
TRUST_FORWARDED_FORis replaced byTRUSTED_PROXIES. Nothing set the oldvariable, so there is nothing to migrate.
What is deliberately not changed
The browser fingerprint stays client-supplied. It is OR-ed with the address,
not substituted for it —
app/ratelimit.pycounts rows matching either key,so rotating the fingerprint cannot buy a fresh budget while the address key
still matches. Its job is the reverse: catching one device that rotates
addresses. Folding the address into it would destroy exactly that property.
Now stated explicitly in
docs/SECURITY.mdso the distinction reads as adecision rather than an oversight.
Verification
Staging container, real requests:
172.23.0.8— the address it came from198.51.100.7— trusted forwarding still works198.51.100.7, 203.0.113.9from a trusted peer203.0.113.9— client-appended hop discardedPlus 8 unit tests over
client_ip()and a regression test that rotating theheader does not reset the anonymous budget. Backend suite: 33 tests pass.
Deploy note
This changes the container command, so it needs a rebuild rather than a restart.
docs/SECURITY.md§3 and §6 anddocs/CONFIGURATION.mdare updated to match.