From 6bd0f4699de15fa01468134c61c5caef35b5fe14 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Wed, 24 Jun 2026 21:17:18 +0200 Subject: [PATCH] feat(secrets): owner-discriminated AAD (SecretOwner) for group secrets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The crypto foundation for group-owned secrets (the §5.3 'single hardest correctness detail'), done first and proven before the storage/resolution layer. - SecretOwner{App(AppId)|Group(GroupId)}; secret_aad/seal/open take the owner. The app AAD is byte-identical to the pre-Phase-3 'secret:{app_id}:{name}', so every existing v1 row decrypts unchanged; group secrets use a disjoint 'secret:group:{group_id}:{name}' namespace (the 'group:' infix separates app/group AAD even for equal UUIDs). - All callers (SDK get/set, secrets_api, apply email-secret read) wrapped in SecretOwner::App — behavior identical for app secrets. - New audit test aad_distinguishes_app_and_group_owner proves the two namespaces don't collide (both directions, even reusing the UUID); the existing cross-app/cross-name swap audit tests stay green. 16 secrets_service tests pass; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 9 ++- crates/manager-core/src/lib.rs | 2 +- crates/manager-core/src/secrets_api.rs | 2 +- crates/manager-core/src/secrets_service.rs | 92 +++++++++++++++++++--- 4 files changed, 88 insertions(+), 17 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 38adcf0..8ae27f6 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -827,8 +827,13 @@ impl ApplyService { (push it with `pic secret set {name}`)" )) })?; - let plaintext = crate::secrets_service::open(&self.master_key, app_id, name, &stored) - .map_err(|e| ApplyError::Invalid(format!("could not read secret `{name}`: {e}")))?; + let plaintext = crate::secrets_service::open( + &self.master_key, + crate::secrets_service::SecretOwner::App(app_id), + name, + &stored, + ) + .map_err(|e| ApplyError::Invalid(format!("could not read secret `{name}`: {e}")))?; // The inbound HMAC must be a non-empty string — an empty/whitespace // key is forgeable (preserves the spirit of audit 2026-06-11 H-B2). match plaintext.as_str() { diff --git a/crates/manager-core/src/lib.rs b/crates/manager-core/src/lib.rs index 7fb9694..d0370c0 100644 --- a/crates/manager-core/src/lib.rs +++ b/crates/manager-core/src/lib.rs @@ -208,7 +208,7 @@ pub use secrets_repo::{ SecretsRepoError, StoredSecret, }; pub use secrets_service::{ - open as open_secret, seal as seal_secret, SecretsConfig, SecretsServiceImpl, + open as open_secret, seal as seal_secret, SecretOwner, SecretsConfig, SecretsServiceImpl, DEFAULT_SECRET_MAX_VALUE_BYTES, }; pub use topic_repo::{PostgresTopicRepo, Topic, TopicAuthMode, TopicRepo, TopicRepoError}; diff --git a/crates/manager-core/src/secrets_api.rs b/crates/manager-core/src/secrets_api.rs index 7289a48..df910f7 100644 --- a/crates/manager-core/src/secrets_api.rs +++ b/crates/manager-core/src/secrets_api.rs @@ -123,7 +123,7 @@ async fn set_secret( // (app_id, name). Same path as the SDK secrets::set. let (ciphertext, nonce, version) = seal( &s.master_key, - app_id, + crate::secrets_service::SecretOwner::App(app_id), &input.name, &input.value, s.max_value_bytes, diff --git a/crates/manager-core/src/secrets_service.rs b/crates/manager-core/src/secrets_service.rs index 6dd981a..17ed3a7 100644 --- a/crates/manager-core/src/secrets_service.rs +++ b/crates/manager-core/src/secrets_service.rs @@ -22,8 +22,8 @@ use std::sync::Arc; use async_trait::async_trait; use picloud_shared::{ - crypto, validate_secret_name, AppId, MasterKey, SdkCallCx, SecretsError, SecretsListPage, - SecretsService, + crypto, validate_secret_name, AppId, GroupId, MasterKey, SdkCallCx, SecretsError, + SecretsListPage, SecretsService, }; use crate::authz::{self, AuthzRepo, Capability}; @@ -33,11 +33,28 @@ use crate::secrets_repo::{SecretsRepo, SecretsRepoError, StoredSecret}; /// `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() +/// Who owns a secret (Phase 3). A secret is owned by exactly one app OR +/// one group; the owner is bound into the AES-GCM AAD so a cross-owner +/// ciphertext swap fails decryption. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SecretOwner { + App(AppId), + Group(GroupId), +} + +/// Audit 2026-06-11 H-D1 — AAD bound into the GCM auth tag so a cross-row +/// swap (moving one owner's ciphertext under another's slot) fails +/// decryption. The **app** form is byte-identical to the pre-Phase-3 +/// `secret:{app_id}:{name}`, so every existing v1 row keeps decrypting +/// unchanged; group secrets use a distinct `secret:group:{group_id}:{name}` +/// namespace (the `group:` infix keeps app and group AAD disjoint even if a +/// group UUID happened to equal an app UUID). +fn secret_aad(owner: SecretOwner, name: &str) -> Vec { + match owner { + SecretOwner::App(app_id) => format!("secret:{app_id}:{name}"), + SecretOwner::Group(group_id) => format!("secret:group:{group_id}:{name}"), + } + .into_bytes() } /// Default per-secret plaintext cap (64 KB). Override with @@ -96,7 +113,7 @@ impl Default for SecretsConfig { /// failure (should not happen for a `serde_json::Value`). pub fn seal( master_key: &MasterKey, - app_id: AppId, + owner: SecretOwner, name: &str, value: &serde_json::Value, max_value_bytes: usize, @@ -109,7 +126,7 @@ pub fn seal( actual: plaintext.len(), }); } - let aad = secret_aad(app_id, name); + let aad = secret_aad(owner, name); let enc = crypto::encrypt_with_aad(&plaintext, &aad, master_key.as_bytes()); Ok((enc.ciphertext, enc.nonce, SECRET_ENVELOPE_V1)) } @@ -125,7 +142,7 @@ pub fn seal( /// [`SecretsError::Corrupted`] when decryption or JSON decoding fails. pub fn open( master_key: &MasterKey, - app_id: AppId, + owner: SecretOwner, name: &str, stored: &StoredSecret, ) -> Result { @@ -136,7 +153,7 @@ pub fn open( master_key.as_bytes(), ), SECRET_ENVELOPE_V1 => { - let aad = secret_aad(app_id, name); + let aad = secret_aad(owner, name); crypto::decrypt_with_aad( &stored.encrypted_value, &stored.nonce, @@ -262,7 +279,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, cx.app_id, name, &stored) { + match open(&self.master_key, SecretOwner::App(cx.app_id), name, &stored) { Ok(value) => Ok(Some(value)), Err(e) => { // A decrypt failure is operationally significant — surface @@ -288,7 +305,7 @@ impl SecretsService for SecretsServiceImpl { self.check_write(cx).await?; let (ciphertext, nonce, version) = seal( &self.master_key, - cx.app_id, + SecretOwner::App(cx.app_id), name, &value, self.max_value_bytes, @@ -677,6 +694,55 @@ mod tests { ); } + #[tokio::test] + async fn aad_distinguishes_app_and_group_owner() { + // Phase 3: app and group secrets live in disjoint AAD namespaces + // (`secret:{app}` vs `secret:group:{group}`). A ciphertext sealed + // for a group must not open as an app secret (and vice-versa), + // even if the raw UUIDs were equal — the `group:` infix separates + // them. Exercise the free seal/open functions directly. + let k = key(); + let app = AppId::new(); + let group = GroupId::new(); + let value = serde_json::json!("shared-config"); + + // Seal under the GROUP owner. + let (ct, nonce, version) = + seal(&k, SecretOwner::Group(group), "db_url", &value, 4096).unwrap(); + let stored = StoredSecret { + encrypted_value: ct, + nonce: nonce.to_vec(), + version, + }; + // Opens fine as the same group owner. + assert_eq!( + open(&k, SecretOwner::Group(group), "db_url", &stored).unwrap(), + value + ); + // Fails as an app owner (AAD mismatch) — even reusing the UUID. + let app_from_group = AppId::from(group.into_inner()); + assert!(matches!( + open(&k, SecretOwner::App(app_from_group), "db_url", &stored), + Err(SecretsError::Corrupted) + )); + // And the symmetric direction: an app-sealed row won't open as a group. + let (ct2, nonce2, v2) = seal(&k, SecretOwner::App(app), "db_url", &value, 4096).unwrap(); + let stored2 = StoredSecret { + encrypted_value: ct2, + nonce: nonce2.to_vec(), + version: v2, + }; + assert!(matches!( + open( + &k, + SecretOwner::Group(GroupId::from(app.into_inner())), + "db_url", + &stored2 + ), + Err(SecretsError::Corrupted) + )); + } + #[tokio::test] async fn aad_blocks_cross_name_ciphertext_swap() { // Same app, different name → AAD mismatch.