From 0f4e9d596f2b3bb85fdec2821fd0e5bcf6907425 Mon Sep 17 00:00:00 2001 From: "glm-5.1" Date: Mon, 15 Jun 2026 06:18:12 +0000 Subject: [PATCH] Update docs: reviews #005/#006 resolved status, AGENTS.md admin refactor - Review #005: update W2/W5/W6 rows to show resolved status - Review #005: update reviewed_code to reference new admin files - Review #006: update status from draft to resolved - Review #006: update Category 4 (SIGHUP/admin reload) for mtime check - Review #006: update Category 6 (admin interface) for HTTP API - Review #006: update P4 and reviewed_code for new file paths - AGENTS.md: update project structure (admin/, config/mod.rs) - AGENTS.md: update architecture concepts (admin HTTP, TOCTOU, wildcard) - AGENTS.md: update config format (admin_key_path) - AGENTS.md: update testing (admin HTTP tests) - AGENTS.md: update common modifications (curl instead of socat) --- AGENTS.md | 35 ++++++-- .../005-admin-socket-security-review.md | 14 +-- docs/reviews/006-attack-surface-review.md | 88 ++++++++++--------- 3 files changed, 86 insertions(+), 51 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0cd5098..54627d4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,6 +35,7 @@ src/ ├── cli.rs # CLI parsing (clap), config loading, validation ├── lib.rs # Library root, module declarations ├── config/ +│ ├── mod.rs # ReloadError, read_and_validate_config(), FullConfig │ ├── static_config.rs # StaticConfig — immutable, requires restart │ ├── dynamic_config.rs# DynamicConfig — hot-reloadable via ArcSwap │ ├── validation.rs # Config validation rules (called at startup and reload) @@ -54,8 +55,9 @@ src/ │ ├── mod.rs # Rate limiting middleware, eviction task │ └── bucket.rs # Token bucket implementation (IPv4 /32, IPv6 /64) ├── admin/ -│ ├── socket.rs # Unix domain socket admin API (reload, status) -│ └── mod.rs +│ ├── auth.rs # Bearer token auth middleware (subtle, SHA-256) +│ ├── handler.rs # HTTP handlers for /admin/reload, /status, /rotate-key +│ └── mod.rs # Re-exports ├── health.rs # Health check endpoint on localhost:9900 ├── logging/ │ ├── mod.rs # Logging init (file + stdout, ANSI disabled) @@ -69,7 +71,7 @@ src/ - **StaticConfig vs DynamicConfig**: Static config (bind addresses, TLS, ports) requires a restart. Dynamic config (sites, rate limits, body limits) - can be reloaded at runtime via SIGHUP or admin socket, using `ArcSwap` for + can be reloaded at runtime via SIGHUP or admin HTTP API, using `ArcSwap` for lock-free reads. - **Multi-listener**: `[[listeners]]` in TOML — each listener has its own bind address, TLS config, and site routing. Sites are collected into a global @@ -78,10 +80,18 @@ src/ (not appended), X-Real-IP is set from the connection's remote address. - **No `/health` on public listener**: Health checking is localhost:9900 only. The main listener does not intercept any paths. +- **Admin HTTP API**: Authenticated Bearer token endpoints on the health check + port (`admin_key_path`). Empty `admin_key_path` disables admin endpoints + (returns 404). Key file contains plaintext token, read once at startup. - **HTTP/2 client-facing only**: ALPN detects h2 vs http/1.1. Upstream connections are always HTTP/1.1. - **IPv6 rate limiting**: IPv6 addresses are normalized to /64 prefixes so addresses within the same /64 share a token bucket. +- **Config reload TOCTOU**: Both SIGHUP and admin HTTP reload paths use + `read_and_validate_config()` which checks file mtime before and after + reading. If mtime changed, reload is rejected with a retry message. +- **Wildcard bind consistency**: `cli_allow_wildcard_bind` is stored in + `ConfigReloadHandle` and used for both startup and reload validation. ## Config Format @@ -99,12 +109,15 @@ TOML. See `docs/architecture/config.md` for full schema. Key validation rules: - `http_port` must be 0 (disabled) or 1–65535; `https_port` must be 1–65535 - `health_check_port` must not conflict with any listener's http_port or https_port on the same bind address +- `admin_key_path` must be an absolute path or empty (empty disables admin); + path traversal (`..`) is rejected; default is `/etc/reverse-proxy/admin-key` ## Testing Tests use `rcgen` for self-signed certificate generation and `reqwest` for HTTP client requests. Integration tests are in `tests/integration_test.rs` -with helpers in `tests/helpers/`. +with helpers in `tests/helpers/`. Admin endpoint tests use `reqwest` HTTP +client with Bearer token authentication. ```bash cargo test # all tests @@ -139,7 +152,19 @@ See `deploy/README.md` for step-by-step setup instructions. Add a `[[listeners.sites]]` entry to config and reload: ```bash -echo "reload" | socat - UNIX-CONNECT:/run/reverse-proxy/admin.sock +curl -X POST -H "Authorization: Bearer $ADMIN_KEY" http://127.0.0.1:9900/admin/reload +``` + +### Checking proxy status + +```bash +curl -H "Authorization: Bearer $ADMIN_KEY" http://127.0.0.1:9900/admin/status +``` + +### Rotating the admin key + +```bash +curl -X POST -H "Authorization: Bearer $ADMIN_KEY" http://127.0.0.1:9900/admin/rotate-key ``` ### Changing rate limits diff --git a/docs/reviews/005-admin-socket-security-review.md b/docs/reviews/005-admin-socket-security-review.md index f7b22ed..a583919 100644 --- a/docs/reviews/005-admin-socket-security-review.md +++ b/docs/reviews/005-admin-socket-security-review.md @@ -2,11 +2,13 @@ status: resolved last_updated: 2026-06-15 reviewed_code: - - src/admin/socket.rs + - src/admin/auth.rs + - src/admin/handler.rs - src/admin/mod.rs - src/main.rs - src/shutdown.rs - src/config/dynamic_config.rs + - src/config/mod.rs - src/config/validation.rs - src/health.rs - src/proxy/handler.rs @@ -32,16 +34,16 @@ ADR-028. Each finding is resolved as follows: | C2 (no authentication) | **Resolved by ADR-028** — Bearer token with constant-time comparison | | C3 (info leak) | **Resolved by ADR-028** — generic error messages, details logged server-side | | W1 (no conn limit) | **Resolved by ADR-028** — axum/TCP backlog handles this naturally | -| W2 (config TOCTOU) | **Tracked separately** — ADR-029, task `fix/config-reload-toctou` | +| W2 (config TOCTOU) | **Resolved by ADR-029** — mtime check before/after read, task `fix/config-reload-toctou` implemented | | W3 (path validation) | **Resolved by ADR-028** — no socket path to validate; `admin_key_path` validation added (config.md rule 20) | | W4 (is_socket_active side effect) | **Resolved by ADR-028** — no stale socket detection needed | -| W5 (wildcard flag) | **Tracked separately** — ADR-030, task `fix/wildcard-flag-reload` | -| W6 (changed_fields in response) | **Will be addressed** — `fix/admin-http-api` task includes `changed_fields` in `/admin/reload` response | +| W5 (wildcard flag) | **Resolved by ADR-030** — `cli_allow_wildcard_bind` stored in `ConfigReloadHandle`, task `fix/wildcard-flag-reload` implemented | +| W6 (changed_fields in response) | **Resolved** — `/admin/reload` endpoint returns status; changed_fields warning logged server-side | | W7 (health check port recon) | **Accepted risk** — localhost-only, minimal information. Admin endpoints add authentication | | S1–S6 (suggestions) | **Resolved by ADR-028** — all suggestions relate to the socket, which is removed | -Implementation tasks: `fix/admin-http-api`, `fix/config-reload-toctou`, -`fix/wildcard-flag-reload`. +Implementation tasks: `fix/admin-http-api` (done), `fix/config-reload-toctou` (done), +`fix/wildcard-flag-reload` (done). ## Purpose diff --git a/docs/reviews/006-attack-surface-review.md b/docs/reviews/006-attack-surface-review.md index 455187b..6bd7df4 100644 --- a/docs/reviews/006-attack-surface-review.md +++ b/docs/reviews/006-attack-surface-review.md @@ -1,6 +1,6 @@ --- -status: draft -last_updated: 2026-06-14 +status: resolved +last_updated: 2026-06-15 reviewed_code: - src/server.rs - src/proxy/handler.rs @@ -14,7 +14,9 @@ reviewed_code: - src/tls/redirect.rs - src/rate_limit/mod.rs - src/rate_limit/bucket.rs - - src/admin/socket.rs + - src/admin/auth.rs + - src/admin/handler.rs + - src/admin/mod.rs - src/shutdown.rs - src/config/static_config.rs - src/config/dynamic_config.rs @@ -316,25 +318,29 @@ decisions (bind addresses, TLS, upstream targets, rate limits, etc.) ### 4.2 Config File Read (Reload — SIGHUP) **Source**: Filesystem (same config file, re-read on SIGHUP) -**Entry**: `src/shutdown.rs:88` — `tokio::fs::read_to_string(config_path).await` +**Entry**: `src/config/mod.rs` — `read_and_validate_config()` (called from + `src/shutdown.rs:handle_sighup_reload`) **Input**: Entire contents of the config file at reload time **Validation**: Same `FullConfig::parse()` + `validate()` pipeline. Failed - parse/validation retains old config (failsafe). + parse/validation retains old config (failsafe). Mtime is checked before and + after reading to detect mid-write changes (ADR-029). **Sink**: Swapped into `ArcSwap` and `ArcSwap` -**Risk**: Medium. **TOCTOU between reads**: If another process is writing the - config file at the exact moment SIGHUP triggers a read, a partial file could - be read. The parse would likely fail (invalid TOML), triggering a reload - error, which is safe. But a carefully timed write could produce a valid-but- - malicious partial file. This is the same issue noted in review #005 (W2). +**Risk**: Low. TOCTOU between reads is mitigated by the mtime check, which + rejects the reload if the file changed during read. A carefully timed write + that starts before the first `stat()` and completes before the read could + still produce a partial file, but this is extremely unlikely in practice. -### 4.3 Config File Read (Reload — Admin Socket) +### 4.3 Config File Read (Reload — Admin HTTP) -**Source**: Filesystem (same config file, re-read on admin "reload" command) -**Entry**: `src/admin/socket.rs:257` +**Source**: Filesystem (same config file, re-read on admin HTTP POST /admin/reload) +**Entry**: `src/config/mod.rs` — `read_and_validate_config()` (called from + `src/admin/handler.rs:reload_handler` and `src/shutdown.rs:handle_sighup_reload`) **Input**: Same as 4.2 -**Validation**: Same pipeline, plus reload mutex serialization -**Risk**: Same as 4.2. Additionally, the admin socket is unauthenticated -(review #005, C2), so any local user can trigger a config re-read. +**Validation**: Same pipeline, plus reload mutex serialization. Additionally, + mtime is checked before and after reading to detect mid-write changes (ADR-029). + Admin HTTP endpoint requires Bearer token authentication (ADR-028). +**Risk**: Lower than 4.2. TOCTOU between reads is mitigated by the mtime check + (rejects reload if file changed during read). Admin endpoint is authenticated. ### 4.4 Config Values Used in URL Construction @@ -416,30 +422,31 @@ decisions (bind addresses, TLS, upstream targets, rate limits, etc.) ## Category 6: Admin Interface -### 6.1 Admin Socket Connections +### 6.1 Admin HTTP Authentication -**Source**: Local process (Unix domain socket) -**Entry**: `src/admin/socket.rs:110` — `listener.accept()` -**Input**: Unix domain socket connections from local processes -**Validation**: No authentication or peer credential checking. Any process with - filesystem access to the socket can connect. -**Risk**: Covered extensively in review #005 (C2). Being replaced by - authenticated HTTP endpoint per the architectural recommendation in that - review. +**Source**: Network (HTTP requests to `/admin/*` on health check port) +**Entry**: `src/admin/auth.rs` — Bearer token authentication middleware +**Input**: HTTP requests with `Authorization: Bearer ` header +**Validation**: Constant-time SHA-256 comparison of provided key against stored + hash. Key hash loaded from file at startup (`src/admin/auth.rs:load_admin_key`). + Admin endpoints return 404 if `admin_key_path` is empty (admin disabled). +**Risk**: Low. Bearer token is validated on every request. Key rotation available + via `/admin/rotate-key`. See review #005 (C2, C3) for the original socket + findings that prompted the migration to authenticated HTTP. -### 6.2 Admin Command Input +### 6.2 Admin HTTP Command Processing -**Source**: Local process (text sent over Unix socket) -**Entry**: `src/admin/socket.rs:167-252` — `handle_connection()` -**Input**: Text command string (expected: "reload", "status", or empty/unknown) +**Source**: Network (HTTP POST/GET requests to `/admin/reload`, `/admin/status`, + `/admin/rotate-key`) +**Entry**: `src/admin/handler.rs` — Axum handlers for admin endpoints +**Input**: HTTP request bodies (minimal — no user input processed beyond auth) **Validation**: -- 4 KiB read limit via `.take(4096)` (`socket.rs:171`) -- 5-second read timeout (`socket.rs:174-175`) -- Newline termination required -- Command allowlist: only "reload" and "status" are recognized -- Unknown commands echo input back: `"unknown command: {input}"` (minor - information disclosure) -**Risk**: Covered in review #005 (C3, W1). +- All admin endpoints require Bearer token auth (401 without valid token) +- Config reload reads file with mtime TOCTOU check (ADR-029) +- Status and rotate-key endpoints return JSON with no user-controlled input +**Risk**: Low. No arbitrary command input as in the previous socket interface. + See review #005 (C1, C2, C3, W1, W3, W4, S1–S6) for the original findings + that were resolved by the HTTP migration (ADR-028). --- @@ -621,11 +628,12 @@ disagree on header parsing. Specific patterns: ### P4. Config File Read on Reload -**File**: `src/shutdown.rs:88`, `src/admin/socket.rs:257` +**File**: `src/config/mod.rs` — `read_and_validate_config()` (called from + `src/shutdown.rs:handle_sighup_reload` and `src/admin/handler.rs:reload_handler`) **Why**: Config is re-read from disk on SIGHUP and admin reload. TOCTOU between -read and use. If config is compromised (via a separate vulnerability or -misconfiguration), the proxy will happily apply attacker-controlled upstream -addresses, creating an effective SSRF. + read and use is mitigated by mtime check (ADR-029). If config is compromised + (via a separate vulnerability or misconfiguration), the proxy will happily + apply attacker-controlled upstream addresses, creating an effective SSRF. ### P5. ACME Directory URL