AkurAI-Build

public
Latest change 2a1193541cf008c406be9f347e44150a58a66130 - docs: preserve the architecture review, backlog, and schema policy by Olafur Bui
# Architect Review 6/10 — Security Architecture (bunfork)

Read-only review of `src/*.rs`, README "Security boundaries", `docs/adversarial/README.md`, and `10-synthesis.md`.

## Overall assessment

Strong posture. The P0s from the 10 red-team passes are demonstrably implemented: decoded-byte secret comparison (`db.rs:411` `secrets_equal` via `ct_eq`), constant-time bearer auth (`server.rs:1093`), digest-owned deploy replacement (`main.rs` `validate_owned_deployment`), descriptor-pinned backup/restore with dev/ino re-checks (`db.rs` `ensure_path_matches_file`), fail-closed artifact admission (symlinks, hard links, hidden/sensitive names, traversal, `%2e%2e` covered by tests), zeroized key material through SQLCipher keying (`db.rs` `apply_key`), and 4xx classification via `VectorInputError`. Static preloading re-hashes every file at load with O_NOFOLLOW + nlink==1 + dev/ino identity checks (`server.rs:328-378`), closing the verify→serve TOCTOU. Same-UID races are explicitly out of scope in the README — acknowledged and consistent with the code.

## Prioritized findings

1. **P1 — Zeroize gap in `load_secret` (db.rs:423-425).** The file branch reads the secret into a plain `String`, then returns `value.trim().to_owned()`. The untrimmed original heap buffer is dropped without zeroization, leaving key/token bytes in freed memory. Read into a `Zeroizing<String>` (or zeroize `value` explicitly) before trimming. This is the only hole in otherwise complete zeroize coverage (keygen, decode, SQLCipher keying are all `Zeroizing`).

2. **P1 — Origin fallback trusts the attacker-supplied Host header (server.rs:1113-1140).** When `BUNFORK_PUBLIC_ORIGIN` is unset, `validate_same_origin` accepts any `Origin` that string-matches `http(s)://{Host}` — both headers are client-controlled, so the check is self-referential and adds no defense on non-loopback binds. The bearer token remains the real gate (browsers can't attach `Authorization` cross-site without CORS), so this is not exploitable alone, but the fallback conveys false assurance. Require `--public-origin` for non-loopback binds, or drop the fallback and document Origin checks as origin-configured-only.

3. **P2 — `deploy`/`test` leak Bunfork secrets into cargo build scripts (main.rs:606, 686, 688, 995).** `build_frontend` deliberately refuses `BUNFORK_DB_KEY`/`BUNFORK_API_TOKEN` (main.rs:418) and env-clears, but `run_process` for `cargo test`/`cargo build` inherits the full environment — dependency `build.rs` code runs with live secrets in env. Scrub both variables (and `BUNFORK_TEST_KEY`-style values) in `run_process` for consistency with the documented build policy.

4. **P2 — Env-var secret fallback persists for the process lifetime (db.rs:427).** After `env::var`, the secret remains in the process environment and is inherited by any spawned child (compounding finding 3 when `deploy` is run in the same shell/session). Consider `std::env::remove_var` after loading, and document that env transport is inferior to key files (which already take precedence).

5. **P2 — Public plaintext binds warn instead of failing (server.rs:200-211, 269-274).** Non-loopback HTTP without an https public origin only logs `warn!`. Given the default `0.0.0.0:3100` and README's "fail-closed" ethos, require an explicit opt-in flag (e.g. `--allow-plaintext-public`) so accidental LAN/WAN exposure of bearer-token traffic is a deliberate choice.

6. **P2 — Static CSP `'unsafe-inline'` script-src could be closed with existing hashes (server.rs:633-660).** The documented rationale (SvelteKit/Next bootstrap inlines) is fair, but `admit` already parses and hashes every file; extracting inline `<script>` hashes at admission and emitting `script-src 'self' 'sha256-…'` in the manifest would remove the last XSS amplifier without a reverse proxy. Optional flag, backwards-compatible.

7. **P2 — Minor header hardening (server.rs:1157, 633).** Neither headers middleware sets `Cross-Origin-Opener-Policy: same-origin` or `Cross-Origin-Resource-Policy: same-origin`. Cheap additions consistent with the existing `frame-ancestors 'none'` stance.

## Notes (no action required)

- `authorize`'s length short-circuit before `ct_eq` is fine: token length is a fixed public constant (64 hex).
- Bearer comparison is over hex text (case-sensitive), stricter than `secrets_equal`'s decoded comparison — acceptable asymmetry.
- The `/.well-known/` namespace is blocked by `reject_hidden_request_paths` (server.rs:1176) — an operational (ACME) limitation, not a security bug.
- Redirect `Location` construction fails closed on CR/LF via `HeaderValue::from_str` (server.rs:439-449) — no header injection.
- Search concurrency gate (semaphore=1 → 429) and the 10M multiply-add budget are sound DoS bounds.