fix(audit-2026-06-11/H-D1): bind AES-GCM AAD on secrets + realtime signing key
At-rest secrets were AES-256-GCM sealed with no Associated
Authentication Data, so 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. This breaks the cross-app isolation boundary
the moment DB write access is achieved.
Adds an AAD-bound envelope (v1) alongside the legacy no-AAD layout (v0),
discriminated by a per-row `version` column:
* shared::crypto — new encrypt_with_aad / decrypt_with_aad using
aes_gcm::aead::Payload { msg, aad }. Originals retained for v0 reads.
Tests: AAD round-trip, AAD-mismatch fails, empty-AAD round-trip.
* migration 0042 — adds `secrets.version SMALLINT NOT NULL DEFAULT 0`
and `app_secrets.realtime_signing_key_version SMALLINT NOT NULL
DEFAULT 0`. Existing rows stay v0; new writes are v1.
* secrets (SDK + admin API) — seal() now binds AAD =
"secret:{app_id}:{name}" and emits v1; open() dispatches on version.
StoredSecret gains a `version` field; SecretsRepo::set takes it.
Both secrets_service::set and secrets_api::set_secret go through the
v1 path. Tests prove a cross-app swap and a cross-name swap both
surface Corrupted, and that a hand-built v0 row still decrypts.
* app_secrets (realtime signing key) — get_or_create_signing_key writes
v1 with AAD = "app_secret:{app_id}:realtime_signing_key"; decode
dispatches on version. Tests cover v0 decode, v1 round-trip, and v1
decode-under-wrong-app failing.
* email-trigger inbound secret — kept on v0 (seal_legacy/open_legacy)
and explicitly deferred: email_trigger_details has no version column
and the trigger_id isn't known at seal time. The audit classes the
email-trigger AAD gap as Medium; folded into v1.2's key-versioning
pass per SECURITY_AUDIT.md.
* expected_schema.txt re-blessed by hand (no local Postgres) for the two
new columns + migration 0042.
Also folds in a let-else clippy fix in auth_api.rs (login Argon2
semaphore acquire, from the H-B1 commit) and two cargo-fmt reflows.
No re-encryption sweep — v0 rows decrypt as-is; the sweep is deferred to
v1.2's key-versioning pass (audit "Notes on remediation methodology").
Audit ref: security_audit/03_crypto_secrets.md (H-D1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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<u8> {
|
||||
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>, [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<serde_json::Value, SecretsError> {
|
||||
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<serde_json::Value, SecretsError> {
|
||||
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();
|
||||
|
||||
Reference in New Issue
Block a user