diff --git a/src/openhuman/keyring/encrypted_file_backend.rs b/src/openhuman/keyring/encrypted_file_backend.rs index eeb350b79..cba7b2951 100644 --- a/src/openhuman/keyring/encrypted_file_backend.rs +++ b/src/openhuman/keyring/encrypted_file_backend.rs @@ -53,10 +53,15 @@ pub fn init_master_key() { Some(key) } Err(e) => { - log::warn!( - "[keyring:encrypted_file] master key unavailable — secrets will be \ - inaccessible this session. Cause: {e}" + log::error!( + "[keyring:encrypted_file] master key load FAILED — refusing to mint a \ + replacement (that would orphan existing secrets, #3311). Secrets are \ + inaccessible this session and recover once OS keychain access is \ + restored. Cause: {e}" ); + // Surface the denied state to the frontend instead of silently + // resetting — this is the "warn before reset" the issue asks for. + crate::openhuman::keyring_consent::policy::notify_master_key_unavailable(&e); None } } @@ -75,10 +80,49 @@ pub fn is_master_key_available() -> bool { MASTER_KEY.get().and_then(|k| k.as_ref()).is_some() } +/// Abstraction over the OS-keychain entry that holds the master key. +/// +/// Exists solely so the load-vs-mint decision in [`load_or_mint_master_key`] +/// can be unit-tested against injected `keyring::Error` variants. A real +/// `keyring::Entry` cannot be exercised non-interactively under `cargo test` +/// (the first access blocks on a GUI permission prompt), so the decision logic +/// is split out behind this trait and tested with a fake. +trait MasterKeyEntry { + fn get_password(&self) -> Result; + fn set_password(&self, value: &str) -> Result<(), keyring::Error>; +} + +impl MasterKeyEntry for keyring::Entry { + fn get_password(&self) -> Result { + keyring::Entry::get_password(self) + } + fn set_password(&self, value: &str) -> Result<(), keyring::Error> { + keyring::Entry::set_password(self, value) + } +} + fn try_load_master_key() -> Result<[u8; KEY_LEN], String> { let entry = keyring::Entry::new(KEYCHAIN_SERVICE, KEYCHAIN_MASTER_KEY_USERNAME) .map_err(|e| format!("keychain entry creation failed: {e}"))?; + load_or_mint_master_key(&entry) +} +/// Load the existing master key, mint a fresh one, or fail safe. +/// +/// **Only a genuine absence (`NoEntry`) may mint a new key.** Every other error +/// — access denied, keychain locked, platform failure — returns `Err` WITHOUT +/// minting or calling `set_password`, leaving the keychain entry untouched. +/// +/// This is the fix for #3311. A macOS app update can change the binary's +/// code-signing identity (or the keychain item's ACL trust), so reading the +/// *existing* master key fails with an access error rather than `NoEntry`. The +/// previous code conflated the two and minted a brand-new key on access +/// denial, orphaning every secret encrypted under the old key — a silent +/// API-key wipe plus disconnected connectors, with no warning. Failing safe +/// keeps the ciphertext intact so it recovers on the next launch once keychain +/// access is restored. The catch-all `Err(e)` arm makes this independent of +/// which exact `keyring` error variant macOS returns on the denial. +fn load_or_mint_master_key(entry: &E) -> Result<[u8; KEY_LEN], String> { match entry.get_password() { Ok(hex_str) => { let bytes = crypto::hex_decode(hex_str.trim())?; @@ -92,7 +136,7 @@ fn try_load_master_key() -> Result<[u8; KEY_LEN], String> { key.copy_from_slice(&bytes); Ok(key) } - Err(keyring::Error::NoEntry) | Err(keyring::Error::NoStorageAccess(_)) => { + Err(keyring::Error::NoEntry) => { let key_bytes = crypto::generate_random_bytes(KEY_LEN); let hex_value = crypto::hex_encode(&key_bytes); entry @@ -109,11 +153,14 @@ fn try_load_master_key() -> Result<[u8; KEY_LEN], String> { let mut key = [0u8; KEY_LEN]; key.copy_from_slice(&key_bytes); log::info!( - "[keyring:encrypted_file] generated and stored new master key in OS keychain" + "[keyring:encrypted_file] no existing master key — generated and stored a new one" ); Ok(key) } - Err(e) => Err(format!("OS keychain access denied or failed: {e}")), + Err(e) => Err(format!( + "OS keychain access unavailable; refusing to mint a replacement master key so \ + existing secrets are preserved (#3311): {e}" + )), } } @@ -322,3 +369,124 @@ impl KeyringBackend for EncryptedFileBackend { "encrypted_file" } } + +#[cfg(test)] +mod tests { + use super::*; + use std::cell::{Cell, RefCell}; + + /// In-memory fake of the keychain entry, so [`load_or_mint_master_key`] can + /// be exercised without a real OS keychain. `absent_error` is a fn pointer + /// because `keyring::Error` is not `Clone` — we mint a fresh error per call. + /// These tests touch no process-wide state (`load_or_mint_master_key` never + /// reads `MASTER_KEY`), so no OnceLock reset seam is needed. + struct FakeEntry { + stored: RefCell>, + absent_error: fn() -> keyring::Error, + set_calls: Cell, + } + + impl FakeEntry { + fn with_stored(value: &str) -> Self { + Self { + stored: RefCell::new(Some(value.to_string())), + absent_error: || keyring::Error::NoEntry, + set_calls: Cell::new(0), + } + } + fn absent(err: fn() -> keyring::Error) -> Self { + Self { + stored: RefCell::new(None), + absent_error: err, + set_calls: Cell::new(0), + } + } + } + + impl MasterKeyEntry for FakeEntry { + fn get_password(&self) -> Result { + match &*self.stored.borrow() { + Some(v) => Ok(v.clone()), + None => Err((self.absent_error)()), + } + } + fn set_password(&self, value: &str) -> Result<(), keyring::Error> { + self.set_calls.set(self.set_calls.get() + 1); + *self.stored.borrow_mut() = Some(value.to_string()); + Ok(()) + } + } + + fn access_denied() -> keyring::Error { + keyring::Error::NoStorageAccess(Box::new(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "keychain access denied", + ))) + } + + fn platform_failure() -> keyring::Error { + keyring::Error::PlatformFailure(Box::new(std::io::Error::other("platform boom"))) + } + + #[test] + fn loads_existing_key_without_minting() { + let hex = "ab".repeat(KEY_LEN); // 32 bytes of 0xab + let entry = FakeEntry::with_stored(&hex); + let key = load_or_mint_master_key(&entry).expect("should load existing key"); + assert_eq!(key, [0xabu8; KEY_LEN]); + assert_eq!(entry.set_calls.get(), 0, "must not overwrite existing key"); + } + + #[test] + fn mints_only_on_no_entry() { + let entry = FakeEntry::absent(|| keyring::Error::NoEntry); + let key = load_or_mint_master_key(&entry).expect("should mint when genuinely absent"); + assert_ne!(key, [0u8; KEY_LEN], "minted key should be random, not zero"); + assert_eq!( + entry.set_calls.get(), + 1, + "should store the freshly minted key" + ); + // The key is now persisted, so a second load returns the same one. + assert!(entry.stored.borrow().is_some()); + } + + #[test] + fn does_not_mint_on_access_denied() { + // The #3311 case: existing key unreadable due to post-update ACL change. + let entry = FakeEntry::absent(access_denied); + let result = load_or_mint_master_key(&entry); + assert!(result.is_err(), "access denial must NOT mint a new key"); + assert_eq!( + entry.set_calls.get(), + 0, + "must never call set_password on access denial — that orphans existing secrets" + ); + assert!( + entry.stored.borrow().is_none(), + "keychain entry left untouched" + ); + } + + #[test] + fn does_not_mint_on_platform_failure() { + // Variant-independence: any non-NoEntry error fails safe, not just + // NoStorageAccess (the exact macOS denial variant is unconfirmed). + let entry = FakeEntry::absent(platform_failure); + let result = load_or_mint_master_key(&entry); + assert!(result.is_err(), "platform failure must NOT mint a new key"); + assert_eq!(entry.set_calls.get(), 0); + } + + #[test] + fn rejects_wrong_length_key_without_minting() { + let entry = FakeEntry::with_stored("abcd"); // 2 bytes, not KEY_LEN + let result = load_or_mint_master_key(&entry); + assert!(result.is_err(), "wrong-length stored key is an error"); + assert_eq!( + entry.set_calls.get(), + 0, + "must not overwrite on length mismatch" + ); + } +} diff --git a/src/openhuman/keyring_consent/policy.rs b/src/openhuman/keyring_consent/policy.rs index ea9ea3b01..ca4e46ed2 100644 --- a/src/openhuman/keyring_consent/policy.rs +++ b/src/openhuman/keyring_consent/policy.rs @@ -141,6 +141,25 @@ pub fn retry_probe() -> KeyringStatus { current_status() } +/// Surface a master-key load failure (e.g. OS keychain access denied after an +/// app update) to the frontend by publishing the consent-required event. +/// +/// Unlike [`check_secret_access`], this is called proactively at core startup +/// when the encrypted-file backend cannot load its master key — so the user is +/// warned *before* any secret read silently returns empty, rather than letting +/// the failure pass unnoticed (the #3311 symptom: keys "wiped" with no warning). +/// It reuses the same `CONSENT_EVENT_PUBLISHED` dedup flag as the lazy gate so +/// we never double-publish if a secret op also hits the gate this session. +pub fn notify_master_key_unavailable(reason: &str) { + warn!("{LOG_PREFIX} master key unavailable: {reason}"); + if !CONSENT_EVENT_PUBLISHED.swap(true, Ordering::SeqCst) { + info!("{LOG_PREFIX} publishing KeyringConsentRequired event (master key unavailable)"); + crate::core::event_bus::publish_global( + crate::core::event_bus::DomainEvent::KeyringConsentRequired, + ); + } +} + /// Publish a decrypt-failure event for frontend notification. pub fn notify_decrypt_failure(field_name: &str, reason: &str) { warn!("{LOG_PREFIX} decrypt failure field={field_name} reason={reason}");