From 102f79dea01cb5578cb9df480a086366a87902b0 Mon Sep 17 00:00:00 2001 From: forkwright Date: Fri, 21 Aug 2026 05:28:38 -0500 Subject: [PATCH] fix(kryphos): stop the audit log recording credential names in cleartext #215 stopped the fjall store revealing credential names. The tamper log kept writing them verbatim, so post-#215 the audit log was the weaker link for the exact threat that issue named: filesystem access without the passphrase. An adversary with the log learned the operator's credential inventory, and from the operation sequence their rotation and revocation cadence, without touching the AEAD. The entry now carries a reference derived from the name under the vault key, the same treatment `lookup_key` gives names in the keyspace. Deterministic, so every entry touching one credential shares a reference and its history stays followable; keyed, so a reader with the file and not the passphrase learns neither the name nor whether two vaults hold the same one. A separate domain from `lookup_key`, deliberately. The two identifiers answer different questions -- one addresses a fjall record, the other groups audit entries -- and a shared domain would make an audit reference usable as a store key, a capability the audit log has no reason to hand anyone. The koinon field is renamed `credential_name` to `credential_ref`, because a field called `credential_name` holding a digest is exactly the kind of lie this repo's audits keep finding. Its doc says what it is and, more usefully, says that koinon does not derive it: what the derivation is, and therefore who can reverse it, belongs to the writer. `vault_mutations_append_intact_tamper_log` asserted the plaintext name round-tripped -- it documented the leak. It now asserts the reference is not the name, is the fixed-width derived form, is stable across one credential's three entries, and differs between credentials. `the_audit_log_does_not_reveal_credential_names` is the sibling of #215's `on_disk_fjall_contents_do_not_reveal_credential_name`, scanning the raw log bytes, with an acceptance partner asserting the operation IS still recorded so the absence is a protected name rather than an empty log. Mutation-checked: reverting the writer to the plaintext name fails both. Closes #378 --- crates/koinon/src/tamper_log_entry.rs | 27 +++++-- .../koinon/src/tamper_log_recovery_tests.rs | 2 +- crates/koinon/src/tamper_log_tests.rs | 2 +- crates/kryphos/src/storage.rs | 40 ++++++++++- crates/kryphos/tests/vault_tamper_audit.rs | 71 ++++++++++++++++++- 5 files changed, 129 insertions(+), 13 deletions(-) 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]