From 8146e9cef826b67c8ad9051657998f76ffd831b1 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Wed, 24 Jun 2026 23:48:53 -0400 Subject: [PATCH 1/2] fix(desktop): always use legacy keyring for blob entry on macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DPK and the legacy keyring are separate OS stores — a blob written by a signed build (DPK) is invisible to dev builds (legacy keyring) and vice versa. A user who ran the signed v0.3.32 release (which migrated per-key entries into a DPK blob) then switched to `tauri dev` would get Ok(None) for every agent key because the dev build reads from the legacy keyring, which is empty. Fix by always using the legacy keyring crate for the blob on macOS. Both signed release and unsigned dev builds now share the same store. The legacy keyring is still the macOS Keychain — secure, just the older SecKeychain API rather than SecItem/DPK. The per-key DPK migration path in migrate_legacy_key is unchanged: it still reads old DPK entries on signed builds for the one-time upgrade from the #1264 per-key format. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/secret_store.rs | 37 +++++++++++---------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index 2fd9c76e4..fba61c145 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -6,12 +6,13 @@ //! 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. +//! `Cargo.toml`. On macOS the legacy `keyring` crate (SecKeychain API) is used +//! for the blob entry so that signed release builds and unsigned dev builds +//! share the same store. DPK (Data Protection Keychain) is used only by the +//! one-time migration path that reads old per-key entries written by #1264. +//! 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 @@ -85,8 +86,7 @@ fn keyring_entry(service: &str, key: &str) -> Result 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}")), - } + self.read_blob_raw_keyring() } #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] @@ -189,13 +185,10 @@ impl SecretStore { Ok(()) } + /// Always uses the legacy keyring crate on macOS — see `read_blob_raw`. #[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}")), - } + self.write_blob_raw_keyring(bytes) } #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] From a2043b1418340c1fac64ddb00b48ed7345693870 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 25 Jun 2026 00:03:50 -0400 Subject: [PATCH 2/2] fix(desktop): fix stale cache overwrite, DPK merge conflict, and add load cache-hit test Three more correctness fixes: 1. load_blob() now only populates the cache when it is still None, preventing a cold reader from overwriting a newer cache entry written by mutate_blob() between the two lock acquisitions. 2. DPK blob migration now uses mutate_blob() with entry().or_insert_with() so existing legacy values are preserved on conflict (legacy wins). 3. Adds a non-ignored load() cache-hit test that runs in CI without the OS keychain. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/app_state.rs | 4 +- .../src-tauri/src/managed_agents/storage.rs | 22 +-- desktop/src-tauri/src/secret_store.rs | 165 ++++++++++++++++-- 3 files changed, 158 insertions(+), 33 deletions(-) diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index c63ecb2ff..4f595c348 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -217,8 +217,8 @@ fn load_or_create_identity(data_dir: &std::path::Path) -> Result { return load_file_or_generate(&legacy_path, data_dir); } - let store = crate::secret_store::SecretStore::keyring(KEYRING_SERVICE); - resolve_identity_with_store(&store, &legacy_path, data_dir) + let store = crate::secret_store::SecretStore::shared(KEYRING_SERVICE); + resolve_identity_with_store(store, &legacy_path, data_dir) } /// Identity resolution over an [`IdentityKeyStore`] seam. Split from diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index b186a8bfb..bcb86230e 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -17,20 +17,16 @@ 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. Cached via -/// `OnceLock` so the in-memory blob cache survives across call sites. +/// which case agent keys stay inline in the `0o600` JSON file. Uses +/// `SecretStore::shared` so identity and agent callers share one instance — +/// and therefore one in-memory cache and one mutex — preventing last-writer-wins +/// races on concurrent blob writes. 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() + if cfg!(feature = "system-keyring") { + Some(SecretStore::shared(KEYRING_SERVICE)) + } else { + None + } } pub fn managed_agents_base_dir(app: &AppHandle) -> Result { diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index fba61c145..862abe6d1 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -60,6 +60,19 @@ impl SecretStore { cache: Mutex::new(None), } } + + /// Return a process-global `SecretStore` for `service`. All callers with + /// the same service name share one instance — and therefore one in-memory + /// cache and one mutex — so concurrent blob read-modify-write operations + /// see each other's writes and the last-writer-wins race is closed. + /// + /// Only one service name (`"buzz-desktop"`) is used in practice. If a + /// second service name is ever needed, this can be extended to a registry. + pub fn shared(service: &'static str) -> &'static SecretStore { + use std::sync::OnceLock; + static INSTANCE: OnceLock = OnceLock::new(); + INSTANCE.get_or_init(|| SecretStore::keyring(service)) + } } /// Whether a keyring error string indicates the backend itself is unavailable @@ -141,7 +154,12 @@ impl SecretStore { } }; - *self.cache.lock().unwrap_or_else(|e| e.into_inner()) = Some(map.clone()); + // Only populate the cache if it is still empty — a concurrent + // mutate_blob() may have written a newer value while we were reading. + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + if guard.is_none() { + *guard = Some(map.clone()); + } Ok(Some(map)) } @@ -176,12 +194,42 @@ impl SecretStore { } } - /// Serialize `map` to JSON and write it as the single blob keychain entry. + /// Atomically load the blob, apply `f` to the map, write back, and update + /// the cache — all while holding the cache mutex. This serializes concurrent + /// writes so no caller can observe a partial update. + /// + /// Deadlock-free: `read_blob_raw` and `write_blob_raw` do not acquire the + /// cache mutex. `load_blob` does acquire it, but `mutate_blob` does not call + /// `load_blob` — it reads from the keyring directly when the cache is cold. #[cfg(feature = "system-keyring")] - fn save_blob(&self, map: &HashMap) -> Result<(), String> { + fn mutate_blob(&self, f: F) -> Result<(), String> + where + F: FnOnce(&mut HashMap), + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + + // If cache is cold, load from keyring now — while holding the lock so + // no concurrent writer can sneak in between the read and our write. + if guard.is_none() { + let raw = self.read_blob_raw()?; + *guard = Some(match raw { + None => HashMap::new(), + 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}"))? + } + }); + } + + let map = guard.as_mut().expect("just initialized above"); + f(map); + + // Write to keyring while still holding the lock. let json = serde_json::to_string(map).map_err(|e| format!("blob serialize: {e}"))?; self.write_blob_raw(json.as_bytes())?; - *self.cache.lock().unwrap_or_else(|e| e.into_inner()) = Some(map.clone()); + + // Cache is already up to date (we mutated it in place). Ok(()) } @@ -215,14 +263,17 @@ impl SecretStore { if map.contains_key(key) { KeyringProbe::Present } else { - KeyringProbe::ReachableButEmpty + // Blob exists but key absent — still check old per-key + // entries so a partial migration (e.g. identity migrated + // first) doesn't silently drop agent keys. + self.probe_legacy_key(key) } } // 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::ReachableButEmpty, + Err(_) => KeyringProbe::Unreachable, // corrupt blob — fail closed } } #[cfg(not(feature = "system-keyring"))] @@ -274,12 +325,23 @@ impl SecretStore { /// 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. + /// and deletes the old item — a one-time migration per key. The same + /// migration fires when the blob exists but the key is absent, covering + /// partial-migration scenarios (e.g. identity migrated first, agents not yet). pub fn load(&self, key: &str) -> Result, String> { #[cfg(feature = "system-keyring")] { match self.load_blob() { - Ok(Some(map)) => Ok(map.get(key).cloned()), + Ok(Some(map)) => { + if let Some(value) = map.get(key) { + Ok(Some(value.clone())) + } else { + // Blob exists but key absent — attempt migration from old + // per-key entry. migrate_legacy_key writes the result into + // the blob if found, so subsequent loads hit the cache. + self.migrate_legacy_key(key) + } + } Ok(None) => { // No blob yet — attempt one-time migration from old per-key // DPK entry (macOS) or return Ok(None) (other platforms). @@ -298,8 +360,39 @@ 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. + /// + /// Also handles a one-time migration from the DPK blob format written by + /// #1267 (before the dev/release split was fixed). Anyone who ran main + /// while #1267 was present has a DPK blob instead of per-key entries; this + /// reads it, merges all keys into the legacy blob, and deletes the DPK blob. #[cfg(all(feature = "system-keyring", target_os = "macos"))] fn migrate_legacy_key(&self, key: &str) -> Result, String> { + // One-time migration: check for a DPK blob (key = BLOB_KEY = "secrets") + // written by #1267 before the dev/release split was fixed. + match generic_password(dpk_opts(&self.service, BLOB_KEY)) { + Ok(bytes) => { + let json = String::from_utf8(bytes).map_err(|e| format!("dpk blob utf8: {e}"))?; + let dpk_map = serde_json::from_str::>(&json) + .map_err(|e| format!("dpk blob json: {e}"))?; + // Merge all keys from the DPK blob into the legacy blob. + self.mutate_blob(|map| { + for (k, v) in &dpk_map { + map.entry(k.clone()).or_insert_with(|| v.clone()); + } + })?; + // Best-effort delete the DPK blob. + let _ = delete_generic_password_options(dpk_opts(&self.service, BLOB_KEY)); + return Ok(dpk_map.get(key).cloned()); + } + Err(ref e) if is_not_found(e) => { + // No DPK blob — fall through to per-key migration. + } + Err(ref e) if is_dpk_unavailable(e) => { + // Unsigned dev build — DPK inaccessible, fall through. + } + Err(e) => return Err(format!("dpk blob read: {e}")), + } + // Try the old per-key DPK entry. match generic_password(dpk_opts(&self.service, key)) { Ok(bytes) => { @@ -349,9 +442,9 @@ impl SecretStore { pub fn store(&self, key: &str, value: &str) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - let mut map = self.load_blob()?.unwrap_or_default(); - map.insert(key.to_string(), value.to_string()); - self.save_blob(&map) + self.mutate_blob(|map| { + map.insert(key.to_string(), value.to_string()); + }) } #[cfg(not(feature = "system-keyring"))] { @@ -364,14 +457,17 @@ impl SecretStore { pub fn delete(&self, key: &str) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - match self.load_blob()? { - Some(mut map) => { - map.remove(key); - self.save_blob(&map) - } - // No blob — nothing to delete. - None => Ok(()), + self.mutate_blob(|map| { + map.remove(key); + })?; + // Best-effort: also delete any old per-key entry for this key to + // prevent resurrection on the next probe/load (migration path). + #[cfg(target_os = "macos")] + let _ = delete_generic_password_options(dpk_opts(&self.service, key)); + if let Ok(entry) = keyring_entry(&self.service, key) { + let _ = entry.delete_credential(); } + Ok(()) } #[cfg(not(feature = "system-keyring"))] { @@ -385,6 +481,39 @@ impl SecretStore { mod tests { use super::*; + // Test-only constructor: pre-seed the cache without touching the OS keychain. + impl SecretStore { + fn with_cache(service: &str, cache: Option>) -> Self { + SecretStore { + service: service.to_string(), + cache: Mutex::new(cache), + } + } + } + + #[test] + fn probe_returns_present_when_key_in_cache() { + let mut map = HashMap::new(); + map.insert("identity".to_string(), "nsec1test".to_string()); + let store = SecretStore::with_cache("buzz-test-cache-hit", Some(map)); + // Cache is warm and contains "identity" — probe must return Present + // without touching the keychain. + assert_eq!(store.probe("identity"), KeyringProbe::Present); + } + + #[test] + fn load_returns_value_when_key_in_cache() { + let mut map = HashMap::new(); + map.insert("identity".to_string(), "nsec1test".to_string()); + let store = SecretStore::with_cache("buzz-test-load-cache-hit", Some(map)); + // Cache is warm and contains "identity" — load must return the value + // without touching the keychain. + assert_eq!( + store.load("identity").unwrap(), + Some("nsec1test".to_string()) + ); + } + #[test] fn availability_error_discriminator() { assert!(is_keyring_availability_error("dbus connection failed"));