fix(security): harden IP allowlist against XFF bypass and spoofing (#3)
Hardens the IP allowlist in require_auth against the two bypasses filed in #3. 1. Bypass via missing X-Forwarded-For (no IP to check, allowlist skipped). 2. Spoofing via attacker-controlled X-Forwarded-For (header trusted unconditionally). Resolves both by deriving the client IP from the socket peer (ConnectInfo<SocketAddr>) and only honoring X-Forwarded-For when the immediate peer is in a new security.trusted_proxies allowlist (default empty = strict). Fails closed with 403 forbidden_ip when a non-empty allowlist is configured and the client IP cannot be determined. Empty ip_whitelist continues to mean allow all (preserved for dev installs). 27 pm-auth tests pass (12 new resolver + 8 new middleware + 7 existing). Spec: tasks/ip-allowlist-spec.md.
This commit is contained in:
Draco-Lunaris-Echo
committed by
GitHub
GitHub
parent
8873b2c70c
commit
3bdae4bcc5
281
tasks/ip-allowlist-spec.md
Normal file
281
tasks/ip-allowlist-spec.md
Normal file
@ -0,0 +1,281 @@
|
||||
# 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.
|
||||
@ -100,6 +100,87 @@ _(filled in at completion)_
|
||||
- **tokio::sync::Mutex over std::sync::Mutex** — Axum handlers must be Send; std::sync::MutexGuard is not Send across await points.
|
||||
- **DashMap session cleanup** — In-memory session stores (DashMap) need periodic cleanup tasks to prevent memory leaks. Pattern: tokio::spawn with interval + retain with time-based cutoff.
|
||||
|
||||
# IP Allowlist Hardening — Implementation Plan (Issue #3)
|
||||
|
||||
Spec: `tasks/ip-allowlist-spec.md` (v0.1.0, awaiting sign-off)
|
||||
|
||||
## Issues Identified
|
||||
1. **Allowlist bypass via missing XFF** — `extract_remote_ip` returns `None` when
|
||||
the header is absent, and the middleware's `if let Some(ip)` block has no
|
||||
`else` branch, so a request without `X-Forwarded-For` skips the check.
|
||||
2. **Allowlist spoofing via XFF** — `extract_remote_ip` reads the header
|
||||
unconditionally; any client can claim to be from a whitelisted IP.
|
||||
3. **No trusted-proxy concept** — there is no config field to declare which
|
||||
intermediate proxies are allowed to set `X-Forwarded-For`.
|
||||
4. **No `ConnectInfo<SocketAddr>` wiring** — the axum listeners in
|
||||
`pm-web/src/main.rs` do not use `into_make_service_with_connect_info`, so
|
||||
the middleware cannot access the real peer address.
|
||||
|
||||
## Phases
|
||||
|
||||
### Phase 1: Resolver helper in pm-auth
|
||||
- [ ] 1a: Add `fn resolve_client_ip(headers, peer, trusted_proxies) -> Option<IpAddr>`
|
||||
- [ ] 1b: Add 12 unit tests in `crates/pm-auth/src/rbac.rs` (cfg(test)) covering
|
||||
the resolution matrix (peer-only, XFF trusted/untrusted, multi-hop,
|
||||
IPv6, malformed, missing peer)
|
||||
- [ ] 1c: Run `cargo test -p pm-auth` and confirm green
|
||||
|
||||
### Phase 2: AuthConfig + SecurityConfig schema
|
||||
- [ ] 2a: Add `trusted_proxies: Arc<RwLock<Vec<IpNet>>>` to `AuthConfig`
|
||||
- [ ] 2b: Add `trusted_proxies: Vec<String>` to `SecurityConfig` in `crates/pm-core/src/config.rs`
|
||||
- [ ] 2c: Update `Default for AppConfig` to include `trusted_proxies: vec![]`
|
||||
- [ ] 2d: Add `update_trusted_proxies` setter on `AuthConfig` (symmetric to
|
||||
`update_ip_whitelist`)
|
||||
- [ ] 2e: Update `config/config.example.toml` with a documented `trusted_proxies`
|
||||
entry and a reverse-proxy runbook comment block
|
||||
- [ ] 2f: Plumb `trusted_proxies` from `SecurityConfig` into `AuthConfig::new`
|
||||
in `pm-web/src/main.rs`
|
||||
- [ ] 2g: Run `cargo check` and `cargo clippy --all-targets`
|
||||
|
||||
### Phase 3: Middleware change
|
||||
- [ ] 3a: Update `require_auth` to extract `ConnectInfo<SocketAddr>` from
|
||||
request extensions and call `resolve_client_ip`
|
||||
- [ ] 3b: Add fail-closed path: non-empty allowlist + unresolvable IP →
|
||||
`403 forbidden_ip`
|
||||
- [ ] 3c: Replace `forbidden("Access denied")` with the new error code in IP-deny path
|
||||
- [ ] 3d: Add `tracing::warn!` with `client_ip`, `peer`, `xff_present`, `reason`
|
||||
- [ ] 3e: Remove the old `extract_remote_ip` (header-only) function
|
||||
- [ ] 3f: Run `cargo check` and `cargo clippy --all-targets`
|
||||
|
||||
### Phase 4: pm-web listener wiring
|
||||
- [ ] 4a: Switch both TCP and TLS axum listeners in `pm-web/src/main.rs` to
|
||||
`into_make_service_with_connect_info::<SocketAddr>()`
|
||||
- [ ] 4b: Run `cargo check -p pm-web`
|
||||
|
||||
### Phase 5: Middleware integration tests
|
||||
- [ ] 5a: Add `TestApp` harness in `crates/pm-auth/src/rbac.rs` cfg(test)
|
||||
(no DB, single-route router, `tower::ServiceExt`-style call)
|
||||
- [ ] 5b: Add 8 middleware integration tests per spec section 6.1
|
||||
(allow empty, deny non-empty, allow in list, fail-closed no peer,
|
||||
spoofed XFF ignored, trusted proxy honors XFF, bad XFF fallback,
|
||||
no-JWT on deny)
|
||||
- [ ] 5c: Run `cargo test -p pm-auth` and confirm green
|
||||
|
||||
### Phase 6: Documentation
|
||||
- [ ] 6a: Update `docs/security-review.md` — update existing IP-allowlist row
|
||||
and reference new code path + `trusted_proxies` field
|
||||
- [ ] 6b: Update `SPEC.md` Security section (one paragraph)
|
||||
- [ ] 6c: Add a "Reverse proxy deployment" runbook under `docs/runbooks/`
|
||||
(optional, per Kelly)
|
||||
|
||||
### Phase 7: Review & commit
|
||||
- [ ] 7a: Self-review against the 8 acceptance criteria in the spec
|
||||
- [ ] 7b: Run `bash /a0/usr/skills/git-workflow/scripts/validate-push.sh`
|
||||
- [ ] 7c: Commit on `fix/3-ip-allowlist-bypass` (per git-workflow skill)
|
||||
- [ ] 7d: Push to `github/fix/3-ip-allowlist-bypass` and open PR against `master`
|
||||
- [ ] 7e: Comment on issue #3 linking the PR; close issue on merge
|
||||
- [ ] 7f: Capture lessons in this file
|
||||
|
||||
## Lessons Learned (this issue)
|
||||
_(filled in at completion)_
|
||||
|
||||
---
|
||||
|
||||
# Host Self-Enrollment Implementation Plan
|
||||
|
||||
## Phases
|
||||
|
||||
Reference in New Issue
Block a user