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..7826a29 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,76 @@ 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. +/// 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. [`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, + 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(&checked_len_prefix("vault_salt", vault_salt.len())?); + aad.extend_from_slice(vault_salt); + aad.extend_from_slice(&checked_len_prefix("name", name.len())?); + aad.extend_from_slice(name.as_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` — 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)] #[expect(clippy::unwrap_used, reason = "test assertions use unwrap for clarity")] #[expect( @@ -168,8 +262,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 +275,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 +289,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 +303,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 +316,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 +401,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,13 +415,46 @@ 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" ); } + // ----------------------------------------------------------------- + // 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 b99c219..5d69804 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 { @@ -107,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.rs b/crates/kryphos/src/storage.rs index a6498de..7c40b4c 100644 --- a/crates/kryphos/src/storage.rs +++ b/crates/kryphos/src/storage.rs @@ -12,17 +12,24 @@ 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::{ 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"; @@ -64,14 +71,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 +183,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 +237,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 +262,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 +284,8 @@ impl Vault { db, keyspace, key, + salt: salt.to_vec(), + write_lock: std::sync::Mutex::new(()), _lock: lock, path: path.to_path_buf(), }) @@ -239,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(); @@ -266,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 ), }); @@ -278,7 +351,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 +362,8 @@ impl Vault { db, keyspace, key, + salt: header.salt, + write_lock: std::sync::Mutex::new(()), _lock: lock, path: path.to_path_buf(), }) @@ -308,15 +383,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 +423,7 @@ impl Vault { let encrypted_metadata = self.encrypt_metadata(&record)?; let entry = StoredEntry { + envelope_version: ENTRY_ENVELOPE_VERSION, encrypted_secret, encrypted_metadata, }; @@ -374,12 +460,46 @@ impl Vault { return EntryRevokedSnafu { name }.fail(); } - // 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)?); + // 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, @@ -426,6 +546,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 +590,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 +614,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 +636,7 @@ impl Vault { let encrypted_metadata = self.encrypt_metadata(&record)?; let entry = StoredEntry { + envelope_version: ENTRY_ENVELOPE_VERSION, encrypted_secret, encrypted_metadata, }; @@ -510,6 +661,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 +695,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 +783,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 +791,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> { @@ -747,3 +909,12 @@ fn fjall_err(e: impl std::fmt::Display) -> VaultError { )] #[path = "storage_tests.rs"] mod tests; + +#[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..41b72f4 --- /dev/null +++ b/crates/kryphos/src/storage_security_tests.rs @@ -0,0 +1,458 @@ +//! Tests for [`super`]; split out from `storage_tests.rs` to keep both +//! 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::*; + +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. 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!( + 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.as_slice(), b"secret-a".as_slice()); +} + +#[test] +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-splice-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + vault + .add("api-entry", CredentialType::ApiKey, b"secret-api") + .unwrap(); + vault + .add("psk-entry", CredentialType::Psk, b"secret-psk") + .unwrap(); + + let key_api = vault.lookup_key("api-entry"); + let key_psk = vault.lookup_key("psk-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 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:?}" + ); +} + +#[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 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(key, tampered).unwrap(); + + let result = vault.get("entry"); + assert!( + result.is_err(), + "an envelope_version edited independently of encrypted_secret must \ + fail authentication, got {result:?}" + ); +} + +#[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 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"], + 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(key, 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 entry migration (akroasis#283 Desired Correction) +// ----------------------------------------------------------------- + +#[test] +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-pre-envelope-vault"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + + let now = Timestamp::now(); + let record = EntryMetadataRecord { + name: CompactString::from("legacy-entry"), + credential_type: CredentialType::ApiKey, + 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, + }], + }; + 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(); + vault.db.persist(fjall::PersistMode::SyncAll).unwrap(); + + // 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.as_slice(), b"legacy-secret".as_slice()); + assert_eq!(decrypted.credential_type, CredentialType::ApiKey); +} + +#[test] +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-pre-envelope-vault-rotate"); + + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + + let now = Timestamp::now(); + let record = EntryMetadataRecord { + name: CompactString::from("legacy-entry"), + credential_type: CredentialType::ApiKey, + 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, + }], + }; + 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(); + vault.db.persist(fjall::PersistMode::SyncAll).unwrap(); + + vault.rotate("legacy-entry", b"v1").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"], + 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.as_slice(), b"v1".as_slice()); +} + +// ----------------------------------------------------------------- +// Concurrent mutation atomicity (akroasis#214) +// ----------------------------------------------------------------- + +#[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; + + 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" + ); + } +} diff --git a/crates/kryphos/src/vault.rs b/crates/kryphos/src/vault.rs index 6ec3262..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,17 +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 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. +/// 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] @@ -212,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. /// @@ -233,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(), }) @@ -253,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( @@ -264,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` @@ -277,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)?, ); @@ -521,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); @@ -536,6 +633,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();