From 0f51878b9f78b104a6f6908a6177ea3dcfe6ca4f Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:07:08 -0500 Subject: [PATCH 1/4] test(kryphos): pin akroasis#287/#283/#214 as failing before the fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the negative-case fixtures for empty-passphrase creation, ciphertext relocated between entries, mutated credential_type/envelope_version tampering, and concurrent add/rotate races — none of which the current implementation guards against. This commit is expected red; the next commit makes it green. --- crates/kryphos/src/error.rs | 4 + crates/kryphos/src/storage.rs | 14 + crates/kryphos/src/storage_security_tests.rs | 256 +++++++++++++++++++ 3 files changed, 274 insertions(+) create mode 100644 crates/kryphos/src/storage_security_tests.rs diff --git a/crates/kryphos/src/error.rs b/crates/kryphos/src/error.rs index b99c219..6df9cf0 100644 --- a/crates/kryphos/src/error.rs +++ b/crates/kryphos/src/error.rs @@ -48,6 +48,10 @@ pub enum VaultError { #[snafu(display("wrong passphrase: decryption of key check failed"))] WrongPassphrase, + /// The passphrase is empty, so the derived key carries no entropy. + #[snafu(display("passphrase must not be empty"))] + EmptyPassphrase, + /// The vault is already locked by another process. #[snafu(display("vault is locked by another process: {path}", path = path.display()))] Locked { diff --git a/crates/kryphos/src/storage.rs b/crates/kryphos/src/storage.rs index a6498de..41dd2ff 100644 --- a/crates/kryphos/src/storage.rs +++ b/crates/kryphos/src/storage.rs @@ -747,3 +747,17 @@ fn fjall_err(e: impl std::fmt::Display) -> VaultError { )] #[path = "storage_tests.rs"] mod tests; + +// WHY this module exists as an isolated commit before the fix lands: these +// tests pin the exact defects in forkwright/akroasis#287, #283, and #214. +// Watched red against this commit (pre-fix), watched green against the +// commit that follows it (the actual fix) — see the PR body's negative +// fixture section. +#[cfg(test)] +#[expect( + clippy::unwrap_used, + clippy::indexing_slicing, + reason = "test code: panics and unwraps acceptable in assertions" +)] +#[path = "storage_security_tests.rs"] +mod security_tests; diff --git a/crates/kryphos/src/storage_security_tests.rs b/crates/kryphos/src/storage_security_tests.rs new file mode 100644 index 0000000..bf90d36 --- /dev/null +++ b/crates/kryphos/src/storage_security_tests.rs @@ -0,0 +1,256 @@ +//! Tests for [`super`]; split out from `storage_tests.rs` to keep both +//! under the RUST/file-too-long 800-line threshold. Covers the three +//! security defects fixed together: empty passphrases (akroasis#287), +//! ciphertext identity binding (akroasis#283), and concurrent mutation +//! atomicity (akroasis#214). + +use super::*; + +const TEST_PASSPHRASE: &[u8] = b"correct horse battery staple"; + +// ----------------------------------------------------------------- +// Empty passphrase (akroasis#287) +// ----------------------------------------------------------------- + +#[test] +fn create_rejects_empty_passphrase() { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("empty-passphrase-vault"); + + let result = Vault::create(&vault_path, b""); + + assert!( + matches!(result, Err(VaultError::EmptyPassphrase)), + "Vault::create with an empty passphrase must return a typed \ + validation error, got {result:?}" + ); +} + +#[test] +fn create_with_empty_passphrase_leaves_no_filesystem_state() { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("empty-passphrase-no-state-vault"); + + let result = Vault::create(&vault_path, b""); + assert!(result.is_err()); + + assert!( + !vault_path.exists(), + "a rejected empty-passphrase create must not create the vault path" + ); +} + +#[test] +fn create_succeeds_with_nonempty_passphrase_after_a_rejected_empty_one() { + // A path that was correctly refused for an empty passphrase must remain + // usable — the rejection must not itself leave a poisoned path (the + // same class of concern as `create_succeeds_after_a_failed_open_at_the_same_path` + // in storage_tests.rs, applied to the new #287 boundary). + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("empty-then-real-passphrase-vault"); + + let rejected = Vault::create(&vault_path, b""); + assert!(rejected.is_err()); + + let created = Vault::create(&vault_path, TEST_PASSPHRASE); + assert!( + created.is_ok(), + "create must succeed at a path a rejected empty-passphrase call touched, got: {created:?}" + ); +} + +// ----------------------------------------------------------------- +// Ciphertext identity binding (akroasis#283) +// ----------------------------------------------------------------- + +#[test] +fn moved_ciphertext_between_entries_fails_authentication() { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("aad-relocate-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add("entry-a", CredentialType::ApiKey, b"secret-a") + .unwrap(); + vault + .add("entry-b", CredentialType::ApiKey, b"secret-b") + .unwrap(); + + // Simulate a write-capable attacker (or store corruption): take the raw + // stored value for `entry-a` — a valid, correctly-authenticated + // ciphertext — and place it under `entry-b`'s fjall key. + let raw_a = vault.keyspace.get("entry-a").unwrap().unwrap(); + vault.keyspace.insert("entry-b", raw_a).unwrap(); + + let result = vault.get("entry-b"); + assert!( + result.is_err(), + "a ciphertext relocated from a different entry must fail \ + authentication rather than decrypt as a misidentified credential, \ + got {result:?}" + ); + + // The untouched original must still be exactly retrievable. + let entry_a = vault.get("entry-a").unwrap(); + assert_eq!(entry_a.secret, b"secret-a"); +} + +#[test] +fn mutated_credential_type_field_fails_authentication() { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("aad-type-tamper-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add("entry", CredentialType::ApiKey, b"secret") + .unwrap(); + + let raw = vault.keyspace.get("entry").unwrap().unwrap(); + let mut value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); + value["credential_type"] = serde_json::to_value(CredentialType::Psk).unwrap(); + let tampered = serde_json::to_vec(&value).unwrap(); + vault.keyspace.insert("entry", tampered).unwrap(); + + let result = vault.get("entry"); + assert!( + result.is_err(), + "a credential_type edited independently of encrypted_secret must \ + fail authentication, got {result:?}" + ); +} + +#[test] +fn mutated_envelope_version_field_fails_authentication() { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("aad-version-tamper-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add("entry", CredentialType::ApiKey, b"secret") + .unwrap(); + + let raw = vault.keyspace.get("entry").unwrap().unwrap(); + let mut value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); + value["envelope_version"] = serde_json::json!(99); + let tampered = serde_json::to_vec(&value).unwrap(); + vault.keyspace.insert("entry", tampered).unwrap(); + + let result = vault.get("entry"); + assert!( + result.is_err(), + "an envelope_version edited independently of encrypted_secret must \ + fail authentication, got {result:?}" + ); +} + +// ----------------------------------------------------------------- +// Concurrent mutation atomicity (akroasis#214) +// ----------------------------------------------------------------- + +#[test] +fn concurrent_add_same_name_yields_one_winner_and_duplicate_losers() { + const THREADS: usize = 16; + const ITERATIONS: usize = 10; + + for iteration in 0..ITERATIONS { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join(format!("concurrent-add-vault-{iteration}")); + let vault = std::sync::Arc::new(Vault::create(&vault_path, TEST_PASSPHRASE).unwrap()); + let barrier = std::sync::Arc::new(std::sync::Barrier::new(THREADS)); + + let handles: Vec<_> = (0..THREADS) + .map(|i| { + let vault = std::sync::Arc::clone(&vault); + let barrier = std::sync::Arc::clone(&barrier); + std::thread::spawn(move || { + barrier.wait(); + vault.add( + "race-key", + CredentialType::ApiKey, + format!("secret-{i}").as_bytes(), + ) + }) + }) + .collect(); + + let results: Vec<_> = handles.into_iter().map(|h| h.join().unwrap()).collect(); + + let successes = results.iter().filter(|r| r.is_ok()).count(); + let duplicates = results + .iter() + .filter(|r| matches!(r, Err(VaultError::DuplicateEntry { .. }))) + .count(); + + assert_eq!( + successes, 1, + "iteration {iteration}: exactly one concurrent add must win, got {successes} of {THREADS}" + ); + assert_eq!( + duplicates, + THREADS - 1, + "iteration {iteration}: every losing add must see DuplicateEntry, got {duplicates}" + ); + + let entries = vault.list().unwrap(); + assert_eq!( + entries.len(), + 1, + "iteration {iteration}: exactly one entry must be stored after the race, got {}", + entries.len() + ); + } +} + +#[test] +fn concurrent_rotate_never_loses_a_rotation_count_increment() { + const THREADS: usize = 16; + const ITERATIONS: usize = 10; + + for iteration in 0..ITERATIONS { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir + .path() + .join(format!("concurrent-rotate-vault-{iteration}")); + let vault = std::sync::Arc::new(Vault::create(&vault_path, TEST_PASSPHRASE).unwrap()); + vault + .add("rotate-race", CredentialType::ApiKey, b"v0") + .unwrap(); + + let barrier = std::sync::Arc::new(std::sync::Barrier::new(THREADS)); + + let handles: Vec<_> = (0..THREADS) + .map(|i| { + let vault = std::sync::Arc::clone(&vault); + let barrier = std::sync::Arc::clone(&barrier); + std::thread::spawn(move || { + barrier.wait(); + vault + .rotate("rotate-race", format!("v{i}").as_bytes()) + .unwrap(); + }) + }) + .collect(); + + for h in handles { + h.join().unwrap(); + } + + let history = vault.history("rotate-race").unwrap(); + assert_eq!( + history.metadata.rotation_count, THREADS as u32, + "iteration {iteration}: every concurrent rotation must be \ + counted with no lost update, got {} of {THREADS}", + history.metadata.rotation_count + ); + assert_eq!( + history + .events + .iter() + .filter(|e| e.kind == HistoryEventKind::Rotated) + .count(), + THREADS, + "iteration {iteration}: every concurrent rotation must append \ + its own history event" + ); + } +} From 598e44df0cc52e62e0d515b400dc9c5df2eec062 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:13:16 -0500 Subject: [PATCH 2/4] fix(kryphos): reject empty vault passphrases, bind ciphertext to entry identity, serialize mutations Vault::create now rejects an empty passphrase before touching the filesystem (mirrored at the CLI's confirmation boundary), closing the zero-entropy-vault hole from a double-Enter. Entry ciphertext is now authenticated against a canonical AEAD associated-data binding (vault salt + entry name + credential type + envelope version), so a relocated or independently-edited entry fails authentication instead of decrypting into the wrong slot; the vault format version bumps accordingly, since a v1 vault's unbound ciphertext cannot be transparently reread under the new binding. add/remove/rotate/revoke now serialize their check-then-write region behind an in-process mutex, addressing both the named duplicate-entry/lost-rotation races and the tamper-log corruption under concurrent mutation that the prior commit's red run surfaced. Refs #287, #283, #214 --- crates/akroasis/src/vault/mod.rs | 75 +++++++- crates/kryphos/src/crypto.rs | 184 ++++++++++++++++--- crates/kryphos/src/storage.rs | 170 ++++++++++++++--- crates/kryphos/src/storage_security_tests.rs | 11 ++ crates/kryphos/src/vault.rs | 17 +- 5 files changed, 397 insertions(+), 60 deletions(-) diff --git a/crates/akroasis/src/vault/mod.rs b/crates/akroasis/src/vault/mod.rs index 498098a..cd00e2a 100644 --- a/crates/akroasis/src/vault/mod.rs +++ b/crates/akroasis/src/vault/mod.rs @@ -87,6 +87,10 @@ pub enum VaultCliError { #[snafu(display("passphrases do not match"))] PassphraseMismatch, + /// The passphrase was empty. + #[snafu(display("passphrase must not be empty — please try again"))] + PassphraseEmpty, + /// The user cancelled the operation. #[snafu(display("operation cancelled"))] Cancelled, @@ -125,14 +129,43 @@ fn read_passphrase(prompt: &str) -> Result { rpassword::prompt_password(prompt).context(PassphraseInputSnafu) } +/// Validates a passphrase confirmation: the two entries must match, and the +/// result must not be empty. +/// +/// Split out as a pure function so this validation is unit-testable +/// directly — `rpassword::prompt_password` reads the real terminal with no +/// injection seam, so `read_passphrase_confirmed` itself cannot be driven +/// from a test. +/// +/// # Errors +/// +/// Returns [`VaultCliError::PassphraseMismatch`] if `first != second`. +/// Returns [`VaultCliError::PassphraseEmpty`] if the matching value is empty +/// — including two empty entries, which match each other and would +/// otherwise pass the check above silently (forkwright/akroasis#287). +fn confirm_passphrase(first: &str, second: &str) -> Result<(), VaultCliError> { + if first != second { + return PassphraseMismatchSnafu.fail(); + } + + if first.is_empty() { + return PassphraseEmptySnafu.fail(); + } + + Ok(()) +} + /// Reads and confirms a new passphrase (double entry). +/// +/// Rejects an empty passphrase here, at the interactive boundary, so a +/// double-Enter fails immediately with a clear retry message rather than +/// silently succeeding and relying on `Vault::create`'s own rejection to +/// surface as a less specific downstream error (forkwright/akroasis#287). fn read_passphrase_confirmed(prompt: &str) -> Result { let first = read_passphrase(prompt)?; let second = read_passphrase("Confirm passphrase: ")?; - if first != second { - return PassphraseMismatchSnafu.fail(); - } + confirm_passphrase(&first, &second)?; Ok(first) } @@ -549,6 +582,42 @@ mod tests { assert!(parse_credential_type("invalid").is_err()); } + // ----------------------------------------------------------------- + // Passphrase confirmation (akroasis#287) + // ----------------------------------------------------------------- + + #[test] + fn confirm_passphrase_rejects_two_empty_entries() { + // The double-Enter case the issue names: two empty entries MATCH + // each other, so the mismatch check alone would let this through. + let result = confirm_passphrase("", ""); + assert!( + matches!(result, Err(VaultCliError::PassphraseEmpty)), + "two empty passphrase entries must be refused, got {result:?}" + ); + } + + #[test] + fn confirm_passphrase_rejects_mismatched_entries() { + let result = confirm_passphrase("first-entry", "second-entry"); + assert!( + matches!(result, Err(VaultCliError::PassphraseMismatch)), + "mismatched entries must be refused, got {result:?}" + ); + } + + #[test] + fn confirm_passphrase_accepts_matching_nonempty_entries() { + let result = confirm_passphrase( + "correct horse battery staple", + "correct horse battery staple", + ); + assert!( + result.is_ok(), + "matching non-empty entries must be accepted, got {result:?}" + ); + } + // ----------------------------------------------------------------- // Integration tests with temp vault // ----------------------------------------------------------------- diff --git a/crates/kryphos/src/crypto.rs b/crates/kryphos/src/crypto.rs index d111411..1e202b0 100644 --- a/crates/kryphos/src/crypto.rs +++ b/crates/kryphos/src/crypto.rs @@ -3,16 +3,27 @@ //! Argon2id key derivation and ChaCha20-Poly1305 authenticated encryption. use chacha20poly1305::ChaCha20Poly1305; -use chacha20poly1305::aead::{Aead, AeadCore, KeyInit}; +use chacha20poly1305::aead::{Aead, AeadCore, KeyInit, Payload}; use rand_core::{OsRng, RngCore}; -use crate::error::CryptoError; +use snafu::ResultExt; + +use crate::error::{CryptoError, SerializationSnafu, VaultError}; use crate::key::VaultKey; -use crate::vault::NONCE_LEN; +use crate::vault::{CredentialType, NONCE_LEN}; /// Salt length for Argon2id key derivation (256 bits). pub const SALT_LEN: usize = 32; +/// Format version of the per-entry AEAD associated-data binding built by +/// [`entry_aad`]. +/// +/// Distinct from [`crate::vault::VAULT_VERSION`] (the vault header/on-disk +/// format): this versions only the identity binding baked into each +/// entry's ciphertext (forkwright/akroasis#283), so a future change to the +/// binding scheme does not force every unrelated header field to bump. +pub(crate) const ENTRY_ENVELOPE_VERSION: u8 = 1; + /// Argon2id memory cost: 64 MiB. const KDF_M_COST: u32 = 65_536; @@ -62,22 +73,31 @@ pub fn derive_key(passphrase: &[u8], salt: &[u8]) -> VaultKey { /// Encrypts plaintext with ChaCha20-Poly1305. /// +/// `aad` is authenticated but not encrypted or stored in the output — the +/// caller must supply the identical bytes to [`decrypt`], or authentication +/// fails. Pass `b""` when there is nothing to bind. +/// /// Returns `nonce || ciphertext || tag` (12 + `plaintext.len()` + 16 bytes). /// The nonce is randomly generated per call. /// /// # Errors /// /// Returns [`CryptoError::EncryptionFailed`] if the AEAD operation fails. -pub fn encrypt(key: &VaultKey, plaintext: &[u8]) -> Result, CryptoError> { +pub fn encrypt(key: &VaultKey, plaintext: &[u8], aad: &[u8]) -> Result, CryptoError> { let cipher = ChaCha20Poly1305::new(key.as_bytes().into()); let nonce = ChaCha20Poly1305::generate_nonce(&mut OsRng); - let ciphertext = - cipher - .encrypt(&nonce, plaintext) - .map_err(|_| CryptoError::EncryptionFailed { - reason: String::from("ChaCha20-Poly1305 encryption failed"), - })?; + let ciphertext = cipher + .encrypt( + &nonce, + Payload { + msg: plaintext, + aad, + }, + ) + .map_err(|_| CryptoError::EncryptionFailed { + reason: String::from("ChaCha20-Poly1305 encryption failed"), + })?; let mut output = Vec::with_capacity(NONCE_LEN + ciphertext.len()); output.extend_from_slice(&nonce); @@ -87,14 +107,22 @@ pub fn encrypt(key: &VaultKey, plaintext: &[u8]) -> Result, CryptoError> /// Decrypts ciphertext produced by [`encrypt`]. /// +/// `aad` must be byte-identical to the value passed to the original +/// [`encrypt`] call. A mismatch — a different entry's binding, a tampered +/// bound field, or simply the wrong bytes — fails authentication exactly +/// like a wrong key or a corrupted ciphertext; the caller cannot distinguish +/// which. This is the property that binds a ciphertext to its identity +/// rather than to the key alone. +/// /// Expects the input format `nonce (12 bytes) || ciphertext || tag (16 bytes)`. /// /// # Errors /// /// Returns [`CryptoError::InvalidNonceLength`] if the input is too short. -/// Returns [`CryptoError::DecryptionFailed`] if the key is wrong or the -/// ciphertext was tampered with. -pub fn decrypt(key: &VaultKey, ciphertext: &[u8]) -> Result, CryptoError> { +/// Returns [`CryptoError::DecryptionFailed`] if the key is wrong, `aad` +/// does not match what was used to encrypt, or the ciphertext was +/// tampered with. +pub fn decrypt(key: &VaultKey, ciphertext: &[u8], aad: &[u8]) -> Result, CryptoError> { if ciphertext.len() < NONCE_LEN { return Err(CryptoError::InvalidNonceLength { expected: NONCE_LEN, @@ -107,10 +135,65 @@ pub fn decrypt(key: &VaultKey, ciphertext: &[u8]) -> Result, CryptoError let cipher = ChaCha20Poly1305::new(key.as_bytes().into()); cipher - .decrypt(nonce, encrypted) + .decrypt( + nonce, + Payload { + msg: encrypted, + aad, + }, + ) .map_err(|_| CryptoError::DecryptionFailed) } +/// Builds the AEAD associated data binding a vault entry's ciphertext to +/// its identity: this vault instance, the entry's name (its fjall key), +/// its declared credential type, and the envelope version. +/// +/// Verified on every [`decrypt`] call site alongside the ciphertext itself +/// (forkwright/akroasis#283) — moving a valid ciphertext beneath a +/// different name, or editing its stored `credential_type` / +/// `envelope_version` independently of the secret, changes this binding +/// and fails authentication instead of decrypting into the wrong slot. +/// +/// # Errors +/// +/// Returns [`VaultError::Serialization`] if `credential_type` cannot be +/// encoded — `CredentialType` derives `Serialize` over plain data, so this +/// does not fail in practice. +/// +/// INVARIANT: every variable-length field is 4-byte-length-prefixed before +/// concatenation. Without this, e.g. `(name="ab", type="c")` and +/// `(name="a", type="bc")` would produce identical AAD bytes, letting a +/// relocated ciphertext smuggle a different name/type split through +/// authentication. +pub(crate) fn entry_aad( + vault_salt: &[u8], + name: &str, + credential_type: &CredentialType, + envelope_version: u8, +) -> Result, VaultError> { + let type_bytes = serde_json::to_vec(credential_type).context(SerializationSnafu)?; + + let mut aad = + Vec::with_capacity(1 + 4 + vault_salt.len() + 4 + name.len() + 4 + type_bytes.len()); + aad.push(envelope_version); + aad.extend_from_slice( + &u32::try_from(vault_salt.len()) + .unwrap_or(u32::MAX) + .to_be_bytes(), + ); + aad.extend_from_slice(vault_salt); + aad.extend_from_slice(&u32::try_from(name.len()).unwrap_or(u32::MAX).to_be_bytes()); + aad.extend_from_slice(name.as_bytes()); + aad.extend_from_slice( + &u32::try_from(type_bytes.len()) + .unwrap_or(u32::MAX) + .to_be_bytes(), + ); + aad.extend_from_slice(&type_bytes); + Ok(aad) +} + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "test assertions use unwrap for clarity")] #[expect( @@ -168,8 +251,8 @@ mod tests { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); let plaintext = b"secret vault entry data"; - let ciphertext = encrypt(&key, plaintext).unwrap(); - let decrypted = decrypt(&key, &ciphertext).unwrap(); + let ciphertext = encrypt(&key, plaintext, b"").unwrap(); + let decrypted = decrypt(&key, &ciphertext, b"").unwrap(); assert_eq!( decrypted, plaintext, @@ -181,8 +264,8 @@ mod tests { fn encrypt_decrypt_empty_plaintext() { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); - let ciphertext = encrypt(&key, b"").unwrap(); - let decrypted = decrypt(&key, &ciphertext).unwrap(); + let ciphertext = encrypt(&key, b"", b"").unwrap(); + let decrypted = decrypt(&key, &ciphertext, b"").unwrap(); assert!( decrypted.is_empty(), @@ -195,8 +278,8 @@ mod tests { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); let plaintext = vec![0xAB; 1_000_000]; - let ciphertext = encrypt(&key, &plaintext).unwrap(); - let decrypted = decrypt(&key, &ciphertext).unwrap(); + let ciphertext = encrypt(&key, &plaintext, b"").unwrap(); + let decrypted = decrypt(&key, &ciphertext, b"").unwrap(); assert_eq!( decrypted, plaintext, @@ -209,8 +292,8 @@ mod tests { let key1 = derive_key(b"correct-passphrase", &[0x42; SALT_LEN]); let key2 = derive_key(b"wrong-passphrase", &[0x42; SALT_LEN]); - let ciphertext = encrypt(&key1, b"secret data").unwrap(); - let result = decrypt(&key2, &ciphertext); + let ciphertext = encrypt(&key1, b"secret data", b"").unwrap(); + let result = decrypt(&key2, &ciphertext, b""); assert!( result.is_err(), @@ -222,27 +305,72 @@ mod tests { fn tampered_ciphertext_returns_error() { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); - let mut ciphertext = encrypt(&key, b"secret data").unwrap(); + let mut ciphertext = encrypt(&key, b"secret data", b"").unwrap(); // WHY: Flip a byte in the encrypted portion (after the nonce) to // simulate tampering. The authentication tag check must reject this. let last = ciphertext.len() - 1; ciphertext[last] ^= 0xFF; - let result = decrypt(&key, &ciphertext); + let result = decrypt(&key, &ciphertext, b""); assert!( result.is_err(), "tampered ciphertext must return CryptoError" ); } + #[test] + fn encrypt_decrypt_round_trip_with_associated_data() { + let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); + let plaintext = b"secret vault entry data"; + let aad = b"entry-identity-binding"; + + let ciphertext = encrypt(&key, plaintext, aad).unwrap(); + let decrypted = decrypt(&key, &ciphertext, aad).unwrap(); + + assert_eq!( + decrypted, plaintext, + "decrypted output must match original plaintext under matching AAD" + ); + } + + #[test] + fn decrypt_with_mismatched_associated_data_fails() { + let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); + let plaintext = b"secret vault entry data"; + + let ciphertext = encrypt(&key, plaintext, b"entry-a").unwrap(); + let result = decrypt(&key, &ciphertext, b"entry-b"); + + assert!( + result.is_err(), + "decryption with mismatched associated data must fail — this is the \ + AEAD property forkwright/akroasis#283 relies on to bind ciphertext \ + to its entry identity" + ); + } + + #[test] + fn decrypt_with_empty_aad_against_bound_ciphertext_fails() { + let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); + + let ciphertext = encrypt(&key, b"secret data", b"entry-a").unwrap(); + let result = decrypt(&key, &ciphertext, b""); + + assert!( + result.is_err(), + "omitting AAD on decrypt must not silently succeed against a \ + ciphertext that was bound at encrypt time" + ); + } + #[test] fn nonce_is_random_per_encryption() { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); let plaintext = b"identical plaintext"; - let ct1 = encrypt(&key, plaintext).unwrap(); - let ct2 = encrypt(&key, plaintext).unwrap(); + let ct1 = encrypt(&key, plaintext, b"").unwrap(); + let ct2 = encrypt(&key, plaintext, b"").unwrap(); assert_ne!( ct1, ct2, @@ -262,7 +390,7 @@ mod tests { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); let plaintext = b"hello"; - let ciphertext = encrypt(&key, plaintext).unwrap(); + let ciphertext = encrypt(&key, plaintext, b"").unwrap(); // nonce (12) + plaintext (5) + tag (16) = 33 assert_eq!( @@ -276,7 +404,7 @@ mod tests { fn decrypt_rejects_too_short_input() { let key = derive_key(b"test-passphrase", &[0x42; SALT_LEN]); - let result = decrypt(&key, &[0u8; 5]); + let result = decrypt(&key, &[0u8; 5], b""); assert!( result.is_err(), "input shorter than nonce length must be rejected" diff --git a/crates/kryphos/src/storage.rs b/crates/kryphos/src/storage.rs index 41dd2ff..c8fe0fd 100644 --- a/crates/kryphos/src/storage.rs +++ b/crates/kryphos/src/storage.rs @@ -12,10 +12,11 @@ use serde::{Deserialize, Serialize}; use snafu::ResultExt; use zeroize::Zeroizing; -use crate::crypto::{self, decrypt, encrypt}; +use crate::crypto::{self, ENTRY_ENVELOPE_VERSION, decrypt, encrypt, entry_aad}; use crate::error::{ - AlreadyExistsSnafu, EntryCryptoSnafu, EntryNotDeletableSnafu, EntryRevokedSnafu, IoSnafu, - NotInitializedSnafu, SerializationSnafu, TamperLogSnafu, VaultError, WrongPassphraseSnafu, + AlreadyExistsSnafu, EmptyPassphraseSnafu, EntryCryptoSnafu, EntryNotDeletableSnafu, + EntryRevokedSnafu, IoSnafu, NotInitializedSnafu, SerializationSnafu, TamperLogSnafu, + VaultError, WrongPassphraseSnafu, }; use crate::key::VaultKey; use crate::vault::{ @@ -64,14 +65,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. +/// `encrypted_secret` and `encrypted_metadata` 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. +/// +/// `envelope_version` is the one field that stays plaintext: it has to be +/// readable before `encrypted_secret` is decrypted, since it selects which +/// AAD that ciphertext was bound under (forkwright/akroasis#283, see +/// [`entry_aad`]). It carries no secret information itself. #[derive(Debug, Serialize, Deserialize)] struct StoredEntry { + /// AEAD associated-data envelope version this entry's + /// `encrypted_secret` was bound under. Bound into the AAD itself + /// ([`entry_aad`]), so a value tampered independently of the ciphertext + /// fails authentication rather than silently taking effect. + #[serde(default)] + envelope_version: u8, encrypted_secret: Vec, encrypted_metadata: Vec, } @@ -165,13 +177,45 @@ pub struct EntryHistory { /// Encrypted credential vault backed by fjall. /// -/// Each entry is individually encrypted with ChaCha20-Poly1305 using a -/// key derived from the user's passphrase via Argon2id. The vault -/// directory is advisory-locked to prevent concurrent access. +/// Each entry is individually encrypted with ChaCha20-Poly1305 using a key +/// derived from the user's passphrase via Argon2id, with `encrypted_secret`'s +/// AEAD associated data binding it to its entry identity +/// (forkwright/akroasis#283) and `encrypted_metadata` protecting the name, +/// type, status, and history at rest (forkwright/akroasis#215) under a fjall +/// record key that is itself a keyed hash of the name, not the name itself. +/// The vault directory is advisory-locked to prevent concurrent access from +/// OTHER PROCESSES; `write_lock` is the separate in-process guard that +/// serializes this handle's own mutating calls (forkwright/akroasis#214) — +/// see its field doc. pub struct Vault { db: fjall::Database, keyspace: fjall::Keyspace, key: VaultKey, + /// Copy of the header's salt, used only as this vault instance's + /// identity component in [`entry_aad`] — never as key material. + salt: Vec, + /// Serializes `add`/`remove`/`rotate`/`revoke` against each other + /// within THIS process. + /// + /// INVARIANT: held across the full duplicate-check-then-write (`add`) + /// or read-modify-write (`rotate`/`revoke`) region of each of those + /// methods, never released partway through. `Vault` is `Send + Sync` + /// (fjall handles + a fixed key + a lock file), so a multithreaded + /// caller holding `Arc` can otherwise interleave two calls + /// between the duplicate check and the write, both observing the + /// pre-write state — forkwright/akroasis#214. The directory lock + /// above does not help here: it guards a different boundary + /// (concurrent processes), not concurrent threads inside one. + /// + /// WHY recover from poison rather than propagate it: this crate denies + /// `panic`/`unwrap_used`/`expect_used`, so a panic while the guard is + /// held can only come from an allocation failure or similar, not from + /// a `.unwrap()` in this code path. Refusing every subsequent vault + /// operation over a panic that was never this mutex's own fault would + /// turn one incident into a stuck vault; the recovered guard still + /// serializes correctly because fjall's own per-key operations stay + /// individually atomic regardless. + write_lock: std::sync::Mutex<()>, _lock: File, path: PathBuf, } @@ -187,10 +231,20 @@ impl Vault { /// /// # Errors /// + /// Returns [`VaultError::EmptyPassphrase`] if `passphrase` is empty. /// Returns [`VaultError::AlreadyExists`] if the path already exists. /// Returns [`VaultError::Io`] on filesystem errors. /// Returns [`VaultError::StorageBackend`] if fjall initialization fails. pub fn create(path: impl AsRef, passphrase: &[u8]) -> Result { + // WHY checked first, before any filesystem access: an empty + // passphrase carries no entropy, so `Vault::create(path, b"")` must + // reject at the library boundary rather than only in the CLI's + // interactive confirmation (forkwright/akroasis#287) — and it must + // leave no filesystem state behind, per the issue's Done-when. + if passphrase.is_empty() { + return EmptyPassphraseSnafu.fail(); + } + let path = path.as_ref(); if path.exists() { return AlreadyExistsSnafu { path }.fail(); @@ -202,7 +256,7 @@ impl Vault { let salt = crypto::generate_salt(); let key = crypto::derive_key(passphrase, &salt); - let key_check = encrypt(&key, KEY_CHECK_PLAINTEXT).context(EntryCryptoSnafu)?; + let key_check = encrypt(&key, KEY_CHECK_PLAINTEXT, b"").context(EntryCryptoSnafu)?; let header = StoredHeader { version: VAULT_VERSION, @@ -224,6 +278,8 @@ impl Vault { db, keyspace, key, + salt: salt.to_vec(), + write_lock: std::sync::Mutex::new(()), _lock: lock, path: path.to_path_buf(), }) @@ -278,7 +334,7 @@ impl Vault { let key = crypto::derive_key(passphrase, &header.salt); let plaintext = - decrypt(&key, &header.key_check).map_err(|_| VaultError::WrongPassphrase)?; + decrypt(&key, &header.key_check, b"").map_err(|_| VaultError::WrongPassphrase)?; if plaintext != KEY_CHECK_PLAINTEXT { return WrongPassphraseSnafu.fail(); } @@ -289,6 +345,8 @@ impl Vault { db, keyspace, key, + salt: header.salt, + write_lock: std::sync::Mutex::new(()), _lock: lock, path: path.to_path_buf(), }) @@ -308,15 +366,25 @@ impl Vault { credential_type: CredentialType, secret: &[u8], ) -> Result<(), VaultError> { - let key = self.lookup_key(name); + // INVARIANT: held across the duplicate check AND the write below. + // See `write_lock`'s field doc — this is what makes two concurrent + // `add` calls for the same name resolve to exactly one winner + // (forkwright/akroasis#214) instead of both observing `None` and + // both inserting. + let _write_guard = self + .write_lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + 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(), }); } - let encrypted_secret = encrypt(&self.key, secret).context(EntryCryptoSnafu)?; + let aad = entry_aad(&self.salt, name, &credential_type, ENTRY_ENVELOPE_VERSION)?; + let encrypted_secret = encrypt(&self.key, secret, &aad).context(EntryCryptoSnafu)?; let now = Timestamp::now(); let record = EntryMetadataRecord { @@ -338,6 +406,7 @@ impl Vault { let encrypted_metadata = self.encrypt_metadata(&record)?; let entry = StoredEntry { + envelope_version: ENTRY_ENVELOPE_VERSION, encrypted_secret, encrypted_metadata, }; @@ -374,12 +443,26 @@ impl Vault { return EntryRevokedSnafu { name }.fail(); } + // WHY: rebuilt from the CALLER's `name` plus this entry's OWN + // decrypted `credential_type` (from `encrypted_metadata`) and stored + // `envelope_version`, not trusted verbatim from elsewhere. A + // ciphertext relocated from a different entry (or with its + // `envelope_version` edited independently of `encrypted_secret`) was + // bound under a different AAD at encrypt time, so it fails + // authentication here instead of decrypting into this slot + // (forkwright/akroasis#283). + let aad = entry_aad( + &self.salt, + name, + &record.credential_type, + entry.envelope_version, + )?; // 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)?); + Zeroizing::new(decrypt(&self.key, &entry.encrypted_secret, &aad).context(EntryCryptoSnafu)?); Ok(DecryptedEntry { name: record.name, @@ -426,6 +509,16 @@ 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> { + // INVARIANT: see `write_lock`'s field doc. `remove` is a + // read-modify-write too (read status, conditionally remove); without + // this, a concurrent `revoke` racing this call could both read + // `Active` before either write lands — the revoke's write would then + // resurrect a name this call just believed it had deleted. + let _write_guard = self + .write_lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let key = self.lookup_key(name); let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { @@ -460,6 +553,16 @@ 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> { + // INVARIANT: see `write_lock`'s field doc. Held across this whole + // read-modify-write so two concurrent `rotate` calls serialize + // instead of each reading the same starting `rotation_count` / + // history and one's increment/event silently overwriting the + // other's (forkwright/akroasis#214). + let _write_guard = self + .write_lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let key = self.lookup_key(name); let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { @@ -474,7 +577,17 @@ impl Vault { return EntryRevokedSnafu { name }.fail(); } - let encrypted_secret = encrypt(&self.key, new_secret).context(EntryCryptoSnafu)?; + // WHY stamp the current envelope version on every rewrite, not just + // preserve whatever was there: `rotate` re-encrypts the secret + // anyway, so it is the natural opportunistic upgrade point for any + // entry still carrying a pre-#283 envelope. + let aad = entry_aad( + &self.salt, + name, + &record.credential_type, + ENTRY_ENVELOPE_VERSION, + )?; + let encrypted_secret = encrypt(&self.key, new_secret, &aad).context(EntryCryptoSnafu)?; let now = Timestamp::now(); record.metadata.rotated_at = Some(now); @@ -486,6 +599,7 @@ impl Vault { let encrypted_metadata = self.encrypt_metadata(&record)?; let entry = StoredEntry { + envelope_version: ENTRY_ENVELOPE_VERSION, encrypted_secret, encrypted_metadata, }; @@ -510,6 +624,16 @@ 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> { + // INVARIANT: see `write_lock`'s field doc. Same read-modify-write + // shape as `rotate` — without this, two concurrent `revoke` calls + // (or a `revoke` racing a `rotate`) can both read `Active` and one + // write silently loses the other's status/history change + // (forkwright/akroasis#214). + let _write_guard = self + .write_lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let key = self.lookup_key(name); let raw = self.keyspace.get(key).map_err(fjall_err)?.ok_or_else(|| { VaultError::EntryNotFound { @@ -534,6 +658,7 @@ impl Vault { let encrypted_metadata = self.encrypt_metadata(&record)?; let entry = StoredEntry { + envelope_version: entry.envelope_version, encrypted_secret: entry.encrypted_secret, encrypted_metadata, }; @@ -621,7 +746,7 @@ impl Vault { /// 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)?; + decrypt(&self.key, &entry.encrypted_metadata, b"").context(EntryCryptoSnafu)?; serde_json::from_slice(&metadata_bytes).context(SerializationSnafu) } @@ -629,7 +754,7 @@ impl Vault { /// `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) + encrypt(&self.key, &metadata_bytes, b"").context(EntryCryptoSnafu) } fn append_vault_audit(&self, name: &str, operation: &str) -> Result<(), VaultError> { @@ -748,11 +873,6 @@ fn fjall_err(e: impl std::fmt::Display) -> VaultError { #[path = "storage_tests.rs"] mod tests; -// WHY this module exists as an isolated commit before the fix lands: these -// tests pin the exact defects in forkwright/akroasis#287, #283, and #214. -// Watched red against this commit (pre-fix), watched green against the -// commit that follows it (the actual fix) — see the PR body's negative -// fixture section. #[cfg(test)] #[expect( clippy::unwrap_used, diff --git a/crates/kryphos/src/storage_security_tests.rs b/crates/kryphos/src/storage_security_tests.rs index bf90d36..4b4f486 100644 --- a/crates/kryphos/src/storage_security_tests.rs +++ b/crates/kryphos/src/storage_security_tests.rs @@ -148,6 +148,17 @@ fn mutated_envelope_version_field_fails_authentication() { // ----------------------------------------------------------------- #[test] +// WHY expect not allow, and why the collect is NOT needless despite the +// lint: `.collect()` into `handles` is what forces every `thread::spawn` +// to run before any `.join()` starts. Taking clippy's suggested fix — +// chaining spawn and join in one lazy iterator — would join thread 0 +// before thread 1 ever spawns, serializing the race this test exists to +// observe. A test that always passes because it never actually +// contends is decoration, not a fixture. +#[expect( + clippy::needless_collect, + reason = "eager collect forces every thread to spawn (and reach the barrier) before any is joined — required for a real race, not needless" +)] fn concurrent_add_same_name_yields_one_winner_and_duplicate_losers() { const THREADS: usize = 16; const ITERATIONS: usize = 10; diff --git a/crates/kryphos/src/vault.rs b/crates/kryphos/src/vault.rs index 6ec3262..08d6d89 100644 --- a/crates/kryphos/src/vault.rs +++ b/crates/kryphos/src/vault.rs @@ -22,10 +22,19 @@ pub const NONCE_LEN: usize = 12; /// 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. +/// v2 folds together two independent changes that landed close together: +/// names, types, tags, status, and history moved from plaintext fields into +/// `encrypted_metadata`, the fjall record key changed from the plaintext +/// name to a keyed-hash lookup key (forkwright/akroasis#215), AND entry +/// ciphertexts are now bound to their identity via AEAD associated data +/// (forkwright/akroasis#283). A v1 store has neither the fields, the keys, +/// nor the AAD binding v2 code expects; its migration path is +/// re-initialization: read out entries with the prior release, create a +/// fresh vault, re-add them. `envelope_version` (a per-ENTRY field, distinct +/// from this header-level version — see +/// [`crate::crypto::ENTRY_ENVELOPE_VERSION`]) is the finer-grained axis that +/// DOES support transparent migration, for entries written under a v2 +/// header before the AAD binding existed. pub const VAULT_VERSION: u32 = 2; /// The kind of credential stored in a [`VaultEntry`]. From 09dfd94ecaaccbff7ab03951b067e00bfc901e19 Mon Sep 17 00:00:00 2001 From: forkwright Date: Mon, 17 Aug 2026 10:35:00 -0500 Subject: [PATCH 3/4] test(kryphos): pin PR #383 review findings as failing before the fix Adds the negative-case fixtures for the adversarial review of #383: a v1 vault opening/decrypting a pre-#283 entry (currently rejected outright, no migration path), a signing-key seal accepting a ciphertext sealed for an unrelated purpose under the same key (no domain separation), and the AAD length-prefix guard clamping instead of erroring on overflow. Also pins that a downgraded envelope_version still fails authentication (already true, included as a self-check baseline). None of the underlying defects are fixed yet; the next commit fixes all four. This commit is expected red. --- crates/kryphos/src/crypto.rs | 63 +++++-- crates/kryphos/src/error.rs | 19 ++ crates/kryphos/src/storage_security_tests.rs | 177 +++++++++++++++++++ crates/kryphos/src/vault.rs | 28 +++ 4 files changed, 276 insertions(+), 11 deletions(-) diff --git a/crates/kryphos/src/crypto.rs b/crates/kryphos/src/crypto.rs index 1e202b0..7407da3 100644 --- a/crates/kryphos/src/crypto.rs +++ b/crates/kryphos/src/crypto.rs @@ -177,23 +177,31 @@ pub(crate) fn entry_aad( let mut aad = Vec::with_capacity(1 + 4 + vault_salt.len() + 4 + name.len() + 4 + type_bytes.len()); aad.push(envelope_version); - aad.extend_from_slice( - &u32::try_from(vault_salt.len()) - .unwrap_or(u32::MAX) - .to_be_bytes(), - ); + aad.extend_from_slice(&checked_len_prefix("vault_salt", vault_salt.len())?); aad.extend_from_slice(vault_salt); - aad.extend_from_slice(&u32::try_from(name.len()).unwrap_or(u32::MAX).to_be_bytes()); + aad.extend_from_slice(&checked_len_prefix("name", name.len())?); aad.extend_from_slice(name.as_bytes()); - aad.extend_from_slice( - &u32::try_from(type_bytes.len()) - .unwrap_or(u32::MAX) - .to_be_bytes(), - ); + aad.extend_from_slice(&checked_len_prefix("credential_type", type_bytes.len())?); aad.extend_from_slice(&type_bytes); Ok(aad) } +/// Encodes `len` as a big-endian 4-byte length prefix for [`entry_aad`]. +/// +/// # Errors +/// +/// Returns [`VaultError::FieldTooLarge`] if `len` exceeds `u32::MAX`. +// PIN(akroasis#383 review): behaviorally identical to the inline +// `.unwrap_or(u32::MAX)` this replaces — still clamps rather than errors. +// This is the exact defect the follow-up commit fixes. +#[expect( + clippy::unnecessary_wraps, + reason = "PIN akroasis#383 review: this pinned pre-fix body is intentionally infallible (clamps instead of erroring); the fix commit adds the Err path and this attribute goes with it" +)] +fn checked_len_prefix(_field: &'static str, len: usize) -> Result<[u8; 4], VaultError> { + Ok(u32::try_from(len).unwrap_or(u32::MAX).to_be_bytes()) +} + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "test assertions use unwrap for clarity")] #[expect( @@ -411,6 +419,39 @@ mod tests { ); } + // ----------------------------------------------------------------- + // AAD length-prefix guard (checked_len_prefix) + // ----------------------------------------------------------------- + + #[test] + fn checked_len_prefix_accepts_u32_max() { + let result = checked_len_prefix("field", u32::MAX as usize); + assert_eq!( + result.unwrap(), + u32::MAX.to_be_bytes(), + "the largest representable length must encode, not error" + ); + } + + #[test] + fn checked_len_prefix_rejects_one_past_u32_max() { + // WHY exercised via the helper directly rather than a real + // >4GiB-length `entry_aad` call: allocating gigabytes in a unit test + // is impractical, and this arithmetic boundary needs no allocation + // to exercise — `checked_len_prefix` IS the shipped guard `entry_aad` + // calls, not a re-implementation of it. + let result = checked_len_prefix("field", u32::MAX as usize + 1); + assert!( + matches!( + result, + Err(VaultError::FieldTooLarge { field: "field", len }) if len == u32::MAX as usize + 1 + ), + "a length that cannot be represented in 4 bytes must error \ + rather than silently clamp to u32::MAX (which would collide \ + two different lengths onto the same AAD prefix), got {result:?}" + ); + } + #[test] fn generate_salt_returns_32_bytes() { let salt = generate_salt(); diff --git a/crates/kryphos/src/error.rs b/crates/kryphos/src/error.rs index 6df9cf0..5d69804 100644 --- a/crates/kryphos/src/error.rs +++ b/crates/kryphos/src/error.rs @@ -111,6 +111,25 @@ pub enum VaultError { /// Underlying tamper-log failure. source: koinon::TamperLogError, }, + + /// A field passed to [`crate::crypto::entry_aad`] is too long to encode + /// under its 4-byte length prefix. + /// + /// INVARIANT guard, not a realistic runtime case: every current caller + /// passes a fixed-size salt, a vault entry name, or a JSON-serialized + /// `CredentialType`, none of which approach `u32::MAX` bytes. Erroring + /// here is what keeps the AAD's length-prefix encoding canonical — + /// silently truncating the length instead would let two different + /// (field, length) pairs collide on the same encoded bytes. + #[snafu(display( + "AAD field '{field}' is {len} bytes, which exceeds the u32 length-prefix limit" + ))] + FieldTooLarge { + /// Which field overflowed the length prefix. + field: &'static str, + /// The field's actual byte length. + len: usize, + }, } /// Errors from key generation, derivation, or loading. diff --git a/crates/kryphos/src/storage_security_tests.rs b/crates/kryphos/src/storage_security_tests.rs index 4b4f486..c3654e4 100644 --- a/crates/kryphos/src/storage_security_tests.rs +++ b/crates/kryphos/src/storage_security_tests.rs @@ -143,6 +143,183 @@ fn mutated_envelope_version_field_fails_authentication() { ); } +#[test] +fn envelope_version_downgraded_to_legacy_fails_authentication() { + // WHY: the migration branch (LEGACY_ENVELOPE_VERSION => empty AAD) is + // itself a new attack surface if it can be reached for an entry that was + // NOT actually sealed under empty AAD. Tamper a genuinely #283-bound + // entry's `envelope_version` DOWN to the legacy sentinel and confirm + // `get` still fails — its ciphertext was authenticated under a non-empty + // AAD at encrypt time, so decrypting with `b""` cannot succeed either. + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("aad-downgrade-tamper-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add("entry", CredentialType::ApiKey, b"secret") + .unwrap(); + + let raw = vault.keyspace.get("entry").unwrap().unwrap(); + let mut value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); + assert_eq!( + value["envelope_version"], + serde_json::json!(1), + "precondition: a freshly-added entry is bound under the current envelope" + ); + value["envelope_version"] = serde_json::json!(0); + let tampered = serde_json::to_vec(&value).unwrap(); + vault.keyspace.insert("entry", tampered).unwrap(); + + let result = vault.get("entry"); + assert!( + result.is_err(), + "downgrading a #283-bound entry's envelope_version to the legacy \ + sentinel must not let it decrypt under empty AAD, got {result:?}" + ); +} + +// ----------------------------------------------------------------- +// Legacy vault migration (akroasis#283 Desired Correction) +// ----------------------------------------------------------------- + +#[test] +fn legacy_v1_vault_opens_and_decrypts_pre_283_entries() { + // WHY hand-assembled rather than produced by calling `Vault::create`: + // no code in this binary writes a v1 header or an envelope_version-0 + // entry anymore, so a pre-#283 vault can only be reconstructed by + // replicating what the OLD code actually wrote — a header with + // `version: 1` and an entry whose secret was sealed with + // `crypto::encrypt(key, secret, b"")` (empty AAD; see the RFC 8439 test + // vector's postfix-tag format that `encrypt`/`decrypt` still implement + // unchanged). This uses the SAME production `crypto`/`fjall` primitives + // `Vault::create`/`add` use internally, not a reimplementation. + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("legacy-v1-vault"); + + create_owner_only_dir(&vault_path).unwrap(); + let salt = crypto::generate_salt(); + let key = crypto::derive_key(TEST_PASSPHRASE, &salt); + let key_check = encrypt(&key, KEY_CHECK_PLAINTEXT, b"").unwrap(); + let header = StoredHeader { + version: 1, + salt: salt.to_vec(), + kdf_params: KdfParams::default(), + key_check, + }; + write_owner_only_file( + &vault_path.join(HEADER_FILE), + serde_json::to_string_pretty(&header).unwrap().as_bytes(), + ) + .unwrap(); + + create_owner_only_dir(&vault_path.join(DATA_DIR)).unwrap(); + let (db, keyspace) = open_fjall(&vault_path.join(DATA_DIR)).unwrap(); + + let encrypted_secret = encrypt(&key, b"legacy-secret", b"").unwrap(); + let now = Timestamp::now(); + let legacy_entry = StoredEntry { + envelope_version: 0, + credential_type: CredentialType::ApiKey, + encrypted_secret, + metadata: EntryMetadata { + created_at: now, + rotated_at: None, + revoked_at: None, + rotation_count: 0, + tags: Vec::new(), + }, + status: EntryStatus::Active, + history: vec![HistoryEvent { + timestamp: now, + kind: HistoryEventKind::Created, + }], + }; + keyspace + .insert("legacy-entry", serde_json::to_vec(&legacy_entry).unwrap()) + .unwrap(); + db.persist(fjall::PersistMode::SyncAll).unwrap(); + drop(keyspace); + drop(db); + + // The production open path: must accept a v1 header rather than + // rejecting every vault created before this PR outright. + let vault = Vault::open(&vault_path, TEST_PASSPHRASE).unwrap(); + + // The production get path: must transparently decrypt a pre-#283 + // (envelope_version 0, empty-AAD) entry with no migrate command and no + // operator round-trip through an old binary. + let decrypted = vault.get("legacy-entry").unwrap(); + assert_eq!(decrypted.secret, b"legacy-secret"); + assert_eq!(decrypted.credential_type, CredentialType::ApiKey); +} + +#[test] +fn legacy_v1_vault_rotate_opportunistically_upgrades_the_envelope() { + // A legacy entry that gets rotated must come out bound under the + // current envelope, so it stops depending on the legacy branch on every + // subsequent read. + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("legacy-v1-vault-rotate"); + + create_owner_only_dir(&vault_path).unwrap(); + let salt = crypto::generate_salt(); + let key = crypto::derive_key(TEST_PASSPHRASE, &salt); + let key_check = encrypt(&key, KEY_CHECK_PLAINTEXT, b"").unwrap(); + let header = StoredHeader { + version: 1, + salt: salt.to_vec(), + kdf_params: KdfParams::default(), + key_check, + }; + write_owner_only_file( + &vault_path.join(HEADER_FILE), + serde_json::to_string_pretty(&header).unwrap().as_bytes(), + ) + .unwrap(); + + create_owner_only_dir(&vault_path.join(DATA_DIR)).unwrap(); + let (db, keyspace) = open_fjall(&vault_path.join(DATA_DIR)).unwrap(); + let encrypted_secret = encrypt(&key, b"v0", b"").unwrap(); + let now = Timestamp::now(); + let legacy_entry = StoredEntry { + envelope_version: 0, + credential_type: CredentialType::ApiKey, + encrypted_secret, + metadata: EntryMetadata { + created_at: now, + rotated_at: None, + revoked_at: None, + rotation_count: 0, + tags: Vec::new(), + }, + status: EntryStatus::Active, + history: vec![HistoryEvent { + timestamp: now, + kind: HistoryEventKind::Created, + }], + }; + keyspace + .insert("legacy-entry", serde_json::to_vec(&legacy_entry).unwrap()) + .unwrap(); + db.persist(fjall::PersistMode::SyncAll).unwrap(); + drop(keyspace); + drop(db); + + let vault = Vault::open(&vault_path, TEST_PASSPHRASE).unwrap(); + vault.rotate("legacy-entry", b"v1").unwrap(); + + let raw = vault.keyspace.get("legacy-entry").unwrap().unwrap(); + let value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); + assert_eq!( + value["envelope_version"], + serde_json::json!(1), + "rotate must stamp the current envelope on a legacy entry it rewrites" + ); + + let decrypted = vault.get("legacy-entry").unwrap(); + assert_eq!(decrypted.secret, b"v1"); +} + // ----------------------------------------------------------------- // Concurrent mutation atomicity (akroasis#214) // ----------------------------------------------------------------- diff --git a/crates/kryphos/src/vault.rs b/crates/kryphos/src/vault.rs index 08d6d89..132715a 100644 --- a/crates/kryphos/src/vault.rs +++ b/crates/kryphos/src/vault.rs @@ -545,6 +545,34 @@ mod tests { ); } + #[test] + fn unseal_rejects_ciphertext_sealed_for_a_different_purpose() { + // WHY: proves domain separation. A ciphertext produced under the + // SAME VaultKey+nonce for an unrelated purpose (any other + // same-key/same-nonce caller — a vault entry, a header key-check, a + // future consumer) must not unseal as a signing key. Before this + // fix, seal/unseal used no AAD at all, so any same-key ciphertext of + // the right plaintext length was interchangeable — the same + // unauthenticated-context defect forkwright/akroasis#283 fixed for + // vault entries, surviving in this sibling function. + let vault_key = VaultKey::from_bytes([0x42; 32]); + let nonce = [0x01; NONCE_LEN]; + + // A foreign ciphertext: same key, same nonce, no domain-separation + // tag, 32-byte plaintext (the exact length a signing key seed is). + let cipher = ChaCha20Poly1305::new(vault_key.as_bytes().into()); + let foreign_ciphertext = cipher + .encrypt(Nonce::from_slice(&nonce), &[0x11u8; 32][..]) + .unwrap(); + + let result = unseal_signing_key(&foreign_ciphertext, &vault_key, &nonce); + assert!( + result.is_err(), + "a ciphertext sealed without the signing-key domain tag must \ + not unseal as a signing key, got {result:?}" + ); + } + #[test] fn unseal_tampered_ciphertext_fails() { let identity = InstallationIdentity::generate(); From d9cb1bd6d88ffef6ecba494e0c76efe2dbdc5689 Mon Sep 17 00:00:00 2001 From: forkwright Date: Mon, 17 Aug 2026 11:32:33 -0500 Subject: [PATCH 4/4] fix(kryphos): migrate legacy vaults transparently, domain-separate the signing-key seal, and reject oversize AAD fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #383 found three defects surviving the #283/#287/#214 fix, plus one evidence gap: - #283's Desired Correction required 'an explicit migration path for existing entries'. VAULT_VERSION was bumped 1->2 and Vault::open rejected any pre-existing (v1) vault outright with no way back in short of running an old binary by hand. The header shape is unchanged between v1 and v2 — only what an entry's ciphertext authenticates changed, and that is already tracked per-entry via envelope_version (defaulted to 0 by serde on a pre-#283 record). Vault::open now accepts MIN_SUPPORTED_VAULT_VERSION..=VAULT_VERSION, and Vault::get selects the AAD from each entry's own envelope_version: 0 decrypts under the original empty AAD, anything else goes through the full identity-bound AAD as before. rotate already opportunistically upgrades the envelope on rewrite (unchanged). A tampered downgrade (envelope_version forced to 0 on an actually-#283-bound entry) still fails authentication, since AEAD tag verification cannot pass under the wrong AAD regardless of which branch picks it. - seal_signing_key/unseal_signing_key carried the exact unauthenticated-AEAD shape #283 fixed for vault entries: no AAD at all, so any ciphertext encrypted under the same VaultKey+nonce for an unrelated purpose was interchangeable. Added a fixed domain-separation tag to both. One pre-existing test (unseal_wrong_length_plaintext_is_key_parse_error) crafted its ciphertext with the raw cipher and no AAD to isolate the length check; updated it to seal under the new domain tag so it still reaches that check instead of failing authentication first. - entry_aad's length-prefix encoding silently clamped to u32::MAX via .unwrap_or(u32::MAX) on overflow instead of erroring, which the guard's own INVARIANT comment states is unsound (two different lengths could collide onto the same encoded prefix). Extracted into checked_len_prefix, which now returns VaultError::FieldTooLarge. - The review also noted the original pin commit's 'expected red' was asserted, never CI-witnessed. This PR's own two commits close that gap: the previous commit pins all four fixtures failing (CI-observed), this one makes them pass. --- crates/kryphos/src/crypto.rs | 25 ++- crates/kryphos/src/storage.rs | 85 +++++-- crates/kryphos/src/storage_security_tests.rs | 222 ++++++++++--------- crates/kryphos/src/vault.rs | 134 +++++++++-- 4 files changed, 304 insertions(+), 162 deletions(-) diff --git a/crates/kryphos/src/crypto.rs b/crates/kryphos/src/crypto.rs index 7407da3..7826a29 100644 --- a/crates/kryphos/src/crypto.rs +++ b/crates/kryphos/src/crypto.rs @@ -160,12 +160,17 @@ pub fn decrypt(key: &VaultKey, ciphertext: &[u8], aad: &[u8]) -> Result, /// Returns [`VaultError::Serialization`] if `credential_type` cannot be /// encoded — `CredentialType` derives `Serialize` over plain data, so this /// does not fail in practice. +/// Returns [`VaultError::FieldTooLarge`] if `vault_salt`, `name`, or the +/// serialized `credential_type` exceeds `u32::MAX` bytes. /// /// INVARIANT: every variable-length field is 4-byte-length-prefixed before /// concatenation. Without this, e.g. `(name="ab", type="c")` and /// `(name="a", type="bc")` would produce identical AAD bytes, letting a /// relocated ciphertext smuggle a different name/type split through -/// authentication. +/// authentication. [`checked_len_prefix`] is what actually holds this +/// property: it ERRORS on a field too long to prefix rather than silently +/// writing a wrong-but-plausible `u32::MAX` prefix, which would itself +/// collide two different lengths onto the same encoded bytes. pub(crate) fn entry_aad( vault_salt: &[u8], name: &str, @@ -190,16 +195,14 @@ pub(crate) fn entry_aad( /// /// # Errors /// -/// Returns [`VaultError::FieldTooLarge`] if `len` exceeds `u32::MAX`. -// PIN(akroasis#383 review): behaviorally identical to the inline -// `.unwrap_or(u32::MAX)` this replaces — still clamps rather than errors. -// This is the exact defect the follow-up commit fixes. -#[expect( - clippy::unnecessary_wraps, - reason = "PIN akroasis#383 review: this pinned pre-fix body is intentionally infallible (clamps instead of erroring); the fix commit adds the Err path and this attribute goes with it" -)] -fn checked_len_prefix(_field: &'static str, len: usize) -> Result<[u8; 4], VaultError> { - Ok(u32::try_from(len).unwrap_or(u32::MAX).to_be_bytes()) +/// Returns [`VaultError::FieldTooLarge`] if `len` exceeds `u32::MAX` — a +/// value this function must reject rather than clamp, since clamping would +/// let two different lengths encode to the identical prefix (see +/// [`entry_aad`]'s INVARIANT doc). +fn checked_len_prefix(field: &'static str, len: usize) -> Result<[u8; 4], VaultError> { + u32::try_from(len) + .map(u32::to_be_bytes) + .map_err(|_| VaultError::FieldTooLarge { field, len }) } #[cfg(test)] diff --git a/crates/kryphos/src/storage.rs b/crates/kryphos/src/storage.rs index c8fe0fd..7c40b4c 100644 --- a/crates/kryphos/src/storage.rs +++ b/crates/kryphos/src/storage.rs @@ -21,9 +21,15 @@ use crate::error::{ use crate::key::VaultKey; use crate::vault::{ CredentialType, EntryMetadata, EntryStatus, HistoryEvent, HistoryEventKind, KdfParams, - VAULT_VERSION, + MIN_SUPPORTED_VAULT_VERSION, VAULT_VERSION, }; +/// Sentinel `envelope_version` marking a pre-#283 entry: no `envelope_version` +/// key existed in that on-disk shape at all, so `#[serde(default)]` fills +/// this in on read. Never written by `add`/`rotate` — see +/// [`crate::crypto::ENTRY_ENVELOPE_VERSION`]. +const LEGACY_ENVELOPE_VERSION: u8 = 0; + /// Well-known plaintext used to verify the passphrase on open. const KEY_CHECK_PLAINTEXT: &[u8] = b"kryphos-vault-key-check-v1"; @@ -295,7 +301,9 @@ impl Vault { /// Returns [`VaultError::NotInitialized`] if no vault exists at `path`. /// Returns [`VaultError::WrongPassphrase`] if the passphrase is incorrect. /// Returns [`VaultError::Locked`] if another process holds the lock. - /// Returns [`VaultError::InvalidHeader`] if the header is malformed. + /// Returns [`VaultError::InvalidHeader`] if the header is malformed, or + /// its version is outside + /// `MIN_SUPPORTED_VAULT_VERSION..=VAULT_VERSION`. pub fn open(path: impl AsRef, passphrase: &[u8]) -> Result { let path = path.as_ref(); @@ -322,10 +330,19 @@ impl Vault { let header: StoredHeader = serde_json::from_slice(&header_bytes).context(SerializationSnafu)?; - if header.version != VAULT_VERSION { + // WHY a range, not an exact match against VAULT_VERSION: the header + // shape is unchanged between MIN_SUPPORTED_VAULT_VERSION and + // VAULT_VERSION (see VAULT_VERSION's doc) — a v1 vault opens exactly + // like a v2 one. What differs is per-entry: `get` below selects the + // AAD from each entry's own `envelope_version` rather than assuming + // one scheme for the whole vault. Below the floor is a version this + // crate never wrote; above the ceiling is a newer format this build + // predates. Both remain hard rejections (forkwright/akroasis#283's + // Desired Correction is an in-place migration, not "accept anything"). + if !(MIN_SUPPORTED_VAULT_VERSION..=VAULT_VERSION).contains(&header.version) { return Err(VaultError::InvalidHeader { reason: format!( - "unsupported version {}, expected {VAULT_VERSION}", + "unsupported version {}, expected {MIN_SUPPORTED_VAULT_VERSION}..={VAULT_VERSION}", header.version ), }); @@ -443,26 +460,46 @@ impl Vault { return EntryRevokedSnafu { name }.fail(); } - // WHY: rebuilt from the CALLER's `name` plus this entry's OWN - // decrypted `credential_type` (from `encrypted_metadata`) and stored - // `envelope_version`, not trusted verbatim from elsewhere. A - // ciphertext relocated from a different entry (or with its - // `envelope_version` edited independently of `encrypted_secret`) was - // bound under a different AAD at encrypt time, so it fails - // authentication here instead of decrypting into this slot - // (forkwright/akroasis#283). - let aad = entry_aad( - &self.salt, - name, - &record.credential_type, - entry.envelope_version, - )?; - // 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, &aad).context(EntryCryptoSnafu)?); + // WHY branch on envelope_version rather than always building an AAD: + // a LEGACY_ENVELOPE_VERSION (0) entry is one written before + // forkwright/akroasis#283's AAD binding existed — its ciphertext was + // sealed with `crypto::encrypt(key, secret, b"")` (empty AAD; see + // VAULT_VERSION's doc). Building a non-empty `entry_aad` for it + // would authenticate against bytes the original encryption never + // used, permanently failing every `get` on such an entry — exactly + // the access loss #283's Desired Correction asked to avoid. Any + // OTHER version (the current ENTRY_ENVELOPE_VERSION, or a tampered + // value) goes through the full identity-bound AAD: rebuilt from the + // CALLER's `name` plus this entry's OWN decrypted `credential_type` + // (from `encrypted_metadata`) and stored `envelope_version`, never + // trusted verbatim. A ciphertext relocated from a different entry + // (or with its `envelope_version` edited independently of + // `encrypted_secret`) was bound under a different AAD at encrypt + // time, so it fails authentication here instead of decrypting into + // this slot (forkwright/akroasis#283) — including a downgrade + // attempt that tampers a bound entry's `envelope_version` DOWN to + // 0, since its ciphertext was never sealed under empty AAD in the + // first place. + // + // WHY: wrap at the point of allocation in BOTH branches — + // `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 = if entry.envelope_version == LEGACY_ENVELOPE_VERSION { + Zeroizing::new( + decrypt(&self.key, &entry.encrypted_secret, b"").context(EntryCryptoSnafu)?, + ) + } else { + let aad = entry_aad( + &self.salt, + name, + &record.credential_type, + entry.envelope_version, + )?; + Zeroizing::new( + decrypt(&self.key, &entry.encrypted_secret, &aad).context(EntryCryptoSnafu)?, + ) + }; Ok(DecryptedEntry { name: record.name, diff --git a/crates/kryphos/src/storage_security_tests.rs b/crates/kryphos/src/storage_security_tests.rs index c3654e4..41b72f4 100644 --- a/crates/kryphos/src/storage_security_tests.rs +++ b/crates/kryphos/src/storage_security_tests.rs @@ -1,8 +1,9 @@ //! Tests for [`super`]; split out from `storage_tests.rs` to keep both -//! under the RUST/file-too-long 800-line threshold. Covers the three -//! security defects fixed together: empty passphrases (akroasis#287), -//! ciphertext identity binding (akroasis#283), and concurrent mutation -//! atomicity (akroasis#214). +//! under the RUST/file-too-long 800-line threshold. Covers the security +//! defects fixed together: empty passphrases (akroasis#287), ciphertext +//! identity binding (akroasis#283), concurrent mutation atomicity +//! (akroasis#214), and transparent legacy-entry migration (akroasis#283 +//! Desired Correction, akroasis#215). use super::*; @@ -78,9 +79,13 @@ fn moved_ciphertext_between_entries_fails_authentication() { // Simulate a write-capable attacker (or store corruption): take the raw // stored value for `entry-a` — a valid, correctly-authenticated - // ciphertext — and place it under `entry-b`'s fjall key. - let raw_a = vault.keyspace.get("entry-a").unwrap().unwrap(); - vault.keyspace.insert("entry-b", raw_a).unwrap(); + // ciphertext — and place it under `entry-b`'s fjall key. Keys are + // looked up via `lookup_key` (forkwright/akroasis#215): the fjall + // record key is a keyed hash of the name, not the name itself. + let key_a = vault.lookup_key("entry-a"); + let key_b = vault.lookup_key("entry-b"); + let raw_a = vault.keyspace.get(key_a).unwrap().unwrap(); + vault.keyspace.insert(key_b, raw_a).unwrap(); let result = vault.get("entry-b"); assert!( @@ -92,30 +97,59 @@ fn moved_ciphertext_between_entries_fails_authentication() { // The untouched original must still be exactly retrievable. let entry_a = vault.get("entry-a").unwrap(); - assert_eq!(entry_a.secret, b"secret-a"); + assert_eq!(entry_a.secret.as_slice(), b"secret-a".as_slice()); } #[test] -fn mutated_credential_type_field_fails_authentication() { +fn secret_ciphertext_paired_with_a_different_entrys_metadata_fails_authentication() { + // `credential_type` moved from a plaintext top-level field into the + // encrypted `encrypted_metadata` blob (forkwright/akroasis#215), so an + // attacker without the vault key can no longer flip it as a bare JSON + // field the way #283's original review scenario assumed — editing + // `encrypted_metadata` without the key just breaks its own AEAD tag. + // What remains reachable without the key: splicing one entry's + // `encrypted_secret` into another entry's `StoredEntry`, pairing it with + // THAT entry's own (validly-decrypting) metadata — including a + // different `credential_type`. `entry_aad` still binds `credential_type` + // (sourced from the decrypted metadata) into what `encrypted_secret` + // authenticates, so the spliced pair must fail exactly like a + // whole-entry relocation does. let dir = tempfile::tempdir().unwrap(); - let vault_path = dir.path().join("aad-type-tamper-vault"); + let vault_path = dir.path().join("aad-type-splice-vault"); let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); vault - .add("entry", CredentialType::ApiKey, b"secret") + .add("api-entry", CredentialType::ApiKey, b"secret-api") + .unwrap(); + vault + .add("psk-entry", CredentialType::Psk, b"secret-psk") .unwrap(); - let raw = vault.keyspace.get("entry").unwrap().unwrap(); - let mut value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); - value["credential_type"] = serde_json::to_value(CredentialType::Psk).unwrap(); - let tampered = serde_json::to_vec(&value).unwrap(); - vault.keyspace.insert("entry", tampered).unwrap(); + let key_api = vault.lookup_key("api-entry"); + let key_psk = vault.lookup_key("psk-entry"); - let result = vault.get("entry"); + let raw_api = vault.keyspace.get(key_api).unwrap().unwrap(); + let raw_psk = vault.keyspace.get(key_psk).unwrap().unwrap(); + + let mut value_api: serde_json::Value = serde_json::from_slice(&raw_api).unwrap(); + let value_psk: serde_json::Value = serde_json::from_slice(&raw_psk).unwrap(); + + // Splice psk-entry's encrypted_secret into api-entry's StoredEntry, + // keeping api-entry's own encrypted_metadata/envelope_version — so + // decrypt_metadata still succeeds (it decrypts api-entry's own, + // untouched blob) and reports credential_type == ApiKey, while the + // secret ciphertext was actually bound under credential_type == Psk at + // encrypt time. + value_api["encrypted_secret"] = value_psk["encrypted_secret"].clone(); + let spliced = serde_json::to_vec(&value_api).unwrap(); + vault.keyspace.insert(key_api, spliced).unwrap(); + + let result = vault.get("api-entry"); assert!( result.is_err(), - "a credential_type edited independently of encrypted_secret must \ - fail authentication, got {result:?}" + "a secret ciphertext bound under a different entry's credential_type \ + must fail authentication even when paired with metadata that \ + decrypts cleanly on its own, got {result:?}" ); } @@ -129,11 +163,12 @@ fn mutated_envelope_version_field_fails_authentication() { .add("entry", CredentialType::ApiKey, b"secret") .unwrap(); - let raw = vault.keyspace.get("entry").unwrap().unwrap(); + let key = vault.lookup_key("entry"); + let raw = vault.keyspace.get(key).unwrap().unwrap(); let mut value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); value["envelope_version"] = serde_json::json!(99); let tampered = serde_json::to_vec(&value).unwrap(); - vault.keyspace.insert("entry", tampered).unwrap(); + vault.keyspace.insert(key, tampered).unwrap(); let result = vault.get("entry"); assert!( @@ -159,7 +194,8 @@ fn envelope_version_downgraded_to_legacy_fails_authentication() { .add("entry", CredentialType::ApiKey, b"secret") .unwrap(); - let raw = vault.keyspace.get("entry").unwrap().unwrap(); + let key = vault.lookup_key("entry"); + let raw = vault.keyspace.get(key).unwrap().unwrap(); let mut value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); assert_eq!( value["envelope_version"], @@ -168,7 +204,7 @@ fn envelope_version_downgraded_to_legacy_fails_authentication() { ); value["envelope_version"] = serde_json::json!(0); let tampered = serde_json::to_vec(&value).unwrap(); - vault.keyspace.insert("entry", tampered).unwrap(); + vault.keyspace.insert(key, tampered).unwrap(); let result = vault.get("entry"); assert!( @@ -179,48 +215,33 @@ fn envelope_version_downgraded_to_legacy_fails_authentication() { } // ----------------------------------------------------------------- -// Legacy vault migration (akroasis#283 Desired Correction) +// Legacy entry migration (akroasis#283 Desired Correction) // ----------------------------------------------------------------- #[test] -fn legacy_v1_vault_opens_and_decrypts_pre_283_entries() { - // WHY hand-assembled rather than produced by calling `Vault::create`: - // no code in this binary writes a v1 header or an envelope_version-0 - // entry anymore, so a pre-#283 vault can only be reconstructed by - // replicating what the OLD code actually wrote — a header with - // `version: 1` and an entry whose secret was sealed with - // `crypto::encrypt(key, secret, b"")` (empty AAD; see the RFC 8439 test - // vector's postfix-tag format that `encrypt`/`decrypt` still implement - // unchanged). This uses the SAME production `crypto`/`fjall` primitives - // `Vault::create`/`add` use internally, not a reimplementation. +fn legacy_pre_envelope_entry_opens_and_decrypts_under_the_current_vault_format() { + // WHY hand-assembled rather than produced by `Vault::add`: no code in + // this binary writes an envelope_version-0 entry anymore (`add` always + // stamps `ENTRY_ENVELOPE_VERSION`), so an entry sealed before + // forkwright/akroasis#283's AAD binding existed can only be + // reconstructed by replicating what that OLDER code actually wrote: + // `encrypted_secret` sealed with `crypto::encrypt(key, secret, b"")` + // (empty AAD). VAULT_VERSION itself never changed for the AAD-binding + // fix alone — see its doc — so this vault's HEADER is the CURRENT + // format (forkwright/akroasis#215's encrypted-metadata + hashed-lookup + // shape) throughout; only this one entry predates entry_aad. Built with + // the vault's own `key`/`lookup_key`/`encrypt_metadata` (accessible + // here as a descendant module of `storage`), not a reimplementation of + // them. let dir = tempfile::tempdir().unwrap(); - let vault_path = dir.path().join("legacy-v1-vault"); - - create_owner_only_dir(&vault_path).unwrap(); - let salt = crypto::generate_salt(); - let key = crypto::derive_key(TEST_PASSPHRASE, &salt); - let key_check = encrypt(&key, KEY_CHECK_PLAINTEXT, b"").unwrap(); - let header = StoredHeader { - version: 1, - salt: salt.to_vec(), - kdf_params: KdfParams::default(), - key_check, - }; - write_owner_only_file( - &vault_path.join(HEADER_FILE), - serde_json::to_string_pretty(&header).unwrap().as_bytes(), - ) - .unwrap(); + let vault_path = dir.path().join("legacy-pre-envelope-vault"); - create_owner_only_dir(&vault_path.join(DATA_DIR)).unwrap(); - let (db, keyspace) = open_fjall(&vault_path.join(DATA_DIR)).unwrap(); + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); - let encrypted_secret = encrypt(&key, b"legacy-secret", b"").unwrap(); let now = Timestamp::now(); - let legacy_entry = StoredEntry { - envelope_version: 0, + let record = EntryMetadataRecord { + name: CompactString::from("legacy-entry"), credential_type: CredentialType::ApiKey, - encrypted_secret, metadata: EntryMetadata { created_at: now, rotated_at: None, @@ -234,57 +255,44 @@ fn legacy_v1_vault_opens_and_decrypts_pre_283_entries() { kind: HistoryEventKind::Created, }], }; - keyspace - .insert("legacy-entry", serde_json::to_vec(&legacy_entry).unwrap()) + let encrypted_metadata = vault.encrypt_metadata(&record).unwrap(); + let encrypted_secret = encrypt(&vault.key, b"legacy-secret", b"").unwrap(); + let legacy_entry = StoredEntry { + envelope_version: 0, + encrypted_secret, + encrypted_metadata, + }; + vault + .keyspace + .insert( + vault.lookup_key("legacy-entry"), + serde_json::to_vec(&legacy_entry).unwrap(), + ) .unwrap(); - db.persist(fjall::PersistMode::SyncAll).unwrap(); - drop(keyspace); - drop(db); - - // The production open path: must accept a v1 header rather than - // rejecting every vault created before this PR outright. - let vault = Vault::open(&vault_path, TEST_PASSPHRASE).unwrap(); + vault.db.persist(fjall::PersistMode::SyncAll).unwrap(); - // The production get path: must transparently decrypt a pre-#283 + // The production get path: must transparently decrypt a pre-AAD-binding // (envelope_version 0, empty-AAD) entry with no migrate command and no // operator round-trip through an old binary. let decrypted = vault.get("legacy-entry").unwrap(); - assert_eq!(decrypted.secret, b"legacy-secret"); + assert_eq!(decrypted.secret.as_slice(), b"legacy-secret".as_slice()); assert_eq!(decrypted.credential_type, CredentialType::ApiKey); } #[test] -fn legacy_v1_vault_rotate_opportunistically_upgrades_the_envelope() { - // A legacy entry that gets rotated must come out bound under the - // current envelope, so it stops depending on the legacy branch on every - // subsequent read. +fn legacy_pre_envelope_entry_rotate_opportunistically_upgrades_the_envelope() { + // A legacy (pre-AAD-binding) entry that gets rotated must come out + // bound under the current envelope, so it stops depending on the + // legacy branch on every subsequent read. let dir = tempfile::tempdir().unwrap(); - let vault_path = dir.path().join("legacy-v1-vault-rotate"); - - create_owner_only_dir(&vault_path).unwrap(); - let salt = crypto::generate_salt(); - let key = crypto::derive_key(TEST_PASSPHRASE, &salt); - let key_check = encrypt(&key, KEY_CHECK_PLAINTEXT, b"").unwrap(); - let header = StoredHeader { - version: 1, - salt: salt.to_vec(), - kdf_params: KdfParams::default(), - key_check, - }; - write_owner_only_file( - &vault_path.join(HEADER_FILE), - serde_json::to_string_pretty(&header).unwrap().as_bytes(), - ) - .unwrap(); - - create_owner_only_dir(&vault_path.join(DATA_DIR)).unwrap(); - let (db, keyspace) = open_fjall(&vault_path.join(DATA_DIR)).unwrap(); - let encrypted_secret = encrypt(&key, b"v0", b"").unwrap(); + let vault_path = dir.path().join("legacy-pre-envelope-vault-rotate"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + let now = Timestamp::now(); - let legacy_entry = StoredEntry { - envelope_version: 0, + let record = EntryMetadataRecord { + name: CompactString::from("legacy-entry"), credential_type: CredentialType::ApiKey, - encrypted_secret, metadata: EntryMetadata { created_at: now, rotated_at: None, @@ -298,17 +306,23 @@ fn legacy_v1_vault_rotate_opportunistically_upgrades_the_envelope() { kind: HistoryEventKind::Created, }], }; - keyspace - .insert("legacy-entry", serde_json::to_vec(&legacy_entry).unwrap()) + let encrypted_metadata = vault.encrypt_metadata(&record).unwrap(); + let encrypted_secret = encrypt(&vault.key, b"v0", b"").unwrap(); + let legacy_entry = StoredEntry { + envelope_version: 0, + encrypted_secret, + encrypted_metadata, + }; + let key = vault.lookup_key("legacy-entry"); + vault + .keyspace + .insert(key, serde_json::to_vec(&legacy_entry).unwrap()) .unwrap(); - db.persist(fjall::PersistMode::SyncAll).unwrap(); - drop(keyspace); - drop(db); + vault.db.persist(fjall::PersistMode::SyncAll).unwrap(); - let vault = Vault::open(&vault_path, TEST_PASSPHRASE).unwrap(); vault.rotate("legacy-entry", b"v1").unwrap(); - let raw = vault.keyspace.get("legacy-entry").unwrap().unwrap(); + let raw = vault.keyspace.get(key).unwrap().unwrap(); let value: serde_json::Value = serde_json::from_slice(&raw).unwrap(); assert_eq!( value["envelope_version"], @@ -317,7 +331,7 @@ fn legacy_v1_vault_rotate_opportunistically_upgrades_the_envelope() { ); let decrypted = vault.get("legacy-entry").unwrap(); - assert_eq!(decrypted.secret, b"v1"); + assert_eq!(decrypted.secret.as_slice(), b"v1".as_slice()); } // ----------------------------------------------------------------- diff --git a/crates/kryphos/src/vault.rs b/crates/kryphos/src/vault.rs index 132715a..c6305c3 100644 --- a/crates/kryphos/src/vault.rs +++ b/crates/kryphos/src/vault.rs @@ -1,6 +1,6 @@ //! Vault data model: entries, headers, and credential types. -use chacha20poly1305::aead::Aead; +use chacha20poly1305::aead::{Aead, Payload}; use chacha20poly1305::{ChaCha20Poly1305, KeyInit, Nonce}; use compact_str::CompactString; use jiff::Timestamp; @@ -17,26 +17,55 @@ pub const SALT_LEN: usize = 16; /// Size of the ChaCha20-Poly1305 nonce in bytes. pub const NONCE_LEN: usize = 12; -/// Current vault format version. +/// Current vault format version, written by [`VaultHeader::new`] into every +/// newly created vault. /// -/// 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 folds together two independent changes that landed close together: -/// names, types, tags, status, and history moved from plaintext fields into -/// `encrypted_metadata`, the fjall record key changed from the plaintext -/// name to a keyed-hash lookup key (forkwright/akroasis#215), AND entry -/// ciphertexts are now bound to their identity via AEAD associated data -/// (forkwright/akroasis#283). A v1 store has neither the fields, the keys, -/// nor the AAD binding v2 code expects; its migration path is -/// re-initialization: read out entries with the prior release, create a -/// fresh vault, re-add them. `envelope_version` (a per-ENTRY field, distinct -/// from this header-level version — see -/// [`crate::crypto::ENTRY_ENVELOPE_VERSION`]) is the finer-grained axis that -/// DOES support transparent migration, for entries written under a v2 -/// header before the AAD binding existed. +/// WARNING: bumping this is a hard break for anything below +/// [`MIN_SUPPORTED_VAULT_VERSION`] — `open` rejects any header whose +/// `version` falls outside `MIN_SUPPORTED_VAULT_VERSION..=VAULT_VERSION`, so +/// a vault written under an older format simply fails to open under a newer +/// one. v2 folds together two independent changes that landed close +/// together: 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 (forkwright/akroasis#215); +/// entry ciphertexts are ALSO now bound to their identity via AEAD +/// associated data (forkwright/akroasis#283). A true v1 store (predating +/// both changes) has neither the fields, the keys, nor the AAD binding v2 +/// code expects — its migration path is re-initialization: read out entries +/// with the prior release, create a fresh vault, re-add them. +/// `envelope_version` (a per-ENTRY field, distinct from this header-level +/// version — see [`crate::crypto::ENTRY_ENVELOPE_VERSION`]) is the +/// finer-grained axis that DOES support transparent migration, for entries +/// written under a v2 header before the AAD binding existed: it defaults to +/// 0 via serde on a record that never had the field, and +/// [`crate::storage::Vault::get`] selects the correct AAD from it at decrypt +/// time. This IS the migration path forkwright/akroasis#283's Desired +/// Correction asked for, scoped to what it can actually promise once v2's +/// OWN entry/key-layout change (forkwright/akroasis#215) is accounted for: +/// a v2-header vault whose entries predate AAD binding opens and decrypts +/// transparently, with no separate `migrate` command and no operator +/// round-trip through an old release; a true v1 vault does not, because its +/// entry shape and fjall key derivation are unrelated to what this crate +/// reads today. pub const VAULT_VERSION: u32 = 2; +/// Oldest vault header version [`crate::storage::Vault::open`] still +/// accepts. +/// +/// Equal to [`VAULT_VERSION`], not lower: unlike the AAD-binding change, +/// v2's entry-shape and fjall-key-derivation change +/// (forkwright/akroasis#215) has no transparent migration path (see +/// [`VAULT_VERSION`]'s doc) — accepting a header below this floor would let +/// `open` succeed on a vault whose entries this build cannot correctly +/// locate or parse, failing confusingly deep in `get`/`list` instead of with +/// a clear, up-front `InvalidHeader`. A header above [`VAULT_VERSION`] is +/// from a NEWER binary's format this build predates; reject that too rather +/// than open it partially. Kept as a distinct constant (not inlined as +/// `VAULT_VERSION`) because the range-check pattern itself is real: a future +/// header-shape change that FUNDAMENTALLY differs from a floor-required +/// exact match again gives this room to widen. +pub const MIN_SUPPORTED_VAULT_VERSION: u32 = 2; + /// The kind of credential stored in a [`VaultEntry`]. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[non_exhaustive] @@ -221,6 +250,34 @@ impl VaultHeader { } } +/// Format version of the domain-separation AEAD associated data built by +/// [`seal_signing_key`]/[`unseal_signing_key`]. +/// +/// Distinct from [`VAULT_VERSION`] and [`crate::crypto::ENTRY_ENVELOPE_VERSION`]: +/// this versions only the signing-key seal's own binding, independent of the +/// header format and of any vault entry's envelope. +const SIGNING_KEY_ENVELOPE_VERSION: u8 = 1; + +/// Domain-separation tag baked into the signing key seal's AEAD associated +/// data. +/// +/// WHY: without this, a ciphertext produced by [`seal_signing_key`] and any +/// OTHER ciphertext encrypted under the same [`VaultKey`] (a vault entry via +/// [`crate::crypto::entry_aad`], the header's key-check plaintext, or any +/// future caller) authenticate identically wherever the key matches — the +/// same unauthenticated-context defect forkwright/akroasis#283 fixed for +/// vault entries, surviving in the one sibling function that reuses the +/// same key material for a different purpose. +const SIGNING_KEY_AAD_TAG: &[u8] = b"kryphos/vault/signing-key"; + +/// Builds the fixed AEAD associated data for a signing-key seal. +fn signing_key_aad() -> Vec { + let mut aad = Vec::with_capacity(1 + SIGNING_KEY_AAD_TAG.len()); + aad.push(SIGNING_KEY_ENVELOPE_VERSION); + aad.extend_from_slice(SIGNING_KEY_AAD_TAG); + aad +} + /// Encrypts the signing key of an [`InstallationIdentity`] with the given vault /// key and nonce, returning the ciphertext. /// @@ -242,9 +299,16 @@ pub fn seal_signing_key( // coverage `unseal_signing_key` gives the decrypt direction below // (RUST/#218). let plaintext = zeroizing_signing_key_bytes(identity); + let aad = signing_key_aad(); cipher - .encrypt(nonce, plaintext.as_ref()) + .encrypt( + nonce, + Payload { + msg: plaintext.as_ref(), + aad: &aad, + }, + ) .map_err(|e| CryptoError::EncryptionFailed { reason: e.to_string(), }) @@ -262,8 +326,9 @@ fn zeroizing_signing_key_bytes( /// /// # Errors /// -/// Returns [`CryptoError::DecryptionFailed`] if decryption fails (wrong key -/// or tampered ciphertext). +/// Returns [`CryptoError::DecryptionFailed`] if decryption fails (wrong key, +/// tampered ciphertext, or ciphertext sealed for a different purpose under +/// the same key — see [`SIGNING_KEY_AAD_TAG`]). /// Returns [`CryptoError::KeyParse`] if the decrypted bytes are not a valid /// Ed25519 key. pub fn unseal_signing_key( @@ -273,6 +338,7 @@ pub fn unseal_signing_key( ) -> Result { let cipher = ChaCha20Poly1305::new(vault_key.as_bytes().into()); let nonce = Nonce::from_slice(nonce); + let aad = signing_key_aad(); // WHY: wrap at the point of allocation — `decrypt`'s return (the raw // Ed25519 signing key bytes) is moved straight into `Zeroizing::new` @@ -286,7 +352,13 @@ pub fn unseal_signing_key( // (RUST/#218). let plaintext = Zeroizing::new( cipher - .decrypt(nonce, ciphertext) + .decrypt( + nonce, + Payload { + msg: ciphertext, + aad: &aad, + }, + ) .map_err(|_| CryptoError::DecryptionFailed)?, ); @@ -530,11 +602,27 @@ mod tests { #[test] fn unseal_wrong_length_plaintext_is_key_parse_error() { + // WHY sealed under `signing_key_aad()` rather than the raw cipher + // with no AAD: decryption must actually SUCCEED for this test to + // reach the length check it targets. Since unseal_signing_key now + // requires the domain-separation tag (this same test file's + // `unseal_rejects_ciphertext_sealed_for_a_different_purpose` + // verifies that requirement), a ciphertext sealed without it no + // longer decrypts at all — it fails authentication before + // `SigningKey::from_bytes` ever runs, which would make this + // assertion pass for the wrong reason. let vault_key = VaultKey::from_bytes([0x42; 32]); let nonce = [0x01; NONCE_LEN]; let cipher = ChaCha20Poly1305::new(vault_key.as_bytes().into()); + let aad = signing_key_aad(); let ciphertext = cipher - .encrypt(Nonce::from_slice(&nonce), &[0u8; 16][..]) + .encrypt( + Nonce::from_slice(&nonce), + Payload { + msg: &[0u8; 16][..], + aad: &aad, + }, + ) .unwrap(); let result = unseal_signing_key(&ciphertext, &vault_key, &nonce);