Skip to content

feat(net): LAN-only filter middleware (#436) [stacked on #473] - #474

Open
olegbrok wants to merge 1 commit into
feat/client-ip-resolver-stackedfrom
feat/lan-filter-stacked
Open

feat(net): LAN-only filter middleware (#436) [stacked on #473]#474
olegbrok wants to merge 1 commit into
feat/client-ip-resolver-stackedfrom
feat/lan-filter-stacked

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

Summary

PR-3 of the #433 web-exposed hardening track. Restricts which network sources the daemon serves when running in LAN mode — RFC1918 / link-local / loopback / ULA accepted by default, everything else dropped with HTTP 403 before any auth or route handler runs.

Stacked on #473 (consumes `ClientIdentity` from #442) which is stacked on #472 (#434 `PINKY_DEPLOYMENT_MODE`).

Closes part of #433. Refs #436.

What

  • `pinky_daemon.net.lan_filter` (new):

    • `DEFAULT_PRIVATE_CIDRS` — RFC1918 + IPv4 link-local + IPv4 loopback + IPv6 ULA + IPv6 link-local + IPv6 loopback. CGNAT (`100.64.0.0/10`) is explicitly NOT included by design — Tailscale operators opt in via `PINKY_LAN_EXTRA_CIDRS` so we don't accidentally trust the wider carrier-grade NAT space.
    • `is_private_ip(addr, allowlist)` — pure helper with v4/v6 type guards (mixed-family allowlist doesn't crash).
    • `resolve_lan_allowed_cidrs(env)` — defaults + parsed `PINKY_LAN_EXTRA_CIDRS` (lenient: malformed entries logged + skipped).
    • `build_lan_filter_middleware(allowlist)` — ASGI middleware factory:
  • `create_api`: caches composed allowlist on `app.state.lan_allowed_cidrs`; registers the middleware early so public-source traffic is dropped before any auth/handler work.

Murzik review followups (from #436) — addressed

Out of scope (other PRs)

Tests (34 new)

`tests/test_lan_filter.py`:

  • `is_private_ip` (parametrised): 9 private accepted, 7 public/just-outside rejected, CGNAT rejected by default + accepted with extras, mixed-family no-crash, empty allowlist
  • `resolve_lan_allowed_cidrs`: defaults / extras / invalid skipped
  • Middleware closure: trusted/web pass-through, LAN accepts private, LAN rejects public, LAN drops untrusted-peer XFF, LAN honors trusted-peer XFF, LAN honors extras, LAN env fallback
  • End-to-end through `create_api` for trusted/LAN/web modes

Plus a one-line adjustment to `test_deployment_mode::test_api_endpoint_exposes_mode_for_each_value` to allowlist TestClient's synthetic peer when looping LAN.

393 tests green across the full hardening suite + existing api tests.

Test plan

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

🤖 Opened by Barsik

PR-3 of the #433 web-exposed hardening track. Stacked on #473 (consumes
ClientIdentity from #442).

What
----
* New `pinky_daemon.net.lan_filter`:
  - `DEFAULT_PRIVATE_CIDRS` — RFC1918 + IPv4 link-local + IPv4 loopback
    + IPv6 ULA + IPv6 link-local + IPv6 loopback. CGNAT
    (100.64.0.0/10) is **not** included by design — operators on
    Tailscale opt in via `PINKY_LAN_EXTRA_CIDRS`.
  - `is_private_ip(addr, allowlist)` — pure helper with v4/v6 type
    guards so a mixed-family allowlist doesn't crash with TypeError.
  - `resolve_lan_allowed_cidrs(env)` — composes defaults + parsed
    extras from `PINKY_LAN_EXTRA_CIDRS` (lenient: malformed entries
    logged + skipped).
  - `build_lan_filter_middleware(allowlist)` — ASGI middleware factory:
    * No-op pass-through in `trusted` and `web` modes.
    * In `lan` mode, computes the canonical client IP via
      `resolve_client_identity` (so XFF parsing matches #442) and
      returns 403 when the IP isn't in the allowlist.
    * Logs reject at INFO with method, path, ip, raw_peer, via_proxy
      so scan/probe traffic is visible without flooding ERROR.

* `create_api`:
  - Reads `PINKY_LAN_EXTRA_CIDRS` once, caches the composed allowlist
    on `app.state.lan_allowed_cidrs`.
  - Registers the LAN filter middleware **early** (before auth, before
    route handlers) so public-source traffic in LAN mode is dropped
    before any password-check work happens.

Murzik review followups (from #436)
-----------------------------------
* [x] Defer trusted-IP resolution to #442 — middleware uses
  `resolve_client_identity` end-to-end; no XFF parsing here.
* [x] LAN middleware ordering: very early (registered first in
  create_api).
* [x] Never trust XFF in LAN mode unless peer ∈ trusted_proxies —
  inherited from #442 via the resolver call.
* [x] CIDR list: RFC1918 / link-local / ULA only. CGNAT explicitly
  excluded; opt-in via env.
* [x] Test coverage: IPv6-mapped IPv4 (covered by #442 tests +
  is_private_ip type-guard test), comma-separated XFF (#442 tests),
  malformed XFF (#442 tests).

Out of scope (deferred)
-----------------------
* Bind defaults — landed in #472 (#434).
* Refuse-to-boot when web + 0.0.0.0 without TLS marker — owned by
  #441.

Tests (34 new)
--------------
* `is_private_ip`: 9 private addresses accepted, 7 public/just-outside
  addresses rejected, CGNAT rejected by default + accepted with
  extras, mixed-family no-crash, empty allowlist rejects everything.
* `resolve_lan_allowed_cidrs`: defaults when unset, extras appended,
  invalid extras skipped.
* Middleware closure: trusted/web pass-through, LAN accepts private,
  LAN rejects public, LAN drops untrusted-peer XFF, LAN honors
  trusted-peer XFF, LAN honors operator-extra CIDRs, LAN falls back
  to env when no allowlist on state.
* End-to-end through `create_api` (TestClient) for trusted (no
  filter), LAN (default-rejects testclient peer), LAN with extras
  (testclient peer allowed), web (no filter).

Adjusted `test_deployment_mode::test_api_endpoint_exposes_mode_for_each_value`
to opt 0.0.0.0/32 into the LAN allowlist so the loop reaches /api in
every mode.

Closes part of #433. Refs #436.

🤖 Authored by Barsik
@olegbrok

Copy link
Copy Markdown
Collaborator Author

Latent test bug in this PR

This PR's edit to `tests/test_deployment_mode.py::test_api_endpoint_exposes_mode_for_each_value` introduced a small indentation bug. The `patch.dict(os.environ, env, clear=True)` wrapper added here (to bypass the new LAN filter) moved the `assert` outside the for-loop:

```python
with patch.dict(os.environ, env, clear=True):
for mode in DeploymentMode:
app = create_api(db_path=tmp_db_path, deployment_mode=mode)
client = TestClient(app)
resp = client.get("/api")
assert resp.json()["deployment_mode"] == mode.value # ← outside loop
```

Result: the test only verifies the last enum value (`mode` and `resp` leak out of the loop, so the assertion happens to pass on whichever value Python iterated last). Test name claims "for_each_value" but only one value is checked.

Fix is one indent level — move the assert inside the for-block. Non-blocking, but worth getting right before merge.

— 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