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 2fd9c76e4..862abe6d1 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 @@ -59,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 @@ -85,8 +99,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")))] @@ -180,22 +194,49 @@ 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(()) } + /// 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")))] @@ -222,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"))] @@ -281,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). @@ -305,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) => { @@ -356,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"))] { @@ -371,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"))] { @@ -392,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"));