diff --git a/crates/koinon/src/tamper_log_entry.rs b/crates/koinon/src/tamper_log_entry.rs index 2a7b326..1b93f23 100644 --- a/crates/koinon/src/tamper_log_entry.rs +++ b/crates/koinon/src/tamper_log_entry.rs @@ -52,17 +52,30 @@ pub enum LogEntryKind { }, /// A credential vault entry lifecycle mutation was committed. VaultMutation { - /// Human-readable credential name affected by the mutation. - credential_name: CompactString, + /// Opaque reference to the credential the mutation affected. + /// + /// NOT a name. The writer derives this from the credential's name + /// under a secret it holds, so the same credential yields the same + /// reference across entries — enough to follow one credential's + /// history through the log — while a reader without that secret learns + /// nothing about what any of them are called + /// (forkwright/akroasis#378). + /// + /// This type does not derive the reference; it only carries it. What + /// the derivation is, and therefore who can reverse it, belongs to the + /// writer. + credential_ref: CompactString, /// Mutation operation, e.g. `"add"`, `"rotate"`, `"revoke"`, or `"remove"`. operation: CompactString, }, } // WHY: manual Debug instead of #[derive(Debug)] — `VaultMutation` carries a -// credential name. It is a label, not the credential's secret value, but -// Debug output lands in logs; redact it so a vault-mutation log entry never -// prints a credential name verbatim (RUST/no-debug-derive-on-public-types). +// credential reference. It is derived rather than plaintext (#378), so this +// redaction is no longer the only thing standing between a credential name and +// a log file; it stays because a stable per-credential identifier is still a +// correlation handle, and Debug output travels further than the audit log does +// (RUST/no-debug-derive-on-public-types). impl std::fmt::Debug for LogEntryKind { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -113,11 +126,11 @@ impl std::fmt::Debug for LogEntryKind { .field("target", target) .finish(), Self::VaultMutation { - credential_name: _, + credential_ref: _, operation, } => f .debug_struct("VaultMutation") - .field("credential_name", &"") + .field("credential_ref", &"") .field("operation", operation) .finish(), } diff --git a/crates/koinon/src/tamper_log_recovery_tests.rs b/crates/koinon/src/tamper_log_recovery_tests.rs index 7212891..f34daa5 100644 --- a/crates/koinon/src/tamper_log_recovery_tests.rs +++ b/crates/koinon/src/tamper_log_recovery_tests.rs @@ -19,7 +19,7 @@ fn test_key() -> ChainKey { fn vault_kind() -> LogEntryKind { LogEntryKind::VaultMutation { - credential_name: CompactString::from("recovery-test-cred"), + credential_ref: CompactString::from("0f1e2d3c4b5a6978"), operation: CompactString::from("test"), } } diff --git a/crates/koinon/src/tamper_log_tests.rs b/crates/koinon/src/tamper_log_tests.rs index f8547ec..d3357bb 100644 --- a/crates/koinon/src/tamper_log_tests.rs +++ b/crates/koinon/src/tamper_log_tests.rs @@ -55,7 +55,7 @@ fn action_kind() -> LogEntryKind { fn vault_mutation_kind() -> LogEntryKind { LogEntryKind::VaultMutation { - credential_name: CompactString::from("incident-radio-key"), + credential_ref: CompactString::from("a1b2c3d4e5f60718"), operation: CompactString::from("rotate"), } } diff --git a/crates/kryphos/src/storage.rs b/crates/kryphos/src/storage.rs index 6ec4056..5869ea0 100644 --- a/crates/kryphos/src/storage.rs +++ b/crates/kryphos/src/storage.rs @@ -60,6 +60,16 @@ const CHAIN_KEY_DOMAIN: &[u8] = b"kryphos/tamper-log/chain-key/v1"; /// to manage, mirroring [`CHAIN_KEY_DOMAIN`]. const LOOKUP_KEY_DOMAIN: &[u8] = b"kryphos/vault/lookup-key/v1"; +/// Domain separator for the audit log's per-credential reference. +/// +/// WHY a domain of its own rather than reusing [`LOOKUP_KEY_DOMAIN`]: the two +/// identifiers answer different questions and should not be interchangeable. +/// The lookup key addresses a record in the fjall store; this one groups +/// entries in the audit log. Deriving both from the same domain would make an +/// audit reference usable as a store key, which is a capability the audit log +/// has no reason to hand anyone. +const AUDIT_REF_DOMAIN: &[u8] = b"kryphos/vault/audit-ref/v1"; + /// Stands in for a vault that has recorded no installation identity. /// /// WHY a verifier that verifies nothing rather than skipping the check: the @@ -1064,6 +1074,34 @@ impl Vault { /// Deterministic (same name -> same key), so `get`/`add`/`remove` stay /// O(1) keyspace lookups without ever storing the name itself. A fresh /// derivation on every call, mirroring [`Self::chain_key`]. + /// Derives the audit log's opaque reference for `name`. + /// + /// Deterministic, so every entry touching one credential shares a + /// reference and its history stays followable; keyed by the vault key, so + /// a reader holding the log file and not the passphrase learns neither the + /// name nor whether two vaults hold the same one. + /// + /// WHY this exists at all: `#215` stopped the fjall store revealing + /// credential names, and the tamper log kept writing them in cleartext — + /// so the audit log became the weaker link for the exact threat that issue + /// named, filesystem access without the passphrase (forkwright/akroasis#378). + /// + /// Rendered hex rather than raw bytes because the field is a + /// `CompactString` that lands in CBOR; 16 hex characters of a keyed BLAKE3 + /// is ample to keep credentials distinct within one vault's log while + /// keeping entries small. + fn audit_ref(&self, name: &str) -> CompactString { + let subkey = blake3::keyed_hash(self.key.as_bytes(), AUDIT_REF_DOMAIN); + let digest = blake3::keyed_hash(subkey.as_bytes(), name.as_bytes()); + let bytes = digest.as_bytes(); + let mut rendered = String::with_capacity(16); + for byte in bytes.iter().take(8) { + use std::fmt::Write as _; + let _ = write!(rendered, "{byte:02x}"); + } + CompactString::from(rendered) + } + fn lookup_key(&self, name: &str) -> [u8; 32] { let subkey = blake3::keyed_hash(self.key.as_bytes(), LOOKUP_KEY_DOMAIN); blake3::keyed_hash(subkey.as_bytes(), name.as_bytes()).into() @@ -1162,7 +1200,7 @@ impl Vault { } }; log.append(LogEntryKind::VaultMutation { - credential_name: CompactString::from(name), + credential_ref: self.audit_ref(name), operation: CompactString::from(operation), }) .context(TamperLogSnafu)?; diff --git a/crates/kryphos/tests/vault_tamper_audit.rs b/crates/kryphos/tests/vault_tamper_audit.rs index 3c41a58..6e3a5bb 100644 --- a/crates/kryphos/tests/vault_tamper_audit.rs +++ b/crates/kryphos/tests/vault_tamper_audit.rs @@ -67,12 +67,22 @@ fn vault_mutations_append_intact_tamper_log() { let (entry, _hash) = koinon::tamper_log::decode_entry(&data[offset..]).unwrap(); match entry.kind { LogEntryKind::VaultMutation { - credential_name, + credential_ref, operation: logged_operation, } => { + // WHY not compared against `name`: the log records a derived + // reference, never the name (#378). What must hold is that the + // reference is opaque and stable — the two properties that let + // an operator follow one credential's history without the file + // telling a reader what any credential is called. + assert_ne!( + credential_ref, name, + "entry {idx} must not carry the plaintext name" + ); assert_eq!( - credential_name, name, - "entry {idx} credential_name mismatch" + credential_ref.len(), + 16, + "entry {idx} reference must be the fixed-width derived form" ); assert_eq!( logged_operation, operation, @@ -82,6 +92,61 @@ fn vault_mutations_append_intact_tamper_log() { other => panic!("entry {idx}: expected VaultMutation, got {other:?}"), } } + + // Stability and distinctness, which is the whole reason the reference is + // derived rather than random: the three entries for one credential share a + // reference, and the other credential's differs. + let refs: Vec = (0..5) + .map(|idx| { + let offset = entry_offset(&data, idx); + let (entry, _) = koinon::tamper_log::decode_entry(&data[offset..]).unwrap(); + match entry.kind { + LogEntryKind::VaultMutation { credential_ref, .. } => credential_ref.to_string(), + other => panic!("entry {idx}: expected VaultMutation, got {other:?}"), + } + }) + .collect(); + assert_eq!(refs[0], refs[1], "one credential must keep one reference"); + assert_eq!(refs[1], refs[2], "one credential must keep one reference"); + assert_eq!(refs[3], refs[4], "one credential must keep one reference"); + assert_ne!( + refs[0], refs[3], + "different credentials must not collide onto one reference" + ); +} + +/// The audit log must not reveal what the credentials are called. +/// +/// The sibling of `on_disk_fjall_contents_do_not_reveal_credential_name`, +/// which #215 added for the keyspace. Until #378 the tamper log kept writing +/// names in cleartext, so the two stores disagreed about the same threat — +/// filesystem access without the passphrase — and the log was the weaker one. +#[test] +fn the_audit_log_does_not_reveal_credential_names() { + let dir = tempfile::tempdir().unwrap(); + let vault_path = dir.path().join("named-vault"); + let vault = Vault::create(&vault_path, TEST_PASSPHRASE).unwrap(); + + vault + .add("incident-radio-key", CredentialType::RadioKey, b"secret") + .unwrap(); + vault.rotate("incident-radio-key", b"secret-2").unwrap(); + + let data = std::fs::read(vault.tamper_log_path()).unwrap(); + assert!( + !data + .windows(b"incident-radio-key".len()) + .any(|w| w == b"incident-radio-key"), + "the credential name must not appear anywhere in the log bytes" + ); + + // The acceptance partner: the operation names ARE still there, so the + // assertion above is reporting a protected name rather than a log that + // failed to record anything. + assert!( + data.windows(b"rotate".len()).any(|w| w == b"rotate"), + "the operation must still be recorded in cleartext" + ); } #[test]