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