Skip to content

feat(net): canonical trusted-client-IP resolver (#442) [stacked on #472] - #473

Open
olegbrok wants to merge 1 commit into
feat/deployment-mode-configfrom
feat/client-ip-resolver-stacked
Open

feat(net): canonical trusted-client-IP resolver (#442) [stacked on #472]#473
olegbrok wants to merge 1 commit into
feat/deployment-mode-configfrom
feat/client-ip-resolver-stacked

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

Summary

PR-2 of the #433 web-exposed hardening track. Single source of truth for what is the real client IP for this request? — every IP-consuming feature in the rest of the track reads from here instead of parsing `X-Forwarded-For` independently.

Stacked on #472 (depends on `PINKY_DEPLOYMENT_MODE` + `app.state.deployment_mode`).

Closes part of #433. Refs #442.

What

  • `pinky_daemon.net` — new subpackage for cross-cutting network helpers.
  • `pinky_daemon.net.client_identity`:
    • `ClientIdentity` frozen dataclass: `ip`, `via_proxy`, `raw_peer`, `forwarded_chain`.
    • `parse_trusted_proxies(value)` — comma-separated CIDRs → tuple of IPNetwork. Bare IPs accepted as /32 or /128. Invalid entries logged and skipped (don't crash startup on operator typos).
    • `resolve_client_identity(request, mode, trusted_proxies)` — the spec from Hardening 9/10: Canonical trusted-client-IP resolver (cross-cutting) #442:
      Mode Behavior
      `trusted` `ip = raw_peer`; XFF / Forwarded ignored entirely
      `lan` / `web` XFF + RFC 7239 Forwarded parsed, but only honored when `raw_peer` ∈ `trusted_proxies`; otherwise dropped + throttled WARNING
    • Forwarded wins over X-Forwarded-For when both present.
    • IPv6-mapped IPv4 (`::ffff:1.2.3.4`) normalised before CIDR comparison so dual-stack listeners match v4 trust rules.
    • Right-to-left walk, skipping trusted hops; falls back to leftmost when whole chain is trusted.
    • Malformed XFF / Forwarded → fall back to `raw_peer`; warning throttled to once per minute per failure class.
    • Loopback is NOT trusted by default — operator opts in via `PINKY_TRUSTED_PROXIES` so audit/rate-limit attribution can't silently flatten to 127.0.0.1.
    • `get_client_identity(request)` — FastAPI dependency. Reads from `app.state`; falls back to env when state unset.
  • `create_api` — reads `PINKY_TRUSTED_PROXIES` once at startup, caches parsed CIDRs on `app.state.trusted_proxies`.

Issue spec coverage

  • Trusted mode ignores XFF entirely
  • Web/LAN with trusted proxy correctly identifies original client
  • Untrusted-proxy XFF dropped (with throttled warning)
  • IPv6-mapped IPv4 normalisation
  • Comma-separated XFF picks correct hop (right-to-left, skip trusted)
  • Malformed XFF doesn't crash; falls back to peer
  • `Forwarded` (RFC 7239) precedence over `X-Forwarded-For`
  • Single FastAPI dependency `Depends(get_client_identity)`
  • Loopback not trusted by default

Tests (47 new)

`tests/test_client_identity.py` covers:

  • Trusted-proxy parsing (8 cases)
  • `trusted` mode behaviour (3 cases)
  • `lan` + `web` parametrised behaviour (12 cases each = 24)
  • Dataclass immutability (2)
  • `get_client_identity` dep with app.state and env fallback (2)
  • End-to-end through `create_api` + TestClient (3)
  • Throttled warning surfacing (1)

286 existing api tests + 26 deployment-mode tests still green locally.

Test plan

  • `pytest tests/test_client_identity.py -v` (47 passing)
  • `pytest tests/test_api.py tests/test_deployment_mode.py` (no regression)
  • `ruff check src/pinky_daemon/net/ src/pinky_daemon/api.py tests/test_client_identity.py` (clean)

Out of scope

The features that consume `ClientIdentity` ship in subsequent PRs:

🤖 Opened by Barsik

PR-2 of the #433 web-exposed hardening track. Establishes one canonical
"what is the real client IP for this request?" answer that #436 (LAN
filter), #438 (rate limit key), #440 (audit attribution), and #441 (boot
guards) all read from instead of each parsing X-Forwarded-For
independently.

Stacks on #472 (PINKY_DEPLOYMENT_MODE).

What
----
* New `pinky_daemon.net` subpackage for cross-cutting network helpers.
* `pinky_daemon.net.client_identity`:
  - `ClientIdentity` frozen dataclass: `ip`, `via_proxy`, `raw_peer`,
    `forwarded_chain` — what every IP-consuming feature operates on.
  - `parse_trusted_proxies(value)` — comma-separated CIDRs → tuple of
    IPNetwork. Bare IPs accepted as /32 or /128. Invalid entries logged
    and skipped (don't crash startup on operator typos in the list).
  - `resolve_client_identity(request, mode, trusted_proxies)`:
    * `trusted` → ip = raw_peer; XFF / Forwarded ignored entirely.
    * `lan` / `web` → XFF + RFC 7239 Forwarded parsed, but only
      honored when `raw_peer` is in `trusted_proxies`. Otherwise the
      chain is dropped and a throttled WARNING is emitted (suspected
      header spoofing).
    * Forwarded wins over X-Forwarded-For.
    * IPv6-mapped IPv4 (`::ffff:1.2.3.4`) normalised to v4 before CIDR
      comparison so dual-stack listeners actually match v4 trust rules.
    * Right-to-left walk of the chain, skipping trusted hops; falls back
      to leftmost if the entire chain is trusted.
    * Malformed XFF / Forwarded → fall back to raw_peer; warning
      throttled to once per minute per failure class.
  - `get_client_identity(request)` — FastAPI dependency. Reads
    `app.state.deployment_mode` and `app.state.trusted_proxies`; falls
    back to env when state isn't set so tests don't have to wire it.
* `create_api`:
  - Reads `PINKY_TRUSTED_PROXIES` once at startup, caches parsed CIDRs
    on `app.state.trusted_proxies`.
  - Loopback is **not** trusted by default — operator opt-in only,
    otherwise audit/rate-limit attribution silently flattens to
    127.0.0.1 when something upstream goes wrong.

Tests (47 new)
--------------
* `parse_trusted_proxies`: empty/None/whitespace; bare v4/v6; multiple
  with whitespace; invalid entries skipped; host-bits-set accepted.
* `trusted` mode: peer wins, XFF + Forwarded both ignored.
* `lan` and `web` modes (parametrised): no-XFF; untrusted-peer XFF
  dropped; trusted-peer XFF honored; multi-hop chain skips trusted
  hops; all-trusted-chain returns leftmost; IPv6-mapped IPv4
  normalisation; Forwarded precedence; quoted bracketed IPv6 in
  Forwarded; XFF with `host:port` form; fully malformed → fallback to
  peer; partial malformed → keep good entries; missing `client`
  attribute → 0.0.0.0.
* `ClientIdentity` immutability and default empty-tuple chain.
* `get_client_identity` reads from app.state and falls back to env.
* End-to-end through `create_api` + TestClient.
* Untrusted-peer warning is logged.

286 existing api tests + 26 deployment-mode tests still green.

Closes part of #433. Refs #442.

🤖 Authored by Barsik
@olegbrok

Copy link
Copy Markdown
Collaborator Author

CI status on stacked PRs #473#479

These 7 PRs target their parent stack branch (not `main`), so GitHub Actions workflows — which are gated on `pull_request: branches: [main]` in both `ci.yml` and `codeql.yml` — don't fire. This is expected for stacked PRs but worth flagging for the review pass.

Options:

  1. Natural cascade — as each PR merges into `main`, the next rebases and gets full CI. Slowest but cleanest.
  2. Combined validation branch — I can create a `feat/hardening-all` branch with all 8 squashed onto `main` and open it as a draft PR purely to run CI on the combined diff. Lets us see the full stack go green before merging individually.
  3. Workflow update — add `feat/*-stacked` patterns to the trigger globs. Heavier, probably overkill.

Recommend option 2 — I can spin that up overnight if you want. Otherwise option 1 is fine; the bottom of the stack (#472) ran clean except for the CodeQL findings I commented there.

— Barsik 🤖

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