From 50db27806ba317cadb366dc2b9c1f02a016c4b75 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Fri, 12 Jun 2026 19:09:37 +0200 Subject: [PATCH] docs(audit-2026-06-11/tier3): handback + independent review artifacts Co-Authored-By: Claude Opus 4.8 (1M context) --- security_audit/HANDBACK_tier3.md | 135 +++++++++++++++++++++++++++++++ security_audit/REVIEW_tier3.md | 133 ++++++++++++++++++++++++++++++ 2 files changed, 268 insertions(+) create mode 100644 security_audit/HANDBACK_tier3.md create mode 100644 security_audit/REVIEW_tier3.md diff --git a/security_audit/HANDBACK_tier3.md b/security_audit/HANDBACK_tier3.md new file mode 100644 index 0000000..a636d0c --- /dev/null +++ b/security_audit/HANDBACK_tier3.md @@ -0,0 +1,135 @@ +# Handback — Security remediation Tier-3 "safety batch" (`fix/audit-2026-06-11-tier3`) + +Implementer: assistant (inline, with iterative testing). +Date: 2026-06-12. +Branch: `fix/audit-2026-06-11-tier3` (8 commits off `main`, unpushed). +Scope: the user-selected "Safety batch" from the post-merge review of `fix/audit-2026-06-11`. + +## Verdict from my own testing: green, ready for your review. + +- `cargo fmt --all --check` → clean. +- `cargo clippy --all-targets --all-features -- -D warnings` → clean (exit 0). +- Full workspace `--include-ignored` against a fresh Postgres 16: **824 passed**; the + previously-failing `authz::deactivating_user_revokes_their_api_keys` now **passes**. +- Full `picloud-cli` suite in isolation (fresh DB + dev key): **76 passed**. +- Two "failures" in the *combined* `--no-fail-fast` run are the same harness artifacts the + original REVIEW.md documented — see "Test-harness artifacts" below. Neither is a code defect. + +## Commits (one finding per commit) + +| Commit | Finding | What landed | +|---|---|---| +| `31ecfae` | PrincipalCache revocation-lag (+ H-E1, test rename) | `evict_user`/`evict_token` on the cache; one shared cache Arc across AuthState/AdminsState/ApiKeysState; evict on deactivation, password change, logout, single API-key delete. Greens the failing test. | +| `95fddf3` | C-2 sanitizer + F-SE-H-03 | `sanitize_stored_content_type` rejects control chars up-front (was passable via an `image/` prefix → header-injection/panic); `disable_symbol("debug")` to match `print`. | +| `e676eb9` | F-SE-H-04 | Scrub `ExecError::Runtime` detail from public data-plane responses; log full text under a correlation id. | +| `d9fb0e7` | F-FS-002 | `guard_collection` on repo `head/get/list/delete`; `validate_files_collection` on the three admin endpoints (clean 422). | +| `3ac1022` | H-1 | Caddy `request_body { max_size 12MB }` (both Caddyfiles, validated); explicit `DefaultBodyLimit::max(1MB)` on the public email-inbound webhook. | +| `bdcc9a6` | H2 (+ M2) | Reject inline `--password`/`--token` (stdin-only); `rpassword` no-echo prompt for admins. | +| `e8cc3af` | H-B1 follow-up | `extract_remote_ip` uses the **last** X-Forwarded-For hop (Caddy-appended real client) instead of the spoofable first hop. | +| `ec4a2aa` | — | `cargo fmt`. | + +## Detail per item + +### 1. PrincipalCache revocation-lag (`31ecfae`) — greens the failing test +The cache (`token-hash → Principal`, 60s TTL) had no eviction hook, so deactivation / +password change / API-key revocation flipped the DB rows while the cache kept serving the +stale principal until TTL. Added `PrincipalCache::evict_user(user_id)` and `evict_token(hash)`. +Wired one **shared** cache `Arc` (hoisted in `lib.rs`) into `AuthState`, `AdminsState`, and +`ApiKeysState` — a per-state cache would let the middleware's own copy keep authenticating a +revoked principal. Evict points: deactivation + password change (`evict_user`), logout +(`evict_token`, precise), single API-key delete (`evict_user`). Unit tests for both methods +plus the existing `authz::deactivating_user_revokes_their_api_keys` integration test now pass. + +**H-E1 note (folded in):** the DB-write invalidation stays best-effort by design (the +deactivation path deliberately doesn't undo on a DB blip; I kept password-change consistent). +The cache evict is what now makes revocation take effect on the *next* request rather than +after the TTL. I did **not** convert it to a hard abort — that would contradict the existing +deliberate design and isn't clearly better. + +### 2. C-2 sanitizer + F-SE-H-03 (`95fddf3`) +`sanitize_stored_content_type("image/png\r\nX-Injected: 1")` previously matched the `image/` +prefix and returned the original (CRLF intact) → response-header injection / `.expect()` panic +on download. Now any control byte (`<0x20` or `0x7f`) coerces to `application/octet-stream` +first. Also `disable_symbol("debug")` (the comment already claimed both were disabled). + +### 3. F-SE-H-04 (`e676eb9`) +`ExecError::Runtime` strings (FS paths, `"blocked by SSRF policy: link-local"`, pool-exhaustion +timing, panic fragments, attacker-thrown strings) were returned verbatim in the public +`/api/v1/execute` + user-route 502 bodies. This handler only serves anonymous data-plane +callers (the authenticated script-test path is in manager-core and keeps verbose errors), so +the full text is logged under a `correlation_id` and the client gets `"script execution error +(ref: )"`. New test asserts a thrown marker string doesn't leak. + +### 4. F-FS-002 (`d9fb0e7`) +Admin files endpoints hit `FsFilesRepo` directly (not via the validating `FilesServiceImpl`). +Added `guard_collection` to repo `head/get/list/delete` (create/update already had it) and +`validate_files_collection` to the three admin endpoints for a clean 422 instead of the repo +guard surfacing as a 500. + +### 5. H-1 (`3ac1022`) +Caddy `request_body { max_size 12MB }` in both Caddyfiles (12MB clears the orchestrator's +10 MiB user-route read; replaces Caddy's multi-GB default). Both validated with +`caddy validate`. Email-inbound webhook gets an explicit `DefaultBodyLimit::max(1MB)`. Admin / +execute keep Axum's 2MB extractor default; user routes keep their 10 MiB manual read. + +### 6. H2 + M2 (`bdcc9a6`) +`--password`/`--token` reject inline values (leak into shell history / `ps` / `/proc`); only +`-` (stdin) is accepted, `PICLOUD_TOKEN` env still covers CI for login. Interactive admins +prompt switched to `rpassword` (true no-echo; the old `read_line` echoed despite the claim). +New CLI test asserts inline `--password hunter2` is rejected. + +### 7. H-B1 follow-up (`e8cc3af`) +`extract_remote_ip` took the **first** X-Forwarded-For entry; Caddy appends the real client as +the **last** hop, so earlier entries are client-spoofable — an attacker could rotate the first +entry to evade the per-IP login limiter. Now takes the last hop. Three unit tests (spoof, +single-hop, missing-header). + +## Deferred (with rationale) + +- **Bootstrap sentinel (H-E2)** — deliberately NOT done. The API can't empty `admin_users` + (the last-active-admin guard blocks delete *and* deactivate of the final active admin), so + `count_active() > 0` always holds via normal operation. The only way to `count_active() == 0` + is direct SQL deletion, which already implies DB-write compromise (at which point an attacker + just inserts their own admin row — bootstrap re-seed is moot). A persistent sentinel would + only defend that niche **and** needs a durable marker = a schema migration, which the chosen + "no new schema" batch scope excludes. Re-fold into the v1.2 schema batch if desired. + +Everything else in the audit's Tier-3/4 (per-app quotas, audit-log table, AAD re-encryption +sweep, absolute session expiry, outbox GC, per-execution SDK counters, per-app trigger/cron +caps, etc.) remains deferred per the chosen scope. + +## Operational notes / behaviour changes + +- `pic admins create/set --password ` and `pic login --token ` now **error**. + Scripts must use `--password -` / `--token -` (piped) or `PICLOUD_TOKEN`. +- Public-route runtime errors no longer echo detail; operators correlate via the logged + `correlation_id`. +- Caddy now rejects request bodies > 12 MB at the proxy. + +## Test-harness artifacts (NOT regressions — same as the original review) + +A combined `cargo test --workspace --no-fail-fast -- --include-ignored` against one DB shows 77 +"failures": +- **76 × picloud-cli** — the CLI fixture spawns the `picloud` binary and bootstraps an admin + into the shared base DB; run concurrently with the rest of the workspace it collides and the + fixture `LazyLock` poisons. **In isolation against a fresh DB the full suite is 76/76 green.** +- **1 × engine doctest** (`set_self_weak`, line 172) — an `ignore`d pseudo-code example that + `--include-ignored` forces to compile. Under a normal `cargo test` it's skipped. + +How to reproduce a clean run: +``` +# fresh DB +docker run -d --rm --name pg -p 127.0.0.1:15495:5432 -e POSTGRES_USER=picloud \ + -e POSTGRES_PASSWORD=picloud -e POSTGRES_DB=picloud postgres:16-alpine +export DATABASE_URL=postgres://picloud:picloud@127.0.0.1:15495/picloud +export PICLOUD_DEV_MODE=true PICLOUD_DEV_INSECURE_KEY=i-understand-this-is-insecure +# everything except CLI (CLI needs its own DB): +cargo test --workspace --exclude picloud-cli -- --include-ignored +# CLI on its own fresh DB: +cargo test -p picloud-cli --test cli -- --include-ignored +``` + +## Suggested next step + +ff-merge `fix/audit-2026-06-11-tier3` into `main` after your review. The branch is off the +current `main` (which already has the Tier-1+2 work) and fast-forwards cleanly. diff --git a/security_audit/REVIEW_tier3.md b/security_audit/REVIEW_tier3.md new file mode 100644 index 0000000..f7a05bf --- /dev/null +++ b/security_audit/REVIEW_tier3.md @@ -0,0 +1,133 @@ +# Independent review — Tier-3 safety batch + +Verdict: APPROVE WITH NITS + +Scope reviewed: `git diff main...fix/audit-2026-06-11-tier3` (8 commits), every touched +source file traced end-to-end, targeted non-DB unit tests run green, clippy clean on the +four touched Rust crates. DB-backed `#[ignore]` integration tests were read but not run. + +## Per-fix verdict + +### 1. PrincipalCache revocation-lag — PASS +Traced the Arc end to end. `crates/picloud/src/lib.rs:503` constructs ONE +`Arc` and `.clone()`s it into `AuthState` (510), `AdminsState` (523), +and `ApiKeysState` (537). The middleware reads via `resolve_principal` → +`state.principal_cache.get(...)` (`auth_middleware.rs:242`) where `state` is the same +`auth_state` wired into both `require_authenticated` (lib.rs:571-573) and `auth_router` +(lib.rs:596). It is genuinely one shared instance, so an evict on the revocation side is +visible to the resolve side. No second `AuthState` is constructed anywhere. +Keying is correct: `evict_user(user_id)` (`retain(p.user_id != user_id)`) on deactivation +(`admin_users_api.rs:339`), password change (`admin_users_api.rs:266`), and single +API-key delete (`api_keys_api.rs:170` — coarse-by-design because the cache is hashed by +token, not key id); `evict_token(&hash)` (precise, single session) on logout +(`auth_api.rs:201`). Unit tests `evict_user_drops_only_that_users_entries` and +`evict_token_drops_only_that_entry` pass and actually assert the targeting (Bob's entry +survives; the user's other session survives logout). +Note: this remains a single-process cache; in cluster mode (deferred) revocation would +still lag on peer nodes. Acceptable for MVP/single-node. + +### 2. content-type sanitizer — PASS +`shared/src/files.rs:280` — `if ct.bytes().any(|b| b < 0x20 || b == 0x7f) { return +SAFE_RENDER_FALLBACK }` runs FIRST, before the `split(';')` / allowlist logic. Byte check +is exactly `< 0x20 || == 0x7f`. Test `control_chars_are_coerced_even_under_an_allowlisted +_prefix` covers `image/png\r\n…`, bare `\n`, `\u{7f}`, `\u{0}` and passes. CRLF in an +allowlisted prefix now coerces to `application/octet-stream`. Closes the header-injection +/ HeaderValue-panic vector. + +### 3. disable debug — PASS +`engine.disable_symbol("debug")` sits next to `disable_symbol("print")` inside +`build_engine` (`executor-core/src/engine.rs:412`), which is the per-call builder invoked +on every execution path (incl. the real run path at engine.rs:283). Test +`debug_and_print_symbols_are_disabled` asserts both `print("x")` and `debug("x")` fail as +`ExecError::Parse`; passes. + +### 4. runtime-error scrub — PASS +The scrub is in `orchestrator-core/src/api.rs:753` inside `impl IntoResponse for ApiError` +— the public data-plane response path (`/api/v1/execute/{id}` + user-route fallback). It +logs full `detail` under a `correlation_id` (warn) and returns `"script execution error +(ref: )"`. I checked there is NO admin/authenticated path reusing this `IntoResponse` +— the only other `ExecError` consumer in manager-core is `dispatcher.rs` (queue/cron +background, stores `last_error` internally, not a public HTTP body). `Parse` / +`InvalidResponse` / `Timeout` / `OperationBudgetExceeded` stay verbose. `Parse` can echo +script-syntax detail, but that is the caller's own script and is deterministic/compile- +shape, not internal infra detail — acceptable. The handback's claim of a separate "verbose +admin script-test path" appears moot (no such endpoint found), but that is harmless — the +public path is the one that mattered and it is scrubbed. + +### 5. files collection validation — PASS +All four repo READ/DELETE methods now guard at method entry: `head` +(`files_repo.rs:353`), `get` (373), `delete` (439), `list` (485); create/update (321,397) +already guarded. `guard_collection` rejects empty / `/` / `\` / `..` / `\0`. The three +admin endpoints call `validate_files_collection` (a re-export of `shared::validate_ +collection`, same reject-set) BEFORE touching the repo — `list_files` (files_api.rs:91), +`get_file` (153), `delete_file` (198) — yielding a clean 422 instead of a repo-surfaced +500. Belt-and-suspenders is real, both layers verified. + +### 6. body limits — PASS +`DefaultBodyLimit::max(1 MiB)` is applied with `.layer(...)` on the email-inbound `Router` +BEFORE `.with_state` (`email_inbound_api.rs:184-186`) — correct axum ordering, layer wraps +the route. Caddy `request_body { max_size 12MB }` is placed inside the site block in both +`caddy/Caddyfile` and `caddy/Caddyfile.prod` (valid placement). 12 MB (Caddy decimal = +12,000,000) ≥ the orchestrator's 10 MiB user-route read (10,485,760, `api.rs:219`), so +legit large invokes pass while the multi-GB Caddy default is replaced. Admin/execute keep +axum's default extractor limit. + +### 7. CLI argv-secret rejection — PASS +`resolve_password` (`admins.rs:51`) rejects `Some(non-dash)` with a clear error, accepts +only `Some("-")` (stdin) and errors on `None`. `login.rs:30` rejects inline `--token`, +accepts `--token -` (stdin) and still honors `PICLOUD_TOKEN` env. Interactive prompts use +`rpassword::prompt_password` (true no-echo) for both password and token; the old echoing +`read_line` trait was removed. `rpassword = "7"` is a declared dep. No internal caller +passes an inline secret: `pic secrets set` already reads value from stdin only +(`secrets.rs`), and the only `resolve_password` callers are `create`/`set`. New test +`create_with_inline_password_is_rejected` asserts `--password hunter2` fails with "inline +is not allowed". + +### 8. X-Forwarded-For last-hop — PASS +`extract_remote_ip` now uses `s.split(',').next_back()` (`auth_api.rs:268`), trims, falls +back to `"unknown"` on absent/empty/whitespace header (verified: `xff(" ")` → trimmed +empty → falls through to `"unknown"`). Trust model is correct for the documented single- +Caddy topology: Caddy's reverse_proxy APPENDS the real peer as the last XFF hop, so earlier +entries are client-spoofable and the last entry is authoritative. Three unit tests (spoof- +stable, single-hop, missing/empty) pass. The doc-comment honestly flags the multi-proxy +caveat as deferred to cluster work. + +## Bootstrap-sentinel deferral — VALID +Verified BOTH guards exist in `admin_users_api.rs`: +- Deactivation path: `patch_admin` at 298-302 returns `LastActiveAdmin` when + `count_active_excluding(id) == 0` (plus a last-owner guard at 310-314). +- Delete path: `delete_admin` at 372-385 mirrors it (`count_active_excluding == 0` → + `LastActiveAdmin`, plus last-owner guard). +The delete guard only fires `if target.is_active`, but an inactive admin is not counted by +`count_active`, so deleting one cannot drive `count_active()` to 0. There is no API path to +`count_active() == 0`; reaching it requires direct SQL (= DB-write compromise, at which +point re-seed is moot). The deferral rationale holds. Sentinel needs a durable marker +(schema migration) which is out of the chosen no-schema scope — reasonable to defer. + +## New issues / regressions found +None blocking. Nits: +- **(nit, stale doc)** `auth_middleware.rs:141-142` still says the `principal_cache` field + is "`None` for tests / harnesses that don't wire it", but the field is a non-Option + `Arc`. Cosmetic leftover; update or drop the sentence. +- **(nit)** `crates/picloud-cli/tests/admins.rs` `create_with_inline_password_is_rejected` + is gated `#[ignore = "needs DATABASE_URL"]`, but the inline rejection happens in + `resolve_password` before any network/DB call, so the test would pass without Postgres. + Harmless over-gating; could be un-ignored for cheap coverage. +- **(observation, out of scope)** The API-key-delete eviction is coarse (`evict_user`, + dropping all of that user's cached sessions+keys) because the cache is keyed by token + hash and the key id can't derive it. Documented and acceptable; a per-key revocation + would need a secondary index. Not a defect. + +## Tests adequacy +Adequate. Each fix has a test that exercises behaviour, not just compilation: +content-type coercion under allowlisted prefix; both `print`+`debug` rejection; XFF last- +hop incl. spoof-stability; `evict_user`/`evict_token` targeting; inline-password rejection; +and a DB-backed `runtime_error_detail_is_scrubbed_from_public_response` that throws a marker +string and asserts it never appears in the public body while a `ref:` id does. The four +non-DB suites I ran are green (5 + 1 + 9 + 3). DB-backed tests read as correct but unrun. + +## Bottom line +All 8 fixes correctly close their stated gaps with no regression or new vulnerability found. +The critical shared-Arc invariant for fix #1 is genuinely satisfied. The bootstrap-sentinel +deferral rationale is verified accurate. Only three cosmetic nits, none blocking. Approve +and ff-merge.