linux_patch_manager/tasks/ip-allowlist-spec.md

# IP Allowlist Hardening — Specification

**Issue:** [#3](https://github.com/Draco-Lunaris/Linux-Patch-Manager/issues/3)
**Component:** `crates/pm-auth/src/rbac.rs`, `crates/pm-core/src/config.rs`
**Spec version:** 0.1.0 (draft)
**Status:** Awaiting Kelly sign-off

---

## 1. Goal

Harden the IP allowlist enforced in the `require_auth` middleware so that:

1. It cannot be bypassed by omitting the `X-Forwarded-For` header.
2. It cannot be spoofed by setting `X-Forwarded-For` to an allowlisted value from
   a client that directly reaches the service.
3. When a non-empty allowlist is configured and no trustworthy client IP can be
   determined, the request is **denied** (fail-closed).

Today the allowlist is a documented production access control (see
`config/config.example.toml` `[security] ip_whitelist`) but, as filed in issue #3,
can be trivially defeated.

## 2. Non-Goals

- Replacing or weakening JWT auth. The allowlist is a defense-in-depth layer; JWT
  validation continues to run.
- Adding rate-limiting behavior (governor's `SmartIpKeyExtractor` is used for rate
  limiting and is out of scope to change here).
- Changes to `pm-worker` or `pm-agent-client` IP handling. This issue is scoped to
  the web/API edge.
- IPv6-specific quirks beyond what `ipnet` already supports. `is_ip_allowed`
  already handles IPv4 and IPv6 CIDRs via `IpNet`.

## 3. Design Decisions (Kelly sign-off, 2026-06-02)

| # | Decision | Resolution |
|---|----------|------------|
| Q1 | Trusted-proxy handling | **Strict (no proxies trusted by default).** Add a new `trusted_proxies: Vec<IpNet>` config field. When the field is **empty** (the default), the allowlist check uses the socket peer IP only and ignores `X-Forwarded-For` entirely. When the field is **non-empty** and the immediate peer is in the list, `X-Forwarded-For` may be honored; otherwise the socket peer IP is used. |
| Q2 | Reuse `SmartIpKeyExtractor` | **Reuse the pattern.** Extract a small, well-tested resolver helper (named `resolve_client_ip`) into `pm-auth` that mirrors `SmartIpKeyExtractor`'s "trust XFF only when peer is in trusted list, else peer IP" semantics, so the IP-allowlist check and the rate-limiter use the same resolution rule. We do not introduce a `pm-web → pm-auth` cycle; the resolver lives in `pm-auth` and is consumed by the middleware directly. (`pm-web` continues to use the governor extractor for its own rate-limiting layer.) |
| Q3 | Fail-closed on unresolvable IP | **Deny.** When the allowlist is non-empty and `resolve_client_ip` cannot determine a client IP (no `ConnectInfo<SocketAddr>`, peer address missing), the request is rejected with `403 forbidden_ip` and a `tracing::warn!` is emitted. |
| Q4 | Backward compat for empty allowlist | **Preserve `ip_whitelist = []` → allow all.** This keeps dev installs and unconfigured deployments working without code changes. Production deployments that set a non-empty list get the hardened behavior automatically. |

## 4. Design

### 4.1 Resolver helper (`crates/pm-auth/src/rbac.rs`)

New function:

```rust
/// Determine the client IP used for IP-allowlist enforcement.
///
/// Resolution rules:
/// 1. Start with the socket peer IP (`SocketAddr::ip()`).
/// 2. If `trusted_proxies` is non-empty **and** the socket peer is in
///    `trusted_proxies`, parse the leftmost entry of the `X-Forwarded-For`
///    header and use it (the immediate untrusted hop).
/// 3. If parsing `X-Forwarded-For` fails or the header is missing, fall back
///    to the socket peer IP.
/// 4. If the socket peer is unknown (no `ConnectInfo<SocketAddr>` is
///    available on the request), return `None` so the caller can apply
///    fail-closed logic when the allowlist is non-empty.
fn resolve_client_ip(
    headers: &HeaderMap,
    peer: Option<IpAddr>,
    trusted_proxies: &[IpNet],
) -> Option<IpAddr>
```

The function is pure, easy to unit test, and has no I/O. Logging is performed by
the caller (middleware) so test assertions can be made on behavior without
capturing tracing output.

### 4.2 Middleware change (`crates/pm-auth/src/rbac.rs`)

`require_auth` is changed to:

1. Extract the peer address from request extensions
   (`req.extensions().get::<ConnectInfo<SocketAddr>>()`).
2. Compute the resolved client IP via `resolve_client_ip`.
3. If `auth_config.ip_whitelist` is non-empty **and** no client IP could be
   resolved, return `403 forbidden_ip` (`"Client IP could not be determined"`)
   with a `tracing::warn!`.
4. If a client IP was resolved and the allowlist rejects it, return
   `403 forbidden_ip` (`"Access denied"`) with a `tracing::warn!` (existing
   message preserved for log continuity).
5. Otherwise continue to JWT validation (unchanged).

`axum::extract::ConnectInfo<SocketAddr>` is added as a request extension by the
axum server in `pm-web/src/main.rs` (one new line in the TCP/TLS listener
configuration; this is a required companion change to the middleware).

The old `extract_remote_ip` (header-only) is removed; the function is
superseded by `resolve_client_ip` and is not exported.

### 4.3 Config schema (`crates/pm-core/src/config.rs`)

Add a field to `SecurityConfig`:

```rust
/// IP addresses (CIDR or single IP) of trusted reverse proxies. When the
/// immediate TCP peer is in this list, `X-Forwarded-For` is honored; otherwise
/// the socket peer IP is used. Default: empty (do not trust `X-Forwarded-For`).
#[serde(default)]
pub trusted_proxies: Vec<String>,
```

The field parses to `Vec<IpNet>` at config-load time and is plumbed into
`AuthConfig::new` as a new `trusted_proxies: Arc<RwLock<Vec<IpNet>>>`
parameter (mirroring the existing `ip_whitelist` runtime-update pattern; an
`update_trusted_proxies` setter is added for symmetry, though no endpoint
needs it for this issue).

`Default for AppConfig` is updated to set `trusted_proxies: vec![]`.
`config/config.example.toml` gets a documented `trusted_proxies = []` entry
with a comment block explaining when to set it.

### 4.4 `pm-web` wiring (`crates/pm-web/src/main.rs`)

The axum listener is changed to use `into_make_service_with_connect_info::<SocketAddr>()`
so that `ConnectInfo<SocketAddr>` is available to extractors and middleware.
This is the documented axum pattern and is a one-line change per listener
(there are currently two listeners in `main.rs`: a TCP one for dev and a
TLS one for prod; both need the connect-info wrapper).

### 4.5 Response shape

Reuse the existing `forbidden` helper. Error code: `forbidden_ip` (new). Body:

```json
{ "error": { "code": "forbidden_ip", "message": "…" } }
```

Status: `403 Forbidden` for all IP rejections. Do not differentiate between
"unresolvable" and "not in allowlist" in the response; the specific reason is
logged server-side only.

### 4.6 Logging

- On allow (allowlist empty or IP matched): no new log line (existing flow
  continues silently).
- On deny (allowlist non-empty and IP not allowed, or IP unresolvable): new
  `tracing::warn!` with `client_ip = %ip_opt`, `peer = %peer_opt`,
  `xff_present = bool`, `reason = %reason`.

The existing `tracing::warn!` for blocked requests is preserved in shape so
log-greppers continue to work.

## 5. Acceptance Criteria

- [ ] A request with a non-empty allowlist and no `X-Forwarded-For` header is
      evaluated against the socket peer IP.
- [ ] A request with a non-empty allowlist and a spoofed `X-Forwarded-For`
      (set by a client that is **not** in `trusted_proxies`) is evaluated
      against the socket peer IP; the spoofed value is ignored.
- [ ] A request with a non-empty allowlist, an empty `trusted_proxies`, and
      no resolvable peer IP is rejected with `403 forbidden_ip`.
- [ ] A request with a non-empty allowlist and a valid `X-Forwarded-For` from
      a peer in `trusted_proxies` is evaluated against the leftmost untrusted
      hop.
- [ ] A request with an empty allowlist is allowed regardless of IP
      resolution (preserved behavior for dev installs).
- [ ] `cargo check` and `cargo clippy --all-targets` pass.
- [ ] `cargo test -p pm-auth` passes with new unit tests for `resolve_client_ip`
      and the middleware allow/deny matrix.
- [ ] `docs/security-review.md` documents the hardened control with a new row
      in the controls table referencing `crates/pm-auth/src/rbac.rs`.

## 6. Test Plan

### 6.1 Unit tests in `crates/pm-auth/src/rbac.rs` (cfg(test) module)

`resolve_client_ip` (12 tests):

1. `peer_only_no_xff` — no XFF, trusted_proxies empty → returns peer.
2. `peer_only_xff_untrusted` — XFF set, peer not in trusted_proxies, trusted_proxies
   non-empty → returns peer (XFF ignored).
3. `peer_only_trusted_proxies_empty_xff_present` — XFF set, trusted_proxies
   empty → returns peer (XFF ignored). [strict default]
4. `xff_trusted_peer_in_list` — XFF set, peer in trusted_proxies → returns
   parsed leftmost XFF entry.
5. `xff_trusted_peer_in_list_malformed_xff` — XFF unparseable, peer in
   trusted_proxies → falls back to peer.
6. `xff_trusted_peer_in_list_empty_xff` — XFF is empty string, peer in
   trusted_proxies → falls back to peer.
7. `xff_trusted_peer_in_list_multi_hop` — "1.2.3.4, 5.6.7.8" with peer in
   trusted_proxies → returns 1.2.3.4 (leftmost).
8. `no_peer_no_xff` — peer None, no XFF → returns None.
9. `no_peer_xff_untrusted` — peer None, XFF set, trusted_proxies empty →
   returns None (caller fails closed).
10. `xff_trusted_whitespace` — XFF `"  1.2.3.4"`, peer in trusted_proxies →
    returns 1.2.3.4 (trim).
11. `trusted_proxies_ipv6` — peer in IPv6 trusted list, IPv6 XFF → returns XFF.
12. `peer_ipv4_xff_ipv6_mismatch_trusted` — peer in trusted list, XFF is IPv6
    → returns parsed IPv6 (mixed family is fine).

`AuthConfig` integration with middleware (8 tests, using a small `TestApp`
harness with a `tower::ServiceExt`-style call into a single-route router —
no DB, no real HTTP listener):

13. `middleware_allows_when_whitelist_empty` — empty list + any IP → 200/ok.
14. `middleware_denies_when_whitelist_non_empty_and_ip_not_in_list` —
    non-empty list + peer outside → 403 `forbidden_ip`.
15. `middleware_allows_when_ip_in_list` — non-empty list + peer inside → 200.
16. `middleware_denies_when_no_peer_resolvable_and_whitelist_non_empty` —
    non-empty list + missing `ConnectInfo` → 403 `forbidden_ip`.
17. `middleware_spoofed_xff_ignored_when_peer_untrusted` — non-empty list +
    peer outside list + XFF inside list → 403 `forbidden_ip`.
18. `middleware_trusted_proxy_honors_xff` — non-empty list + peer in
    `trusted_proxies` + XFF inside list → 200.
19. `middleware_trusted_proxy_falls_back_to_peer_on_bad_xff` — peer in
    `trusted_proxies` + unparseable XFF + peer outside list → 403
    `forbidden_ip`.
20. `middleware_no_jwt_when_ip_blocked` — blocked request never reaches
    JWT validation (no `validate_access_token` call on deny path; covered by
    passing an obviously invalid token and asserting 403 not 401).

### 6.2 Test harness

A small `TestApp` helper builds a one-route `axum::Router` with a stub
handler that returns `200 OK` and a `require_auth` middleware. The harness
provides:

- A configurable `AuthConfig` (whitelist, trusted_proxies).
- A way to attach `ConnectInfo<SocketAddr>` (via a request-extension
  pre-set in the test).
- A way to add/omit the `Authorization: Bearer` header (for the
  `middleware_no_jwt_when_ip_blocked` test).

No real TCP listener, no DB, no async runtime beyond `#[tokio::test]`.

## 7. Risk Analysis

- **Risk: breaking change for deployments behind a reverse proxy that did not
  configure `trusted_proxies`.** Today, `X-Forwarded-For` from any caller is
  trusted (or, with the new code, ignored). After this change, such deployments
  will see the allowlist evaluate against the **proxy's** IP, which may not be
  in the allowlist and will cause 403s.
  - **Mitigation:** Document `trusted_proxies` prominently in
    `config/config.example.toml` with a clear warning. The default empty list
    is fail-closed (403), not fail-open, so misconfigured deployments will
    notice immediately rather than silently allowing traffic.
  - **Operational runbook:** add a "reverse proxy" section to the install
    docs describing the required config change.

- **Risk: dev installs behind a corporate proxy that injects XFF.** Same as
  above; documented in the example config and the runbook.

- **Risk: missing `ConnectInfo<SocketAddr>` in some test or alternate
  listener.** The middleware handles this gracefully (returns `None` from
  `resolve_client_ip` → fail-closed when allowlist non-empty → 403). The
  unit test matrix covers this path explicitly.

- **Risk: regression in JWT auth path.** The deny path short-circuits before
  JWT validation (test 20). The allow path is unchanged.

- **Risk: governor rate-limiter inconsistency.** `pm-web`'s rate-limiter
  uses `SmartIpKeyExtractor` from the `governor` crate, which has its own
  resolution semantics (governor's defaults). If Kelly wants the rate
  limiter to share `resolve_client_ip`, that's a follow-up issue and is
  called out in the lessons file as a known consistency gap.

## 8. Documentation Updates

- `config/config.example.toml`: new `trusted_proxies = []` entry with
  multi-line comment block.
- `docs/security-review.md`: new row in the controls table; update the
  existing IP-allowlist row to point to the new code path and the new
  `trusted_proxies` field.
- `docs/runbooks/`: (optional, per Kelly) add a short "Reverse proxy
  deployment" runbook.
- `SPEC.md`: (optional, per Kelly) one-paragraph update in the Security
  section.

## 9. Out of Scope / Follow-ups

- Sharing `resolve_client_ip` with the governor rate-limiter in `pm-web`
  (consistency improvement, separate change).
- mTLS client-cert CN/SAN allowlist (defense-in-depth beyond IP).
- Per-route IP allowlist (different routes, different lists). Current
  allowlist is global.