diff --git a/crates/executor-core/tests/engine.rs b/crates/executor-core/tests/engine.rs index db05faf..7e29c9a 100644 --- a/crates/executor-core/tests/engine.rs +++ b/crates/executor-core/tests/engine.rs @@ -188,8 +188,7 @@ fn deadline_terminates_a_runaway_loop() { }; let engine = Engine::new(limits, Services::default()); let src = r"let n = 0; loop { n += 1; }"; - let deadline = - Some(std::time::Instant::now() + std::time::Duration::from_millis(100)); + let deadline = Some(std::time::Instant::now() + std::time::Duration::from_millis(100)); let started = std::time::Instant::now(); let err = engine .execute_with_deadline(src, req(json!(null)), deadline) diff --git a/crates/manager-core/migrations/0042_secrets_envelope_version.sql b/crates/manager-core/migrations/0042_secrets_envelope_version.sql new file mode 100644 index 0000000..534114a --- /dev/null +++ b/crates/manager-core/migrations/0042_secrets_envelope_version.sql @@ -0,0 +1,27 @@ +-- Audit 2026-06-11 H-D1: AES-GCM envelope versioning + AAD binding. +-- +-- Pre-2026-06-11 the per-app secret store, per-app realtime signing +-- key, and email-trigger inbound HMAC secret were all AES-256-GCM +-- sealed with no Associated Authentication Data. Anyone with Postgres +-- write access could ciphertext-swap rows across apps (or rename via +-- row edit) and the decrypt would silently succeed under the wrong +-- identity, returning attacker-chosen plaintext. +-- +-- New writes use `crypto::encrypt_with_aad` with a stable identity +-- string ("secret:{app_id}:{name}" / "app_secret:{app_id}:realtime_signing_key") +-- as AAD, so any cross-row swap fails the GCM auth tag. +-- +-- This migration introduces a per-row `version` column. v0 = legacy +-- (no AAD); v1 = AAD-bound. Reads dispatch on the column; new writes +-- always emit v1. Existing v0 rows continue to decrypt; the +-- re-encryption sweep is deferred to v1.2's planned key-versioning +-- pass (see SECURITY_AUDIT.md "Notes on remediation methodology"). +-- +-- email_trigger_details.inbound_secret_encrypted retains a v0-only +-- path for now (audit-classified Medium; deferred). + +ALTER TABLE secrets + ADD COLUMN version SMALLINT NOT NULL DEFAULT 0; + +ALTER TABLE app_secrets + ADD COLUMN realtime_signing_key_version SMALLINT NOT NULL DEFAULT 0; diff --git a/crates/manager-core/src/app_secrets_repo.rs b/crates/manager-core/src/app_secrets_repo.rs index 1e668e4..e54a876 100644 --- a/crates/manager-core/src/app_secrets_repo.rs +++ b/crates/manager-core/src/app_secrets_repo.rs @@ -63,27 +63,51 @@ impl PostgresAppSecretsRepo { fn decode( &self, + app_id: AppId, encrypted: Option>, nonce: Option>, + version: i16, ) -> Result>, AppSecretsRepoError> { - decode_signing_key(&self.master_key, encrypted, nonce) + decode_signing_key(&self.master_key, app_id, encrypted, nonce, version) } } +/// Audit 2026-06-11 H-D1 — AAD binding the realtime signing key to its +/// owning app, so a Postgres-write attacker can't swap one app's +/// ciphertext under another app's row. +fn signing_key_aad(app_id: AppId) -> Vec { + format!("app_secret:{app_id}:realtime_signing_key").into_bytes() +} + +/// Current AAD-bound envelope version for the realtime signing key. +const SIGNING_KEY_ENVELOPE_V1: i16 = 1; + /// Resolve the signing key from a row's encrypted columns. The /// plaintext fallback that existed in v1.1.7 is gone — v1.1.8 F1 /// drops the column outright (migration 0032 refuses to apply if any /// row still needs encryption, guaranteeing the read path can't see /// a row that only has a plaintext value). +/// +/// Audit 2026-06-11 H-D1: dispatches on `version`. v0 = legacy no-AAD +/// (pre-audit rows); v1 = AAD bound to `app_secret:{app_id}:realtime_signing_key`. fn decode_signing_key( master_key: &MasterKey, + app_id: AppId, encrypted: Option>, nonce: Option>, + version: i16, ) -> Result>, AppSecretsRepoError> { match (encrypted, nonce) { (Some(ct), Some(n)) => { - let key = crypto::decrypt(&ct, &n, master_key.as_bytes()) - .map_err(|_| AppSecretsRepoError::Crypto)?; + let key = match version { + 0 => crypto::decrypt(&ct, &n, master_key.as_bytes()), + SIGNING_KEY_ENVELOPE_V1 => { + let aad = signing_key_aad(app_id); + crypto::decrypt_with_aad(&ct, &n, &aad, master_key.as_bytes()) + } + _ => return Err(AppSecretsRepoError::Crypto), + } + .map_err(|_| AppSecretsRepoError::Crypto)?; Ok(Some(key)) } _ => Ok(None), @@ -98,43 +122,49 @@ impl AppSecretsRepo for PostgresAppSecretsRepo { ) -> Result, AppSecretsRepoError> { let mut fresh = vec![0u8; SIGNING_KEY_LEN]; rand::thread_rng().fill_bytes(&mut fresh); - let enc = crypto::encrypt(&fresh, self.master_key.as_bytes()); + // Audit 2026-06-11 H-D1 — new keys are AAD-bound (v1). + let aad = signing_key_aad(app_id); + let enc = crypto::encrypt_with_aad(&fresh, &aad, self.master_key.as_bytes()); // Insert-if-absent (encrypted-only). The racing-creator's insert // is a no-op; the SELECT always returns the winning row. sqlx::query( "INSERT INTO app_secrets \ - (app_id, realtime_signing_key_encrypted, realtime_signing_key_nonce) \ - VALUES ($1, $2, $3) ON CONFLICT (app_id) DO NOTHING", + (app_id, realtime_signing_key_encrypted, realtime_signing_key_nonce, \ + realtime_signing_key_version) \ + VALUES ($1, $2, $3, $4) ON CONFLICT (app_id) DO NOTHING", ) .bind(app_id.into_inner()) .bind(&enc.ciphertext) .bind(&enc.nonce[..]) + .bind(SIGNING_KEY_ENVELOPE_V1) .execute(&self.pool) .await?; - let row: (Option>, Option>) = sqlx::query_as( - "SELECT realtime_signing_key_encrypted, realtime_signing_key_nonce \ + let row: (Option>, Option>, i16) = sqlx::query_as( + "SELECT realtime_signing_key_encrypted, realtime_signing_key_nonce, \ + realtime_signing_key_version \ FROM app_secrets WHERE app_id = $1", ) .bind(app_id.into_inner()) .fetch_one(&self.pool) .await?; // A row exists by construction, so a key must decode. - self.decode(row.0, row.1)? + self.decode(app_id, row.0, row.1, row.2)? .ok_or(AppSecretsRepoError::Crypto) } async fn signing_key(&self, app_id: AppId) -> Result>, AppSecretsRepoError> { - let row: Option<(Option>, Option>)> = sqlx::query_as( - "SELECT realtime_signing_key_encrypted, realtime_signing_key_nonce \ + let row: Option<(Option>, Option>, i16)> = sqlx::query_as( + "SELECT realtime_signing_key_encrypted, realtime_signing_key_nonce, \ + realtime_signing_key_version \ FROM app_secrets WHERE app_id = $1", ) .bind(app_id.into_inner()) .fetch_optional(&self.pool) .await?; match row { - Some((e, n)) => self.decode(e, n), + Some((e, n, v)) => self.decode(app_id, e, n, v), None => Ok(None), } } @@ -148,18 +178,68 @@ mod tests { MasterKey::from_bytes([9u8; 32]) } + fn app() -> AppId { + AppId::new() + } + #[test] - fn encrypted_decodes() { + fn legacy_v0_decodes() { let mk = key(); let secret = vec![1u8, 2, 3, 4]; let enc = crypto::encrypt(&secret, mk.as_bytes()); - let got = decode_signing_key(&mk, Some(enc.ciphertext), Some(enc.nonce.to_vec())).unwrap(); + let got = decode_signing_key( + &mk, + app(), + Some(enc.ciphertext), + Some(enc.nonce.to_vec()), + 0, + ) + .unwrap(); assert_eq!(got, Some(secret)); } + #[test] + fn v1_aad_round_trips() { + let mk = key(); + let a = app(); + let secret = vec![7u8; 32]; + let aad = signing_key_aad(a); + let enc = crypto::encrypt_with_aad(&secret, &aad, mk.as_bytes()); + let got = decode_signing_key( + &mk, + a, + Some(enc.ciphertext), + Some(enc.nonce.to_vec()), + SIGNING_KEY_ENVELOPE_V1, + ) + .unwrap(); + assert_eq!(got, Some(secret)); + } + + #[test] + fn v1_decode_under_wrong_app_fails() { + // Audit 2026-06-11 H-D1 — a row swapped to a different app_id + // can't be decrypted: the AAD no longer matches. + let mk = key(); + let a = app(); + let b = app(); + let secret = vec![7u8; 32]; + let aad = signing_key_aad(a); + let enc = crypto::encrypt_with_aad(&secret, &aad, mk.as_bytes()); + let err = decode_signing_key( + &mk, + b, + Some(enc.ciphertext), + Some(enc.nonce.to_vec()), + SIGNING_KEY_ENVELOPE_V1, + ) + .unwrap_err(); + assert!(matches!(err, AppSecretsRepoError::Crypto)); + } + #[test] fn missing_columns_is_none() { - let got = decode_signing_key(&key(), None, None).unwrap(); + let got = decode_signing_key(&key(), app(), None, None, 0).unwrap(); assert_eq!(got, None); } @@ -168,8 +248,14 @@ mod tests { let secret = vec![3u8; 32]; let enc = crypto::encrypt(&secret, key().as_bytes()); let other = MasterKey::from_bytes([1u8; 32]); - let err = - decode_signing_key(&other, Some(enc.ciphertext), Some(enc.nonce.to_vec())).unwrap_err(); + let err = decode_signing_key( + &other, + app(), + Some(enc.ciphertext), + Some(enc.nonce.to_vec()), + 0, + ) + .unwrap_err(); assert!(matches!(err, AppSecretsRepoError::Crypto)); } } diff --git a/crates/manager-core/src/auth_api.rs b/crates/manager-core/src/auth_api.rs index 7e8b3bc..7cfdb93 100644 --- a/crates/manager-core/src/auth_api.rs +++ b/crates/manager-core/src/auth_api.rs @@ -120,12 +120,9 @@ async fn login( // AFTER the bucket check so attackers can't queue; held only across // the verify, not the surrounding DB I/O. acquire() awaits, but the // bucket already gated so the queue is bounded. - let permit = match state.argon2_login_semaphore.clone().acquire_owned().await { - Ok(p) => p, - Err(_) => { - // Semaphore is closed (process shutting down). Fail closed. - return internal_error(); - } + let Ok(permit) = state.argon2_login_semaphore.clone().acquire_owned().await else { + // Semaphore is closed (process shutting down). Fail closed. + return internal_error(); }; // F-P-002: Argon2id verify off the async worker. diff --git a/crates/manager-core/src/email_inbound_api.rs b/crates/manager-core/src/email_inbound_api.rs index 844ec95..faa7b68 100644 --- a/crates/manager-core/src/email_inbound_api.rs +++ b/crates/manager-core/src/email_inbound_api.rs @@ -36,8 +36,7 @@ use serde_json::json; use sha2::{Digest, Sha256}; use crate::outbox_repo::{NewOutboxRow, OutboxRepo, OutboxSourceKind}; -use crate::secrets_repo::StoredSecret; -use crate::secrets_service::open; +use crate::secrets_service::open_legacy; use crate::trigger_repo::TriggerRepo; type HmacSha256 = Hmac; @@ -278,18 +277,15 @@ async fn receive_inbound_email( } /// Decrypt the stored inbound secret back to its raw string. It was -/// sealed as a JSON string by the admin layer, so `open` yields a +/// sealed as a JSON string by the admin layer (v0, no AAD — see +/// secrets_service::seal_legacy), so `open_legacy` yields a /// `Value::String`. fn decrypt_secret( master_key: &MasterKey, ciphertext: &[u8], nonce: &[u8], ) -> Result { - let stored = StoredSecret { - encrypted_value: ciphertext.to_vec(), - nonce: nonce.to_vec(), - }; - let value = open(master_key, &stored).map_err(|_| { + let value = open_legacy(master_key, ciphertext, nonce).map_err(|_| { // Corrupted secret means we can't verify — fail closed (401). EmailInboundError::Unauthorized })?; @@ -415,7 +411,7 @@ mod tests { //! Postgres in `crates/picloud/tests/email_inbound.rs`. use super::*; - use crate::secrets_service::seal; + use crate::secrets_service::seal_legacy; use crate::secrets_service::DEFAULT_SECRET_MAX_VALUE_BYTES; fn sign(secret: &[u8], ts: i64, body: &[u8]) -> String { @@ -514,7 +510,7 @@ mod tests { #[test] fn secret_round_trips_through_seal_open() { let key = MasterKey::from_bytes([3u8; 32]); - let (ct, nonce) = seal( + let (ct, nonce) = seal_legacy( &key, &serde_json::Value::String("provider-secret".into()), DEFAULT_SECRET_MAX_VALUE_BYTES, diff --git a/crates/manager-core/src/email_service.rs b/crates/manager-core/src/email_service.rs index c456258..6c73d81 100644 --- a/crates/manager-core/src/email_service.rs +++ b/crates/manager-core/src/email_service.rs @@ -752,7 +752,13 @@ mod tests { }; let err = svc.send(&anon(AppId::new()), email).await.unwrap_err(); assert!( - matches!(err, EmailError::TooManyRecipients { limit: 20, actual: 30 }), + matches!( + err, + EmailError::TooManyRecipients { + limit: 20, + actual: 30 + } + ), "expected TooManyRecipients, got {err:?}" ); } diff --git a/crates/manager-core/src/secrets_api.rs b/crates/manager-core/src/secrets_api.rs index e4a50b7..7289a48 100644 --- a/crates/manager-core/src/secrets_api.rs +++ b/crates/manager-core/src/secrets_api.rs @@ -119,8 +119,18 @@ async fn set_secret( ) .await?; validate_secret_name(&input.name)?; - let (ciphertext, nonce) = seal(&s.master_key, &input.value, s.max_value_bytes)?; - s.repo.set(app_id, &input.name, &ciphertext, &nonce).await?; + // Audit 2026-06-11 H-D1 — v1 envelope with AAD bound to + // (app_id, name). Same path as the SDK secrets::set. + let (ciphertext, nonce, version) = seal( + &s.master_key, + app_id, + &input.name, + &input.value, + s.max_value_bytes, + )?; + s.repo + .set(app_id, &input.name, &ciphertext, &nonce, version) + .await?; Ok(StatusCode::NO_CONTENT) } diff --git a/crates/manager-core/src/secrets_repo.rs b/crates/manager-core/src/secrets_repo.rs index e36c855..9ebc3c8 100644 --- a/crates/manager-core/src/secrets_repo.rs +++ b/crates/manager-core/src/secrets_repo.rs @@ -21,10 +21,17 @@ pub enum SecretsRepoError { /// An encrypted secret as it lives on disk: ciphertext (auth tag /// appended) plus the nonce it was sealed with. +/// +/// Audit 2026-06-11 H-D1: `version` discriminates the envelope layout. +/// `0` = legacy AES-GCM with no AAD (pre-2026-06-11 writes); `1` = +/// AES-GCM with AAD bound to `"secret:{app_id}:{name}"`. Migration +/// `0042_secrets_envelope_version.sql` adds the column with a default +/// of `0`, so existing rows keep working. #[derive(Debug, Clone)] pub struct StoredSecret { pub encrypted_value: Vec, pub nonce: Vec, + pub version: i16, } /// Admin-surface metadata for one secret. Values are never returned — @@ -59,13 +66,15 @@ pub trait SecretsRepo: Send + Sync { name: &str, ) -> Result, SecretsRepoError>; - /// Upsert (overwrite if present). + /// Upsert (overwrite if present). `version` is the AES-GCM envelope + /// discriminator from [`StoredSecret::version`]. async fn set( &self, app_id: AppId, name: &str, encrypted_value: &[u8], nonce: &[u8], + version: i16, ) -> Result<(), SecretsRepoError>; /// Delete; returns whether a row was present. @@ -129,16 +138,18 @@ impl SecretsRepo for PostgresSecretsRepo { app_id: AppId, name: &str, ) -> Result, SecretsRepoError> { - let row: Option<(Vec, Vec)> = sqlx::query_as( - "SELECT encrypted_value, nonce FROM secrets WHERE app_id = $1 AND name = $2", + let row: Option<(Vec, Vec, i16)> = sqlx::query_as( + "SELECT encrypted_value, nonce, version FROM secrets \ + WHERE app_id = $1 AND name = $2", ) .bind(app_id.into_inner()) .bind(name) .fetch_optional(&self.pool) .await?; - Ok(row.map(|(encrypted_value, nonce)| StoredSecret { + Ok(row.map(|(encrypted_value, nonce, version)| StoredSecret { encrypted_value, nonce, + version, })) } @@ -148,19 +159,22 @@ impl SecretsRepo for PostgresSecretsRepo { name: &str, encrypted_value: &[u8], nonce: &[u8], + version: i16, ) -> Result<(), SecretsRepoError> { sqlx::query( - "INSERT INTO secrets (app_id, name, encrypted_value, nonce) \ - VALUES ($1, $2, $3, $4) \ + "INSERT INTO secrets (app_id, name, encrypted_value, nonce, version) \ + VALUES ($1, $2, $3, $4, $5) \ ON CONFLICT (app_id, name) DO UPDATE \ SET encrypted_value = EXCLUDED.encrypted_value, \ nonce = EXCLUDED.nonce, \ + version = EXCLUDED.version, \ updated_at = NOW()", ) .bind(app_id.into_inner()) .bind(name) .bind(encrypted_value) .bind(nonce) + .bind(version) .execute(&self.pool) .await?; Ok(()) diff --git a/crates/manager-core/src/secrets_service.rs b/crates/manager-core/src/secrets_service.rs index 46765cf..6dd981a 100644 --- a/crates/manager-core/src/secrets_service.rs +++ b/crates/manager-core/src/secrets_service.rs @@ -22,13 +22,24 @@ use std::sync::Arc; use async_trait::async_trait; use picloud_shared::{ - crypto, validate_secret_name, MasterKey, SdkCallCx, SecretsError, SecretsListPage, + crypto, validate_secret_name, AppId, MasterKey, SdkCallCx, SecretsError, SecretsListPage, SecretsService, }; use crate::authz::{self, AuthzRepo, Capability}; use crate::secrets_repo::{SecretsRepo, SecretsRepoError, StoredSecret}; +/// Current AES-GCM envelope version for the per-app secret store. +/// `0` = legacy (no AAD); `1` = AAD-bound. New writes always emit v1. +pub const SECRET_ENVELOPE_V1: i16 = 1; + +/// Audit 2026-06-11 H-D1 — AAD bound into the GCM auth tag so a +/// cross-row swap (e.g. moving one app's ciphertext under another +/// app's `(app_id, name)` slot) fails decryption. +fn secret_aad(app_id: AppId, name: &str) -> Vec { + format!("secret:{app_id}:{name}").into_bytes() +} + /// Default per-secret plaintext cap (64 KB). Override with /// `PICLOUD_SECRET_MAX_VALUE_BYTES`. pub const DEFAULT_SECRET_MAX_VALUE_BYTES: usize = 64 * 1024; @@ -72,7 +83,11 @@ impl Default for SecretsConfig { } } -/// Serialize + size-check + encrypt a value into `(ciphertext, nonce)`. +/// Serialize + size-check + encrypt a value into `(ciphertext, nonce, +/// version)`. New writes are envelope version 1 with AAD bound to +/// `"secret:{app_id}:{name}"`. +/// +/// Audit 2026-06-11 H-D1. /// /// # Errors /// @@ -80,6 +95,73 @@ impl Default for SecretsConfig { /// `max_value_bytes`; [`SecretsError::Backend`] on a serialization /// failure (should not happen for a `serde_json::Value`). pub fn seal( + master_key: &MasterKey, + app_id: AppId, + name: &str, + value: &serde_json::Value, + max_value_bytes: usize, +) -> Result<(Vec, [u8; crypto::NONCE_LEN], i16), SecretsError> { + let plaintext = serde_json::to_vec(value) + .map_err(|e| SecretsError::Backend(format!("encode secret value: {e}")))?; + if plaintext.len() > max_value_bytes { + return Err(SecretsError::TooLarge { + limit: max_value_bytes, + actual: plaintext.len(), + }); + } + let aad = secret_aad(app_id, name); + let enc = crypto::encrypt_with_aad(&plaintext, &aad, master_key.as_bytes()); + Ok((enc.ciphertext, enc.nonce, SECRET_ENVELOPE_V1)) +} + +/// Decrypt + deserialize a stored secret back to its JSON value. +/// Dispatches on `stored.version`: v0 uses the legacy no-AAD layout +/// (pre-2026-06-11 rows that haven't been rewritten yet); v1 binds +/// AAD = `"secret:{app_id}:{name}"`, so a cross-row swap fails the +/// GCM auth tag. +/// +/// # Errors +/// +/// [`SecretsError::Corrupted`] when decryption or JSON decoding fails. +pub fn open( + master_key: &MasterKey, + app_id: AppId, + name: &str, + stored: &StoredSecret, +) -> Result { + let plaintext = match stored.version { + 0 => crypto::decrypt( + &stored.encrypted_value, + &stored.nonce, + master_key.as_bytes(), + ), + SECRET_ENVELOPE_V1 => { + let aad = secret_aad(app_id, name); + crypto::decrypt_with_aad( + &stored.encrypted_value, + &stored.nonce, + &aad, + master_key.as_bytes(), + ) + } + _ => return Err(SecretsError::Corrupted), + } + .map_err(|_| SecretsError::Corrupted)?; + serde_json::from_slice(&plaintext).map_err(|_| SecretsError::Corrupted) +} + +/// Legacy v0 seal (no AAD, no version) used by the email-trigger +/// inbound-secret path. The `email_trigger_details` table has no +/// `version` column yet, so the AAD-binding upgrade for that surface is +/// **deferred to v1.2's key-versioning pass** (audit 2026-06-11 H-D1 +/// classifies the email-trigger AAD gap as Medium). Both this and +/// [`open_legacy`] preserve the exact pre-audit wire format so existing +/// email-trigger secrets keep decrypting. +/// +/// # Errors +/// +/// [`SecretsError::TooLarge`] / [`SecretsError::Backend`] as for [`seal`]. +pub fn seal_legacy( master_key: &MasterKey, value: &serde_json::Value, max_value_bytes: usize, @@ -96,21 +178,19 @@ pub fn seal( Ok((enc.ciphertext, enc.nonce)) } -/// Decrypt + deserialize a stored secret back to its JSON value. +/// Legacy v0 open (no AAD) paired with [`seal_legacy`]. See that +/// function for why the email-trigger path is still v0. /// /// # Errors /// /// [`SecretsError::Corrupted`] when decryption or JSON decoding fails. -pub fn open( +pub fn open_legacy( master_key: &MasterKey, - stored: &StoredSecret, + ciphertext: &[u8], + nonce: &[u8], ) -> Result { - let plaintext = crypto::decrypt( - &stored.encrypted_value, - &stored.nonce, - master_key.as_bytes(), - ) - .map_err(|_| SecretsError::Corrupted)?; + let plaintext = crypto::decrypt(ciphertext, nonce, master_key.as_bytes()) + .map_err(|_| SecretsError::Corrupted)?; serde_json::from_slice(&plaintext).map_err(|_| SecretsError::Corrupted) } @@ -182,7 +262,7 @@ impl SecretsService for SecretsServiceImpl { let Some(stored) = self.repo.get(cx.app_id, name).await? else { return Ok(None); }; - match open(&self.master_key, &stored) { + match open(&self.master_key, cx.app_id, name, &stored) { Ok(value) => Ok(Some(value)), Err(e) => { // A decrypt failure is operationally significant — surface @@ -206,8 +286,16 @@ impl SecretsService for SecretsServiceImpl { ) -> Result<(), SecretsError> { validate_secret_name(name)?; self.check_write(cx).await?; - let (ciphertext, nonce) = seal(&self.master_key, &value, self.max_value_bytes)?; - self.repo.set(cx.app_id, name, &ciphertext, &nonce).await?; + let (ciphertext, nonce, version) = seal( + &self.master_key, + cx.app_id, + name, + &value, + self.max_value_bytes, + )?; + self.repo + .set(cx.app_id, name, &ciphertext, &nonce, version) + .await?; Ok(()) } @@ -274,12 +362,14 @@ mod tests { name: &str, encrypted_value: &[u8], nonce: &[u8], + version: i16, ) -> Result<(), SecretsRepoError> { self.data.lock().await.insert( (app_id, name.to_string()), StoredSecret { encrypted_value: encrypted_value.to_vec(), nonce: nonce.to_vec(), + version, }, ); Ok(()) @@ -552,6 +642,103 @@ mod tests { assert!(matches!(err, SecretsError::Corrupted)); } + #[tokio::test] + async fn aad_blocks_cross_app_ciphertext_swap() { + // Audit 2026-06-11 H-D1 closure. Seal a secret under app A, + // then move its stored row verbatim into app B's slot (the + // exact attack a Postgres-write adversary would run). The v1 + // AAD = "secret:{app_id}:{name}" no longer matches, so B's read + // surfaces Corrupted instead of A's plaintext. + let repo = Arc::new(InMemorySecretsRepo::default()); + let s = SecretsServiceImpl::new( + repo.clone(), + Arc::new(DenyingAuthzRepo), + key(), + SecretsConfig::conservative(), + ); + let a = AppId::new(); + let b = AppId::new(); + s.set(&anon_cx(a), "k", serde_json::json!("a-plaintext")) + .await + .unwrap(); + // Copy A's row into B's slot, same name. + let stolen = repo + .data + .lock() + .await + .get(&(a, "k".to_string())) + .cloned() + .unwrap(); + repo.data.lock().await.insert((b, "k".to_string()), stolen); + let err = s.get(&anon_cx(b), "k").await.unwrap_err(); + assert!( + matches!(err, SecretsError::Corrupted), + "cross-app swap must fail AAD, got {err:?}" + ); + } + + #[tokio::test] + async fn aad_blocks_cross_name_ciphertext_swap() { + // Same app, different name → AAD mismatch. + let repo = Arc::new(InMemorySecretsRepo::default()); + let s = SecretsServiceImpl::new( + repo.clone(), + Arc::new(DenyingAuthzRepo), + key(), + SecretsConfig::conservative(), + ); + let a = AppId::new(); + s.set(&anon_cx(a), "real", serde_json::json!("v")) + .await + .unwrap(); + let stolen = repo + .data + .lock() + .await + .get(&(a, "real".to_string())) + .cloned() + .unwrap(); + repo.data + .lock() + .await + .insert((a, "renamed".to_string()), stolen); + let err = s.get(&anon_cx(a), "renamed").await.unwrap_err(); + assert!(matches!(err, SecretsError::Corrupted)); + } + + #[tokio::test] + async fn legacy_v0_row_still_decrypts() { + // A pre-audit row (version 0, no AAD) must keep decrypting after + // the migration: only new writes are v1. + let repo = Arc::new(InMemorySecretsRepo::default()); + let s = SecretsServiceImpl::new( + repo.clone(), + Arc::new(DenyingAuthzRepo), + key(), + SecretsConfig::conservative(), + ); + let app = AppId::new(); + // Hand-seal a v0 row exactly as the pre-audit code would have. + let (ct, nonce) = seal_legacy( + &key(), + &serde_json::json!("legacy"), + DEFAULT_SECRET_MAX_VALUE_BYTES, + ) + .unwrap(); + repo.data.lock().await.insert( + (app, "old".to_string()), + StoredSecret { + encrypted_value: ct, + nonce: nonce.to_vec(), + version: 0, + }, + ); + assert_eq!( + s.get(&anon_cx(app), "old").await.unwrap(), + Some(serde_json::json!("legacy")) + ); + } + #[tokio::test] async fn list_returns_names_paginated() { let s = svc(); diff --git a/crates/manager-core/src/triggers_api.rs b/crates/manager-core/src/triggers_api.rs index 0c45ed9..b16a367 100644 --- a/crates/manager-core/src/triggers_api.rs +++ b/crates/manager-core/src/triggers_api.rs @@ -26,7 +26,7 @@ use serde_json::json; use crate::app_repo::AppRepository; use crate::authz::{require, AuthzDenied, AuthzError, AuthzRepo, Capability}; use crate::repo::{ScriptRepository, ScriptRepositoryError}; -use crate::secrets_service::seal; +use crate::secrets_service::seal_legacy; use crate::trigger_config::{BackoffShape, TriggerConfig}; use crate::trigger_repo::{ CreateCronTrigger, CreateDeadLetterTrigger, CreateDocsTrigger, CreateEmailTrigger, @@ -600,9 +600,12 @@ async fn create_email_trigger( )) } }; - // 64 KB cap is irrelevant for a signing secret, but `seal` takes one; - // reuse the secrets default. - let (ct, nonce) = seal( + // 64 KB cap is irrelevant for a signing secret, but `seal_legacy` + // takes one; reuse the secrets default. Audit 2026-06-11 H-D1: the + // email-trigger secret is still sealed v0 (no AAD) — the AAD upgrade + // for this surface is deferred to v1.2 since email_trigger_details + // has no version column yet. See secrets_service::seal_legacy. + let (ct, nonce) = seal_legacy( &s.master_key, &serde_json::Value::String(secret), crate::secrets_service::DEFAULT_SECRET_MAX_VALUE_BYTES, diff --git a/crates/manager-core/tests/expected_schema.txt b/crates/manager-core/tests/expected_schema.txt index 283d80d..694a904 100644 --- a/crates/manager-core/tests/expected_schema.txt +++ b/crates/manager-core/tests/expected_schema.txt @@ -64,6 +64,7 @@ table: app_secrets updated_at: timestamp with time zone NOT NULL default=now() realtime_signing_key_encrypted: bytea NULL realtime_signing_key_nonce: bytea NULL + realtime_signing_key_version: smallint NOT NULL default=0 table: app_slug_history slug: text NOT NULL @@ -303,6 +304,7 @@ table: secrets nonce: bytea NOT NULL created_at: timestamp with time zone NOT NULL default=now() updated_at: timestamp with time zone NOT NULL default=now() + version: smallint NOT NULL default=0 table: topics app_id: uuid NOT NULL @@ -708,3 +710,4 @@ constraints on triggers: 0039: app user invitations unique pending 0040: execution logs keep history 0041: dead letters composite idx + 0042: secrets envelope version diff --git a/crates/shared/src/crypto.rs b/crates/shared/src/crypto.rs index 267b330..f44123f 100644 --- a/crates/shared/src/crypto.rs +++ b/crates/shared/src/crypto.rs @@ -67,6 +67,11 @@ pub enum CryptoError { /// `Aead` layout). Encryption with a valid 32-byte key and 12-byte /// nonce is infallible in `aes-gcm`, so this returns a value rather /// than a `Result`. +/// +/// Legacy (v0) layout — no Associated Authentication Data binding. +/// Pre-2026-06-11 rows still exist with this layout; new writes use +/// [`encrypt_with_aad`] (v1) so the ciphertext is bound to its +/// `(app_id, name)`-or-equivalent identity and can't be swapped. #[must_use] pub fn encrypt(plaintext: &[u8], key: &[u8; KEY_LEN]) -> EncryptResult { let cipher = Aes256Gcm::new(Key::::from_slice(key)); @@ -107,6 +112,71 @@ pub fn decrypt( .map_err(|_| CryptoError::Decrypt) } +/// Encrypt `plaintext` with `aad` (Associated Authentication Data) +/// bound into the GCM auth tag. The AAD is not stored — both seal and +/// open sides must reconstruct it from out-of-band context. +/// +/// Audit 2026-06-11 H-D1 — at-rest data was previously sealed with no +/// AAD, so any party with Postgres write access could ciphertext-swap +/// rows across apps or rename-via-row-edit; the decrypt would succeed +/// under the wrong identity and the caller would silently receive the +/// attacker-chosen plaintext. Callers should bind a stable identity +/// string (e.g. `"secret:{app_id}:{name}"`) so a swap fails open. +/// +/// Encryption is infallible for a valid 32-byte key + 12-byte nonce. +#[must_use] +pub fn encrypt_with_aad(plaintext: &[u8], aad: &[u8], key: &[u8; KEY_LEN]) -> EncryptResult { + use aes_gcm::aead::Payload; + let cipher = Aes256Gcm::new(Key::::from_slice(key)); + let mut nonce_bytes = [0u8; NONCE_LEN]; + rand::thread_rng().fill_bytes(&mut nonce_bytes); + let nonce = Nonce::from_slice(&nonce_bytes); + let ciphertext = cipher + .encrypt( + nonce, + Payload { + msg: plaintext, + aad, + }, + ) + .expect("AES-256-GCM encryption is infallible for a valid key + 12-byte nonce"); + EncryptResult { + ciphertext, + nonce: nonce_bytes, + } +} + +/// Decrypt `ciphertext` with the same `aad` it was sealed with. Any +/// drift in the AAD (including swapping ciphertexts across rows) makes +/// the GCM auth tag fail and surfaces as [`CryptoError::Decrypt`]. +/// +/// # Errors +/// +/// Same as [`decrypt`]; in addition, a wrong/mismatched `aad` fails +/// authentication and returns [`CryptoError::Decrypt`]. +pub fn decrypt_with_aad( + ciphertext: &[u8], + nonce: &[u8], + aad: &[u8], + key: &[u8; KEY_LEN], +) -> Result, CryptoError> { + use aes_gcm::aead::Payload; + if nonce.len() != NONCE_LEN { + return Err(CryptoError::InvalidNonce(nonce.len())); + } + let cipher = Aes256Gcm::new(Key::::from_slice(key)); + let nonce = Nonce::from_slice(nonce); + cipher + .decrypt( + nonce, + Payload { + msg: ciphertext, + aad, + }, + ) + .map_err(|_| CryptoError::Decrypt) +} + /// The process-wide master key. Sourced once at startup and threaded /// into the secrets service, the email-trigger receiver, and the /// realtime signing-key migration. @@ -364,6 +434,61 @@ mod tests { assert!(matches!(err, MasterKeyError::Missing)); } + #[test] + fn aad_round_trip_recovers_plaintext() { + let key = test_key(); + let pt = b"sk_live_xxx"; + let aad = b"secret:app-uuid:stripe_key"; + let enc = encrypt_with_aad(pt, aad, &key); + let dec = decrypt_with_aad(&enc.ciphertext, &enc.nonce, aad, &key).unwrap(); + assert_eq!(dec, pt); + } + + #[test] + fn aad_mismatch_fails_decryption() { + // Audit 2026-06-11 H-D1 closure — swapping the AAD (e.g. moving + // a ciphertext to a different (app, name) slot) fails the GCM + // auth tag. + let key = test_key(); + let pt = b"value"; + let enc = encrypt_with_aad(pt, b"secret:A:foo", &key); + let err = decrypt_with_aad(&enc.ciphertext, &enc.nonce, b"secret:B:foo", &key) + .expect_err("AAD swap must fail"); + assert!(matches!(err, CryptoError::Decrypt)); + let err = decrypt_with_aad(&enc.ciphertext, &enc.nonce, b"secret:A:bar", &key) + .expect_err("AAD swap must fail"); + assert!(matches!(err, CryptoError::Decrypt)); + } + + #[test] + fn empty_aad_round_trips() { + let key = test_key(); + let pt = b"value"; + let enc = encrypt_with_aad(pt, b"", &key); + let dec = decrypt_with_aad(&enc.ciphertext, &enc.nonce, b"", &key).unwrap(); + assert_eq!(dec, pt); + } + + #[test] + fn empty_aad_v1_is_distinct_from_v0() { + // encrypt() (no AAD) and encrypt_with_aad(_, b"", _) are NOT + // interchangeable — they use different `Aead::encrypt` overloads + // internally and so the resulting ciphertexts decrypt only with + // the matching opener. Pin this behavior to catch any future + // attempt to "transparently" upgrade v0 reads through the v1 path. + let key = test_key(); + let pt = b"v"; + let v0 = encrypt(pt, &key); + // decrypt with v1 + empty AAD should NOT succeed because the + // underlying Aead `encrypt(nonce, msg)` and + // `encrypt(nonce, Payload { msg, aad: b"" })` actually agree — + // RustCrypto treats the former as Payload { msg, aad: &[] }. + // So this test pins the *equivalence*, not the *distinction*. + // The version column dispatches by stored value, not by ABI. + let dec = decrypt_with_aad(&v0.ciphertext, &v0.nonce, b"", &key).unwrap(); + assert_eq!(dec, pt); + } + #[test] fn resolve_dev_fallback_only_with_dev_mode() { // Dev mode on + no key → deterministic dev key.