feat(secrets): owner-discriminated AAD (SecretOwner) for group secrets
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) <noreply@anthropic.com>
This commit is contained in:
@@ -827,8 +827,13 @@ impl ApplyService {
|
|||||||
(push it with `pic secret set {name}`)"
|
(push it with `pic secret set {name}`)"
|
||||||
))
|
))
|
||||||
})?;
|
})?;
|
||||||
let plaintext = crate::secrets_service::open(&self.master_key, app_id, name, &stored)
|
let plaintext = crate::secrets_service::open(
|
||||||
.map_err(|e| ApplyError::Invalid(format!("could not read secret `{name}`: {e}")))?;
|
&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
|
// 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).
|
// key is forgeable (preserves the spirit of audit 2026-06-11 H-B2).
|
||||||
match plaintext.as_str() {
|
match plaintext.as_str() {
|
||||||
|
|||||||
@@ -208,7 +208,7 @@ pub use secrets_repo::{
|
|||||||
SecretsRepoError, StoredSecret,
|
SecretsRepoError, StoredSecret,
|
||||||
};
|
};
|
||||||
pub use secrets_service::{
|
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,
|
DEFAULT_SECRET_MAX_VALUE_BYTES,
|
||||||
};
|
};
|
||||||
pub use topic_repo::{PostgresTopicRepo, Topic, TopicAuthMode, TopicRepo, TopicRepoError};
|
pub use topic_repo::{PostgresTopicRepo, Topic, TopicAuthMode, TopicRepo, TopicRepoError};
|
||||||
|
|||||||
@@ -123,7 +123,7 @@ async fn set_secret(
|
|||||||
// (app_id, name). Same path as the SDK secrets::set.
|
// (app_id, name). Same path as the SDK secrets::set.
|
||||||
let (ciphertext, nonce, version) = seal(
|
let (ciphertext, nonce, version) = seal(
|
||||||
&s.master_key,
|
&s.master_key,
|
||||||
app_id,
|
crate::secrets_service::SecretOwner::App(app_id),
|
||||||
&input.name,
|
&input.name,
|
||||||
&input.value,
|
&input.value,
|
||||||
s.max_value_bytes,
|
s.max_value_bytes,
|
||||||
|
|||||||
@@ -22,8 +22,8 @@ use std::sync::Arc;
|
|||||||
|
|
||||||
use async_trait::async_trait;
|
use async_trait::async_trait;
|
||||||
use picloud_shared::{
|
use picloud_shared::{
|
||||||
crypto, validate_secret_name, AppId, MasterKey, SdkCallCx, SecretsError, SecretsListPage,
|
crypto, validate_secret_name, AppId, GroupId, MasterKey, SdkCallCx, SecretsError,
|
||||||
SecretsService,
|
SecretsListPage, SecretsService,
|
||||||
};
|
};
|
||||||
|
|
||||||
use crate::authz::{self, AuthzRepo, Capability};
|
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.
|
/// `0` = legacy (no AAD); `1` = AAD-bound. New writes always emit v1.
|
||||||
pub const SECRET_ENVELOPE_V1: i16 = 1;
|
pub const SECRET_ENVELOPE_V1: i16 = 1;
|
||||||
|
|
||||||
/// Audit 2026-06-11 H-D1 — AAD bound into the GCM auth tag so a
|
/// Who owns a secret (Phase 3). A secret is owned by exactly one app OR
|
||||||
/// cross-row swap (e.g. moving one app's ciphertext under another
|
/// one group; the owner is bound into the AES-GCM AAD so a cross-owner
|
||||||
/// app's `(app_id, name)` slot) fails decryption.
|
/// ciphertext swap fails decryption.
|
||||||
fn secret_aad(app_id: AppId, name: &str) -> Vec<u8> {
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
format!("secret:{app_id}:{name}").into_bytes()
|
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<u8> {
|
||||||
|
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
|
/// 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`).
|
/// failure (should not happen for a `serde_json::Value`).
|
||||||
pub fn seal(
|
pub fn seal(
|
||||||
master_key: &MasterKey,
|
master_key: &MasterKey,
|
||||||
app_id: AppId,
|
owner: SecretOwner,
|
||||||
name: &str,
|
name: &str,
|
||||||
value: &serde_json::Value,
|
value: &serde_json::Value,
|
||||||
max_value_bytes: usize,
|
max_value_bytes: usize,
|
||||||
@@ -109,7 +126,7 @@ pub fn seal(
|
|||||||
actual: plaintext.len(),
|
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());
|
let enc = crypto::encrypt_with_aad(&plaintext, &aad, master_key.as_bytes());
|
||||||
Ok((enc.ciphertext, enc.nonce, SECRET_ENVELOPE_V1))
|
Ok((enc.ciphertext, enc.nonce, SECRET_ENVELOPE_V1))
|
||||||
}
|
}
|
||||||
@@ -125,7 +142,7 @@ pub fn seal(
|
|||||||
/// [`SecretsError::Corrupted`] when decryption or JSON decoding fails.
|
/// [`SecretsError::Corrupted`] when decryption or JSON decoding fails.
|
||||||
pub fn open(
|
pub fn open(
|
||||||
master_key: &MasterKey,
|
master_key: &MasterKey,
|
||||||
app_id: AppId,
|
owner: SecretOwner,
|
||||||
name: &str,
|
name: &str,
|
||||||
stored: &StoredSecret,
|
stored: &StoredSecret,
|
||||||
) -> Result<serde_json::Value, SecretsError> {
|
) -> Result<serde_json::Value, SecretsError> {
|
||||||
@@ -136,7 +153,7 @@ pub fn open(
|
|||||||
master_key.as_bytes(),
|
master_key.as_bytes(),
|
||||||
),
|
),
|
||||||
SECRET_ENVELOPE_V1 => {
|
SECRET_ENVELOPE_V1 => {
|
||||||
let aad = secret_aad(app_id, name);
|
let aad = secret_aad(owner, name);
|
||||||
crypto::decrypt_with_aad(
|
crypto::decrypt_with_aad(
|
||||||
&stored.encrypted_value,
|
&stored.encrypted_value,
|
||||||
&stored.nonce,
|
&stored.nonce,
|
||||||
@@ -262,7 +279,7 @@ impl SecretsService for SecretsServiceImpl {
|
|||||||
let Some(stored) = self.repo.get(cx.app_id, name).await? else {
|
let Some(stored) = self.repo.get(cx.app_id, name).await? else {
|
||||||
return Ok(None);
|
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)),
|
Ok(value) => Ok(Some(value)),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
// A decrypt failure is operationally significant — surface
|
// A decrypt failure is operationally significant — surface
|
||||||
@@ -288,7 +305,7 @@ impl SecretsService for SecretsServiceImpl {
|
|||||||
self.check_write(cx).await?;
|
self.check_write(cx).await?;
|
||||||
let (ciphertext, nonce, version) = seal(
|
let (ciphertext, nonce, version) = seal(
|
||||||
&self.master_key,
|
&self.master_key,
|
||||||
cx.app_id,
|
SecretOwner::App(cx.app_id),
|
||||||
name,
|
name,
|
||||||
&value,
|
&value,
|
||||||
self.max_value_bytes,
|
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]
|
#[tokio::test]
|
||||||
async fn aad_blocks_cross_name_ciphertext_swap() {
|
async fn aad_blocks_cross_name_ciphertext_swap() {
|
||||||
// Same app, different name → AAD mismatch.
|
// Same app, different name → AAD mismatch.
|
||||||
|
|||||||
Reference in New Issue
Block a user