From 2e700a69b99c3a3e6cbe444f9983b400f04a66fa Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Wed, 24 Jun 2026 22:56:58 -0400 Subject: [PATCH 1/3] perf(desktop): consolidate keychain secrets into a single blob entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Buzz stored each secret as a separate keychain item — the identity key plus one entry per managed agent (6 agents = 7 items total). On dev builds (old keychain, null ACL) every read and write triggers a prompt: 15-20+ on first launch. On release builds (DPK) the first launch prompts once per key (7 total). Goose solved this identically: one entry, all secrets as a JSON map, one prompt total. Switch SecretStore to the same pattern. A single blob entry (service = the store's service name, username = "secrets") holds all keys as a JSON map. The map is cached in-memory after the first read, so subsequent probe/load/store/delete calls within the same process never touch the keychain again. One prompt on first launch, zero after. Migration: on first launch after upgrade the blob doesn't exist yet. load() falls back to reading the old per-key DPK entry (macOS) or legacy keyring entry (all platforms), writes it into the new blob, and deletes the old item — one-time per key, transparent to callers. The is_dpk_unavailable (-34018) fallback from #1266 is incorporated here: unsigned dev builds fall back to the legacy keyring crate for the single blob entry. The probe/load/store/delete semantics seen by callers (IdentityKeyStore, KeyStore) are unchanged. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/Cargo.lock | 1 + desktop/src-tauri/src/secret_store.rs | 420 ++++++++++++++++---------- 2 files changed, 259 insertions(+), 162 deletions(-) diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index 34180c95f..32e41d622 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -879,6 +879,7 @@ dependencies = [ "rodio", "rubato", "rusqlite", + "security-framework 3.7.0", "serde", "serde_json", "serde_yaml", diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index 93b35838a..410afd611 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -1,10 +1,17 @@ //! OS keyring access for desktop nsec private keys. //! -//! Backed by the `keyring` crate (macOS Keychain / Windows Credential Manager / -//! Linux Secret Service via D-Bus). The chosen backend is selected at compile -//! time by the per-target feature in `Cargo.toml`. The `system-keyring` -//! feature gates the whole store; when it is off, [`SecretStore`] is unusable -//! and callers fall back to their own `0o600` file storage. +//! All secrets are stored as a single JSON blob under one keychain entry +//! (service = the store's service name, username = `"secrets"`). This means +//! exactly one OS prompt per process lifetime regardless of how many keys are +//! stored — the same pattern used by Goose. +//! +//! The chosen backend is selected at compile time by the per-target feature in +//! `Cargo.toml`. On macOS the modern Data Protection Keychain API is used; +//! unsigned dev builds (which lack the hardened-runtime entitlement) fall back +//! to the legacy `keyring` crate automatically. Windows and Linux use the +//! `keyring` crate directly. The `system-keyring` feature gates the whole +//! store; when it is off, [`SecretStore`] is unusable and callers fall back to +//! their own `0o600` file storage. //! //! The store is deliberately NOT on any env-read path. `BUZZ_PRIVATE_KEY` //! resolution for harnessed agents and CI is handled upstream (an env @@ -12,6 +19,9 @@ //! adding an env tier here would duplicate that precedence and create a //! divergent-behavior trap. +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; + /// Result of probing the keyring before a migration: distinguishes "reachable /// but holds no entry" (safe to migrate into) from "unreachable this boot" /// (must NOT migrate — re-importing from a leftover plaintext file could @@ -27,10 +37,16 @@ pub enum KeyringProbe { Unreachable, } -/// An OS keyring, addressed by service name. Each logical secret is a distinct -/// key within the service (passed to each operation as the keyring "username"). +/// Username used for the single blob keychain entry. All secrets are stored +/// as a JSON map under this name within the service. +const BLOB_KEY: &str = "secrets"; + +/// An OS keyring, addressed by service name. All secrets are stored in a +/// single JSON blob entry (one OS prompt per process lifetime). pub struct SecretStore { service: String, + /// In-memory cache of the deserialized blob. `None` means "not yet loaded". + cache: Arc>>>, } impl SecretStore { @@ -40,6 +56,7 @@ impl SecretStore { pub fn keyring(service: impl Into) -> Self { SecretStore { service: service.into(), + cache: Arc::new(Mutex::new(None)), } } } @@ -97,55 +114,122 @@ fn dpk_opts(service: &str, key: &str) -> PasswordOptions { } impl SecretStore { - /// Probe whether `key` exists and whether the backend is reachable. - pub fn probe(&self, key: &str) -> KeyringProbe { - // macOS: probe the Data Protection Keychain first. If DPK is - // unavailable (unsigned dev build), fall back to the legacy keyring - // crate path. Items still in the old keychain will be migrated on the - // first `load` call. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] + /// Read the blob from the keychain and return the deserialized map. + /// + /// Returns `Ok(None)` when no blob entry exists yet (first launch or + /// fresh install). Returns `Err` when the backend is unavailable or the + /// stored JSON is corrupt. + /// + /// On success the result is stored in `self.cache` so subsequent calls + /// within the same process return immediately without a keychain round-trip. + #[cfg(feature = "system-keyring")] + fn load_blob(&self) -> Result>, String> { { - match generic_password(dpk_opts(&self.service, key)) { - Ok(_) => KeyringProbe::Present, - Err(ref e) if is_not_found(e) => KeyringProbe::ReachableButEmpty, - Err(ref e) if is_dpk_unavailable(e) => { - // DPK unavailable (unsigned build) — fall back to keyring. - match keyring_entry(&self.service, key) { - Ok(entry) => match entry.get_password() { - Ok(_) => KeyringProbe::Present, - Err(keyring::Error::NoEntry) => KeyringProbe::ReachableButEmpty, - Err(e) if is_keyring_availability_error(&e.to_string()) => { - KeyringProbe::Unreachable - } - Err(_) => KeyringProbe::ReachableButEmpty, - }, - Err(e) if is_keyring_availability_error(&e.to_string()) => { - KeyringProbe::Unreachable - } - Err(_) => KeyringProbe::Unreachable, - } - } - Err(ref e) if is_keyring_availability_error(&e.to_string()) => { - KeyringProbe::Unreachable - } - Err(_) => KeyringProbe::ReachableButEmpty, + let guard = self.cache.lock().unwrap(); + if let Some(ref map) = *guard { + return Ok(Some(map.clone())); } } - // Non-macOS system-keyring path (Windows, Linux). - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] + + let raw = self.read_blob_raw()?; + let map = match raw { + None => return Ok(None), + Some(bytes) => { + let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; + serde_json::from_str::>(&json) + .map_err(|e| format!("blob json: {e}"))? + } + }; + + *self.cache.lock().unwrap() = Some(map.clone()); + Ok(Some(map)) + } + + /// Read the raw blob bytes from the keychain. `Ok(None)` = not found. + #[cfg(all(feature = "system-keyring", target_os = "macos"))] + fn read_blob_raw(&self) -> Result>, String> { + match generic_password(dpk_opts(&self.service, BLOB_KEY)) { + Ok(bytes) => Ok(Some(bytes)), + Err(ref e) if is_not_found(e) => Ok(None), + Err(ref e) if is_dpk_unavailable(e) => { + // Unsigned dev build — fall back to legacy keyring crate. + self.read_blob_raw_keyring() + } + Err(e) => Err(format!("keyring read: {e}")), + } + } + + #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] + fn read_blob_raw(&self) -> Result>, String> { + self.read_blob_raw_keyring() + } + + /// Read blob via the legacy `keyring` crate (Windows, Linux, or macOS dev + /// builds that lack hardened-runtime entitlements). + #[cfg(feature = "system-keyring")] + fn read_blob_raw_keyring(&self) -> Result>, String> { + let entry = + keyring_entry(&self.service, BLOB_KEY).map_err(|e| format!("keyring entry: {e}"))?; + match entry.get_password() { + Ok(s) => Ok(Some(s.into_bytes())), + Err(keyring::Error::NoEntry) => Ok(None), + Err(e) if is_keyring_availability_error(&e.to_string()) => { + Err(format!("keyring unavailable: {e}")) + } + Err(e) => Err(format!("keyring read: {e}")), + } + } + + /// Serialize `map` to JSON and write it as the single blob keychain entry. + /// Invalidates the cache so the next `load_blob` re-reads from the store. + #[cfg(feature = "system-keyring")] + fn save_blob(&self, map: &HashMap) -> Result<(), String> { + let json = serde_json::to_string(map).map_err(|e| format!("blob serialize: {e}"))?; + self.write_blob_raw(json.as_bytes())?; + // Invalidate cache — next load_blob will re-read from keychain. + *self.cache.lock().unwrap() = None; + Ok(()) + } + + #[cfg(all(feature = "system-keyring", target_os = "macos"))] + fn write_blob_raw(&self, bytes: &[u8]) -> Result<(), String> { + match set_generic_password_options(bytes, dpk_opts(&self.service, BLOB_KEY)) { + Ok(()) => Ok(()), + Err(ref e) if is_dpk_unavailable(e) => self.write_blob_raw_keyring(bytes), + Err(e) => Err(format!("keyring write: {e}")), + } + } + + #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] + fn write_blob_raw(&self, bytes: &[u8]) -> Result<(), String> { + self.write_blob_raw_keyring(bytes) + } + + #[cfg(feature = "system-keyring")] + fn write_blob_raw_keyring(&self, bytes: &[u8]) -> Result<(), String> { + let value = std::str::from_utf8(bytes).map_err(|e| format!("blob utf8 encode: {e}"))?; + let entry = + keyring_entry(&self.service, BLOB_KEY).map_err(|e| format!("keyring entry: {e}"))?; + entry + .set_password(value) + .map_err(|e| format!("keyring write: {e}")) + } + + /// Probe whether `key` exists and whether the backend is reachable. + pub fn probe(&self, key: &str) -> KeyringProbe { + #[cfg(feature = "system-keyring")] { - match keyring_entry(&self.service, key) { - Ok(entry) => match entry.get_password() { - Ok(_) => KeyringProbe::Present, - Err(keyring::Error::NoEntry) => KeyringProbe::ReachableButEmpty, - Err(e) if is_keyring_availability_error(&e.to_string()) => { - KeyringProbe::Unreachable + match self.load_blob() { + Ok(Some(map)) => { + if map.contains_key(key) { + KeyringProbe::Present + } else { + KeyringProbe::ReachableButEmpty } - Err(_) => KeyringProbe::ReachableButEmpty, - }, - Err(e) if is_keyring_availability_error(&e.to_string()) => { - KeyringProbe::Unreachable } + // No blob yet — backend is reachable, key is absent. + Ok(None) => KeyringProbe::ReachableButEmpty, + Err(e) if is_keyring_availability_error(&e) => KeyringProbe::Unreachable, Err(_) => KeyringProbe::Unreachable, } } @@ -158,55 +242,22 @@ impl SecretStore { /// Load the secret for `key`. `Ok(None)` when there is no entry; `Err` only /// when the backend errored in a way that is not "missing". + /// + /// On first launch after an upgrade from the per-key DPK format, the blob + /// will not exist yet. In that case the macOS path falls back to reading the + /// old per-key DPK entry for `key` specifically, writes it into a new blob, + /// and deletes the old item — a one-time migration per key. pub fn load(&self, key: &str) -> Result, String> { - // macOS: try Data Protection Keychain first; fall back to old keychain - // and migrate on a miss (one-time per item). If DPK is unavailable - // (unsigned dev build, errSecMissingEntitlement), use the legacy - // keyring crate path directly — no migration needed in that case. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] + #[cfg(feature = "system-keyring")] { - match generic_password(dpk_opts(&self.service, key)) { - Ok(bytes) => String::from_utf8(bytes) - .map(Some) - .map_err(|e| format!("keyring utf8: {e}")), - Err(ref e) if is_not_found(e) => { - // Not in DPK — check old keychain and migrate if found. - let entry = keyring_entry(&self.service, key) - .map_err(|e| format!("keyring entry: {e}"))?; - match entry.get_password() { - Ok(old_val) => { - // Migrate to DPK. - self.store(key, &old_val)?; - // Best-effort cleanup from old keychain. - let _ = entry.delete_credential(); - Ok(Some(old_val)) - } - Err(keyring::Error::NoEntry) => Ok(None), - Err(e) => Err(format!("keyring get: {e}")), - } - } - Err(ref e) if is_dpk_unavailable(e) => { - // DPK unavailable (unsigned build) — use keyring directly. - let entry = keyring_entry(&self.service, key) - .map_err(|e| format!("keyring entry: {e}"))?; - match entry.get_password() { - Ok(secret) => Ok(Some(secret)), - Err(keyring::Error::NoEntry) => Ok(None), - Err(e) => Err(format!("keyring get: {e}")), - } + match self.load_blob() { + Ok(Some(map)) => Ok(map.get(key).cloned()), + Ok(None) => { + // No blob yet — attempt one-time migration from old per-key + // DPK entry (macOS) or return Ok(None) (other platforms). + self.migrate_legacy_key(key) } - Err(e) => Err(format!("keyring get: {e}")), - } - } - // Non-macOS system-keyring path (Windows, Linux). - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - { - let entry = - keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; - match entry.get_password() { - Ok(secret) => Ok(Some(secret)), - Err(keyring::Error::NoEntry) => Ok(None), - Err(e) => Err(format!("keyring get: {e}")), + Err(e) => Err(e), } } #[cfg(not(feature = "system-keyring"))] @@ -216,35 +267,63 @@ impl SecretStore { } } + /// On first launch after upgrading from the per-key DPK format, read the + /// old DPK entry for `key`, write it into a new blob, and delete the old + /// item. Returns `Ok(None)` when no old entry exists. + #[cfg(all(feature = "system-keyring", target_os = "macos"))] + fn migrate_legacy_key(&self, key: &str) -> Result, String> { + // Try the old per-key DPK entry. + match generic_password(dpk_opts(&self.service, key)) { + Ok(bytes) => { + let value = String::from_utf8(bytes).map_err(|e| format!("keyring utf8: {e}"))?; + // Write into blob (creates the blob if it doesn't exist). + self.store(key, &value)?; + // Best-effort cleanup of the old per-key entry. + let _ = delete_generic_password_options(dpk_opts(&self.service, key)); + Ok(Some(value)) + } + Err(ref e) if is_not_found(e) => { + // Also check the old keyring-crate entry (pre-#1264 installs). + self.migrate_legacy_key_keyring(key) + } + Err(ref e) if is_dpk_unavailable(e) => { + // Unsigned dev build — check old keyring-crate entry only. + self.migrate_legacy_key_keyring(key) + } + Err(e) => Err(format!("keyring get: {e}")), + } + } + + #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] + fn migrate_legacy_key(&self, key: &str) -> Result, String> { + // Non-macOS: no DPK, just check the old keyring-crate per-key entry. + self.migrate_legacy_key_keyring(key) + } + + /// Check the old per-key `keyring` crate entry (pre-#1264 format) and + /// migrate it into the blob if found. + #[cfg(feature = "system-keyring")] + fn migrate_legacy_key_keyring(&self, key: &str) -> Result, String> { + let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + match entry.get_password() { + Ok(value) => { + self.store(key, &value)?; + let _ = entry.delete_credential(); + Ok(Some(value)) + } + Err(keyring::Error::NoEntry) => Ok(None), + Err(e) => Err(format!("keyring get: {e}")), + } + } + /// Store `value` for `key`. Reports `Err` on availability failures — callers /// decide whether to fall back to file storage. pub fn store(&self, key: &str, value: &str) -> Result<(), String> { - // macOS: write directly to the Data Protection Keychain. If DPK is - // unavailable (unsigned dev build), fall back to the legacy keyring - // crate path. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] - { - match set_generic_password_options(value.as_bytes(), dpk_opts(&self.service, key)) { - Ok(()) => Ok(()), - Err(ref e) if is_dpk_unavailable(e) => { - // DPK unavailable (unsigned build) — use keyring directly. - let entry = keyring_entry(&self.service, key) - .map_err(|e| format!("keyring entry: {e}"))?; - entry - .set_password(value) - .map_err(|e| format!("keyring set: {e}")) - } - Err(e) => Err(format!("keyring set: {e}")), - } - } - // Non-macOS system-keyring path (Windows, Linux). - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] + #[cfg(feature = "system-keyring")] { - let entry = - keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; - entry - .set_password(value) - .map_err(|e| format!("keyring set: {e}")) + let mut map = self.load_blob()?.unwrap_or_default(); + map.insert(key.to_string(), value.to_string()); + self.save_blob(&map) } #[cfg(not(feature = "system-keyring"))] { @@ -255,45 +334,15 @@ impl SecretStore { /// Delete the secret for `key`. A missing entry is not an error. pub fn delete(&self, key: &str) -> Result<(), String> { - // macOS: delete from both DPK and old keychain (best-effort on old). - // If DPK is unavailable (unsigned dev build), fall back to the legacy - // keyring crate path. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] + #[cfg(feature = "system-keyring")] { - // Delete from Data Protection Keychain; missing is fine. - match delete_generic_password_options(dpk_opts(&self.service, key)) { - Ok(()) => {} - Err(ref e) if is_not_found(e) => {} - Err(ref e) if is_dpk_unavailable(e) => { - // DPK unavailable (unsigned build) — use keyring directly. - let entry = keyring_entry(&self.service, key) - .map_err(|e| format!("keyring entry: {e}"))?; - return match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), - Err(e) => Err(format!("keyring delete: {e}")), - }; + match self.load_blob()? { + Some(mut map) => { + map.remove(key); + self.save_blob(&map) } - Err(e) => return Err(format!("keyring delete: {e}")), - } - // Best-effort cleanup from old keychain. - if let Ok(entry) = keyring_entry(&self.service, key) { - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => {} - Err(e) => { - eprintln!("buzz-desktop: old-keychain delete for {key}: {e}"); - } - } - } - Ok(()) - } - // Non-macOS system-keyring path (Windows, Linux). - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - { - let entry = - keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), - Err(e) => Err(format!("keyring delete: {e}")), + // No blob — nothing to delete. + None => Ok(()), } } #[cfg(not(feature = "system-keyring"))] @@ -324,15 +373,62 @@ mod tests { #[cfg(target_os = "macos")] #[test] - fn dpk_unavailable_discriminator() { + fn dpk_error_discriminators() { // errSecMissingEntitlement = -34018 signals unsigned dev build. let e = SFError::from_code(-34018); assert!(is_dpk_unavailable(&e)); + assert!(!is_not_found(&e)); // errSecItemNotFound = -25300 is not a DPK-unavailable error. let e = SFError::from_code(-25300); + assert!(is_not_found(&e)); assert!(!is_dpk_unavailable(&e)); - // errSecDuplicateItem = -25299 is not a DPK-unavailable error. - let e = SFError::from_code(-25299); - assert!(!is_dpk_unavailable(&e)); + } + + // Integration tests that exercise the real OS keychain. Skipped in CI + // (unsigned builds lack keychain entitlements); run locally with: + // cargo test -p buzz-desktop -- --ignored blob_ + // + // Each test uses a unique service name to avoid cross-test pollution. + + #[ignore = "requires real OS keychain (run locally)"] + #[test] + fn blob_stores_and_retrieves_multiple_keys() { + let store = SecretStore::keyring("buzz-test-blob-multi"); + store.store("key_a", "val_a").unwrap(); + store.store("key_b", "val_b").unwrap(); + assert_eq!(store.load("key_a").unwrap(), Some("val_a".to_string())); + assert_eq!(store.load("key_b").unwrap(), Some("val_b".to_string())); + assert_eq!(store.load("key_c").unwrap(), None); + // Cleanup. + let _ = store.delete("key_a"); + let _ = store.delete("key_b"); + } + + #[ignore = "requires real OS keychain (run locally)"] + #[test] + fn blob_probe_present_absent_unreachable() { + let store = SecretStore::keyring("buzz-test-blob-probe"); + // No blob yet — key absent, backend reachable. + assert_eq!(store.probe("identity"), KeyringProbe::ReachableButEmpty); + store.store("identity", "nsec1test").unwrap(); + // Key now present. + assert_eq!(store.probe("identity"), KeyringProbe::Present); + // Different key — blob exists but key absent. + assert_eq!(store.probe("other"), KeyringProbe::ReachableButEmpty); + // Cleanup. + let _ = store.delete("identity"); + } + + #[ignore = "requires real OS keychain (run locally)"] + #[test] + fn blob_delete_removes_key_not_others() { + let store = SecretStore::keyring("buzz-test-blob-delete"); + store.store("keep", "keep_val").unwrap(); + store.store("remove", "remove_val").unwrap(); + store.delete("remove").unwrap(); + assert_eq!(store.load("keep").unwrap(), Some("keep_val".to_string())); + assert_eq!(store.load("remove").unwrap(), None); + // Cleanup. + let _ = store.delete("keep"); } } From d9f17bfeec11cb699ded616af67ee8818e24a0d9 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Wed, 24 Jun 2026 23:28:47 -0400 Subject: [PATCH 2/3] fix(desktop): address review findings for keychain blob consolidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit probe() only checked the new blob format, so users upgrading from per-key DPK (#1264) would get silent identity rotation — probe returned ReachableButEmpty, resolve_identity_with_store never called load(), and generate_and_persist created a new key. Add probe_legacy_key() to check old per-key DPK/keyring entries when the blob is absent. Also: save_blob() now updates the cache instead of invalidating it (avoids unnecessary keychain re-reads on sequential operations), non-availability errors in probe() map to ReachableButEmpty (matching old behavior instead of triggering fail-close), agent_secret_store() caches its SecretStore via OnceLock so the blob cache survives across call sites, Arc dropped from the cache field (SecretStore is never cloned), mutex locks recover from poisoning instead of panicking, and a migration integration test is added. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/storage.rs | 25 +++-- desktop/src-tauri/src/secret_store.rs | 93 ++++++++++++++++--- 2 files changed, 98 insertions(+), 20 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 495c7c1dd..b186a8bfb 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -17,13 +17,20 @@ fn agent_keyring_name(pubkey: &str) -> String { } /// The agent secret store. `None` when the build has no keyring backend, in -/// which case agent keys stay inline in the `0o600` JSON file. -fn agent_secret_store() -> Option { - if cfg!(feature = "system-keyring") { - Some(SecretStore::keyring(KEYRING_SERVICE)) - } else { - None - } +/// which case agent keys stay inline in the `0o600` JSON file. Cached via +/// `OnceLock` so the in-memory blob cache survives across call sites. +fn agent_secret_store() -> Option<&'static SecretStore> { + use std::sync::OnceLock; + static STORE: OnceLock> = OnceLock::new(); + STORE + .get_or_init(|| { + if cfg!(feature = "system-keyring") { + Some(SecretStore::keyring(KEYRING_SERVICE)) + } else { + None + } + }) + .as_ref() } pub fn managed_agents_base_dir(app: &AppHandle) -> Result { @@ -173,7 +180,7 @@ fn hydrate_keys(records: &mut [ManagedAgentRecord]) { let Some(store) = agent_secret_store() else { return; }; - hydrate_keys_with(&store, records); + hydrate_keys_with(store, records); } /// Testable core of [`hydrate_keys`], generic over the [`KeyStore`] seam. @@ -257,7 +264,7 @@ fn persist_agent_keys(records: &mut [ManagedAgentRecord]) { // unreachable) so it is not lost, and `Nothing` (empty key) because // there is no verified entry to claim. This is a save-local clone, so // callers keep their keys regardless. - if migrate_inline_key(&store, record) == KeyMigration::Persisted { + if migrate_inline_key(store, record) == KeyMigration::Persisted { record.private_key_nsec.clear(); } } diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index 410afd611..4af54e1d1 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -20,7 +20,7 @@ //! divergent-behavior trap. use std::collections::HashMap; -use std::sync::{Arc, Mutex}; +use std::sync::Mutex; /// Result of probing the keyring before a migration: distinguishes "reachable /// but holds no entry" (safe to migrate into) from "unreachable this boot" @@ -46,7 +46,7 @@ const BLOB_KEY: &str = "secrets"; pub struct SecretStore { service: String, /// In-memory cache of the deserialized blob. `None` means "not yet loaded". - cache: Arc>>>, + cache: Mutex>>, } impl SecretStore { @@ -56,7 +56,7 @@ impl SecretStore { pub fn keyring(service: impl Into) -> Self { SecretStore { service: service.into(), - cache: Arc::new(Mutex::new(None)), + cache: Mutex::new(None), } } } @@ -125,7 +125,7 @@ impl SecretStore { #[cfg(feature = "system-keyring")] fn load_blob(&self) -> Result>, String> { { - let guard = self.cache.lock().unwrap(); + let guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); if let Some(ref map) = *guard { return Ok(Some(map.clone())); } @@ -141,7 +141,7 @@ impl SecretStore { } }; - *self.cache.lock().unwrap() = Some(map.clone()); + *self.cache.lock().unwrap_or_else(|e| e.into_inner()) = Some(map.clone()); Ok(Some(map)) } @@ -181,13 +181,11 @@ impl SecretStore { } /// Serialize `map` to JSON and write it as the single blob keychain entry. - /// Invalidates the cache so the next `load_blob` re-reads from the store. #[cfg(feature = "system-keyring")] fn save_blob(&self, map: &HashMap) -> Result<(), String> { let json = serde_json::to_string(map).map_err(|e| format!("blob serialize: {e}"))?; self.write_blob_raw(json.as_bytes())?; - // Invalidate cache — next load_blob will re-read from keychain. - *self.cache.lock().unwrap() = None; + *self.cache.lock().unwrap_or_else(|e| e.into_inner()) = Some(map.clone()); Ok(()) } @@ -227,10 +225,11 @@ impl SecretStore { KeyringProbe::ReachableButEmpty } } - // No blob yet — backend is reachable, key is absent. - Ok(None) => KeyringProbe::ReachableButEmpty, + // No blob yet — check old per-key entries so callers that + // gate `load()` on `Present` still trigger migration. + Ok(None) => self.probe_legacy_key(key), Err(e) if is_keyring_availability_error(&e) => KeyringProbe::Unreachable, - Err(_) => KeyringProbe::Unreachable, + Err(_) => KeyringProbe::ReachableButEmpty, } } #[cfg(not(feature = "system-keyring"))] @@ -240,6 +239,42 @@ impl SecretStore { } } + /// Check old per-key DPK/keyring entries for `key`. Used by `probe()` when + /// the blob doesn't exist yet (first launch after upgrade). + #[cfg(all(feature = "system-keyring", target_os = "macos"))] + fn probe_legacy_key(&self, key: &str) -> KeyringProbe { + match generic_password(dpk_opts(&self.service, key)) { + Ok(_) => KeyringProbe::Present, + Err(ref e) if is_not_found(e) => self.probe_legacy_key_keyring(key), + Err(ref e) if is_dpk_unavailable(e) => self.probe_legacy_key_keyring(key), + Err(ref e) if is_keyring_availability_error(&e.to_string()) => { + KeyringProbe::Unreachable + } + Err(_) => KeyringProbe::ReachableButEmpty, + } + } + + #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] + fn probe_legacy_key(&self, key: &str) -> KeyringProbe { + self.probe_legacy_key_keyring(key) + } + + #[cfg(feature = "system-keyring")] + fn probe_legacy_key_keyring(&self, key: &str) -> KeyringProbe { + match keyring_entry(&self.service, key) { + Ok(entry) => match entry.get_password() { + Ok(_) => KeyringProbe::Present, + Err(keyring::Error::NoEntry) => KeyringProbe::ReachableButEmpty, + Err(e) if is_keyring_availability_error(&e.to_string()) => { + KeyringProbe::Unreachable + } + Err(_) => KeyringProbe::ReachableButEmpty, + }, + Err(e) if is_keyring_availability_error(&e.to_string()) => KeyringProbe::Unreachable, + Err(_) => KeyringProbe::Unreachable, + } + } + /// Load the secret for `key`. `Ok(None)` when there is no entry; `Err` only /// when the backend errored in a way that is not "missing". /// @@ -431,4 +466,40 @@ mod tests { // Cleanup. let _ = store.delete("keep"); } + + #[ignore = "requires real OS keychain (run locally)"] + #[test] + fn blob_migration_from_per_key_entry() { + let svc = "buzz-test-blob-migration"; + let key = "identity"; + let value = "nsec1migrationtest"; + + // Seed a per-key entry (old format) — no blob exists. + let entry = keyring_entry(svc, key).unwrap(); + entry.set_password(value).unwrap(); + + // Fresh store — no blob in the keychain yet. + let store = SecretStore::keyring(svc); + + // probe should find the legacy key. + assert_eq!(store.probe(key), KeyringProbe::Present); + + // load should migrate it into the blob and return the value. + assert_eq!(store.load(key).unwrap(), Some(value.to_string())); + + // Old per-key entry should be cleaned up. + let entry = keyring_entry(svc, key).unwrap(); + assert!(matches!( + entry.get_password(), + Err(keyring::Error::NoEntry) + )); + + // Key is now in the blob — probe confirms. + let store2 = SecretStore::keyring(svc); + assert_eq!(store2.probe(key), KeyringProbe::Present); + assert_eq!(store2.load(key).unwrap(), Some(value.to_string())); + + // Cleanup. + let _ = store2.delete(key); + } } From 9bad1782963eea352edaab217b8677eb9a61e77e Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Wed, 24 Jun 2026 23:31:40 -0400 Subject: [PATCH 3/3] style: fix rustfmt in migration test Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/secret_store.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index 4af54e1d1..2fd9c76e4 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -489,10 +489,7 @@ mod tests { // Old per-key entry should be cleaned up. let entry = keyring_entry(svc, key).unwrap(); - assert!(matches!( - entry.get_password(), - Err(keyring::Error::NoEntry) - )); + assert!(matches!(entry.get_password(), Err(keyring::Error::NoEntry))); // Key is now in the blob — probe confirms. let store2 = SecretStore::keyring(svc);