From c50bc1bff9de6f71f9880bb001ec78a6cc1ba609 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:13:28 -0500 Subject: [PATCH 1/2] fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest DecryptedEntry.secret and the config resolver's intermediate string were bare Vec/String with no zeroization on drop, so decrypted plaintext credentials lingered in freed heap memory. Wrap at the point of allocation (Zeroizing>, Zeroizing) so the guarantee is type-enforced rather than assumed; the same class in unseal_signing_key's ephemeral plaintext buffer is hardened too. StoredEntry also kept credential name (as the plaintext fjall key), type, metadata, status, and history unencrypted, so filesystem access to the vault revealed the operator's entire credential inventory without ever attacking the secret's AEAD. Both are now encrypted: metadata moves into a second ChaCha20-Poly1305 field (encrypted_metadata, kept separate from encrypted_secret so list() never touches secret ciphertext) and the fjall record key becomes a keyed-BLAKE3 hash of the name, mirroring the existing tamper-log chain-key derivation. VAULT_VERSION bumps to 2 (hard break, no migration point pre-1.0) since the on-disk shape changed incompatibly. --- crates/akroasis/src/vault/mod.rs | 7 +- crates/kryphos/src/config.rs | 19 ++- crates/kryphos/src/storage.rs | 177 +++++++++++++++++++++------- crates/kryphos/src/storage_tests.rs | 98 ++++++++++++++- crates/kryphos/src/vault.rs | 25 +++- docs/fjall-column-encryption.md | 26 ++-- 6 files changed, 287 insertions(+), 65 deletions(-) diff --git a/crates/akroasis/src/vault/mod.rs b/crates/akroasis/src/vault/mod.rs index 7c41307..498098a 100644 --- a/crates/akroasis/src/vault/mod.rs +++ b/crates/akroasis/src/vault/mod.rs @@ -579,7 +579,7 @@ mod tests { .unwrap(); let entry = vault.get("test-key").unwrap(); - assert_eq!(entry.secret, b"secret-123"); + assert_eq!(entry.secret.as_slice(), b"secret-123".as_slice()); assert_eq!(entry.credential_type, CredentialType::ApiKey); } @@ -640,7 +640,7 @@ mod tests { vault.rotate("rotate-key", b"new-secret").unwrap(); let entry = vault.get("rotate-key").unwrap(); - assert_eq!(entry.secret, b"new-secret"); + assert_eq!(entry.secret.as_slice(), b"new-secret".as_slice()); } #[test] @@ -658,7 +658,8 @@ mod tests { let entry = vault.get("binary-key").unwrap(); assert_eq!( - entry.secret, non_utf8_secret, + entry.secret.as_slice(), + non_utf8_secret, "secret bytes must round-trip exactly, with no UTF-8 lossy substitution" ); } diff --git a/crates/kryphos/src/config.rs b/crates/kryphos/src/config.rs index b9c03aa..dc91693 100644 --- a/crates/kryphos/src/config.rs +++ b/crates/kryphos/src/config.rs @@ -2,6 +2,7 @@ use figment::value::{Dict, Map, Value}; use figment::{Error, Metadata, Profile, Provider}; +use zeroize::Zeroizing; use crate::storage::Vault; @@ -82,13 +83,25 @@ impl

VaultProvider

{ .get(entry_name) .map_err(|e| Error::from(e.to_string()))?; - let secret_str = String::from_utf8(decrypted.secret).map_err(|_| { + // WHY: `decrypted.secret` is `Zeroizing>` and has + // no public escape hatch to a bare `Vec`/`String` — + // `str::from_utf8` borrows instead of consuming, so the + // validated bytes never leave the zeroizing wrapper. + // `decrypted` (and its `secret` field) is scrubbed on + // drop at the end of this function. + let validated = std::str::from_utf8(&decrypted.secret).map_err(|_| { Error::from(format!( "vault entry '{entry_name}' contains non-UTF-8 data" )) })?; - - Ok(Value::String(tag, secret_str)) + let secret_str = Zeroizing::new(validated.to_owned()); + + // NOTE: `figment::Value::String` requires an owned, + // non-zeroizing `String` — this clone (deref past the + // `Zeroizing` wrapper first) is the one unavoidable copy + // that crosses into a type we don't control. `secret_str` + // itself still zeroizes on drop immediately after. + Ok(Value::String(tag, (*secret_str).clone())) } else { Ok(Value::String(tag, s)) } diff --git a/crates/kryphos/src/storage.rs b/crates/kryphos/src/storage.rs index 61fa201..a6498de 100644 --- a/crates/kryphos/src/storage.rs +++ b/crates/kryphos/src/storage.rs @@ -10,6 +10,7 @@ use jiff::Timestamp; use koinon::{ChainKey, LogEntryKind, TamperLog, VerificationResult}; use serde::{Deserialize, Serialize}; use snafu::ResultExt; +use zeroize::Zeroizing; use crate::crypto::{self, decrypt, encrypt}; use crate::error::{ @@ -45,6 +46,13 @@ const TAMPER_LOG_FILE: &str = "tamper.log"; /// vault key from a leaked chain key is infeasible. const CHAIN_KEY_DOMAIN: &[u8] = b"kryphos/tamper-log/chain-key/v1"; +/// Domain-separation tag for deriving the fjall lookup-key subkey from +/// the vault's [`VaultKey`] (see [`Vault::lookup_key`]). +/// +/// Reuses the vault's existing secret rather than requiring a second one +/// to manage, mirroring [`CHAIN_KEY_DOMAIN`]. +const LOOKUP_KEY_DOMAIN: &[u8] = b"kryphos/vault/lookup-key/v1"; + /// On-disk vault header stored as JSON. #[derive(Debug, Serialize, Deserialize)] struct StoredHeader { @@ -55,10 +63,25 @@ struct StoredHeader { } /// Entry as stored in fjall (JSON-serialized value). +/// +/// Both fields are independently-nonced ChaCha20-Poly1305 ciphertexts. +/// `encrypted_metadata` decrypts to an [`EntryMetadataRecord`] carrying +/// the name, type, metadata, status, and history — none of it readable +/// from the fjall data directory without the vault key. Keeping it +/// separate from `encrypted_secret` means listing entries (which needs +/// only the metadata) never touches secret ciphertext. #[derive(Debug, Serialize, Deserialize)] struct StoredEntry { - credential_type: CredentialType, encrypted_secret: Vec, + encrypted_metadata: Vec, +} + +/// Plaintext form of everything but the secret value; JSON-serialized +/// and encrypted as `StoredEntry::encrypted_metadata`. +#[derive(Debug, Serialize, Deserialize)] +struct EntryMetadataRecord { + name: CompactString, + credential_type: CredentialType, metadata: EntryMetadata, #[serde(default)] status: EntryStatus, @@ -74,7 +97,12 @@ pub struct DecryptedEntry { /// What kind of credential this is. pub credential_type: CredentialType, /// The decrypted secret bytes. - pub secret: Vec, + /// + /// Wrapped in [`Zeroizing`] at the point of allocation (the return + /// of [`decrypt`], moved straight in — no unwrapped copy exists in + /// between) so the plaintext is scrubbed on drop rather than left in + /// freed heap memory. + pub secret: Zeroizing>, /// Associated metadata. pub metadata: EntryMetadata, } @@ -280,7 +308,9 @@ impl Vault { credential_type: CredentialType, secret: &[u8], ) -> Result<(), VaultError> { - if self.keyspace.get(name).map_err(fjall_err)?.is_some() { + let key = self.lookup_key(name); + + if self.keyspace.get(key).map_err(fjall_err)?.is_some() { return Err(VaultError::DuplicateEntry { name: name.to_owned(), }); @@ -289,9 +319,9 @@ impl Vault { let encrypted_secret = encrypt(&self.key, secret).context(EntryCryptoSnafu)?; let now = Timestamp::now(); - let entry = StoredEntry { + let record = EntryMetadataRecord { + name: CompactString::from(name), credential_type, - encrypted_secret, metadata: EntryMetadata { created_at: now, rotated_at: None, @@ -305,9 +335,15 @@ impl Vault { kind: HistoryEventKind::Created, }], }; + let encrypted_metadata = self.encrypt_metadata(&record)?; + + let entry = StoredEntry { + encrypted_secret, + encrypted_metadata, + }; let value = serde_json::to_vec(&entry).context(SerializationSnafu)?; - self.keyspace.insert(name, value).map_err(fjall_err)?; + self.keyspace.insert(key, value).map_err(fjall_err)?; self.db .persist(fjall::PersistMode::SyncAll) .map_err(fjall_err)?; @@ -324,50 +360,57 @@ impl Vault { /// Returns [`VaultError::EntryRevoked`] if the entry has been revoked. /// Returns [`VaultError::EntryCrypto`] if decryption fails. pub fn get(&self, name: &str) -> Result { - let raw = self.keyspace.get(name).map_err(fjall_err)?.ok_or_else(|| { + let key = self.lookup_key(name); + let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { name: name.to_owned(), } })?; let entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let record = self.decrypt_metadata(&entry)?; - if entry.status == EntryStatus::Revoked { + if record.status == EntryStatus::Revoked { return EntryRevokedSnafu { name }.fail(); } - let secret = decrypt(&self.key, &entry.encrypted_secret).context(EntryCryptoSnafu)?; + // WHY: wrap at the point of allocation — `decrypt`'s return is moved + // straight into `Zeroizing::new` with no intermediate unwrapped + // binding, so there is no plaintext copy that this fix leaves + // unscrubbed on drop. + let secret = + Zeroizing::new(decrypt(&self.key, &entry.encrypted_secret).context(EntryCryptoSnafu)?); Ok(DecryptedEntry { - name: CompactString::from(name), - credential_type: entry.credential_type, + name: record.name, + credential_type: record.credential_type, secret, - metadata: entry.metadata, + metadata: record.metadata, }) } /// Lists all entries in the vault (names and metadata only). /// - /// No secrets are decrypted or returned. + /// No secrets are decrypted or returned: only `encrypted_metadata` is + /// touched, never `encrypted_secret`. /// /// # Errors /// /// Returns [`VaultError::StorageBackend`] on iteration errors. + /// Returns [`VaultError::EntryCrypto`] if metadata decryption fails. pub fn list(&self) -> Result, VaultError> { let mut entries = Vec::new(); for guard in self.keyspace.iter() { - let (key, value) = guard.into_inner().map_err(fjall_err)?; - let name = std::str::from_utf8(&key).map_err(|e| VaultError::StorageBackend { - message: format!("invalid UTF-8 key: {e}"), - })?; + let (_key, value) = guard.into_inner().map_err(fjall_err)?; let entry: StoredEntry = serde_json::from_slice(&value).context(SerializationSnafu)?; + let record = self.decrypt_metadata(&entry)?; entries.push(EntryInfo { - name: CompactString::from(name), - credential_type: entry.credential_type, - status: entry.status, - metadata: entry.metadata, + name: record.name, + credential_type: record.credential_type, + status: record.status, + metadata: record.metadata, }); } @@ -383,19 +426,21 @@ impl Vault { /// Returns [`VaultError::EntryNotFound`] if no entry with this name exists. /// Returns [`VaultError::EntryNotDeletable`] if the entry is revoked. pub fn remove(&self, name: &str) -> Result<(), VaultError> { - let raw = self.keyspace.get(name).map_err(fjall_err)?.ok_or_else(|| { + let key = self.lookup_key(name); + let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { name: name.to_owned(), } })?; let entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let record = self.decrypt_metadata(&entry)?; - if entry.status == EntryStatus::Revoked { + if record.status == EntryStatus::Revoked { return EntryNotDeletableSnafu { name }.fail(); } - self.keyspace.remove(name).map_err(fjall_err)?; + self.keyspace.remove(key).map_err(fjall_err)?; self.db .persist(fjall::PersistMode::SyncAll) .map_err(fjall_err)?; @@ -415,30 +460,38 @@ impl Vault { /// Returns [`VaultError::EntryRevoked`] if the entry has been revoked. /// Returns [`VaultError::EntryCrypto`] if encryption fails. pub fn rotate(&self, name: &str, new_secret: &[u8]) -> Result<(), VaultError> { - let raw = self.keyspace.get(name).map_err(fjall_err)?.ok_or_else(|| { + let key = self.lookup_key(name); + let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { name: name.to_owned(), } })?; - let mut entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let mut record = self.decrypt_metadata(&entry)?; - if entry.status == EntryStatus::Revoked { + if record.status == EntryStatus::Revoked { return EntryRevokedSnafu { name }.fail(); } - entry.encrypted_secret = encrypt(&self.key, new_secret).context(EntryCryptoSnafu)?; + let encrypted_secret = encrypt(&self.key, new_secret).context(EntryCryptoSnafu)?; let now = Timestamp::now(); - entry.metadata.rotated_at = Some(now); - entry.metadata.rotation_count += 1; - entry.history.push(HistoryEvent { + record.metadata.rotated_at = Some(now); + record.metadata.rotation_count += 1; + record.history.push(HistoryEvent { timestamp: now, kind: HistoryEventKind::Rotated, }); + let encrypted_metadata = self.encrypt_metadata(&record)?; + + let entry = StoredEntry { + encrypted_secret, + encrypted_metadata, + }; let value = serde_json::to_vec(&entry).context(SerializationSnafu)?; - self.keyspace.insert(name, value).map_err(fjall_err)?; + self.keyspace.insert(key, value).map_err(fjall_err)?; self.db .persist(fjall::PersistMode::SyncAll) .map_err(fjall_err)?; @@ -457,28 +510,36 @@ impl Vault { /// Returns [`VaultError::EntryNotFound`] if no entry with this name exists. /// Returns [`VaultError::EntryRevoked`] if the entry is already revoked. pub fn revoke(&self, name: &str) -> Result<(), VaultError> { - let raw = self.keyspace.get(name).map_err(fjall_err)?.ok_or_else(|| { + let key = self.lookup_key(name); + let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { name: name.to_owned(), } })?; - let mut entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let mut record = self.decrypt_metadata(&entry)?; - if entry.status == EntryStatus::Revoked { + if record.status == EntryStatus::Revoked { return EntryRevokedSnafu { name }.fail(); } let now = Timestamp::now(); - entry.status = EntryStatus::Revoked; - entry.metadata.revoked_at = Some(now); - entry.history.push(HistoryEvent { + record.status = EntryStatus::Revoked; + record.metadata.revoked_at = Some(now); + record.history.push(HistoryEvent { timestamp: now, kind: HistoryEventKind::Revoked, }); + let encrypted_metadata = self.encrypt_metadata(&record)?; + + let entry = StoredEntry { + encrypted_secret: entry.encrypted_secret, + encrypted_metadata, + }; let value = serde_json::to_vec(&entry).context(SerializationSnafu)?; - self.keyspace.insert(name, value).map_err(fjall_err)?; + self.keyspace.insert(key, value).map_err(fjall_err)?; self.db .persist(fjall::PersistMode::SyncAll) .map_err(fjall_err)?; @@ -496,19 +557,21 @@ impl Vault { /// /// Returns [`VaultError::EntryNotFound`] if no entry with this name exists. pub fn history(&self, name: &str) -> Result { - let raw = self.keyspace.get(name).map_err(fjall_err)?.ok_or_else(|| { + let key = self.lookup_key(name); + let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { name: name.to_owned(), } })?; let entry: StoredEntry = serde_json::from_slice(&raw).context(SerializationSnafu)?; + let record = self.decrypt_metadata(&entry)?; Ok(EntryHistory { - name: CompactString::from(name), - status: entry.status, - metadata: entry.metadata, - events: entry.history, + name: record.name, + status: record.status, + metadata: record.metadata, + events: record.history, }) } @@ -543,6 +606,32 @@ impl Vault { ChainKey::from_bytes(blake3::keyed_hash(self.key.as_bytes(), CHAIN_KEY_DOMAIN).into()) } + /// Derives the fjall record key for `name` via a two-step keyed BLAKE3 + /// hash of the vault key, so credential names never appear as fjall + /// keys on disk. + /// + /// Deterministic (same name -> same key), so `get`/`add`/`remove` stay + /// O(1) keyspace lookups without ever storing the name itself. A fresh + /// derivation on every call, mirroring [`Self::chain_key`]. + fn lookup_key(&self, name: &str) -> [u8; 32] { + let subkey = blake3::keyed_hash(self.key.as_bytes(), LOOKUP_KEY_DOMAIN); + blake3::keyed_hash(subkey.as_bytes(), name.as_bytes()).into() + } + + /// Decrypts and parses an entry's `encrypted_metadata` field. + fn decrypt_metadata(&self, entry: &StoredEntry) -> Result { + let metadata_bytes = + decrypt(&self.key, &entry.encrypted_metadata).context(EntryCryptoSnafu)?; + serde_json::from_slice(&metadata_bytes).context(SerializationSnafu) + } + + /// Serializes and encrypts an [`EntryMetadataRecord`] for storage as + /// `StoredEntry::encrypted_metadata`. + fn encrypt_metadata(&self, record: &EntryMetadataRecord) -> Result, VaultError> { + let metadata_bytes = serde_json::to_vec(record).context(SerializationSnafu)?; + encrypt(&self.key, &metadata_bytes).context(EntryCryptoSnafu) + } + fn append_vault_audit(&self, name: &str, operation: &str) -> Result<(), VaultError> { let mut log = TamperLog::open(self.tamper_log_path(), self.chain_key()).context(TamperLogSnafu)?; diff --git a/crates/kryphos/src/storage_tests.rs b/crates/kryphos/src/storage_tests.rs index ccd7ebf..a986191 100644 --- a/crates/kryphos/src/storage_tests.rs +++ b/crates/kryphos/src/storage_tests.rs @@ -62,7 +62,7 @@ fn add_and_get_round_trip() { let entry = vault.get("openai-key").unwrap(); assert_eq!(entry.name, "openai-key"); assert_eq!(entry.credential_type, CredentialType::ApiKey); - assert_eq!(entry.secret, secret); + assert_eq!(entry.secret.as_slice(), secret.as_slice()); } #[test] @@ -157,7 +157,11 @@ fn entries_persist_across_open_close() { let vault = Vault::open(&vault_path, TEST_PASSPHRASE).unwrap(); let entry = vault.get("persistent").unwrap(); - assert_eq!(entry.secret, b"cert-pem", "secret must survive close/open"); + assert_eq!( + entry.secret.as_slice(), + b"cert-pem".as_slice(), + "secret must survive close/open" + ); } #[test] @@ -190,7 +194,8 @@ fn rotate_updates_secret_and_preserves_name() { let entry = vault.get("api-key").unwrap(); assert_eq!(entry.name, "api-key", "name must be preserved after rotate"); assert_eq!( - entry.secret, b"new-secret", + entry.secret.as_slice(), + b"new-secret".as_slice(), "secret must be updated after rotate" ); assert_eq!( @@ -617,3 +622,90 @@ fn open_reports_absence_rather_than_io_failure_for_a_bare_directory() { "a headerless directory must report typed absence, got: {result:?}" ); } + +// ----------------------------------------------------------------- +// Secret zeroization (akroasis#218) +// ----------------------------------------------------------------- + +#[test] +fn decrypted_secret_is_zeroized_on_drop_by_type() { + // Regression for forkwright/akroasis#218: `DecryptedEntry.secret` used + // to be a bare `Vec`, which does not implement `ZeroizeOnDrop` — + // this assertion would FAIL TO COMPILE against that field type. Wrapping + // it in `Zeroizing>` (at the point `decrypt` returns, with no + // intermediate unwrapped copy) makes the guarantee type-enforced rather + // than a claim: `Zeroizing: ZeroizeOnDrop` for any `Z: Zeroize`, so + // this now compiles and holds for every `DecryptedEntry` the shipped + // `Vault::get` returns. + fn assert_zeroizes_on_drop(_: &T) {} + + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("zeroize-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add("zeroize-key", CredentialType::ApiKey, b"scrub-me-on-drop") + .unwrap(); + + let entry = vault.get("zeroize-key").unwrap(); + assert_zeroizes_on_drop(&entry.secret); +} + +// ----------------------------------------------------------------- +// At-rest metadata encryption (akroasis#215) +// ----------------------------------------------------------------- + +#[test] +fn on_disk_fjall_contents_do_not_reveal_credential_name() { + // Regression for forkwright/akroasis#215: `StoredEntry` used to keep + // credential_type/metadata/status/history as plaintext JSON, and the + // fjall record KEY was the credential name itself in cleartext — so the + // name appeared on disk twice over, once as the key and (implicitly, via + // being findable) as an index into the plaintext value. This reads + // every file fjall actually wrote under the vault's `data/` directory + // and asserts the name is not a byte-for-byte substring of any of them. + // + // Before the fix this failed: the name was the literal fjall key, so it + // appears verbatim in the LSM tree's persisted pages. After the fix the + // fjall key is a keyed BLAKE3 hash of the name and the value is two + // ChaCha20-Poly1305 ciphertexts, neither of which can contain the + // plaintext name as a substring without breaking either primitive. + const DISTINCTIVE_NAME: &str = "surveillance-counter-mesh-psk-zzyzx9182"; + + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("metadata-at-rest-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add(DISTINCTIVE_NAME, CredentialType::Psk, b"mesh-secret-value") + .unwrap(); + drop(vault); + + let mut on_disk = Vec::new(); + collect_file_bytes(&vault_path.join(DATA_DIR), &mut on_disk); + + assert!( + !on_disk.is_empty(), + "validation check: fjall must have written SOMETHING to the data directory" + ); + assert!( + !on_disk + .windows(DISTINCTIVE_NAME.len()) + .any(|window| window == DISTINCTIVE_NAME.as_bytes()), + "credential name must not appear in plaintext anywhere under the fjall data directory" + ); +} + +/// Recursively reads every regular file under `dir`, appending its bytes to +/// `out`. Test-only: used to scan fjall's actual on-disk output for +/// plaintext leakage rather than trusting the in-process API surface. +fn collect_file_bytes(dir: &std::path::Path, out: &mut Vec) { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + collect_file_bytes(&path, out); + } else if let Ok(bytes) = std::fs::read(&path) { + out.extend_from_slice(&bytes); + } + } +} diff --git a/crates/kryphos/src/vault.rs b/crates/kryphos/src/vault.rs index 3e44920..3796d02 100644 --- a/crates/kryphos/src/vault.rs +++ b/crates/kryphos/src/vault.rs @@ -6,6 +6,7 @@ use compact_str::CompactString; use jiff::Timestamp; use serde::{Deserialize, Serialize}; use snafu::ResultExt; +use zeroize::Zeroizing; use crate::error::{CryptoError, KeyParseSnafu}; use crate::key::{InstallationIdentity, SigningKey, VaultKey}; @@ -17,7 +18,15 @@ pub const SALT_LEN: usize = 16; pub const NONCE_LEN: usize = 12; /// Current vault format version. -pub const VAULT_VERSION: u32 = 1; +/// +/// WARNING: bumping this is a hard break, not a migration point — `open` +/// rejects any header whose `version` does not match exactly, so a vault +/// written under a prior version simply fails to open under a newer one. +/// v2 changed `StoredEntry`'s on-disk shape: names, types, tags, status, and +/// history moved from plaintext fields into `encrypted_metadata`, and the +/// fjall record key changed from the plaintext name to a keyed-hash lookup +/// key, so a v1 store has neither the fields nor the keys v2 code expects. +pub const VAULT_VERSION: u32 = 2; /// The kind of credential stored in a [`VaultEntry`]. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -244,9 +253,17 @@ pub fn unseal_signing_key( let cipher = ChaCha20Poly1305::new(vault_key.as_bytes().into()); let nonce = Nonce::from_slice(nonce); - let plaintext = cipher - .decrypt(nonce, ciphertext) - .map_err(|_| CryptoError::DecryptionFailed)?; + // WHY: wrap at the point of allocation — `decrypt`'s return (the raw + // Ed25519 signing key bytes) is moved straight into `Zeroizing::new` + // with no intermediate unwrapped binding, so this ephemeral copy is + // scrubbed on drop rather than left in freed heap memory. `SigningKey` + // itself protects the key it constructs (ZeroizeOnDrop, RUST/#218); this + // covers the plaintext buffer that exists only until then. + let plaintext = Zeroizing::new( + cipher + .decrypt(nonce, ciphertext) + .map_err(|_| CryptoError::DecryptionFailed)?, + ); let signing = SigningKey::from_bytes(&plaintext).context(KeyParseSnafu)?; diff --git a/docs/fjall-column-encryption.md b/docs/fjall-column-encryption.md index e7345ca..ea5603a 100644 --- a/docs/fjall-column-encryption.md +++ b/docs/fjall-column-encryption.md @@ -3,7 +3,8 @@ Issue #132 tracks a future declarative encryption layer for fjall-backed stores. Current main does not have a generic table/column store abstraction: the only fjall-backed runtime store is `kryphos::Vault`, and it already -encrypts credential secrets through a typed field before serializing the row. +encrypts every field of a credential record — secret, name, type, metadata, +status, and history — through two typed fields before serializing the row. This note defines the boundary to use when akroasis adds its first mixed plaintext/ciphertext fjall schema for signals, references, or other indexed @@ -12,11 +13,18 @@ runtime data. It is not an implementation of #132. ## Current State - `crates/kryphos/src/storage.rs` owns one fjall keyspace named `entries`. -- `StoredEntry` keeps `encrypted_secret` as the only encrypted field; metadata, - status, and history remain structured so vault listing and lifecycle logic can - run without decrypting secret material. -- `Vault::add`, `Vault::get`, and `Vault::rotate` call the existing - ChaCha20-Poly1305 helpers directly for that one field. +- `StoredEntry` holds two independently-nonced ChaCha20-Poly1305 ciphertexts: + `encrypted_secret` and `encrypted_metadata`. `encrypted_metadata` decrypts to + an `EntryMetadataRecord` carrying name, credential type, metadata, status, + and history (akroasis#215) — nothing about a credential is plaintext at + rest. The fjall record KEY is a keyed-BLAKE3 hash of the name, not the name + itself, so no credential name appears verbatim in the fjall data directory. +- `Vault::list` decrypts only `encrypted_metadata`, never `encrypted_secret` — + callers already hold the vault key (an unlocked `Vault`), so this is the + "explicit decrypted view" the Non-Goals below require, not a bypass of it. +- `Vault::add`, `Vault::get`, `Vault::rotate`, `Vault::revoke`, and + `Vault::history` call the existing ChaCha20-Poly1305 helpers directly for + both fields via `Vault::encrypt_metadata`/`Vault::decrypt_metadata`. - There is no fjall-backed signal store in current main. Mesh signals are produced in memory and forwarded through the collector/processor path. @@ -44,8 +52,10 @@ fields may remain plaintext for indexing, filtering, or redacted display. ## Non-Goals -- Do not retrofit `kryphos::Vault` only to satisfy the shape. Its existing typed - `encrypted_secret` model is clearer than a generic map until another store +- Do not retrofit `kryphos::Vault` onto the generic `ColumnCodec`/ + `ENCRYPTED_FIELDS` shape only for consistency with future stores. Its + existing typed-field model (now two fields: `encrypted_secret` and + `encrypted_metadata`) is clearer than a generic map until another store proves the abstraction. - Do not encrypt fields that are required for safe listing or lifecycle checks unless the caller has an explicit decrypted view. From ed93526856333a6b31f09f6a776585e00f51139f Mon Sep 17 00:00:00 2001 From: forkwright Date: Mon, 17 Aug 2026 10:31:06 -0500 Subject: [PATCH 2/2] fix(kryphos): zeroize the two signing-key buffers #218's fix missed seal_signing_key held the raw Ed25519 signing key in an ordinary [u8; 32] before encrypting it -- the encrypt-direction sibling of the buffer unseal_signing_key already wraps in Zeroizing, left uncovered when that fix landed. SigningKey::from_bytes made the same mistake one call-frame further in: TryInto<[u8; N]> copies the caller's bytes into a fresh stack array that ed25519_dalek::SigningKey::from_bytes reads and never scrubs, so unseal_signing_key's own Zeroizing wrap stopped one frame short of the ZeroizeOnDrop-protected key it constructs. Both are now type-enforced the same way the decrypt direction already is: zeroizing_signing_key_bytes (vault.rs) and zeroizing_key_array (key.rs) return Zeroizing<[u8; N]> rather than a bare array, and each has a dispositive by-construction test (assert_zeroizes_on_drop against the actual returned value, mirroring decrypted_secret_is_zeroized_on_drop_by_type in storage_tests.rs) -- [u8; N] alone does not implement ZeroizeOnDrop, so the assertion fails to compile against the pre-fix bare-array return and compiles clean against the wrapped one. Corrects the unseal_signing_key WHY comment, which claimed coverage ended at SigningKey's own ZeroizeOnDrop impl -- that claim skipped the from_bytes copy this fix closes. --- crates/kryphos/src/key.rs | 47 +++++++++++++++++++++++++++++++----- crates/kryphos/src/vault.rs | 48 +++++++++++++++++++++++++++++++++---- 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/crates/kryphos/src/key.rs b/crates/kryphos/src/key.rs index 4dc1e85..c55106e 100644 --- a/crates/kryphos/src/key.rs +++ b/crates/kryphos/src/key.rs @@ -4,7 +4,7 @@ use std::fmt; use serde::{Deserialize, Serialize}; use subtle::ConstantTimeEq; -use zeroize::{Zeroize, ZeroizeOnDrop}; +use zeroize::{Zeroize, ZeroizeOnDrop, Zeroizing}; use ed25519_dalek::Signer; @@ -44,11 +44,7 @@ impl SigningKey { /// /// Returns [`KeyError::WrongKeyLength`] if `bytes` is not 32 bytes. pub fn from_bytes(bytes: &[u8]) -> Result { - let arr: [u8; SIGNING_KEY_LEN] = - bytes.try_into().map_err(|_| KeyError::WrongKeyLength { - expected: SIGNING_KEY_LEN, - actual: bytes.len(), - })?; + let arr = zeroizing_key_array(bytes)?; Ok(Self { inner: ed25519_dalek::SigningKey::from_bytes(&arr), }) @@ -83,6 +79,26 @@ impl SigningKey { } } +/// Copies `bytes` into a fixed-size array wrapped for zero-on-drop. +/// +/// WHY: `TryInto<[u8; N]>` makes an unavoidable copy crossing from the +/// caller's slice into a stack array; `SigningKey::from_bytes` is the +/// call-frame directly above `unseal_signing_key` (vault.rs), whose own +/// decrypt-output copy is `Zeroizing`-wrapped (RUST/#218) — this closes the +/// next frame so that coverage does not stop one call short. +/// +/// # Errors +/// +/// Returns [`KeyError::WrongKeyLength`] if `bytes` is not `SIGNING_KEY_LEN` +/// bytes. +fn zeroizing_key_array(bytes: &[u8]) -> Result, KeyError> { + let arr: [u8; SIGNING_KEY_LEN] = bytes.try_into().map_err(|_| KeyError::WrongKeyLength { + expected: SIGNING_KEY_LEN, + actual: bytes.len(), + })?; + Ok(Zeroizing::new(arr)) +} + impl fmt::Debug for SigningKey { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str("SigningKey([REDACTED])") @@ -319,6 +335,25 @@ mod tests { assert!(result.is_err()); } + /// Dispositive by construction, same mechanism as + /// `decrypted_secret_is_zeroized_on_drop_by_type` (kryphos storage + /// tests): `[u8; N]` alone does not implement `ZeroizeOnDrop` (only + /// `Zeroize`), so this specific bound fails to compile against a bare + /// array and passes only because `zeroizing_key_array` returns + /// `Zeroizing<[u8; N]>`. + #[test] + fn signing_key_from_bytes_intermediate_array_is_zeroize_on_drop_by_type() { + fn assert_zeroizes_on_drop(_: &T) {} + + let bytes = [0x11; SIGNING_KEY_LEN]; + let arr = zeroizing_key_array(&bytes).unwrap(); + assert_zeroizes_on_drop(&arr); + assert_eq!( + *arr, bytes, + "wrapped array must carry the same bytes through" + ); + } + #[test] fn signing_key_debug_is_redacted() { let sk = SigningKey::generate(); diff --git a/crates/kryphos/src/vault.rs b/crates/kryphos/src/vault.rs index 3796d02..6ec3262 100644 --- a/crates/kryphos/src/vault.rs +++ b/crates/kryphos/src/vault.rs @@ -9,7 +9,7 @@ use snafu::ResultExt; use zeroize::Zeroizing; use crate::error::{CryptoError, KeyParseSnafu}; -use crate::key::{InstallationIdentity, SigningKey, VaultKey}; +use crate::key::{InstallationIdentity, SIGNING_KEY_LEN, SigningKey, VaultKey}; /// Size of the Argon2id salt in bytes. pub const SALT_LEN: usize = 16; @@ -227,7 +227,12 @@ pub fn seal_signing_key( ) -> Result, CryptoError> { let cipher = ChaCha20Poly1305::new(vault_key.as_bytes().into()); let nonce = Nonce::from_slice(nonce); - let plaintext = identity.signing_key().to_bytes(); + // WHY: `to_bytes()` returns the raw Ed25519 signing key in an ordinary + // array; wrap immediately so this ephemeral copy is scrubbed on drop + // rather than left on the stack after `encrypt` returns — the same + // coverage `unseal_signing_key` gives the decrypt direction below + // (RUST/#218). + let plaintext = zeroizing_signing_key_bytes(identity); cipher .encrypt(nonce, plaintext.as_ref()) @@ -236,6 +241,13 @@ pub fn seal_signing_key( }) } +/// Copies the signing key's raw bytes into a zero-on-drop buffer. +fn zeroizing_signing_key_bytes( + identity: &InstallationIdentity, +) -> Zeroizing<[u8; SIGNING_KEY_LEN]> { + Zeroizing::new(identity.signing_key().to_bytes()) +} + /// Decrypts a signing key from vault ciphertext, reconstructing the /// [`InstallationIdentity`]. /// @@ -256,9 +268,13 @@ pub fn unseal_signing_key( // WHY: wrap at the point of allocation — `decrypt`'s return (the raw // Ed25519 signing key bytes) is moved straight into `Zeroizing::new` // with no intermediate unwrapped binding, so this ephemeral copy is - // scrubbed on drop rather than left in freed heap memory. `SigningKey` - // itself protects the key it constructs (ZeroizeOnDrop, RUST/#218); this - // covers the plaintext buffer that exists only until then. + // scrubbed on drop rather than left in freed heap memory. + // `SigningKey::from_bytes` (key.rs) makes one further copy crossing + // into its own fixed-size array before constructing the + // `ZeroizeOnDrop`-protected `ed25519_dalek::SigningKey`; that copy is + // wrapped the same way (`zeroizing_key_array`), so no unprotected frame + // remains between this decrypt output and the protected key it becomes + // (RUST/#218). let plaintext = Zeroizing::new( cipher .decrypt(nonce, ciphertext) @@ -457,6 +473,28 @@ mod tests { ); } + /// Dispositive by construction, same mechanism as + /// `decrypted_secret_is_zeroized_on_drop_by_type` (kryphos storage + /// tests): `[u8; N]` alone does not implement `ZeroizeOnDrop` (only + /// `Zeroize`), so this specific bound fails to compile against a bare + /// array and passes only because `zeroizing_signing_key_bytes` returns + /// `Zeroizing<[u8; N]>` — the encrypt-direction counterpart of the + /// `Zeroizing` wrap `unseal_signing_key` already applies on decrypt. + #[test] + fn seal_signing_key_plaintext_buffer_is_zeroize_on_drop_by_type() { + use zeroize::ZeroizeOnDrop; + fn assert_zeroizes_on_drop(_: &T) {} + + let identity = InstallationIdentity::generate(); + let plaintext = zeroizing_signing_key_bytes(&identity); + assert_zeroizes_on_drop(&plaintext); + assert_eq!( + *plaintext, + identity.signing_key().to_bytes(), + "wrapped buffer must carry the same bytes through" + ); + } + #[test] fn unseal_with_wrong_key_fails() { let identity = InstallationIdentity::generate();