PiCloud/security_audit/03_crypto_secrets.md

# Cryptography & Secret Handling

Audit date: 2026-06-11
Scope: `PICLOUD_SECRET_KEY` master-key sourcing, dev-mode bypass, AES-256-GCM envelope (`shared/src/crypto.rs`), per-app `secrets::*` store, encrypted email-trigger HMAC secret, `app_secrets` realtime signing key, API key + session token format and storage, HMAC verification paths, RNG selection, on-disk CLI credentials.

Methodology: read `crates/shared/src/{crypto,secrets,subscriber_token}.rs`, `crates/manager-core/src/{secrets_service,secrets_repo,app_secrets_repo,secrets_api,email_inbound_api,auth,auth_middleware,auth_api,auth_bootstrap,api_key_repo,admin_session_repo,users_service}.rs`, `crates/executor-core/src/sdk/{secrets.rs,stdlib/random.rs}`, `crates/picloud/src/main.rs`, `crates/picloud-cli/src/config.rs`, `crates/manager-core/migrations/{0022_app_secrets.sql,0023_secrets.sql}`. Cross-checked `AUDIT.md` to avoid duplicating fixed findings (`F-S-009` PICLOUD_DEV_INSECURE_KEY ack landed; verified the gate in `crypto.rs:207-212`).

## High

### `secrets` ciphertext is not bound to `(app_id, name)` — cross-tenant ciphertext swap by anyone with DB-write access
- **Where**: `crates/shared/src/crypto.rs:71-85` (encrypt API has no AAD parameter), `crates/manager-core/src/secrets_service.rs:82-97` (`seal` calls `crypto::encrypt(&plaintext, master_key.as_bytes())` — no AAD), `crates/manager-core/src/secrets_repo.rs:145-167` (set), `crates/manager-core/migrations/0023_secrets.sql:14-22` (no AAD/MAC column).
- **What**: AES-256-GCM accepts associated data for free, but PiCloud's envelope wraps only the plaintext. The `secrets` table stores `(app_id, name, encrypted_value, nonce)` and nothing in the ciphertext authenticates the tuple. Every secret in the database is encrypted under the *same* process-wide master key. Therefore: anyone who can issue a `UPDATE secrets SET encrypted_value=$1, nonce=$2 WHERE app_id=$3 AND name=$4` (or a row-level `INSERT ... ON CONFLICT DO UPDATE`) with payload lifted from another row can substitute *app B's* `stripe_key` ciphertext into *app A's* `stripe_key` row — and the very next `secrets::get("stripe_key")` in an app A script returns app B's plaintext sk_live token. The same applies row-to-row within an app (rename `db_password` → `db_admin_password` and decryption still succeeds). The blueprint's claim that the secrets store survives "row-level corruption" should also cover "row-level malicious overwrite," and it does not.
- **Who can do this**: anyone with Postgres write access — a leaked `picloud` DB user; a backup attacker who restores a doctored dump; a sidecar that reads `DATABASE_URL` from env and reuses it; a future SQL-injection bug; a logical-replica that becomes writable on failover.
- **Impact**: cross-tenant secret disclosure (High) — same severity as the corresponding code-path bug would carry, since the blueprint promises strict app isolation and the script-side `cx.app_id` discipline is the *only* claimed control. AAD would make the attack a guaranteed `AeadError` instead of a successful decrypt.
- **Recommendation**: add `encrypt_with_aad` / `decrypt_with_aad` thin wrappers in `crates/shared/src/crypto.rs` using `Aes256Gcm::encrypt`'s existing `Payload { msg, aad }` form, and bind `aad = format!("secret:{app_id}:{name}").into_bytes()` from `seal`/`open` callsites. Same for `app_secrets` (`bind aad = "realtime_signing_key:{app_id}"`) and the email-trigger inbound secret (`bind aad = "email_trigger:{trigger_id}"`). Existing rows must migrate: either run a v1.1.10+ startup sweep that decrypts (no AAD), re-seals (with AAD), updates the row, or version-tag rows with a `key_version: u8` byte prepended so old + new can coexist during rollout. The blueprint's §12.x next-deliverable list already has a v1.2 "key versioning + re-encryption pass" — fold AAD migration into the same pass.

### Master-key sourcing accepts both standard and URL-safe base64, but the dev-key fallback is a SHA-256 of a public string — making "dev mode" trivially decryptable post-hoc
- **Where**: `crates/shared/src/crypto.rs:178-187` (`from_base64` uses `STANDARD` engine only), `crates/shared/src/crypto.rs:246-251` (`dev_key` = `Sha256::digest(b"picloud-dev-master-key-v1.1.7")`).
- **What**: The dev key is `SHA-256("picloud-dev-master-key-v1.1.7")`. Anyone reading the source can produce the exact 32-byte key. That is *correct* for a dev hatch, but if any operator misreads the comment ("Stable across restarts so dev secrets survive a reboot, but obviously not a real secret — the input is public") and runs a dev-mode instance against a real Stripe key, the attacker recovers every secret in any leaked DB dump from any dev install instantly. The startup gate (`MasterKeyError::DevModeUnacknowledged`) helps, but the warning is one `tracing::warn!` line — easy to miss in `RUST_LOG=info` operator setups, and not surfaced anywhere the dashboard would show.
- **Impact**: Dev-mode dump exfil → full secret recovery (a Critical *if* it ever ships to prod; gating it as a High because the `DEV_INSECURE_KEY` ack is the first real defense and post-`F-S-009` it must be explicitly set).
- **Recommendation**: (1) raise the warning to a sustained `tracing::error!`-level beacon re-emitted every ~5 minutes while dev-mode is active, so it lands in any observability backend. (2) Bake a startup health-check that fails liveness *if* `dev_mode && active_secret_count > 0 && process_uptime > 1h` — i.e., dev keys aren't allowed to accumulate prod-shaped data over time. (3) Surface "running with dev master key" as a banner in the dashboard `/version` payload so an operator visiting the UI sees it.

## Medium

### `MasterKey` and decrypted secret plaintext never zeroize on drop
- **Where**: `crates/shared/src/crypto.rs:117-119` (`MasterKey { key: [u8; KEY_LEN] }` — no `Zeroize`/`ZeroizeOnDrop`), `crates/shared/src/crypto.rs:95-108` (`decrypt` returns `Vec<u8>`), `crates/manager-core/src/secrets_service.rs:104-115` (`open` returns `serde_json::Value`), `crates/manager-core/src/email_inbound_api.rs:197-214` (`decrypt_secret` returns `String`).
- **What**: The 32-byte master key is held in `Arc<MasterKey>` shared across every service; copies of decrypted plaintext (stripe keys, OAuth secrets, HMAC secrets) live in heap `Vec<u8>`/`String`/`serde_json::Value` and rely on glibc allocator-zero to scrub them. A crash dump, a `core` file, an `/proc/$pid/mem` read, or a Spectre-style speculation leak from a coresident workload all see plaintext. The `secrecy` and `zeroize` crates exist precisely for this.
- **Impact**: post-mortem secret recovery from memory artifacts; widens the blast radius of any RCE that lands enough to dump memory.
- **Recommendation**: derive `Zeroize` on `MasterKey` (newtype `[u8; KEY_LEN]` already trivial); switch `crypto::decrypt`'s return to a `Zeroizing<Vec<u8>>` and propagate. For the email-trigger HMAC secret, wrap in `secrecy::SecretString` between decrypt and HMAC use. Whatever the SDK returns to the Rhai engine is necessarily long-lived (the Rhai Dynamic owns it), so the zeroize boundary should at least cover the intermediate `Vec<u8>` and the master key itself.

### Email-trigger inbound HMAC secret re-uses `crypto::encrypt` with no AAD (same bug class as above) and decrypts to an attacker-controlled string that becomes the verifier key
- **Where**: `crates/manager-core/src/triggers_api.rs:585-610` (admin seals incoming `input.inbound_secret`), `crates/manager-core/src/email_inbound_api.rs:148-156, 197-214` (decrypts then uses `secret.as_bytes()` as HMAC key).
- **What**: Same "no AAD binding to `trigger_id`" pattern as the per-app `secrets` finding above, except the consequence is sharper: the decrypted bytes become the *HMAC verifier key* on a public, unauthenticated webhook URL. If an attacker with DB-write access swaps trigger A's `inbound_secret_encrypted` for trigger B's, every signed payload addressed to trigger A is verified against trigger B's secret — and the attacker has just relocated their inbound webhook to a different script with different effects. The replay-dedup nonce (`F-S-010` follow-up at `email_inbound_api.rs:64-89`) does not help; the signature itself passes.
- **Impact**: stronger variant of the secrets-table ciphertext-swap — it converts row-write access into ability to misdirect signed webhooks to scripts the attacker chose.
- **Recommendation**: bind `aad = format!("email_trigger:{trigger_id}").into_bytes()` at the `seal`/`open` callsites in `triggers_api.rs:589-610` and `email_inbound_api.rs:148-156`. Migration story is the same as the `secrets` table — single startup sweep can re-encrypt with AAD.

### `app_secrets` realtime signing key is GCM-encrypted with no AAD — swap one app's key in, sign tokens that the SSE verifier accepts for the wrong app
- **Where**: `crates/manager-core/src/app_secrets_repo.rs:99-101` (encrypt with no AAD), `crates/manager-core/src/app_secrets_repo.rs:78-91` (decrypt with no AAD), `crates/manager-core/migrations/0022_app_secrets.sql`.
- **What**: Each app owns one HMAC signing key for realtime subscriber tokens (`crates/shared/src/subscriber_token.rs`). Both halves of the envelope (`realtime_signing_key_encrypted`, `realtime_signing_key_nonce`) live in `app_secrets`; nothing binds the row to `app_id`. A DB-write attacker who copies app B's `realtime_signing_key_*` columns into app A's row lets app A's SSE verifier accept tokens signed with app B's key — which by extension lets the attacker mint a token "for app A" using app B's key (which they own or can extract). Not a confidentiality break in the same way as the secrets-table case (the signing key is generated server-side, never visible to the attacker), but a clear authenticity break: the attacker can replay/forward tokens across apps.
- **Impact**: cross-app realtime channel access if the attacker also controls (or has read access to) the source app's signing-key plaintext. Lower than the secrets finding because the attacker needs both DB-write AND knowledge of one app's plaintext signing key — but the attack lives in the same `crypto::encrypt`-with-no-AAD pattern.
- **Recommendation**: bind `aad = format!("realtime_signing_key:{app_id}").into_bytes()`. Same migration sweep.

### `auth_middleware::resolve_principal` `principal_cache` keyed only on the SHA-256(token) — a stolen cache entry pre-image-strips the token's identity
- **Where**: `crates/manager-core/src/auth_middleware.rs:202-216`.
- **What**: The post-`F-P-009` cache is keyed on `hash_token(token)` (SHA-256 hex), but the cached `Principal` itself carries `user_id`, `instance_role`, `scopes`, `app_binding`. A read from the cache returns those directly without re-validating the session row, until cache TTL expires. If a session is *server-side revoked* (admin deactivates the user, password reset wipes sessions, key is revoked) but the cache still holds the entry, the next request that lands in the same process within the TTL window authenticates as the revoked principal. Tied with the F-S-001 dashboard-self-password-change gap from the prior audit (sessions not invalidated on rotation), the cache window becomes the "you successfully revoked nothing for 300 seconds" trap. Confirmed by reading `verify_session`+`verify_api_key`: neither rechecks the DB after a cache hit.
- **Impact**: revocation lag bounded by cache TTL. Severity depends on TTL — the audit context says 60-300 s in the recommendation. Acceptable for normal session expiry but harmful for active-revocation cases.
- **Recommendation**: (1) keep the cache; (2) on every "credentials changed" event (`logout`, `delete_session`, `expire_all_for_user`, `set_active(false)`, password change) emit an in-process "evict by `user_id`" hint that walks the cache and removes any entry with `Principal.user_id == revoked`. (3) Document the upper-bound revocation lag in `auth_middleware.rs`'s module doc so the next reader can reason about it.

### Decrypted secret materializes as a `String` in `email_inbound_api::decrypt_secret`; nothing zeroizes it before the HMAC instantiation borrows it
- **Where**: `crates/manager-core/src/email_inbound_api.rs:197-214`.
- **What**: Compounds the broader zeroize finding above. Worth a separate line because the HMAC secret is the strongest known authenticity primitive in the system; its memory residue is the most attractive heap target.
- **Recommendation**: subsumed by the broader Medium above; flag here for callsite-specific Audit traceability.

## Low

### `crypto::CryptoError::Decrypt` and `CryptoError::InvalidNonce` are distinguishable, leaking a 1-bit "row corruption vs. tag mismatch" signal
- **Where**: `crates/shared/src/crypto.rs:50-62, 95-108`.
- **What**: A `CryptoError::InvalidNonce(n)` response tells the caller "the nonce column in this row has length n, not 12" before the AEAD even runs. The two error variants are folded together at the secrets-service layer (`secrets_service.rs:113` maps both to `SecretsError::Corrupted`), so the leak is internal-only today — but the `pub` enum on the `shared` crate guarantees future callers may keep them distinct. AEAD design lore is "fail with a single opaque error"; the documentation even references this. The check is also redundant: `Nonce::from_slice` will panic on the wrong length, and the existing service layer already collapses both. Inline the length check into the AEAD path and drop `InvalidNonce`.
- **Recommendation**: collapse into a single `CryptoError::Decrypt`. The data-shape check (length != 12) can return the same variant without revealing which precondition failed — no realistic caller benefits from the distinction.

### `seal` and `open` accept plaintext lengths up to `usize::MAX`; only `SecretsService::set` enforces the 64 KB cap before sealing
- **Where**: `crates/manager-core/src/secrets_service.rs:82-97`.
- **What**: `seal` checks `plaintext.len() > max_value_bytes` *after* serializing — a malicious admin API path that wires a different `max_value_bytes` (or skips it) could end up feeding a 100 MB JSON-encoded value into `Aes256Gcm::encrypt` and allocate the ciphertext. The current `secrets_api` callsite at `secrets_api.rs:122` passes the configured cap, but the function is `pub`; future re-users may forget. Defense in depth: cap `seal` at a hard ceiling (e.g. 16 MiB) regardless of the configured limit.
- **Recommendation**: add `const HARD_PLAINTEXT_CEILING_BYTES: usize = 16 * 1024 * 1024;` and return `SecretsError::TooLarge` when `plaintext.len() > min(max_value_bytes, HARD_PLAINTEXT_CEILING_BYTES)`. Cheap belt for any future caller.

### Email-inbound nonce dedup is process-local; clustered deployment can be replay-bypassed
- **Where**: `crates/manager-core/src/email_inbound_api.rs:63-89`.
- **What**: The comment in the module already calls this out (`Per-process is fine for single-node MVP and dev. … cluster mode (v1.3+) will need a shared store`). Flagging as **Info-with-a-pin** because moving to cluster mode without fixing this regresses to "5-minute replay window" — that is, the F-S-010 fix becomes silently no-op for inbound traffic that round-robins across nodes.
- **Recommendation**: ship a `Trait NonceDedup` with a Postgres impl backed by a 5-min-TTL table (or Redis when that lands in v1.3+) before any cluster-mode flag flips on.

### `auth.rs::hex` reimplements lowercase hex by hand
- **Where**: `crates/manager-core/src/auth.rs:97-105`.
- **What**: Custom hex routine; correct but unmemorable. `hex::encode` is already a transitive dep (used in `email_inbound_api.rs`). Replacing closes one less footgun-shaped surface (`HEX` array bounds-by-construction is fine, but it's not what should land in `cargo audit` output).
- **Recommendation**: replace with `hex::encode(&digest)`.

## Info

### Nonce generator is `rand::thread_rng()` (OS-seeded CSPRNG via `ChaCha12`) — fine for 2^32 encryptions per key
- **Where**: `crates/shared/src/crypto.rs:73-83`, `crates/manager-core/src/app_secrets_repo.rs:99-101`.
- **What**: `thread_rng()` is documented as a CSPRNG seeded from `OsRng`. For AES-GCM with a 96-bit random nonce, the safe encryption budget is ~2^32 messages per key before nonce-collision probability becomes non-negligible — well above any realistic per-app secret-write rate. No concern at the current scale; revisit if the same master key is ever used for high-volume per-row encryption (logs, pubsub bodies, etc.).
- **Recommendation**: none for now; note in the master-key rotation design doc (v1.2+) that the per-key encryption budget is sub-2^32 messages by policy.

### `OsRng` is used for every other security-critical RNG (session tokens, API keys, `random::*` SDK, app signing key) — no `SmallRng`/`thread_rng` misuse for tokens
- **Where**: `crates/manager-core/src/auth.rs:82-86, 143-147` (`OsRng`), `crates/executor-core/src/sdk/stdlib/random.rs:7-9, 51` (`OsRng`).
- **What**: Verified across the workspace: every security-sensitive RNG path resolves to `OsRng` (or `Uuid::new_v4`, which is also `getrandom`-backed). The lone `thread_rng()` callers are GCM nonces and the realtime signing-key generator — both crypto-strength wrappers around the OS RNG. Acceptable.

### Constant-time HMAC verification is correct everywhere it matters
- **Where**: `crates/shared/src/subscriber_token.rs:97-100` (`mac.verify_slice`), `crates/manager-core/src/email_inbound_api.rs:249-255` (`mac.verify_slice`).
- **What**: Both signed-token verifiers use the `hmac` crate's constant-time `verify_slice`. Session token lookup uses SHA-256(token) hashed before any DB comparison — and the DB does the equality on the hash, so timing-side-channel risk on the wire token is bounded by the cost of SHA-256 (negligible vs. the network jitter floor). Argon2 verify is also constant-time by `argon2` crate construction. No `==`/`eq`/`.starts_with` on a remotely-supplied secret found anywhere in the workspace.

### CLI on-disk credentials are mode-0600 enforced, with re-set on each write
- **Where**: `crates/picloud-cli/src/config.rs:91-110`.
- **What**: `write_private` opens with `mode(0o600)` and re-applies permissions after write — the belt-and-suspenders comment is accurate. Tested by `posix_mode_is_0600`. No accidental loosening on overwrite.

### Bootstrap secrets are not logged
- **Where**: `crates/manager-core/src/auth_bootstrap.rs:91-130` and `crates/picloud/src/main.rs:42-53`.
- **What**: `bootstrap_first_admin_with` logs only `username` on success; both `password` and `password_hash` env-var values are never echoed. `tracing::warn!` paths use the env-var *names* but never values. No `PICLOUD_*` value leaks to logs.

### Key rotation is explicitly out of scope and documented
- **Where**: `crates/shared/src/crypto.rs:19-22`.
- **What**: Module-level doc states "Key rotation is out of scope for v1.1.7. Changing PICLOUD_SECRET_KEY between deploys orphans every existing ciphertext (it can no longer be decrypted). v1.2+ adds key-version columns + a re-encryption pass." Aligned with the AAD-migration recommendation above — both should land together in v1.2.

### TLS termination: Caddy fronts everything; upstream is plaintext on the same loopback by design
- **Where**: design contract from `CLAUDE.md`.
- **What**: No cross-host plaintext upstream observed. Single-node MVP runs everything on loopback; v1.3+ cluster mode is out of scope for this audit. Note the v1.3+ split must mutual-TLS the manager↔orchestrator and orchestrator↔executor hops, or move the cluster onto a private network with a network-layer encryption guarantee.

## Counts by severity
- Critical: 0
- High: 2
- Medium: 5
- Low: 3
- Info: 6