diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index 43854761b50..b9c52d2f6ae 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -1,9 +1,20 @@ //! OS keyring access for desktop nsec private keys. //! -//! 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. +//! Each secret is stored as its **own** OS credential (service = the store's +//! service name, username = the secret's key name, e.g. `"identity"` or +//! `"agent:"`). A separate chunked *index* (`"secrets-index"`, +//! `"secrets-index-1"`, …) records which key names exist so the whole set can +//! be enumerated without a platform-specific credential-enumeration API. +//! +//! Secrets used to live in a single JSON blob under one entry (username = +//! `"secrets"`). That blob is a hard cap, not a style choice: the `keyring` +//! crate rejects any write over Windows' `CRED_MAX_CREDENTIAL_BLOB_SIZE` +//! (2560 bytes) *before* calling `CredWriteW`, so once the blob held an +//! identity plus 8 agents (~2380 bytes) every subsequent agent write failed +//! atomically and fell back to inline plaintext storage. One credential per +//! secret removes the shared budget — each agent key is ~140 bytes on its own. +//! [`SecretStore::ensure_migrated`] performs the one-time, verified, all-or- +//! nothing move out of the legacy blob on first access. //! //! The chosen backend is selected at compile time by the per-target feature in //! `Cargo.toml`. On macOS the legacy `keyring` crate (SecKeychain API) is used @@ -39,10 +50,53 @@ pub enum KeyringProbe { Unreachable, } -/// Username used for the single blob keychain entry. All secrets are stored -/// as a JSON map under this name within the service. +/// Username of the **legacy** single-blob keychain entry. No longer written; +/// read once by [`SecretStore::ensure_migrated`] and deleted after every secret +/// it held has been rewritten as its own credential and read back verified. const BLOB_KEY: &str = "secrets"; +/// Username of index chunk 0. Chunk `i > 0` lives at `"secrets-index-"`. +/// +/// The index holds key *names* only — never secret values — so a corrupt or +/// partially-written index can never lose a secret: `load()` addresses each +/// credential by name and never consults the index. +#[cfg(feature = "system-keyring")] +const INDEX_KEY: &str = "secrets-index"; + +/// Character budget for one index chunk. Windows caps a credential blob at +/// `CRED_MAX_CREDENTIAL_BLOB_SIZE` = 2560 bytes and the backend measures the +/// UTF-16 encoding, so 1000 chars = 2000 bytes leaves ample headroom. Without +/// chunking the index would simply reintroduce the blob's cliff at ~17 agents +/// (an `"agent:"` name is ~70 chars). +#[cfg(feature = "system-keyring")] +const INDEX_CHUNK_CHARS: usize = 1000; + +/// Hard ceiling on index chunks, so a corrupt chunk count can never spin the +/// reader over unbounded credential lookups. 512 chunks ≈ 6500 secrets. +#[cfg(feature = "system-keyring")] +const MAX_INDEX_CHUNKS: usize = 512; + +/// Username of index chunk `i`. +#[cfg(feature = "system-keyring")] +fn index_chunk_key(i: usize) -> String { + if i == 0 { + INDEX_KEY.to_string() + } else { + format!("{INDEX_KEY}-{i}") + } +} + +/// One index chunk as stored. `chunks` is present only on chunk 0, where it +/// declares how many chunks the reader must visit. +#[cfg(feature = "system-keyring")] +#[derive(serde::Serialize, serde::Deserialize, Default)] +struct IndexChunk { + #[serde(default, skip_serializing_if = "Option::is_none")] + chunks: Option, + #[serde(default)] + names: Vec, +} + // ── Interprocess advisory lock ───────────────────────────────────────────── // // Two concurrent Buzz processes (e.g. the signed DMG build and an unsigned dev @@ -213,12 +267,23 @@ impl Drop for BlobLockGuard { // ── End interprocess advisory lock ──────────────────────────────────────── -/// An OS keyring, addressed by service name. All secrets are stored in a -/// single JSON blob entry (one OS prompt per process lifetime). +/// An OS keyring, addressed by service name. Each secret is one credential +/// under that service, keyed by the secret's name. pub struct SecretStore { service: String, - /// In-memory cache of the deserialized blob. `None` means "not yet loaded". + /// In-memory cache of key→value pairs already read from (or written to) + /// the OS this process. **Partial**: a missing entry means "not loaded", + /// never "not stored", so a cache miss always falls through to the OS. cache: Mutex>>, + /// `true` once the legacy blob has been migrated away (or was never + /// present). Left `false` on failure so the next access retries — the + /// migration is idempotent. + migrated: Mutex, + /// Test-only: route all credential I/O to an in-process fake backend that + /// enforces the same Windows blob-size cap the real backend does. + #[cfg(test)] + #[allow(dead_code)] + fake: bool, } impl SecretStore { @@ -229,6 +294,9 @@ impl SecretStore { SecretStore { service: service.into(), cache: Mutex::new(None), + migrated: Mutex::new(false), + #[cfg(test)] + fake: false, } } @@ -298,200 +366,420 @@ fn dpk_opts(service: &str, key: &str) -> PasswordOptions { } impl SecretStore { - /// 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. + // ── Credential I/O ──────────────────────────────────────────────────── + // + // Every credential read/write/delete in this module funnels through these + // three functions, so the test fake has exactly one injection point and a + // fake-backed store can never reach the real OS keychain. + + /// Read one credential under this service. `Ok(None)` = no such entry. /// - /// On success the result is stored in `self.cache` so subsequent calls - /// within the same process return immediately without a keychain round-trip. + /// Always uses the `keyring` crate — on macOS that is the legacy + /// SecKeychain API, so signed release builds and unsigned dev builds share + /// one store. DPK is used only by the `migrate_legacy_key` read paths. #[cfg(feature = "system-keyring")] - fn load_blob(&self) -> Result>, String> { + fn entry_get(&self, key: &str) -> Result, String> { + #[cfg(test)] { - let guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - if let Some(ref map) = *guard { - return Ok(Some(map.clone())); + if self.fake { + return fake_backend::get(&self.service, key); } } + let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + match entry.get_password() { + Ok(s) => Ok(Some(s)), + 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}")), + } + } - 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}"))? + /// Write one credential under this service. + #[cfg(feature = "system-keyring")] + fn entry_set(&self, key: &str, value: &str) -> Result<(), String> { + #[cfg(test)] + { + if self.fake { + return fake_backend::set(&self.service, key, value); } - }; + } + let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + entry + .set_password(value) + .map_err(|e| format!("keyring write: {e}")) + } - // 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()); + /// Delete one credential under this service. A missing entry is not an error. + #[cfg(feature = "system-keyring")] + fn entry_delete(&self, key: &str) -> Result<(), String> { + #[cfg(test)] + { + if self.fake { + return fake_backend::delete(&self.service, key); + } + } + 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) if is_keyring_availability_error(&e.to_string()) => { + Err(format!("keyring unavailable: {e}")) + } + Err(e) => Err(format!("keyring delete: {e}")), } - Ok(Some(map)) } - /// Read the raw blob bytes from the keychain. `Ok(None)` = not found. - /// - /// Always uses the legacy keyring crate on macOS so that signed and - /// unsigned (dev) builds share the same store. DPK is only used by - /// `migrate_legacy_key` to read old per-key entries written by #1264. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] + /// Read the raw **legacy** blob bytes. `Ok(None)` = not found (the normal + /// state after migration). + #[cfg(feature = "system-keyring")] fn read_blob_raw(&self) -> Result>, String> { - self.read_blob_raw_keyring() + Ok(self.entry_get(BLOB_KEY)?.map(String::into_bytes)) } - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - fn read_blob_raw(&self) -> Result>, String> { - self.read_blob_raw_keyring() + /// Read the credential holding the secret named `key`, bypassing the cache. + #[cfg(feature = "system-keyring")] + fn read_secret_raw(&self, key: &str) -> Result, String> { + self.entry_get(key) } - /// Read blob via the legacy `keyring` crate (Windows, Linux, or macOS dev - /// builds that lack hardened-runtime entitlements). + // ── Name index ──────────────────────────────────────────────────────── + + /// Read every indexed key name plus the chunk count currently on disk. + /// + /// A missing or corrupt chunk is skipped rather than fatal: the index is + /// only an enumeration aid, and `load()` addresses credentials by name. #[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}")) + fn read_index_raw(&self) -> Result<(Vec, usize), String> { + let Some(head_raw) = self.entry_get(INDEX_KEY)? else { + return Ok((Vec::new(), 0)); + }; + let head: IndexChunk = + serde_json::from_str(&head_raw).map_err(|e| format!("index json: {e}"))?; + let count = head.chunks.unwrap_or(1).clamp(1, MAX_INDEX_CHUNKS); + let mut names = head.names; + for i in 1..count { + if let Some(raw) = self.entry_get(&index_chunk_key(i))? { + if let Ok(chunk) = serde_json::from_str::(&raw) { + names.extend(chunk.names); + } } - Err(e) => Err(format!("keyring read: {e}")), } + names.sort(); + names.dedup(); + Ok((names, count)) } - /// Atomically load the blob, apply `f` to a candidate map, write back if - /// changed, and only then advance the cache. + /// Replace the index with `names`, splitting it across as many chunks as + /// the per-credential size cap requires. /// - /// **Cross-process safety**: acquires an exclusive advisory file lock - /// (`flock(2)` on Unix, `LockFileEx` on Windows) before reading, mutating, - /// and writing. The lock is keyed by service name and stored in the system - /// temp directory, making it reachable from both the signed DMG build and - /// unsigned dev builds. Inside the lock a fresh `read_blob_raw()` is always - /// performed (even when the cache is warm) so a concurrent process's write - /// is never silently dropped. - /// - /// **Idempotent**: when `f` leaves the candidate equal to the freshly-read - /// map, `write_blob_raw` is skipped entirely. On macOS the legacy - /// `SecKeychain` API treats a write as a distinct ACL operation from the - /// "Always Allow"-ed read, so skipping no-op writes eliminates the keychain - /// prompt that fires when saving an agent whose model changed but whose key - /// did not. + /// Tail chunks are written before chunk 0 so the count chunk 0 declares is + /// never larger than the data actually present. `prev_chunks` is the count + /// read alongside the names, used to drop chunks a shrinking index leaves + /// behind. + #[cfg(feature = "system-keyring")] + fn write_index(&self, names: &[String], prev_chunks: usize) -> Result<(), String> { + let mut sorted: Vec = names.to_vec(); + sorted.sort(); + sorted.dedup(); + + let mut chunks: Vec> = vec![Vec::new()]; + let mut used = 0usize; + for name in sorted { + // +4 covers the two quotes, the comma and JSON escaping slack. + let cost = name.chars().count() + 4; + if used + cost > INDEX_CHUNK_CHARS + && !chunks.last().map(Vec::is_empty).unwrap_or(true) + && chunks.len() < MAX_INDEX_CHUNKS + { + chunks.push(Vec::new()); + used = 0; + } + used += cost; + if let Some(last) = chunks.last_mut() { + last.push(name); + } + } + let n_chunks = chunks.len(); + + for i in (1..n_chunks).rev() { + let body = serde_json::to_string(&IndexChunk { + chunks: None, + names: chunks[i].clone(), + }) + .map_err(|e| format!("index serialize: {e}"))?; + self.entry_set(&index_chunk_key(i), &body)?; + } + let head = serde_json::to_string(&IndexChunk { + chunks: Some(n_chunks), + names: chunks[0].clone(), + }) + .map_err(|e| format!("index serialize: {e}"))?; + self.entry_set(INDEX_KEY, &head)?; + + for i in n_chunks..prev_chunks.min(MAX_INDEX_CHUNKS) { + let _ = self.entry_delete(&index_chunk_key(i)); + } + Ok(()) + } + + /// Add `keys` to the index if any are missing. No write when all present. + #[cfg(feature = "system-keyring")] + fn index_insert(&self, keys: &[String]) -> Result<(), String> { + let (mut names, prev) = self.read_index_raw()?; + let mut changed = false; + for key in keys { + if !names.iter().any(|n| n == key) { + names.push(key.clone()); + changed = true; + } + } + if changed { + self.write_index(&names, prev)?; + } + Ok(()) + } + + /// Drop `key` from the index. No write when it was not listed. + #[cfg(feature = "system-keyring")] + fn index_remove(&self, key: &str) -> Result<(), String> { + let (mut names, prev) = self.read_index_raw()?; + let before = names.len(); + names.retain(|n| n != key); + if names.len() != before { + self.write_index(&names, prev)?; + } + Ok(()) + } + + // ── One-time blob → per-credential migration ────────────────────────── + + /// Move every secret out of the legacy single-blob entry into its own + /// credential, exactly once, without ever producing a partial state. /// - /// **Copy-on-write**: the candidate `next` is a separate allocation from - /// `current`. The cache is only replaced with `next` after `write_blob_raw` - /// succeeds. On write failure the cache is cleared to `None` so the next - /// caller re-reads from the keychain rather than building on a stale state. + /// Sequence, under the interprocess advisory lock: + /// 1. Read the blob. Absent → nothing to do, mark migrated. + /// 2. For each secret: write its own credential, then **read it back from + /// the OS and verify** the value matches. + /// 3. Add every name to the index. + /// 4. Only then delete the blob entry. /// - /// 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 inside the file lock. + /// If any single write, read-back, verify or index write fails, every + /// credential this attempt touched is restored to its prior value (deleted + /// when there was none), the blob is left **intact**, and `migrated` stays + /// `false` so the next access retries. Reads prefer the per-credential + /// value and fall back to the blob, so an aborted attempt is invisible. #[cfg(feature = "system-keyring")] - fn mutate_blob(&self, f: F) -> Result<(), String> - where - F: FnOnce(&mut HashMap), - { - // Acquire the interprocess advisory lock first. All Buzz processes - // using the same service name contend on the same lockfile at - // /tmp/buzz-keychain--.lock (a deterministic per-user - // path invariant to $TMPDIR), so only one process performs a - // read-modify-write at a time. - let _lock = acquire_blob_lock(&self.service)?; + fn ensure_migrated(&self) -> Result<(), String> { + { + let guard = self.migrated.lock().unwrap_or_else(|e| e.into_inner()); + if *guard { + return Ok(()); + } + } - // Always do a fresh read from the keychain while holding the lock — - // this is the critical correction over the prior warm-cache path. A - // stale warm cache would make us build our candidate on an outdated - // baseline and drop keys written by another process. - let raw = self.read_blob_raw()?; - let current: HashMap = 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}"))? + // Cross-process exclusion: another Buzz process may be migrating the + // same service right now. Note this must not be held by our caller — + // `flock(2)` on a second fd in the same process would self-deadlock. + let _lock = acquire_blob_lock(&self.service)?; + { + let guard = self.migrated.lock().unwrap_or_else(|e| e.into_inner()); + if *guard { + return Ok(()); } + } + + let Some(bytes) = self.read_blob_raw()? else { + // No legacy blob: fresh install, or another process already + // finished the migration. + *self.migrated.lock().unwrap_or_else(|e| e.into_inner()) = true; + return Ok(()); }; + let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; + let map = serde_json::from_str::>(&json) + .map_err(|e| format!("blob json: {e}"))?; + + // (name, value before this attempt touched it) — the rollback journal. + let mut touched: Vec<(String, Option)> = Vec::new(); + for (key, value) in &map { + let prior = match self.read_secret_raw(key) { + Ok(p) => p, + Err(e) => { + self.rollback_migration(&touched); + return Err(format!("blob migration: read {key}: {e}")); + } + }; + if prior.as_deref() == Some(value.as_str()) { + continue; // already migrated by an earlier attempt + } + if let Err(e) = self.entry_set(key, value) { + self.rollback_migration(&touched); + return Err(format!("blob migration: write {key}: {e}")); + } + touched.push((key.clone(), prior)); + // Read back from the OS — proves the round-trip, not just that an + // in-process buffer was updated. + match self.read_secret_raw(key) { + Ok(Some(got)) if got == *value => {} + other => { + self.rollback_migration(&touched); + return Err(format!( + "blob migration: read-back verify failed for {key}: {other:?}" + )); + } + } + } - // Build the candidate state in a separate allocation so that a write - // failure below cannot leave the cache ahead of durable storage. - let mut next = current.clone(); - f(&mut next); + let names: Vec = map.keys().cloned().collect(); + if let Err(e) = self.index_insert(&names) { + self.rollback_migration(&touched); + return Err(format!("blob migration: index: {e}")); + } + + // Every secret is durable and verified in its own credential; the blob + // is now redundant. A failure here is not data loss — the blob is + // simply retried on the next process start. + if let Err(e) = self.entry_delete(BLOB_KEY) { + eprintln!("buzz-desktop: secret_store: blob migrated but not deleted: {e}"); + } - // Skip the keychain write when the candidate equals the freshly-read - // durable state — no I/O needed and no keychain ACL prompt on macOS. - if next == current { - // Update the cache to the fresh read even on no-op so subsequent - // reads in this process see any keys another process may have added. + { let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - *guard = Some(current); - return Ok(()); + let cached = guard.get_or_insert_with(HashMap::new); + for (key, value) in map { + cached.insert(key, value); + } } + *self.migrated.lock().unwrap_or_else(|e| e.into_inner()) = true; + Ok(()) + } - // Write to keyring while still holding the file lock. - let json = serde_json::to_string(&next).map_err(|e| format!("blob serialize: {e}"))?; - match self.write_blob_raw(json.as_bytes()) { - Ok(()) => { - // Advance the cache to `next` only after the durable write succeeds. - let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - *guard = Some(next); - Ok(()) + /// Undo the credential writes journalled in `touched`, newest first. + /// Best-effort by construction: the blob is still intact, so anything that + /// cannot be restored here is re-derived on the next migration attempt. + #[cfg(feature = "system-keyring")] + fn rollback_migration(&self, touched: &[(String, Option)]) { + for (key, prior) in touched.iter().rev() { + let _ = match prior { + Some(old) => self.entry_set(key, old), + None => self.entry_delete(key), + }; + } + } + + /// Run [`Self::ensure_migrated`] but let a failure fall through to the + /// caller's own OS access, which produces the more precise error. + #[cfg(feature = "system-keyring")] + fn try_migrate(&self) { + if let Err(e) = self.ensure_migrated() { + eprintln!("buzz-desktop: secret_store: blob migration deferred: {e}"); + } + } + + /// Write `entries` as one credential each, then record their names in the + /// index, then advance the cache. Replaces the old `mutate_blob`. + /// + /// **Cross-process safety**: acquires the same exclusive advisory lock + /// (`flock(2)` on Unix, a named kernel mutex on Windows) the blob path used, + /// keyed by service name, so a concurrent process cannot interleave an + /// index read-modify-write with ours. The index is always re-read fresh + /// inside the lock, never from cache, so another process's names are never + /// dropped. Per-secret values no longer share a record at all, so a write + /// here cannot clobber a secret it does not name. + /// + /// **Idempotent**: a credential whose stored value already equals the new + /// one is not rewritten. On macOS the legacy `SecKeychain` API treats a + /// write as a distinct ACL operation from the "Always Allow"-ed read, so + /// skipping no-op writes still avoids the prompt that would otherwise fire + /// when saving an agent whose model changed but whose key did not. + /// + /// **Cache honesty**: only keys that reached the OS are added to the cache, + /// so a failed write is never visible to a later `load()`. + /// + /// Deadlock-free: `ensure_migrated` takes the same lock, so it must run to + /// completion *before* the lock is acquired here, not inside it. + #[cfg(feature = "system-keyring")] + fn store_secrets(&self, entries: &HashMap) -> Result<(), String> { + self.ensure_migrated()?; + let _lock = acquire_blob_lock(&self.service)?; + + let mut written: Vec = Vec::new(); + let mut first_err: Option = None; + for (key, value) in entries { + match self.read_secret_raw(key) { + // Already durable with this exact value — no write, no prompt. + Ok(Some(cur)) if cur == *value => written.push(key.clone()), + Ok(_) => match self.entry_set(key, value) { + Ok(()) => written.push(key.clone()), + Err(e) => { + first_err.get_or_insert(e); + } + }, + Err(e) => { + first_err.get_or_insert(e); + } } - Err(e) => { - // On write failure, clear the cache so the next caller re-reads - // from the keychain rather than building on a stale state. - let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - *guard = None; - Err(e) + } + + if !written.is_empty() { + if let Err(e) = self.index_insert(&written) { + first_err.get_or_insert(e); } } - } - /// 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> { - self.write_blob_raw_keyring(bytes) + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + let map = guard.get_or_insert_with(HashMap::new); + for key in &written { + if let Some(value) = entries.get(key) { + map.insert(key.clone(), value.clone()); + } + } + } + + match first_err { + Some(e) => Err(e), + None => Ok(()), + } } - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - fn write_blob_raw(&self, bytes: &[u8]) -> Result<(), String> { - self.write_blob_raw_keyring(bytes) + /// Value for `key` if this process has already read or written it. Never + /// authoritative for absence — a miss must fall through to the OS. + #[cfg(feature = "system-keyring")] + fn cached(&self, key: &str) -> Option { + let guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + guard.as_ref().and_then(|m| m.get(key).cloned()) } + /// Record `key`→`value` as read from the OS. #[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}")) + fn cache_put(&self, key: &str, value: &str) { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + guard + .get_or_insert_with(HashMap::new) + .insert(key.to_string(), value.to_string()); } /// Probe whether `key` exists and whether the backend is reachable. pub fn probe(&self, key: &str) -> KeyringProbe { #[cfg(feature = "system-keyring")] { - match self.load_blob() { - Ok(Some(map)) => { - if map.contains_key(key) { - KeyringProbe::Present - } else { - // 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), + self.try_migrate(); + if self.cached(key).is_some() { + return KeyringProbe::Present; + } + match self.read_secret_raw(key) { + Ok(Some(_)) => KeyringProbe::Present, + // No credential of its own — check the shapes an unfinished + // migration can leave behind (legacy blob, then old DPK items) + // so callers that gate `load()` on `Present` still fire. + Ok(None) => match self.blob_value(key) { + Ok(Some(_)) => KeyringProbe::Present, + Ok(None) => self.probe_legacy_key(key), + Err(_) => KeyringProbe::Unreachable, // corrupt blob — fail closed + }, Err(e) if is_keyring_availability_error(&e) => KeyringProbe::Unreachable, - Err(_) => KeyringProbe::Unreachable, // corrupt blob — fail closed + Err(_) => KeyringProbe::Unreachable, } } #[cfg(not(feature = "system-keyring"))] @@ -501,14 +789,28 @@ 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). + /// Value for `key` still sitting in the legacy blob, if the blob survives + /// (i.e. a migration attempt has not yet succeeded). This is what keeps an + /// aborted migration invisible to readers. + #[cfg(feature = "system-keyring")] + fn blob_value(&self, key: &str) -> Result, String> { + let Some(bytes) = self.read_blob_raw()? else { + return Ok(None); + }; + let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; + let map = serde_json::from_str::>(&json) + .map_err(|e| format!("blob json: {e}"))?; + Ok(map.get(key).cloned()) + } + + /// Check old per-key DPK entries for `key`. Used by `probe()` once the + /// key's own credential and the legacy blob have both come up empty. #[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_not_found(e) => KeyringProbe::ReachableButEmpty, + Err(ref e) if is_dpk_unavailable(e) => KeyringProbe::ReachableButEmpty, Err(ref e) if is_keyring_availability_error(&e.to_string()) => { KeyringProbe::Unreachable } @@ -517,56 +819,36 @@ impl SecretStore { } #[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, - } + fn probe_legacy_key(&self, _key: &str) -> KeyringProbe { + // No DPK off macOS: the key's own credential is the only shape, and + // `probe` already found it absent. + KeyringProbe::ReachableButEmpty } /// 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. 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). + /// Resolution order: in-process cache → the key's own credential → the + /// legacy blob (only reachable while a migration attempt is outstanding) → + /// the old per-key DPK item on macOS, which is migrated in place and + /// deleted. Pre-blob installs are handled for free: their per-key `keyring` + /// entries live at exactly the address this format uses. pub fn load(&self, key: &str) -> Result, String> { #[cfg(feature = "system-keyring")] { - match self.load_blob() { - 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). - self.migrate_legacy_key(key) - } - Err(e) => Err(e), + self.try_migrate(); + if let Some(value) = self.cached(key) { + return Ok(Some(value)); } + if let Some(value) = self.read_secret_raw(key)? { + self.cache_put(key, &value); + return Ok(Some(value)); + } + // A migration that aborted leaves the blob authoritative. + if let Some(value) = self.blob_value(key)? { + return Ok(Some(value)); + } + self.migrate_legacy_key(key) } #[cfg(not(feature = "system-keyring"))] { @@ -575,17 +857,68 @@ impl SecretStore { } } - /// Read the secret for `key` without any legacy-migration side effects. + /// Read every stored secret without any legacy-migration side effects. /// - /// Read the entire blob without any legacy-migration side effects. + /// Returns the full key→value map, `Ok(None)` when nothing has ever been + /// stored for this service, and `Err` only when the backend is unavailable. + /// Never calls `migrate_legacy_key`. /// - /// Returns the full key→value map when a blob exists, `Ok(None)` when no - /// blob has been written yet, and `Err` only when the backend is - /// unavailable. Never calls `migrate_legacy_key`. + /// The key set comes from the index, unioned with any names still in an + /// unmigrated blob and with `"identity"` (which predates the index and can + /// exist as a bare credential on a pre-blob install). pub fn load_all_readonly(&self) -> Result>, String> { #[cfg(feature = "system-keyring")] { - self.load_blob() + self.try_migrate(); + + let (mut names, _) = self.read_index_raw()?; + let indexed = !names.is_empty(); + + // A blob only survives here when a migration attempt aborted; its + // contents are still the authoritative copy for those keys. + let blob: HashMap = match self.read_blob_raw()? { + 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}"))? + } + None => HashMap::new(), + }; + for name in blob.keys() { + if !names.iter().any(|n| n == name) { + names.push(name.clone()); + } + } + if !names.iter().any(|n| n == "identity") { + names.push("identity".to_string()); + } + + let mut map = HashMap::new(); + for name in names { + match self.read_secret_raw(&name)? { + Some(value) => { + map.insert(name, value); + } + // Indexed but no credential: an index entry outliving its + // secret is a benign inconsistency, not a read failure. + None => { + if let Some(value) = blob.get(&name) { + map.insert(name, value.clone()); + } + } + } + } + + if map.is_empty() && !indexed { + return Ok(None); // nothing has ever been stored + } + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + let cached = guard.get_or_insert_with(HashMap::new); + for (k, v) in &map { + cached.insert(k.clone(), v.clone()); + } + } + Ok(Some(map)) } #[cfg(not(feature = "system-keyring"))] { @@ -593,19 +926,15 @@ impl SecretStore { } } - /// Insert all entries from `entries` into the blob in a single mutation. + /// Store every entry in `entries`, one credential each. /// - /// Entries that already exist in the blob are overwritten; entries not - /// present in `entries` are left unchanged. If the resulting blob is - /// identical to what is already stored, no keychain write occurs. + /// Entries that already exist are overwritten; keys not present in + /// `entries` are left untouched. A credential whose stored value already + /// equals the new one is not rewritten, so a no-op save costs no write. pub fn store_all(&self, entries: &HashMap) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - self.mutate_blob(|map| { - for (k, v) in entries { - map.insert(k.clone(), v.clone()); - } - }) + self.store_secrets(entries) } #[cfg(not(feature = "system-keyring"))] { @@ -615,13 +944,18 @@ 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. + /// old DPK entry for `key`, write it into the key's own credential, 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. + /// reads it, lifts every key it holds into its own credential, and deletes + /// the DPK blob. + /// + /// The pre-#1264 `keyring`-crate per-key entries need no handling: they sit + /// at exactly the address the current format uses, so `load` already found + /// them before reaching here. #[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") @@ -631,12 +965,17 @@ impl SecretStore { 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()); + // Lift each key into its own credential, never overwriting one + // that already exists (the existing copy is the newer one). + let mut fresh = HashMap::new(); + for (k, v) in &dpk_map { + if self.read_secret_raw(k)?.is_none() { + fresh.insert(k.clone(), v.clone()); } - })?; + } + if !fresh.is_empty() { + self.store_secrets(&fresh)?; + } // Best-effort delete the DPK blob. let _ = delete_generic_password_options(dpk_opts(&self.service, BLOB_KEY)); return Ok(dpk_map.get(key).cloned()); @@ -654,44 +993,23 @@ impl SecretStore { 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). + // Write into the key's own credential. 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(ref e) if is_not_found(e) => Ok(None), + Err(ref e) if is_dpk_unavailable(e) => Ok(None), 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}")), - } + fn migrate_legacy_key(&self, _key: &str) -> Result, String> { + // No DPK off macOS, and the pre-#1264 per-key `keyring` entries share + // the current format's address — `load` has already checked there. + Ok(None) } /// Verify that `key` holds `expected` by reading directly from the OS @@ -705,17 +1023,11 @@ impl SecretStore { pub fn verify_stored_raw(&self, key: &str, expected: &str) -> Result { #[cfg(feature = "system-keyring")] { - let raw = self.read_blob_raw()?; - match raw { - None => Ok(false), - Some(bytes) => { - let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; - let map = - serde_json::from_str::>(&json) - .map_err(|e| format!("blob json: {e}"))?; - Ok(map.get(key).is_some_and(|v| v == expected)) - } + if self.read_secret_raw(key)?.as_deref() == Some(expected) { + return Ok(true); } + // A key still awaiting migration is durable in the blob. + Ok(self.blob_value(key)?.as_deref() == Some(expected)) } #[cfg(not(feature = "system-keyring"))] { @@ -729,9 +1041,9 @@ impl SecretStore { pub fn store(&self, key: &str, value: &str) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - self.mutate_blob(|map| { - map.insert(key.to_string(), value.to_string()); - }) + let mut one = HashMap::with_capacity(1); + one.insert(key.to_string(), value.to_string()); + self.store_secrets(&one) } #[cfg(not(feature = "system-keyring"))] { @@ -740,43 +1052,44 @@ impl SecretStore { } } - /// Delete the entire keychain blob for this service, plus all legacy per-key - /// entries that could resurrect an identity on next boot. + /// Delete every secret credential for this service, plus every legacy shape + /// that could resurrect an identity on next boot. /// /// Order of operations: - /// 1. Read the blob to collect every key name (e.g. `identity`, agent keys). + /// 1. Collect every key name: the index, plus any names left in an + /// unmigrated blob, plus `"identity"` unconditionally. /// 2. Delete legacy per-key DPK entries for every key + the DPK blob itself. - /// 3. Delete legacy per-key keyring entries for every key. - /// 4. Delete the blob entry. + /// 3. Delete each key's own credential. + /// 4. Delete the legacy blob entry and every index chunk. /// 5. Clear the in-memory cache. /// /// This is the correct wipe path for sign-out: the old `delete_all` skipped - /// step 1–3 so stale per-key entries could be re-imported on the next launch - /// via `migrate_legacy_key`. This method prevents that resurrection. + /// steps 1–3 so stale per-key entries could be re-imported on the next + /// launch via `migrate_legacy_key`. This method prevents that resurrection. pub fn delete_all_with_legacy_cleanup(&self) -> Result<(), String> { #[cfg(feature = "system-keyring")] { let _lock = acquire_blob_lock(&self.service)?; - // Step 1: read current blob keys (best-effort; no entry = empty set). - let blob_keys: Vec = match self.read_blob_raw() { - Ok(Some(bytes)) => { - let json = String::from_utf8(bytes).unwrap_or_default(); - serde_json::from_str::>(&json) - .map(|m| m.into_keys().collect()) - .unwrap_or_default() + // Step 1: every name we know about (best-effort; errors = empty set). + let (mut all_keys, index_chunks) = self.read_index_raw().unwrap_or_default(); + if let Ok(Some(bytes)) = self.read_blob_raw() { + let json = String::from_utf8(bytes).unwrap_or_default(); + if let Ok(map) = serde_json::from_str::>(&json) { + for key in map.into_keys() { + if !all_keys.contains(&key) { + all_keys.push(key); + } + } } - _ => vec![], - }; - - // Always include "identity" even if the blob is empty or absent — - // it may exist only as a legacy per-key entry. - let mut all_keys = blob_keys; + } + // Always include "identity" even when nothing is indexed — it may + // exist only as a bare credential from a pre-blob install. if !all_keys.contains(&"identity".to_string()) { all_keys.push("identity".to_string()); } - // Steps 2 & 3: delete legacy per-key entries for every key. + // Steps 2 & 3: delete the DPK entry and the credential for every key. for key in &all_keys { #[cfg(target_os = "macos")] { @@ -787,19 +1100,8 @@ impl SecretStore { Err(e) => return Err(format!("dpk per-key delete {key}: {e}")), } } - { - let entry = keyring_entry(&self.service, key) - .map_err(|e| format!("keyring entry constructor {key}: {e}"))?; - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => {} - Err(e) if is_keyring_availability_error(&e.to_string()) => { - return Err(format!("keyring unavailable deleting {key}: {e}")); - } - Err(e) => { - return Err(format!("keyring per-key delete {key}: {e}")); - } - } - } + self.entry_delete(key) + .map_err(|e| format!("keyring per-key delete {key}: {e}"))?; } // Step 2 (cont.): also delete the legacy DPK blob written by #1267. #[cfg(target_os = "macos")] @@ -812,22 +1114,17 @@ impl SecretStore { } } - // Step 4: delete the main blob entry. - { - let entry = keyring_entry(&self.service, BLOB_KEY) - .map_err(|e| format!("keyring entry constructor blob: {e}"))?; - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => {} - Err(e) if is_keyring_availability_error(&e.to_string()) => { - return Err(format!("keyring unavailable: {e}")); - } - Err(e) => { - return Err(format!("keyring blob delete: {e}")); - } - } + // Step 4: delete the legacy blob entry and every index chunk. + self.entry_delete(BLOB_KEY) + .map_err(|e| format!("keyring blob delete: {e}"))?; + for i in 0..index_chunks.clamp(1, MAX_INDEX_CHUNKS) { + self.entry_delete(&index_chunk_key(i)) + .map_err(|e| format!("keyring index delete {i}: {e}"))?; } - // Step 5: clear the in-memory cache. + // Step 5: clear the in-memory cache. The blob is gone, so there is + // nothing left to migrate either. + *self.migrated.lock().unwrap_or_else(|e| e.into_inner()) = true; let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); *guard = None; Ok(()) @@ -854,16 +1151,11 @@ impl SecretStore { Ok(Some(_)) => return false, Err(_) => return false, } - // 2. Per-key "identity" via legacy keyring must be absent. - match keyring_entry(&self.service, "identity") { - Ok(entry) => match entry.get_password() { - Err(keyring::Error::NoEntry) => {} - Ok(_) => return false, - // Any other error (availability, unknown, transient) → fail closed. - // Only explicit NoEntry is proof of absence. - Err(_) => return false, - }, - // Constructor failure → cannot verify → fail closed. + // 2. The "identity" credential itself must be absent. Only an + // explicit "no entry" is proof of absence; any error fails closed. + match self.entry_get("identity") { + Ok(None) => {} + Ok(Some(_)) => return false, Err(_) => return false, } // 3. DPK blob (macOS only). @@ -901,16 +1193,23 @@ impl SecretStore { pub fn delete(&self, key: &str) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - self.mutate_blob(|map| { - map.remove(key); - })?; - // Best-effort: also delete any old per-key entry for this key to + // Migrate first: a key still sitting in the blob would otherwise + // survive the delete and be resurrected on the next load. + self.try_migrate(); + let _lock = acquire_blob_lock(&self.service)?; + + self.entry_delete(key)?; + self.index_remove(key)?; + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(map) = guard.as_mut() { + map.remove(key); + } + } + // Best-effort: also delete any old DPK 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"))] @@ -921,6 +1220,131 @@ impl SecretStore { } } +/// In-process stand-in for the OS credential store, used by the unit tests so +/// they never touch (or depend on) a real keychain. +/// +/// It reproduces the constraint that motivated per-credential storage: a write +/// whose value exceeds Windows' `CRED_MAX_CREDENTIAL_BLOB_SIZE` is rejected +/// before anything is stored, exactly as `keyring`'s Windows backend rejects it +/// ahead of `CredWriteW`. Sizes are measured over the UTF-16 encoding the +/// backend actually writes. +#[cfg(all(test, feature = "system-keyring"))] +mod fake_backend { + use std::collections::{HashMap, HashSet}; + use std::sync::{Mutex, OnceLock}; + + /// `CRED_MAX_CREDENTIAL_BLOB_SIZE` from `wincred.h`. + pub const MAX_BLOB_BYTES: usize = 2560; + + type Creds = HashMap<(String, String), String>; + + fn creds() -> &'static Mutex { + static CREDS: OnceLock> = OnceLock::new(); + CREDS.get_or_init(|| Mutex::new(HashMap::new())) + } + + fn failures() -> &'static Mutex> { + static FAILURES: OnceLock>> = OnceLock::new(); + FAILURES.get_or_init(|| Mutex::new(HashSet::new())) + } + + /// UTF-16 byte length, i.e. what the Windows backend measures. + pub fn blob_bytes(value: &str) -> usize { + value.encode_utf16().count() * 2 + } + + pub fn get(service: &str, key: &str) -> Result, String> { + let guard = creds().lock().unwrap_or_else(|e| e.into_inner()); + Ok(guard.get(&(service.to_string(), key.to_string())).cloned()) + } + + pub fn set(service: &str, key: &str, value: &str) -> Result<(), String> { + let id = (service.to_string(), key.to_string()); + if failures() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .contains(&id) + { + return Err("keyring write: injected backend failure".to_string()); + } + if blob_bytes(value) > MAX_BLOB_BYTES { + return Err(format!( + "keyring write: credential blob too long ({} bytes > {MAX_BLOB_BYTES})", + blob_bytes(value) + )); + } + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(id, value.to_string()); + Ok(()) + } + + pub fn delete(service: &str, key: &str) -> Result<(), String> { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&(service.to_string(), key.to_string())); + Ok(()) + } + + // ── helpers for tests ───────────────────────────────────────────────── + + /// Write a credential directly, bypassing the failure injection. + pub fn seed(service: &str, key: &str, value: &str) { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert((service.to_string(), key.to_string()), value.to_string()); + } + + /// Read a credential directly, bypassing `SecretStore` entirely. + pub fn raw(service: &str, key: &str) -> Option { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(&(service.to_string(), key.to_string())) + .cloned() + } + + /// Every credential name that exists under `service`, sorted. + pub fn names_for(service: &str) -> Vec { + let guard = creds().lock().unwrap_or_else(|e| e.into_inner()); + let mut names: Vec = guard + .keys() + .filter(|(s, _)| s == service) + .map(|(_, k)| k.clone()) + .collect(); + names.sort(); + names + } + + /// Make every write to `service`/`key` fail, simulating a denied prompt or + /// a transient backend outage part-way through a migration. + pub fn fail_writes_for(service: &str, key: &str) { + failures() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert((service.to_string(), key.to_string())); + } + + pub fn clear_failures() { + failures().lock().unwrap_or_else(|e| e.into_inner()).clear(); + } + + /// Drop all state for `service`. + pub fn reset(service: &str) { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .retain(|(s, _), _| s != service); + failures() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .retain(|(s, _)| s != service); + } +} + #[cfg(all(test, feature = "system-keyring"))] mod tests { use super::*; @@ -931,10 +1355,35 @@ mod tests { SecretStore { service: service.to_string(), cache: Mutex::new(cache), + // A pre-seeded cache stands in for "already read from the OS", + // so there is nothing left to migrate. + migrated: Mutex::new(true), + fake: true, + } + } + + /// Store backed by the in-process fake credential backend. + fn with_fake(service: &str) -> Self { + SecretStore { + service: service.to_string(), + cache: Mutex::new(None), + migrated: Mutex::new(false), + fake: true, } } } + /// Serialize `entries` the way the legacy blob stored them. + fn blob_json(entries: &HashMap) -> String { + serde_json::to_string(entries).unwrap() + } + + /// A realistically-sized agent key name/value pair (`agent:` → + /// `nsec…`), the shape that made the blob overflow. + fn agent_pair(i: usize) -> (String, String) { + (format!("agent:npub1{:0>58}", i), format!("nsec1{:0>58}", i)) + } + #[test] fn probe_returns_present_when_key_in_cache() { let mut map = HashMap::new(); @@ -1303,4 +1752,341 @@ mod tests { // Agent key should also be gone. assert_eq!(store3.load("agent:abc123").unwrap(), None); } + + // ── Blob → per-credential migration (fake backend, no OS keychain) ───── + + #[test] + fn migration_moves_every_blob_secret_into_its_own_credential() { + let svc = "buzz-test-fake-migrate-basic"; + fake_backend::reset(svc); + + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), "nsec1identityvalue".to_string()); + blob.insert("agent:aaa".to_string(), "nsec1aaa".to_string()); + blob.insert("agent:bbb".to_string(), "nsec1bbb".to_string()); + fake_backend::seed(svc, BLOB_KEY, &blob_json(&blob)); + + // First access triggers the migration. + let store = SecretStore::with_fake(svc); + assert_eq!( + store.load("identity").unwrap(), + Some("nsec1identityvalue".to_string()) + ); + + for (key, value) in &blob { + assert_eq!( + fake_backend::raw(svc, key).as_deref(), + Some(value.as_str()), + "{key} must now live in its own credential" + ); + } + assert_eq!( + fake_backend::raw(svc, BLOB_KEY), + None, + "the legacy blob must be deleted once every secret is written and verified" + ); + assert!( + fake_backend::raw(svc, INDEX_KEY).is_some(), + "the index must list the migrated names" + ); + + // A cold store sees the full set, and migrating again is a no-op. + let reader = SecretStore::with_fake(svc); + let all = reader.load_all_readonly().unwrap().unwrap(); + assert_eq!(all, blob, "every secret must survive the migration"); + reader.ensure_migrated().unwrap(); + assert_eq!(reader.load("agent:bbb").unwrap(), Some("nsec1bbb".into())); + + fake_backend::reset(svc); + } + + #[test] + fn migration_rolls_back_and_leaves_blob_intact_when_one_write_fails() { + let svc = "buzz-test-fake-migrate-rollback"; + fake_backend::reset(svc); + + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), "nsec1identityvalue".to_string()); + for i in 0..6 { + let (key, value) = agent_pair(i); + blob.insert(key, value); + } + let original = blob_json(&blob); + fake_backend::seed(svc, BLOB_KEY, &original); + + // One secret's write fails part-way through (denied prompt / outage). + let (doomed, _) = agent_pair(3); + fake_backend::fail_writes_for(svc, &doomed); + + let store = SecretStore::with_fake(svc); + let err = store.ensure_migrated().unwrap_err(); + assert!( + err.contains(&doomed), + "the error must name the secret that failed: {err}" + ); + + assert_eq!( + fake_backend::raw(svc, BLOB_KEY).as_deref(), + Some(original.as_str()), + "the blob must be left byte-for-byte intact so the migration can retry" + ); + assert_eq!( + fake_backend::names_for(svc), + vec![BLOB_KEY.to_string()], + "rollback must leave no partially-migrated credentials and no index" + ); + + // Reads still resolve, out of the surviving blob — the abort is invisible. + assert_eq!( + store.load("identity").unwrap(), + Some("nsec1identityvalue".to_string()) + ); + let (key5, value5) = agent_pair(5); + assert_eq!(store.load(&key5).unwrap(), Some(value5)); + assert_eq!(store.probe(&doomed), KeyringProbe::Present); + + // Once the backend recovers, the retry completes. + fake_backend::clear_failures(); + store.ensure_migrated().unwrap(); + assert_eq!(fake_backend::raw(svc, BLOB_KEY), None); + let (key3, value3) = agent_pair(3); + assert_eq!( + fake_backend::raw(svc, &key3).as_deref(), + Some(value3.as_str()) + ); + assert_eq!( + SecretStore::with_fake(svc) + .load_all_readonly() + .unwrap() + .unwrap(), + blob + ); + + fake_backend::reset(svc); + } + + #[test] + fn migration_rollback_restores_prior_credential_values() { + // The previous test aborts at whatever point `HashMap` iteration puts + // the doomed key. This one fails the *index* write, which runs only + // after every secret has been written and verified, so the rollback + // journal is guaranteed full — including one credential that already + // existed and must be put back rather than deleted. + let svc = "buzz-test-fake-migrate-rollback-restore"; + fake_backend::reset(svc); + + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), "nsec1new-identity".to_string()); + for i in 0..4 { + let (key, value) = agent_pair(i); + blob.insert(key, value); + } + let original = blob_json(&blob); + fake_backend::seed(svc, BLOB_KEY, &original); + fake_backend::seed(svc, "identity", "nsec1stale-identity"); + fake_backend::fail_writes_for(svc, INDEX_KEY); + + let store = SecretStore::with_fake(svc); + let err = store.ensure_migrated().unwrap_err(); + assert!( + err.contains("index"), + "the index write must be the failure: {err}" + ); + + assert_eq!( + fake_backend::raw(svc, BLOB_KEY).as_deref(), + Some(original.as_str()), + "the blob must survive a failure at the index step too" + ); + assert_eq!( + fake_backend::raw(svc, "identity").as_deref(), + Some("nsec1stale-identity"), + "a credential that existed before the migration must be restored to \ + its prior value, not deleted" + ); + for i in 0..4 { + let (key, _) = agent_pair(i); + assert_eq!( + fake_backend::raw(svc, &key), + None, + "{key} was created by this attempt and must be rolled back" + ); + } + assert_eq!( + fake_backend::names_for(svc), + vec!["identity".to_string(), BLOB_KEY.to_string()], + "nothing else may be left behind" + ); + + // The retry then completes, and the blob's copy wins over the stale one. + fake_backend::clear_failures(); + store.ensure_migrated().unwrap(); + assert_eq!( + fake_backend::raw(svc, "identity").as_deref(), + Some("nsec1new-identity") + ); + assert_eq!(fake_backend::raw(svc, BLOB_KEY), None); + assert_eq!( + SecretStore::with_fake(svc) + .load_all_readonly() + .unwrap() + .unwrap(), + blob + ); + + fake_backend::reset(svc); + } + + #[test] + fn sixteen_agents_plus_identity_all_reach_the_credential_store() { + // The regression this whole change exists for: as one JSON blob, + // identity + 8 agents already filled ~2380 of the 2560 bytes Windows + // allows in a credential, so agent 9 onward could never be written at + // all and silently fell back to inline plaintext. One credential per + // secret removes the shared budget. + let svc = "buzz-test-fake-sixteen-agents"; + fake_backend::reset(svc); + + let store = SecretStore::with_fake(svc); + let identity = format!("nsec1{:0>58}", 999); + store.store("identity", &identity).unwrap(); + + let mut expected = HashMap::new(); + expected.insert("identity".to_string(), identity); + for i in 0..16 { + let (key, value) = agent_pair(i); + store + .store(&key, &value) + .unwrap_or_else(|e| panic!("agent {i} must reach the credential store: {e}")); + expected.insert(key, value); + } + + // Cold store: every secret round-trips out of the OS, not the cache. + let reader = SecretStore::with_fake(svc); + for (key, value) in &expected { + assert_eq!( + reader.load(key).unwrap().as_deref(), + Some(value.as_str()), + "{key} must load back" + ); + } + assert_eq!( + reader.load_all_readonly().unwrap().unwrap(), + expected, + "identity + 16 agents must all be enumerable" + ); + + // Proof the test is not vacuous: the old format put this whole set in + // one credential, and that write is still rejected by the backend. + let as_one_blob = blob_json(&expected); + assert!( + fake_backend::blob_bytes(&as_one_blob) > fake_backend::MAX_BLOB_BYTES, + "regression guard: the fixture must exceed the single-credential cap" + ); + assert!( + fake_backend::set(svc, "would-be-blob", &as_one_blob).is_err(), + "regression guard: the single-blob write must still fail — that is the bug being fixed" + ); + // The index is over one credential's worth of names too, so it chunks. + assert!( + fake_backend::raw(svc, &index_chunk_key(1)).is_some(), + "the name index must spill into a second chunk rather than overflow" + ); + // And nothing written anywhere is over the cap. + for name in fake_backend::names_for(svc) { + let value = fake_backend::raw(svc, &name).unwrap(); + assert!( + fake_backend::blob_bytes(&value) <= fake_backend::MAX_BLOB_BYTES, + "credential {name} is {} bytes, over the OS cap", + fake_backend::blob_bytes(&value) + ); + } + + // Deleting one agent leaves the other fifteen and the identity alone. + let (gone, _) = agent_pair(7); + store.delete(&gone).unwrap(); + let after = SecretStore::with_fake(svc) + .load_all_readonly() + .unwrap() + .unwrap(); + assert_eq!(after.len(), 16); + assert!(!after.contains_key(&gone)); + assert!(after.contains_key("identity")); + + fake_backend::reset(svc); + } + + #[test] + fn identity_survives_blob_migration_on_both_service_names() { + // The human identity shares the blob with the agent keys, on both the + // release and the dev service name. Neither may lose it. + for svc in ["buzz-desktop", "buzz-desktop-dev"] { + fake_backend::reset(svc); + + let expected = format!("nsec1identity-{svc}"); + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), expected.clone()); + blob.insert("agent:aaa".to_string(), "nsec1aaa".to_string()); + fake_backend::seed(svc, BLOB_KEY, &blob_json(&blob)); + + let store = SecretStore::with_fake(svc); + assert_eq!( + store.load("identity").unwrap(), + Some(expected.clone()), + "{svc}: identity must survive the migration" + ); + assert_eq!(store.probe("identity"), KeyringProbe::Present, "{svc}"); + assert!( + store.verify_stored_raw("identity", &expected).unwrap(), + "{svc}: identity must verify against the OS, not the cache" + ); + assert_eq!( + fake_backend::raw(svc, "identity").as_deref(), + Some(expected.as_str()), + "{svc}: identity must have its own credential" + ); + assert_eq!(fake_backend::raw(svc, BLOB_KEY), None, "{svc}: blob gone"); + + // Sign-out still wipes it everywhere. + store.delete_all_with_legacy_cleanup().unwrap(); + assert!(store.verify_fully_wiped(), "{svc}: wipe must verify"); + assert_eq!( + fake_backend::names_for(svc), + Vec::::new(), + "{svc}: no credential may survive sign-out" + ); + + fake_backend::reset(svc); + } + } + + // The point of this guard is to spell out each signature verbatim, so + // factoring one out into a type alias would defeat it. + #[allow(clippy::type_complexity)] + #[test] + fn public_api_surface_is_unchanged() { + // Amputation guard: every public entry point callers depend on must + // keep its exact signature. `storage.rs`, `app_state.rs`, `reset.rs`, + // `identity.rs` and `pairing.rs` all bind to these. + let _: fn(String) -> SecretStore = SecretStore::keyring; + let _: fn(&'static str) -> SecretStore = SecretStore::keyring; + let _: fn(&'static str) -> &'static SecretStore = SecretStore::shared; + let _: fn(&SecretStore, &str) -> KeyringProbe = SecretStore::probe; + let _: fn(&SecretStore, &str) -> Result, String> = SecretStore::load; + let _: fn(&SecretStore) -> Result>, String> = + SecretStore::load_all_readonly; + let _: fn(&SecretStore, &str, &str) -> Result<(), String> = SecretStore::store; + let _: fn(&SecretStore, &HashMap) -> Result<(), String> = + SecretStore::store_all; + let _: fn(&SecretStore, &str, &str) -> Result = + SecretStore::verify_stored_raw; + let _: fn(&SecretStore, &str) -> Result<(), String> = SecretStore::delete; + let _: fn(&SecretStore) -> Result<(), String> = SecretStore::delete_all_with_legacy_cleanup; + let _: fn(&SecretStore) -> bool = SecretStore::verify_fully_wiped; + + // KeyringProbe's variants are matched on by callers. + let _ = KeyringProbe::Present; + let _ = KeyringProbe::ReachableButEmpty; + let _ = KeyringProbe::Unreachable; + } }