Skip to content

fix(pg-pkg): rate-limit on real client IP (X-Forwarded-For) behind a trusted proxy#230

Merged
rubenhensen merged 2 commits into
mainfrom
fix/pkg-ratelimit-forwarded-for
Jul 10, 2026
Merged

fix(pg-pkg): rate-limit on real client IP (X-Forwarded-For) behind a trusted proxy#230
rubenhensen merged 2 commits into
mainfrom
fix/pkg-ratelimit-forwarded-for

Conversation

@rubenhensen

Copy link
Copy Markdown
Contributor

What

Add an opt-in X-Forwarded-For-aware key extractor for the /v2 rate limiter
so per-client limiting works when the PKG runs behind a reverse proxy.

#226 introduced per-IP rate limiting keyed on the TCP peer address
(PeerIpKeyExtractor). Behind a reverse proxy (our ingress-nginx) the peer is
always the proxy, so every client shares one bucket — the sensitive
key-issuing endpoints (2 req/s, burst 10) throttle the whole deployment as if
it were a single user. The code comment already noted peer-IP limiting is
"meaningless" behind a proxy.

Change

  • New middleware/ratelimit.rs with ClientIpKeyExtractor, a custom
    actix_governor::KeyExtractor (actix-governor 0.10 ships no SmartIp
    extractor). When trust_forwarded_for is set it keys on the rightmost
    X-Forwarded-For entry — the hop appended by the trusted proxy — falling
    back to the peer address when the header is absent/unparseable. It applies
    the same IPv6 /56 normalization as PeerIpKeyExtractor.
  • New --ratelimit-trust-forwarded-for / PKG_RATELIMIT_TRUST_FORWARDED_FOR
    flag (default false). Off by default so a directly-exposed PKG never trusts
    client-supplied headers; operators opt in when they front the PKG with a
    trusted proxy.
  • Wired into both governor configs and the startup log line.

Why the rightmost entry (security)

X-Forwarded-For is a list each proxy appends to. With
use-forwarded-headers: true on ingress-nginx, a client-supplied XFF is kept
and the real client IP is appended last — so only the rightmost entry is
trustworthy; anything left of it is spoofable. We take strictly the rightmost
entry and never walk left (a garbage rightmost falls back to the peer). This
mirrors the XFF_DEPTH=1 handling used elsewhere on the platform behind the
same ingress.

Tests

middleware/ratelimit.rs unit tests cover: peer mode ignores XFF; trusted
single entry; trusted rightmost wins over a spoofed left entry; fallback to
peer when the header is missing; fallback to peer on an unparseable rightmost
(no left-walk); IPv6 /56 normalization.

Ops / deployment note

This flag is off by default; deployments that front the PKG with a trusted
proxy must set PKG_RATELIMIT_TRUST_FORWARDED_FOR=true for per-client limiting
to take effect. (postguard-ops change tracked separately.)

…trusted proxy

Per-IP rate limiting (#226) keys on the TCP peer address, which is always
the proxy when the PKG runs behind ingress-nginx, so every client shares one
bucket. Add ClientIpKeyExtractor and an opt-in --ratelimit-trust-forwarded-for
/ PKG_RATELIMIT_TRUST_FORWARDED_FOR flag (default false) that keys on the
rightmost (trusted, proxy-appended) X-Forwarded-For entry, falling back to the
peer address. The rightmost entry is used so client-supplied entries cannot
spoof the key.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dobby consolidated review — APPROVE ✅

Solid, well-scoped fix. The rightmost-X-Forwarded-For trust model is the correct choice behind an appending proxy (ingress-nginx), the peer-address fallback on a missing/unparseable rightmost entry avoids left-walking into spoofable client data, and the /56 IPv6 normalization mirrors PeerIpKeyExtractor. Off-by-default is the safe posture for a directly-exposed PKG.

Verification (this HEAD):

  • cargo fmt --manifest-path pg-pkg/Cargo.toml --all -- --checkpasses. The earlier long-#[clap(...)] formatting concern is already resolved by e078b26 style(pg-pkg): wrap long clap attribute (rustfmt).
  • pg-pkg suite: 53 tests pass, incl. the 10 ratelimit unit tests; clippy clean.
  • Rule checks (fmt-before-push, tests-required-on-fixes, conventional-commit title, promised-vs-delivered, core-security): all compliant. Title is conventional; every promised piece (extractor, flag, governor wiring, startup log, tests) is delivered.

One non-blocking defense-in-depth note inline. Good to merge.


/// The rightmost address in `X-Forwarded-For`, if present and parseable.
fn forwarded_ip(req: &ServiceRequest) -> Option<IpAddr> {
let value = req.headers().get(X_FORWARDED_FOR)?.to_str().ok()?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (defense-in-depth): HeaderMap::get returns only the first X-Forwarded-For header line. The rightmost-entry trust model is therefore sound only because ingress-nginx merges hops into one comma-separated header (proxy_add_x_forwarded_for) — correct for the stated infra. If a proxy ever emitted XFF as a separate second header line after a client-supplied one, get() would return the spoofable client line. Fine as-is behind ingress-nginx; noting for future defense-in-depth (get_all + take last).

@rubenhensen
rubenhensen merged commit 149c845 into main Jul 10, 2026
23 of 25 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant