diff --git a/SECURITY.md b/SECURITY.md index 77472c423a..96222e029d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -67,9 +67,30 @@ unless the subscriber is a member. ### Append-Only Audit Log All events are written to a tamper-evident audit log (`buzz-audit`). Each -log entry is chained to the previous one via an HMAC, making retroactive -modification detectable. The audit log is designed for SOX-grade compliance -and eDiscovery. +log entry is chained to the previous one via a SHA-256 hash chain. Because the +chain is keyless, it is tamper-evident but not tamper-resistant: it detects +accidental corruption or single-row edits, but an attacker with database write +access can recompute the entire chain after editing. The audit log is designed +for SOX-grade compliance and eDiscovery. + +### Desktop Secret Storage — OS Keyring + +The Buzz desktop app stores nsec private keys in the operating system keyring +rather than in plaintext files: macOS Keychain, Windows Credential Manager, or +the Linux Secret Service (`gnome-keyring` / `kwallet` via D-Bus). This covers +both the human identity key and every managed-agent key. + +On first launch after upgrading, existing plaintext keys are migrated into the +keyring: the key is imported, read back to verify the round-trip, and only then +is the plaintext deleted. Migration runs only when the keyring is reachable — +if the backend is unavailable that session, the app keeps reading from the +plaintext file and does **not** migrate, so a transient outage cannot resurrect +a rotated key from a leftover file. + +When no keyring backend is available (headless Linux with no Secret Service, for +example), keys fall back to a `0o600` owner-only file. The `BUZZ_PRIVATE_KEY` +environment variable, when set, always takes precedence over both stores — this +is how harnessed agents and CI receive their identity. ### Input Validation diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index a92231ad64..679ce2ed51 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -41,7 +41,7 @@ const overrides = new Map([ // harness-persona-sync: persona-runtime resolution threaded into the spawn // path here. Load-bearing feature growth; queued to split in the resolver // unify refactor followup. - ["src-tauri/src/managed_agents/runtime.rs", 1966], + ["src-tauri/src/managed_agents/runtime.rs", 1969], ["src-tauri/src/managed_agents/personas.rs", 1080], ["src-tauri/src/managed_agents/persona_card.rs", 1050], // applyWorkspace reposDir parameter plus the validateReposDir binding, @@ -53,11 +53,15 @@ const overrides = new Map([ // harness-persona-sync feature growth, queued to split in the resolver-unify // refactor followup. discovery.rs is dominated by the new test module // (the effective_agent_command / divergent / create-time override matrix); - // types.rs adds the persona/instance harness fields; migration_tests.rs adds - // the harness-sync migration coverage. Load-bearing, not generic debt. + // types.rs adds the persona/instance harness fields. Load-bearing, not + // generic debt. ["src-tauri/src/managed_agents/discovery.rs", 1043], ["src-tauri/src/managed_agents/types.rs", 1010], - ["src-tauri/src/migration_tests.rs", 1033], + // migration_tests.rs carries the harness-sync migration coverage plus the + // patch_json_records owner-only writeback regression test (SECURITY.md:90 + // crash-safe 0o600 fallback). Load-bearing security + feature coverage, not + // generic debt growth. Approved override; still queued to split. + ["src-tauri/src/migration_tests.rs", 1063], ["src-tauri/src/nostr_convert.rs", 1126], ["src/shared/api/relayClientSession.ts", 1022], ["src-tauri/src/migration.rs", 1295], @@ -70,6 +74,11 @@ const overrides = new Map([ // the Inbox nav badge — a small overage from load-bearing badge plumbing, // not generic debt growth. Approved override; still queued to split. ["src/app/AppShell.tsx", 1008], + // PersistBackend enum + marker-on-keyring-success plumbing and its three + // fail-closed regression tests (silent identity rotation on keyring outage). + // A small overage from load-bearing security plumbing on a file already at + // 893 lines, not generic debt growth. Approved override; still queued to split. + ["src-tauri/src/app_state.rs", 1012], ]); await runFileSizeCheck({ diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index e3c4a1e9e2..32ce245a39 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -865,6 +865,7 @@ dependencies = [ "futures-util", "hex", "infer", + "keyring", "libc", "mesh-llm-host-runtime", "mesh-llm-sdk", diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index 7b0d422133..a500b70dfc 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -17,8 +17,11 @@ name = "buzz_lib" crate-type = ["staticlib", "cdylib", "rlib"] [features] -default = [] +default = ["system-keyring"] mesh-llm = ["dep:mesh-llm-sdk", "dep:mesh-llm-host-runtime"] +# OS keyring backing for desktop secret storage (nsec private keys). When +# disabled, secrets fall back to 0o600 files. On by default for real builds. +system-keyring = ["dep:keyring"] [build-dependencies] serde = { version = "1", features = ["derive"] } @@ -31,9 +34,14 @@ ctrlc = { version = "3", features = ["termination"] } [target.'cfg(target_os = "macos")'.dependencies] objc2-app-kit = { version = "0.3.2", default-features = false, features = ["NSHapticFeedback"] } +keyring = { version = "3.6.3", default-features = false, features = ["apple-native", "vendored"], optional = true } [target.'cfg(windows)'.dependencies] windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem", "Win32_System_JobObjects", "Win32_System_Threading", "Win32_Foundation"] } +keyring = { version = "3.6.3", default-features = false, features = ["windows-native", "vendored"], optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +keyring = { version = "3.6.3", default-features = false, features = ["sync-secret-service", "vendored"], optional = true } [dependencies] atomic-write-file = "0.3" diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index a20dc815e0..c63ecb2fff 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -45,30 +45,41 @@ pub struct AppState { pub mesh_coordinator: AsyncMutex>, } -pub fn build_app_state() -> AppState { - // Env var takes precedence (dev/CI). If absent, resolve_persisted_identity() - // in setup() will replace the ephemeral placeholder with a persisted key. - let (keys, source) = match std::env::var("BUZZ_PRIVATE_KEY") { +/// Parse the `BUZZ_PRIVATE_KEY` env var into identity keys. `Some` means the +/// env var was present and valid and MUST win over any persisted/keyring key +/// (the dev/CI/harness override). `None` means absent or malformed — callers +/// fall through to persisted resolution. A malformed value is logged and +/// treated as absent rather than left on an ephemeral identity. +fn identity_from_env() -> Option { + match std::env::var("BUZZ_PRIVATE_KEY") { Ok(nsec) => match Keys::parse(nsec.trim()) { - Ok(keys) => (keys, "configured"), + Ok(keys) => Some(keys), Err(error) => { eprintln!("buzz-desktop: invalid BUZZ_PRIVATE_KEY: {error}"); - (Keys::generate(), "ephemeral") + None } }, Err(std::env::VarError::NotUnicode(_)) => { eprintln!("buzz-desktop: BUZZ_PRIVATE_KEY contains invalid UTF-8"); - (Keys::generate(), "ephemeral") + None } - Err(std::env::VarError::NotPresent) => (Keys::generate(), "ephemeral"), - }; - - if source == "configured" { - eprintln!( - "buzz-desktop: configured identity pubkey {}", - keys.public_key().to_hex() - ); + Err(std::env::VarError::NotPresent) => None, } +} + +pub fn build_app_state() -> AppState { + // Env var takes precedence (dev/CI). If absent, resolve_persisted_identity() + // in setup() will replace the ephemeral placeholder with a persisted key. + let keys = match identity_from_env() { + Some(keys) => { + eprintln!( + "buzz-desktop: configured identity pubkey {}", + keys.public_key().to_hex() + ); + keys + } + None => Keys::generate(), + }; AppState { keys: Mutex::new(keys), @@ -135,10 +146,8 @@ pub fn resolve_persisted_identity(app: &AppHandle, state: &AppState) -> Result<( // Only skip file-based resolution if the env var was present AND parsed // successfully. A malformed env var should fall through to the persisted // key rather than leaving the app on an ephemeral identity. - if let Ok(nsec) = std::env::var("BUZZ_PRIVATE_KEY") { - if Keys::parse(nsec.trim()).is_ok() { - return Ok(()); - } + if identity_from_env().is_some() { + return Ok(()); } let data_dir = app @@ -146,48 +155,357 @@ pub fn resolve_persisted_identity(app: &AppHandle, state: &AppState) -> Result<( .app_data_dir() .map_err(|e| format!("app data dir: {e}"))?; std::fs::create_dir_all(&data_dir).map_err(|e| format!("create app data dir: {e}"))?; - let key_path = data_dir.join("identity.key"); - // Try to load an existing key. - if key_path.exists() { - match load_key_file(&key_path) { + let keys = load_or_create_identity(&data_dir)?; + *state.keys.lock().map_err(|e| e.to_string())? = keys; + Ok(()) +} + +/// Service name for the desktop OS keyring. Shared by the human identity key +/// and managed-agent keys (each addressed by a distinct key name within it). +pub(crate) const KEYRING_SERVICE: &str = "buzz-desktop"; + +/// Keyring key name for the human identity nsec. +const IDENTITY_KEY_NAME: &str = "identity"; + +/// Filename of the marker written once a successful keyring migration deletes +/// the legacy `identity.key`. Its presence is the only durable signal that a +/// key once lived in the keyring — used to tell a genuine first-ever launch +/// (no key anywhere, generating is correct) from a post-migration boot whose +/// keyring is merely unreachable (the key IS in the keyring, must NOT generate). +const MIGRATION_MARKER_NAME: &str = "identity.migrated"; + +/// The keyring operations the identity resolution flow needs. Abstracted so the +/// corrupt-keyring recovery decision ([`recover_from_keyring`]) can be +/// unit-tested against a fake without touching the live OS keyring. +trait IdentityKeyStore { + fn probe(&self, name: &str) -> crate::secret_store::KeyringProbe; + fn load(&self, name: &str) -> Result, String>; + fn store(&self, name: &str, value: &str) -> Result<(), String>; + fn delete(&self, name: &str) -> Result<(), String>; +} + +impl IdentityKeyStore for crate::secret_store::SecretStore { + fn probe(&self, name: &str) -> crate::secret_store::KeyringProbe { + crate::secret_store::SecretStore::probe(self, name) + } + fn load(&self, name: &str) -> Result, String> { + crate::secret_store::SecretStore::load(self, name) + } + fn store(&self, name: &str, value: &str) -> Result<(), String> { + crate::secret_store::SecretStore::store(self, name, value) + } + fn delete(&self, name: &str) -> Result<(), String> { + crate::secret_store::SecretStore::delete(self, name) + } +} + +/// Resolve the human identity key: migrate a legacy `identity.key` into the +/// keyring when safe, otherwise load from whichever backend holds it, else +/// generate-and-save. +/// +/// Migration rule (prevents stale-key resurrection): only import the plaintext +/// file when the keyring is REACHABLE-but-empty. If the keyring is UNREACHABLE +/// this boot, fall back to reading the file directly and do NOT migrate — a +/// later import from a leftover (possibly rotated) file could resurrect an old +/// key. +fn load_or_create_identity(data_dir: &std::path::Path) -> Result { + let legacy_path = data_dir.join("identity.key"); + + // No keyring available in this build: the `0o600` file is the only store. + if !cfg!(feature = "system-keyring") { + 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) +} + +/// Identity resolution over an [`IdentityKeyStore`] seam. Split from +/// [`load_or_create_identity`] so the probe/recover branches are testable +/// without the live OS keyring. +fn resolve_identity_with_store( + store: &impl IdentityKeyStore, + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result { + use crate::secret_store::KeyringProbe; + + match store.probe(IDENTITY_KEY_NAME) { + KeyringProbe::Present => { + if let Some(nsec) = store.load(IDENTITY_KEY_NAME)? { + match Keys::parse(nsec.trim()) { + Ok(keys) => { + eprintln!( + "buzz-desktop: persisted identity pubkey {}", + keys.public_key().to_hex() + ); + // The key is authoritative in the keyring. A leftover + // `identity.key` means a prior migration's `remove_file` + // failed (transient AV lock, read-only mount, EPERM) and + // never retried — clean it up now so plaintext does not + // linger on disk. + cleanup_leftover_identity_file(legacy_path); + return Ok(keys); + } + // The corruption is in the KEYRING, not the file. Clear the + // bad keyring value and recover from the file (or generate + // fresh) — do NOT quarantine a valid leftover `identity.key` + // that holds the user's only good key. + Err(error) => { + return recover_from_keyring( + store, + legacy_path, + data_dir, + &error.to_string(), + ); + } + } + } + // Probe said Present but load found nothing — treat as empty. + } + KeyringProbe::ReachableButEmpty => { + // One-time migration: import the legacy plaintext file, read-back + // verify, THEN delete it. + if legacy_path.exists() { + if let Some(keys) = migrate_identity_file(store, legacy_path, data_dir)? { + return Ok(keys); + } + } + } + KeyringProbe::Unreachable => { + // Keyring down this boot. If a recoverable file is present, use it + // (and do NOT migrate — re-importing later could resurrect a + // rotated key). With NO file, the marker disambiguates two states + // that are otherwise byte-identical (Unreachable + no file): + // - marker present → the key was migrated into the keyring and the + // file deleted. The real key is unreachable, not gone. Fail + // CLOSED — generating here would silently rotate the identity. + // - no marker → genuine first-ever launch with nothing to protect. + // Generate to the `0o600` file (legitimate first-run). + if !legacy_path.exists() && migration_marker_path(data_dir).exists() { + return Err( + "identity key is in the OS keyring but the keyring is unavailable this boot; \ + retry once the keyring (Keychain / Credential Manager / Secret Service) is reachable" + .to_string(), + ); + } + return load_file_or_generate(legacy_path, data_dir); + } + } + + generate_and_persist(store, legacy_path, data_dir) +} + +/// Recover from a corrupt nsec in the keyring (parse failed). Clear the bad +/// keyring value, then migrate a valid leftover `identity.key` if one exists, +/// generating fresh only as a last resort. The keyring delete is best-effort: +/// a delete failure logs and continues — it must never block startup. +fn recover_from_keyring( + store: &impl IdentityKeyStore, + legacy_path: &std::path::Path, + data_dir: &std::path::Path, + error: &str, +) -> Result { + eprintln!("buzz-desktop: corrupt nsec in keyring ({error}), clearing and recovering from file"); + if let Err(e) = store.delete(IDENTITY_KEY_NAME) { + eprintln!("buzz-desktop: failed to clear corrupt keyring value: {e}"); + } + if legacy_path.exists() { + if let Some(keys) = migrate_identity_file(store, legacy_path, data_dir)? { + return Ok(keys); + } + } + generate_and_persist(store, legacy_path, data_dir) +} + +/// Load the `0o600` identity file, quarantining corruption, else generate and +/// save a fresh key to the file. Used when no keyring is available. +fn load_file_or_generate( + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result { + if legacy_path.exists() { + match load_key_file(legacy_path) { Ok(keys) => { eprintln!( "buzz-desktop: persisted identity pubkey {}", keys.public_key().to_hex() ); - *state.keys.lock().map_err(|e| e.to_string())? = keys; - return Ok(()); + return Ok(keys); } - Err(error) => { - // Corrupted — quarantine with a timestamp so prior backups - // are never overwritten. - let ts = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_secs()) - .unwrap_or(0); - let bad_name = format!("identity.key.bad.{ts}"); + Err(error) => quarantine_corrupt_key(legacy_path, data_dir, &error), + } + } + let keys = Keys::generate(); + save_key_file(legacy_path, &keys)?; + eprintln!( + "buzz-desktop: generated and saved identity pubkey {}", + keys.public_key().to_hex() + ); + Ok(keys) +} + +/// Import the plaintext `identity.key` into the store, verify the round-trip, +/// then delete the file. Returns `Ok(None)` if the file was corrupt (caller +/// continues to generate-and-save). +fn migrate_identity_file( + store: &impl IdentityKeyStore, + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result, String> { + let keys = match load_key_file(legacy_path) { + Ok(keys) => keys, + Err(error) => { + eprintln!("buzz-desktop: corrupt identity.key during migration ({error}), skipping"); + return Ok(None); + } + }; + let nsec = keys + .secret_key() + .to_bech32() + .map_err(|e| format!("encode nsec: {e}"))?; + + store.store(IDENTITY_KEY_NAME, &nsec)?; + // Read-back verify before deleting the plaintext file. + match store.load(IDENTITY_KEY_NAME)? { + Some(stored) if stored == nsec => { + // Crash-safe ordering: record that the key now lives in the keyring + // (marker write + fsync) BEFORE deleting the file. A crash between + // the two must never leave "file gone, no marker" — that state is + // indistinguishable from a fresh install and would silently rotate + // the identity on the next keyring-unreachable boot. If the marker + // cannot be written, keep the file so the key is never stranded. + let marker_path = migration_marker_path(data_dir); + if let Err(e) = write_migration_marker(&marker_path) { eprintln!( - "buzz-desktop: corrupt identity.key ({error}), quarantining to {bad_name}" + "buzz-desktop: keyring import ok but failed to write migration marker ({e}); \ + keeping identity.key so the key is not stranded" ); - let bad_path = data_dir.join(bad_name); - if std::fs::rename(&key_path, &bad_path).is_err() { - let _ = std::fs::remove_file(&key_path); - } + return Ok(Some(keys)); + } + if let Err(e) = std::fs::remove_file(legacy_path) { + eprintln!("buzz-desktop: keyring import ok but failed to delete identity.key: {e}"); + } else { + eprintln!("buzz-desktop: migrated identity key into OS keyring"); } + Ok(Some(keys)) } + _ => Err("keyring read-back verify failed for identity key".to_string()), } +} - // First run (or recovery from corruption): generate and save. - let keys = Keys::generate(); - save_key_file(&key_path, &keys)?; +/// Path of the migration-completed marker within `data_dir`. +fn migration_marker_path(data_dir: &std::path::Path) -> std::path::PathBuf { + data_dir.join(MIGRATION_MARKER_NAME) +} +/// Atomically write (and fsync) the migration-completed marker. The content is +/// irrelevant — only the file's durable existence is the signal — so a single +/// byte keeps it minimal. Atomicity + fsync guarantee that once this returns +/// `Ok`, the marker survives a crash, which is what makes deleting the legacy +/// file afterward safe. +fn write_migration_marker(marker_path: &std::path::Path) -> Result<(), String> { + use atomic_write_file::AtomicWriteFile; + + let mut file = AtomicWriteFile::open(marker_path) + .map_err(|e| format!("open migration marker for atomic write: {e}"))?; + file.write_all(b"1") + .map_err(|e| format!("write migration marker: {e}"))?; + file.commit() + .map_err(|e| format!("commit migration marker: {e}")) +} + +/// Which backend `persist_identity` wrote to. The caller writes the migration +/// marker only after a keyring success — on the file-fallback arm the key is on +/// disk and a marker would wrongly trip the next Unreachable boot into failing +/// closed. +enum PersistBackend { + Keyring, + File, +} + +/// Generate a fresh identity, persist it through the store, return it. +/// +/// On a keyring-backed persist no file is written, so a later +/// keyring-Unreachable boot would see "no file, no marker" (identical to a +/// fresh install) and silently rotate the identity. Writing the marker here +/// makes that boot fail closed. If the marker write fails, fall back to the +/// `0o600` file so the key is never keyring-only-without-marker. +fn generate_and_persist( + store: &impl IdentityKeyStore, + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result { + let keys = Keys::generate(); + if let PersistBackend::Keyring = persist_identity(store, &keys, legacy_path)? { + let marker_path = migration_marker_path(data_dir); + if let Err(e) = write_migration_marker(&marker_path) { + eprintln!( + "buzz-desktop: stored identity in keyring but failed to write migration marker \ + ({e}); saving identity.key fallback so the key is not stranded" + ); + save_key_file(legacy_path, &keys)?; + } + } eprintln!( "buzz-desktop: generated and saved identity pubkey {}", keys.public_key().to_hex() ); - *state.keys.lock().map_err(|e| e.to_string())? = keys; - Ok(()) + Ok(keys) +} + +/// Persist `keys` through the store, falling back to the `0o600` file when the +/// keyring write fails on an availability error. Reports which backend held the +/// key so the caller can write the migration marker only on keyring success. +fn persist_identity( + store: &impl IdentityKeyStore, + keys: &Keys, + legacy_path: &std::path::Path, +) -> Result { + let nsec = keys + .secret_key() + .to_bech32() + .map_err(|e| format!("encode nsec: {e}"))?; + match store.store(IDENTITY_KEY_NAME, &nsec) { + Ok(()) => Ok(PersistBackend::Keyring), + Err(keyring_err) => { + eprintln!("buzz-desktop: keyring write failed ({keyring_err}), using file fallback"); + save_key_file(legacy_path, keys)?; + Ok(PersistBackend::File) + } + } +} + +/// Best-effort removal of a leftover `identity.key` once the keyring is the +/// authoritative store. Idempotent: a missing file is success. Logs but does +/// not error on failure — a delete failure must never block startup. +fn cleanup_leftover_identity_file(legacy_path: &std::path::Path) { + if !legacy_path.exists() { + return; + } + match std::fs::remove_file(legacy_path) { + Ok(()) => eprintln!("buzz-desktop: removed leftover identity.key (key is in keyring)"), + Err(e) => eprintln!("buzz-desktop: failed to remove leftover identity.key: {e}"), + } +} + +/// Quarantine a corrupt `identity.key` with a timestamp so prior backups are +/// never overwritten. +fn quarantine_corrupt_key(key_path: &std::path::Path, data_dir: &std::path::Path, error: &str) { + if !key_path.exists() { + return; + } + let ts = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + let bad_name = format!("identity.key.bad.{ts}"); + eprintln!("buzz-desktop: corrupt identity.key ({error}), quarantining to {bad_name}"); + let bad_path = data_dir.join(bad_name); + if std::fs::rename(key_path, &bad_path).is_err() { + let _ = std::fs::remove_file(key_path); + } } fn load_key_file(path: &std::path::Path) -> Result { @@ -241,6 +559,50 @@ mod tests { assert_eq!(a.public_key().to_hex(), b.public_key().to_hex()); } + /// `BUZZ_PRIVATE_KEY` is process-global; serialize the env-mutating tests + /// so they don't race each other under the parallel test runner. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Run `body` with `BUZZ_PRIVATE_KEY` set to `value` (or unset when `None`), + /// restoring the prior value afterward. + fn with_env_key(value: Option<&str>, body: impl FnOnce() -> T) -> T { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let prior = std::env::var("BUZZ_PRIVATE_KEY").ok(); + match value { + Some(v) => std::env::set_var("BUZZ_PRIVATE_KEY", v), + None => std::env::remove_var("BUZZ_PRIVATE_KEY"), + } + let out = body(); + match prior { + Some(v) => std::env::set_var("BUZZ_PRIVATE_KEY", v), + None => std::env::remove_var("BUZZ_PRIVATE_KEY"), + } + out + } + + #[test] + fn identity_from_env_wins_when_valid() { + let configured = Keys::generate(); + let nsec = configured.secret_key().to_bech32().unwrap(); + + let resolved = + with_env_key(Some(&nsec), identity_from_env).expect("valid env key must resolve"); + + assert_key_eq(&configured, &resolved); + } + + #[test] + fn identity_from_env_none_when_absent() { + assert!(with_env_key(None, identity_from_env).is_none()); + } + + #[test] + fn identity_from_env_none_when_malformed() { + // A malformed env var falls through to persisted resolution rather than + // winning — otherwise a typo'd key would silently shadow the real one. + assert!(with_env_key(Some("not-a-valid-nsec"), identity_from_env).is_none()); + } + #[test] fn save_and_load_round_trip() { let dir = tempfile::tempdir().unwrap(); @@ -278,6 +640,34 @@ mod tests { assert!(load_key_file(&path).is_err()); } + #[test] + fn cleanup_removes_leftover_identity_file() { + // Item 1: a leftover identity.key (from a migration whose remove_file + // failed) is deleted once the keyring is authoritative, so plaintext + // does not linger on disk. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); + save_key_file(&path, &Keys::generate()).unwrap(); + assert!(path.exists()); + + cleanup_leftover_identity_file(&path); + + assert!(!path.exists()); + } + + #[test] + fn cleanup_is_noop_when_no_leftover_file() { + // Idempotent: the cleanup runs on every keyring-Present boot, so a + // missing file must be a silent success, not an error or panic. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); + assert!(!path.exists()); + + cleanup_leftover_identity_file(&path); + + assert!(!path.exists()); + } + #[test] fn save_creates_file_with_valid_nsec() { let dir = tempfile::tempdir().unwrap(); @@ -319,4 +709,303 @@ mod tests { let loaded = load_key_file(&path).unwrap(); assert_key_eq(&keys2, &loaded); } + + use std::cell::RefCell; + use std::collections::HashMap; + + use crate::secret_store::KeyringProbe; + + /// In-memory [`IdentityKeyStore`] for testing identity recovery without the + /// OS keyring. Seeded with an initial value and a probe outcome; records + /// every `delete`/`store` so tests can assert the keyring was cleared and + /// rewritten. `write_and_verify` succeeds (store then load reflects it). + struct FakeIdentityStore { + probe: KeyringProbe, + slot: RefCell>, + deleted: RefCell>, + /// When true, `store` returns an availability error, driving the + /// keyring-write-failure → file-fallback arm of `persist_identity`. + store_fails: bool, + } + + impl FakeIdentityStore { + fn present_with(value: &str) -> Self { + let mut slot = HashMap::new(); + slot.insert(IDENTITY_KEY_NAME.to_string(), value.to_string()); + Self { + probe: KeyringProbe::Present, + slot: RefCell::new(slot), + deleted: RefCell::new(Vec::new()), + store_fails: false, + } + } + + /// Backend down this boot: probe is `Unreachable` and the slot is empty + /// (the real key, if any, is in the keyring we cannot reach). + fn unreachable() -> Self { + Self { + probe: KeyringProbe::Unreachable, + slot: RefCell::new(HashMap::new()), + deleted: RefCell::new(Vec::new()), + store_fails: false, + } + } + + /// Backend reachable with no entry — drives the one-time migration path. + /// `store`/`load` go through the slot, so a read-back verify succeeds. + fn reachable_but_empty() -> Self { + Self { + probe: KeyringProbe::ReachableButEmpty, + slot: RefCell::new(HashMap::new()), + deleted: RefCell::new(Vec::new()), + store_fails: false, + } + } + + /// Reachable-but-empty probe whose `store` always fails — exercises the + /// keyring-write-failure → `0o600` file-fallback arm. + fn store_failing() -> Self { + Self { + probe: KeyringProbe::ReachableButEmpty, + slot: RefCell::new(HashMap::new()), + deleted: RefCell::new(Vec::new()), + store_fails: true, + } + } + } + + impl IdentityKeyStore for FakeIdentityStore { + fn probe(&self, _name: &str) -> KeyringProbe { + self.probe + } + fn load(&self, name: &str) -> Result, String> { + Ok(self.slot.borrow().get(name).cloned()) + } + fn store(&self, name: &str, value: &str) -> Result<(), String> { + if self.store_fails { + return Err("simulated keyring write failure".to_string()); + } + self.slot + .borrow_mut() + .insert(name.to_string(), value.to_string()); + Ok(()) + } + fn delete(&self, name: &str) -> Result<(), String> { + self.deleted.borrow_mut().push(name.to_string()); + self.slot.borrow_mut().remove(name); + Ok(()) + } + } + + #[test] + fn corrupt_keyring_recovers_valid_file_without_rotating() { + // The load-bearing regression guard. When the keyring holds a corrupt + // nsec (Present) AND a valid `identity.key` is on disk (leftover from a + // failed prior migration), recovery must RECOVER THE FILE'S identity — + // not quarantine the file and rotate to a fresh key (the original + // hazard). The corrupt keyring value must be cleared and replaced by the + // file's key (migrated in). + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + let file_keys = Keys::generate(); + save_key_file(&legacy_path, &file_keys).unwrap(); + + let store = FakeIdentityStore::present_with("not-a-valid-nsec"); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // The FILE's identity is recovered — NOT a freshly generated one. + assert_key_eq(&file_keys, &resolved); + // The corrupt keyring value was cleared. + assert_eq!(store.deleted.borrow().as_slice(), [IDENTITY_KEY_NAME]); + // The keyring now holds the file's key (migrated in, read-back verified). + let file_nsec = file_keys.secret_key().to_bech32().unwrap(); + assert_eq!( + store + .slot + .borrow() + .get(IDENTITY_KEY_NAME) + .map(String::as_str), + Some(file_nsec.as_str()) + ); + // The valid file was migrated (deleted), not quarantined to .bad.*. + assert!(!legacy_path.exists()); + assert!(std::fs::read_dir(dir.path()).unwrap().all(|e| !e + .unwrap() + .file_name() + .to_string_lossy() + .contains(".bad."))); + } + + #[test] + fn corrupt_keyring_generates_fresh_only_when_no_file() { + // With a corrupt keyring value and NO file on disk, generate-fresh is + // the correct last resort — and the corrupt keyring value is cleared + // first. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + assert!(!legacy_path.exists()); + + let store = FakeIdentityStore::present_with("not-a-valid-nsec"); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + assert_eq!(store.deleted.borrow().as_slice(), [IDENTITY_KEY_NAME]); + // A fresh, valid key was persisted to the keyring (replacing the cleared + // corrupt value). + let stored = store.slot.borrow().get(IDENTITY_KEY_NAME).cloned(); + assert_eq!( + stored.as_deref(), + Some(resolved.secret_key().to_bech32().unwrap().as_str()) + ); + } + + #[test] + fn valid_keyring_is_used_and_leftover_file_cleaned_up() { + // The happy path is unchanged: a valid keyring value is used as-is, and + // a leftover plaintext file is cleaned up (keyring is authoritative). + let keyring_keys = Keys::generate(); + let nsec = keyring_keys.secret_key().to_bech32().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + save_key_file(&legacy_path, &Keys::generate()).unwrap(); + + let store = FakeIdentityStore::present_with(&nsec); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + assert_key_eq(&keyring_keys, &resolved); + assert!(store.deleted.borrow().is_empty()); + assert!(!legacy_path.exists()); + } + + #[test] + fn unreachable_post_migration_fails_closed_when_marker_present() { + // The silent-rotation hazard (Wes Comment 1). After a migration the + // file is gone and the marker exists; a later boot with the keyring + // unreachable must FAIL CLOSED — the real key is in the keyring, not + // gone, so generating a fresh one would silently rotate the identity. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + write_migration_marker(&migration_marker_path(dir.path())).unwrap(); + assert!(!legacy_path.exists()); + + let store = FakeIdentityStore::unreachable(); + let result = resolve_identity_with_store(&store, &legacy_path, dir.path()); + + assert!( + result.is_err(), + "must fail closed, not generate a fresh key" + ); + // No identity file was written — nothing was generated or persisted. + assert!(!legacy_path.exists()); + } + + #[test] + fn unreachable_first_run_generates_to_file_when_no_marker() { + // Genuine first-EVER launch on a machine whose keyring is down: no file, + // no marker. There is no prior identity to protect, so generating to the + // `0o600` file is correct — fail-closed here would block a legitimate + // first launch. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + assert!(!legacy_path.exists()); + assert!(!migration_marker_path(dir.path()).exists()); + + let store = FakeIdentityStore::unreachable(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // A fresh key was generated and persisted to the file (keyring is down). + let from_file = load_key_file(&legacy_path).unwrap(); + assert_key_eq(&resolved, &from_file); + } + + #[test] + fn migration_writes_marker_before_deleting_file() { + // Crash-safe ordering: a successful migration must leave the marker on + // disk AND remove the file. The marker existing while the file is gone + // is the durable post-migration signal the Unreachable arm relies on; + // "file gone, no marker" must never be the resting state. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + let file_keys = Keys::generate(); + save_key_file(&legacy_path, &file_keys).unwrap(); + + // ReachableButEmpty drives the one-time migration path. + let store = FakeIdentityStore::reachable_but_empty(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + assert_key_eq(&file_keys, &resolved); + // Marker written, file deleted — the safe resting state. + assert!(migration_marker_path(dir.path()).exists()); + assert!(!legacy_path.exists()); + } + + #[test] + fn fresh_keyring_generate_writes_marker() { + // Fix 1 (Pinky comment 1): a fresh install generating straight into a + // reachable-but-empty keyring must write the marker. Without it, "no + // file, no marker" matches a never-launched machine, so a later + // Unreachable boot would silently rotate the key. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + assert!(!legacy_path.exists()); + + let store = FakeIdentityStore::reachable_but_empty(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // The key was stored in the keyring (not the file), and the marker marks it. + assert!(!legacy_path.exists()); + assert!(migration_marker_path(dir.path()).exists()); + assert_eq!( + store + .slot + .borrow() + .get(IDENTITY_KEY_NAME) + .map(String::as_str), + Some(resolved.secret_key().to_bech32().unwrap().as_str()) + ); + } + + #[test] + fn fresh_keyring_generate_then_unreachable_fails_closed() { + // The end-to-end guard for Fix 1: after a fresh keyring-created identity + // (marker written, no file), a later boot with the keyring unreachable + // must FAIL CLOSED rather than generate a new key and rotate identity. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + + // First boot: fresh generate into a reachable keyring. + let reachable = FakeIdentityStore::reachable_but_empty(); + resolve_identity_with_store(&reachable, &legacy_path, dir.path()).unwrap(); + assert!(!legacy_path.exists()); + assert!(migration_marker_path(dir.path()).exists()); + + // Second boot: keyring is down. No file + marker present → fail closed. + let unreachable = FakeIdentityStore::unreachable(); + let result = resolve_identity_with_store(&unreachable, &legacy_path, dir.path()); + + assert!( + result.is_err(), + "must fail closed, not generate a fresh key" + ); + assert!(!legacy_path.exists()); + } + + #[test] + fn fresh_generate_keyring_failure_falls_back_to_file_without_marker() { + // Fix 1 correctness on the file-fallback arm: when the keyring write + // FAILS during a fresh generate, the key must land in the `0o600` file + // and the marker must NOT be written — a marker here would wrongly trip + // the next Unreachable boot into failing closed even though the key is + // sitting in the file. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + + let store = FakeIdentityStore::store_failing(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // Key persisted to the file (fallback), and recoverable from it. + let from_file = load_key_file(&legacy_path).unwrap(); + assert_key_eq(&resolved, &from_file); + // No marker: the file is the authoritative store, not the keyring. + assert!(!migration_marker_path(dir.path()).exists()); + } } diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 5ca1807132..14a2faafce 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -8,11 +8,12 @@ use crate::{ find_managed_agent_mut, invoke_provider, load_managed_agents, load_personas, managed_agent_avatar_url, managed_agent_log_path, managed_agents_base_dir, normalize_agent_args, provider_deploy, read_log_tail, resolve_provider_binary, - save_managed_agents, start_managed_agent_process, stop_managed_agent_process, - sync_managed_agent_processes, try_regenerate_nest, validate_provider_config, BackendKind, - BackendProviderInfo, CreateManagedAgentRequest, CreateManagedAgentResponse, - ManagedAgentLogResponse, ManagedAgentRecord, ManagedAgentSummary, RelayMeshConfig, - DEFAULT_ACP_COMMAND, DEFAULT_AGENT_PARALLELISM, DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, + save_managed_agents, spawn_key_refusal, start_managed_agent_process, + stop_managed_agent_process, sync_managed_agent_processes, try_regenerate_nest, + validate_provider_config, BackendKind, BackendProviderInfo, CreateManagedAgentRequest, + CreateManagedAgentResponse, ManagedAgentLogResponse, ManagedAgentRecord, + ManagedAgentSummary, RelayMeshConfig, DEFAULT_ACP_COMMAND, DEFAULT_AGENT_PARALLELISM, + DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, }, relay::{relay_ws_url_with_override, sync_managed_agent_profile}, util::now_iso, @@ -148,6 +149,13 @@ fn build_deploy_payload( state: &AppState, record: &ManagedAgentRecord, ) -> Result { + // Fail closed when the private key is unavailable (keyring outage leaves it + // empty after hydration). Without this, a provider deploy would serialize + // `"private_key_nsec": ""` and launch the agent with no identity — the same + // hazard the local spawn path already refuses via this guard. + if let Some(error) = spawn_key_refusal(record) { + return Err(error); + } // Merge persona env_vars + agent env_vars for provider deploy. Same // precedence as local spawn: persona first, agent overrides last. Without // this, provider-backed agents wouldn't receive credentials saved on the @@ -1149,6 +1157,8 @@ pub fn delete_managed_agent( return Err(format!("agent {pubkey} not found")); } save_managed_agents(&app, &records)?; + // Remove the agent's nsec from the keyring after the record is gone. + crate::managed_agents::delete_agent_key(&pubkey); } try_regenerate_nest(&app); Ok(()) diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 8cf55b13e3..55fa38a5a1 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -11,6 +11,7 @@ mod models; pub mod nostr_convert; mod prevent_sleep; mod relay; +mod secret_store; mod templates; mod util; diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 65fd220bec..9ed093237e 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -6,7 +6,7 @@ use crate::{ managed_agents::{ append_log_marker, known_acp_runtime, login_shell_path, managed_agent_log_path, missing_command_message, normalize_agent_args, open_log_file, resolve_command, - ManagedAgentProcess, ManagedAgentRecord, ManagedAgentSummary, + spawn_key_refusal, ManagedAgentProcess, ManagedAgentRecord, ManagedAgentSummary, }, util::now_iso, }; @@ -1499,6 +1499,9 @@ pub fn spawn_agent_child( record: &ManagedAgentRecord, owner_hex: Option<&str>, ) -> Result { + if let Some(error) = spawn_key_refusal(record) { + return Err(error); + } let log_path = managed_agent_log_path(app, &record.pubkey)?; append_log_marker( &log_path, diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 2bd975ca06..495c7c1dd1 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -6,7 +6,25 @@ use std::{ use tauri::{AppHandle, Manager}; +use crate::app_state::KEYRING_SERVICE; use crate::managed_agents::ManagedAgentRecord; +use crate::secret_store::{KeyringProbe, SecretStore}; + +/// Keyring key name for an agent's nsec, namespaced from the human identity +/// key (`"identity"`) which shares the service. +fn agent_keyring_name(pubkey: &str) -> String { + format!("agent:{pubkey}") +} + +/// 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 + } +} pub fn managed_agents_base_dir(app: &AppHandle) -> Result { let dir = app @@ -32,6 +50,100 @@ pub fn managed_agent_log_path(app: &AppHandle, pubkey: &str) -> Result KeyringProbe; + /// Read a key. `Ok(None)` is "no such entry" (absent); `Err` is a backend + /// failure (keyring unreachable) — the caller MUST NOT collapse the two. + fn load(&self, name: &str) -> Result, String>; + /// Write `value` and read it back to confirm before the caller strips the + /// inline copy. + fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String>; +} + +impl KeyStore for SecretStore { + fn probe(&self, name: &str) -> KeyringProbe { + SecretStore::probe(self, name) + } + fn load(&self, name: &str) -> Result, String> { + SecretStore::load(self, name) + } + fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { + self.store(name, value)?; + match self.load(name)? { + Some(stored) if stored == value => Ok(()), + _ => Err("keyring read-back verify failed".to_string()), + } + } +} + +/// Outcome of attempting to lift a record's inline key into the keyring. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum KeyMigration { + /// Written to the keyring and read-back verified. Safe to drop the inline + /// copy when serializing. + Persisted, + /// Could not persist (keyring unreachable, or write/verify failed). The key + /// must stay inline (0o600 file fallback); do NOT drop it. + KeptInline, + /// The record carried no inline key, so there was nothing to migrate. Kept + /// distinct from [`KeyMigration::Persisted`] so an empty key is never + /// mistaken for "verified present in the keyring" — an empty key after a + /// keyring outage means the secret is currently unavailable, not persisted. + Nothing, +} + +/// Attempt to lift one record's inline key into the keyring with read-back +/// verify. Pure decision logic — does NOT mutate the record, so the caller +/// chooses whether to strip the inline copy based on the returned outcome. +/// +/// The single source of truth for the migrate-vs-keep decision, shared by the +/// load-time opportunistic re-migrate ([`hydrate_keys`]) and the save-time +/// chokepoint ([`persist_agent_keys`]). An empty key returns +/// [`KeyMigration::Nothing`] — never [`KeyMigration::Persisted`], so a record +/// left empty by a keyring outage is not mistaken for one verified present. +fn migrate_inline_key(store: &impl KeyStore, record: &ManagedAgentRecord) -> KeyMigration { + if record.private_key_nsec.is_empty() { + return KeyMigration::Nothing; + } + let name = agent_keyring_name(&record.pubkey); + match store.probe(&name) { + // Keyring down this boot: keep the key inline (file fallback), do NOT + // migrate — re-importing later could resurrect a rotated key. + KeyringProbe::Unreachable => KeyMigration::KeptInline, + KeyringProbe::Present | KeyringProbe::ReachableButEmpty => { + match store.write_and_verify(&name, &record.private_key_nsec) { + Ok(()) => KeyMigration::Persisted, + Err(e) => { + eprintln!( + "buzz-desktop: keyring write for agent {} failed ({e}), keeping inline", + record.pubkey + ); + KeyMigration::KeptInline + } + } + } + } +} + +/// Refuse to spawn an agent whose private key is unavailable. Returns +/// `Some(error)` when `private_key_nsec` is empty — after [`hydrate_keys`] an +/// empty key means a keyring outage or a genuinely absent secret, NOT a +/// deliberately keyless agent. Spawning anyway would inject an empty +/// `BUZZ_PRIVATE_KEY`/`NOSTR_PRIVATE_KEY`, launching with no identity. Callers +/// (the spawn path) must fail closed (Wes storage.rs:158). +pub(crate) fn spawn_key_refusal(record: &ManagedAgentRecord) -> Option { + record.private_key_nsec.is_empty().then(|| { + format!( + "agent {} has no private key available — the OS keyring may be unreachable. \ + Refusing to start without an identity; retry once the keyring is reachable.", + record.pubkey + ) + }) +} + pub fn load_managed_agents(app: &AppHandle) -> Result, String> { let path = managed_agents_store_path(app)?; if !path.exists() { @@ -40,7 +152,69 @@ pub fn load_managed_agents(app: &AppHandle) -> Result, S let content = fs::read_to_string(&path) .map_err(|error| format!("failed to read agent store: {error}"))?; - serde_json::from_str(&content).map_err(|error| format!("failed to parse agent store: {error}")) + let mut records: Vec = serde_json::from_str(&content) + .map_err(|error| format!("failed to parse agent store: {error}"))?; + + hydrate_keys(&mut records); + Ok(records) +} + +/// Fill in each record's in-memory `private_key_nsec` from the keyring, and +/// opportunistically re-migrate any key that is still inline. +/// +/// - Empty key → fetch it from the keyring (the normal keyring-backed case). +/// - Non-empty key → the JSON carried it inline because the keyring was +/// unreachable at its last save. Re-migrate it now ([`migrate_inline_key`]): +/// if the keyring is reachable this boot, write-verify-strip so the next save +/// writes clean JSON and plaintext stops lingering on disk; if still +/// unreachable, leave it inline. This makes the strip deterministic on the +/// next reachable boot rather than waiting for a non-deterministic save. +fn hydrate_keys(records: &mut [ManagedAgentRecord]) { + let Some(store) = agent_secret_store() else { + return; + }; + hydrate_keys_with(&store, records); +} + +/// Testable core of [`hydrate_keys`], generic over the [`KeyStore`] seam. +/// +/// A keyring LOAD error (`Err`) is an OUTAGE — distinct from `Ok(None)` +/// (genuinely absent). On an outage the key is left empty and the record is +/// surfaced as unavailable rather than silently swallowed: callers must refuse +/// to spawn an agent whose key could not be read (see the empty-key bail in +/// `spawn_agent_child`). Empty here never means "fine" — it means "no usable +/// key this boot." +fn hydrate_keys_with(store: &impl KeyStore, records: &mut [ManagedAgentRecord]) { + for record in records.iter_mut() { + if record.private_key_nsec.is_empty() { + match store.load(&agent_keyring_name(&record.pubkey)) { + Ok(Some(nsec)) => record.private_key_nsec = nsec, + Ok(None) => { + eprintln!( + "buzz-desktop: agent {} has no key in JSON or keyring", + record.pubkey + ); + } + // Outage, NOT absence: the key may exist in the keyring but is + // unreadable this boot. Leave it empty so the spawn path + // refuses rather than launching with no identity. + Err(e) => { + eprintln!( + "buzz-desktop: agent {} key unavailable — keyring read failed ({e}); \ + agent will be refused until the keyring is reachable", + record.pubkey + ); + } + } + } else { + // Inline residue from a prior keyring-unreachable save. Lift it + // into the keyring now (side effect) but KEEP it in memory — the + // returned record must carry the key for readers. The next save + // then strips it from JSON. Outcome is intentionally ignored: + // on failure the key simply stays inline until a later boot. + let _ = migrate_inline_key(store, record); + } + } } pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> Result<(), String> { @@ -52,11 +226,50 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R .then_with(|| left.pubkey.cmp(&right.pubkey)) }); + // Persist each key to the keyring; on success blank the inline copy so it + // is skipped from JSON (`skip_serializing_if = "String::is_empty"`). If the + // keyring is unreachable, the key stays inline. + persist_agent_keys(&mut sorted); + let path = managed_agents_store_path(app)?; let payload = serde_json::to_vec_pretty(&sorted) .map_err(|error| format!("failed to serialize agent store: {error}"))?; - atomic_write_json(&path, &payload) + // `managed-agents.json` carries plaintext agent nsecs in the keyringless + // fallback. Write it owner-only (`0o600`) unconditionally — harmless for the + // keyring-backed case (it is the user's own agent store) and closes the + // umask window a post-write `chmod` would leave open. + atomic_write_json_restricted(&path, &payload) +} + +/// Write each record's in-memory key to the keyring and blank the inline copy +/// on success. Keys that cannot be persisted (keyring unreachable) stay inline +/// in the JSON. Mutates `records` (a save-local clone) — the caller's in-memory +/// records keep their keys. +fn persist_agent_keys(records: &mut [ManagedAgentRecord]) { + let Some(store) = agent_secret_store() else { + // No keyring backend: keys stay inline. + return; + }; + for record in records.iter_mut() { + // Only a verified keyring entry lets us drop the inline copy. Both + // other outcomes keep the key inline: `KeptInline` (keyring + // 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 { + record.private_key_nsec.clear(); + } + } +} +/// Remove an agent's key from the keyring (best-effort). Called when an agent +/// is deleted so its secret does not linger in the OS store. +pub fn delete_agent_key(pubkey: &str) { + if let Some(store) = agent_secret_store() { + if let Err(e) = store.delete(&agent_keyring_name(pubkey)) { + eprintln!("buzz-desktop: failed to delete agent {pubkey} key from keyring: {e}"); + } + } } /// Atomic, symlink-preserving JSON write. @@ -70,6 +283,34 @@ pub(crate) fn atomic_write_json(path: &Path, payload: &[u8]) -> Result<(), Strin .map_err(|e| format!("failed to rename {}: {e}", resolved.display())) } +/// Atomic, symlink-preserving JSON write that creates the file `0o600` BEFORE +/// any bytes hit disk — closing the umask window the post-write `chmod` left +/// open. Used for `managed-agents.json`, which carries plaintext agent nsecs in +/// the keyringless fallback. Mirrors [`crate::app_state::save_key_file`]. +/// +/// Canonicalizes `path` first so the write lands at the real target, preserving +/// any symlink at `path` exactly like [`atomic_write_json`]. +pub(crate) fn atomic_write_json_restricted(path: &Path, payload: &[u8]) -> Result<(), String> { + use atomic_write_file::AtomicWriteFile; + + let resolved = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + let mut file = AtomicWriteFile::open(&resolved) + .map_err(|e| format!("open {} for atomic write: {e}", resolved.display()))?; + + // Set owner-only permissions before writing the secret bytes. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(std::fs::Permissions::from_mode(0o600)) + .map_err(|e| format!("set {} permissions: {e}", resolved.display()))?; + } + + file.write_all(payload) + .map_err(|e| format!("write {}: {e}", resolved.display()))?; + file.commit() + .map_err(|e| format!("commit {}: {e}", resolved.display())) +} + /// Maximum log file size before rotation (10 MB). const MAX_LOG_FILE_SIZE: u64 = 10 * 1024 * 1024; @@ -213,16 +454,243 @@ pub fn meaningful_agent_error_from_log(path: &Path) -> Option { #[cfg(test)] mod tests { + use std::cell::RefCell; + use std::collections::HashMap; use std::io::Write as _; use tempfile::NamedTempFile; + use super::{ + agent_keyring_name, hydrate_keys_with, migrate_inline_key, KeyMigration, KeyStore, + KeyringProbe, ManagedAgentRecord, + }; + + /// In-memory [`KeyStore`] for testing the migrate decision without the OS + /// keyring. `reachable=false` simulates a backend outage; `fail_verify` + /// simulates a write whose read-back does not confirm. + struct FakeKeyStore { + reachable: bool, + fail_verify: bool, + stored: RefCell>, + } + + impl FakeKeyStore { + fn reachable() -> Self { + Self { + reachable: true, + fail_verify: false, + stored: RefCell::new(HashMap::new()), + } + } + fn unreachable() -> Self { + Self { + reachable: false, + fail_verify: false, + stored: RefCell::new(HashMap::new()), + } + } + fn verify_fails() -> Self { + Self { + reachable: true, + fail_verify: true, + stored: RefCell::new(HashMap::new()), + } + } + /// Seed a key as already present in the keyring. + fn with_key(self, name: &str, value: &str) -> Self { + self.stored + .borrow_mut() + .insert(name.to_string(), value.to_string()); + self + } + } + + impl KeyStore for FakeKeyStore { + fn probe(&self, _name: &str) -> KeyringProbe { + if self.reachable { + KeyringProbe::ReachableButEmpty + } else { + KeyringProbe::Unreachable + } + } + fn load(&self, name: &str) -> Result, String> { + // An unreachable backend errors on read (outage), distinct from a + // reachable backend returning `Ok(None)` for an absent entry. + if !self.reachable { + return Err("keyring backend unreachable".to_string()); + } + Ok(self.stored.borrow().get(name).cloned()) + } + fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { + if self.fail_verify { + return Err("read-back verify failed".to_string()); + } + self.stored + .borrow_mut() + .insert(name.to_string(), value.to_string()); + Ok(()) + } + } + + fn record_with_key(nsec: &str) -> ManagedAgentRecord { + serde_json::from_str(&format!( + r#"{{ + "pubkey": "agent-pubkey", + "name": "test-agent", + "private_key_nsec": "{nsec}", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }}"# + )) + .expect("sample record") + } + + #[test] + fn migrate_persists_and_signals_stripping_when_keyring_reachable() { + // Item 2: an inline key (residue from a prior keyring-unreachable save) + // is written to the keyring and verified when the backend is reachable, + // so the next save can drop it from JSON. + let store = FakeKeyStore::reachable(); + let record = record_with_key("nsec1realkey"); + + let outcome = migrate_inline_key(&store, &record); + + assert_eq!(outcome, KeyMigration::Persisted); + assert_eq!( + store + .stored + .borrow() + .get(&agent_keyring_name("agent-pubkey")) + .map(String::as_str), + Some("nsec1realkey") + ); + } + + #[test] + fn migrate_keeps_inline_when_keyring_unreachable() { + // No-resurrection guard: a transient outage must NOT migrate; the key + // stays inline (file fallback) so it is not lost. + let store = FakeKeyStore::unreachable(); + let record = record_with_key("nsec1realkey"); + + let outcome = migrate_inline_key(&store, &record); + + assert_eq!(outcome, KeyMigration::KeptInline); + assert!(store.stored.borrow().is_empty()); + } + + #[test] + fn migrate_keeps_inline_when_verify_fails() { + // A write whose read-back does not confirm must keep the key inline — + // never drop plaintext on an unverified write. + let store = FakeKeyStore::verify_fails(); + let record = record_with_key("nsec1realkey"); + + assert_eq!( + migrate_inline_key(&store, &record), + KeyMigration::KeptInline + ); + } + + #[test] + fn migrate_reports_nothing_for_empty_key() { + // A record whose key already lives in the keyring (empty inline) has + // nothing to migrate. It must NOT be reported as `Persisted` — an + // empty key after a keyring outage means the secret is unavailable, + // not verified present (Wes storage.rs:158). + let store = FakeKeyStore::reachable(); + let record = record_with_key(""); + + assert_eq!(migrate_inline_key(&store, &record), KeyMigration::Nothing); + assert!(store.stored.borrow().is_empty()); + } + + #[test] + fn hydrate_fills_key_from_keyring_when_reachable() { + // The normal keyring-backed case: an empty inline key is filled from + // the keyring on load. + let store = + FakeKeyStore::reachable().with_key(&agent_keyring_name("agent-pubkey"), "nsec1stored"); + let mut records = vec![record_with_key("")]; + + hydrate_keys_with(&store, &mut records); + + assert_eq!(records[0].private_key_nsec, "nsec1stored"); + } + + #[test] + fn hydrate_leaves_key_empty_on_keyring_outage() { + // Outage edge (Wes storage.rs:158): when the keyring read ERRORS, the + // key must be left empty — never silently treated as resolved — so the + // spawn path refuses rather than launching the agent with no identity. + let store = FakeKeyStore::unreachable(); + let mut records = vec![record_with_key("")]; + + hydrate_keys_with(&store, &mut records); + + assert!( + records[0].private_key_nsec.is_empty(), + "an unreadable key must stay empty, not be fabricated" + ); + } + + #[test] + fn spawn_refused_when_private_key_empty() { + // The spawn path MUST refuse a record left empty by an outage/absence + // before injecting an empty BUZZ_PRIVATE_KEY / NOSTR_PRIVATE_KEY — never + // launch an agent with no identity (Wes storage.rs:158). + let record = record_with_key(""); + assert!( + super::spawn_key_refusal(&record).is_some(), + "an agent with no private key must be refused" + ); + } + + #[test] + fn spawn_allowed_when_private_key_present() { + // A record carrying a key must not be blocked by the refusal guard. + let record = record_with_key("nsec1realkey"); + assert!(super::spawn_key_refusal(&record).is_none()); + } + fn write_log(content: &str) -> NamedTempFile { let mut file = NamedTempFile::new().expect("temp log"); file.write_all(content.as_bytes()).expect("write log"); file } + /// The keyringless fallback write must land `0o600` from the write itself — + /// not a post-write `chmod` — so a crash in the umask window can never leave + /// plaintext agent nsecs world-readable (Wes storage.rs:239, SECURITY.md:90). + #[cfg(unix)] + #[test] + fn restricted_write_lands_owner_only_without_post_write_chmod() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("managed-agents.json"); + + super::atomic_write_json_restricted(&path, br#"[{"private_key_nsec":"nsec1secret"}]"#) + .expect("restricted write"); + + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "secret-bearing write must be owner-only"); + assert_eq!( + std::fs::read_to_string(&path).expect("read back"), + r#"[{"private_key_nsec":"nsec1secret"}]"# + ); + } + #[test] fn meaningful_agent_error_from_log_promotes_wrapped_llm_auth() { let file = write_log("noise\nAgent reported error: llm auth: denied\n"); diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 09dac97d70..7f595bccc0 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -88,9 +88,16 @@ pub struct ManagedAgentRecord { pub name: String, #[serde(default)] pub persona_id: Option, - /// `#[serde(default)]` so an old build still parses a store whose inline - /// key was stripped after a keyring build migrated it into the Keychain. - #[serde(default)] + /// nsec private key. Held in memory but persisted to the OS keyring (keyed + /// by `pubkey`) rather than serialized to `managed-agents.json`. The + /// storage layer blanks this before writing JSON once the key is safely in + /// the keyring, and re-hydrates it from the keyring on load. + /// + /// It is only serialized inline (the `0o600` JSON fallback) when the + /// keyring is unreachable — `skip_serializing_if` keeps it out of JSON in + /// the normal keyring-backed case. `default` also lets an old build parse a + /// store whose inline key was already migrated out and blanked. + #[serde(default, skip_serializing_if = "String::is_empty")] pub private_key_nsec: String, /// NIP-OA auth tag JSON. Computed at agent creation time. /// @@ -656,354 +663,4 @@ pub fn validate_respond_to_allowlist(input: &[String]) -> Result, St } #[cfg(test)] -mod tests { - use super::{ManagedAgentRecord, PersonaRecord}; - use std::path::PathBuf; - - #[test] - fn persona_record_defaults_active_when_field_is_missing() { - let record: PersonaRecord = serde_json::from_str( - r#"{ - "id": "builtin:fizz", - "display_name": "Fizz", - "avatar_url": null, - "system_prompt": "Prompt", - "created_at": "2026-03-19T00:00:00Z", - "updated_at": "2026-03-19T00:00:00Z" - }"#, - ) - .expect("legacy persona payload should deserialize"); - - assert!(record.is_active); - assert!(!record.is_builtin); - assert_eq!(record.runtime, None); - assert_eq!(record.model, None); - assert!(record.name_pool.is_empty()); - } - - /// Legacy agent records (created before NIP-OA) lack the `auth_tag` field. - /// `#[serde(default)]` must ensure they deserialize with `auth_tag: None`. - #[test] - fn managed_agent_record_without_auth_tag_deserializes() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("legacy agent record without auth_tag should deserialize"); - - assert_eq!(record.auth_tag, None); - assert_eq!(record.avatar_url, None); - assert_eq!(record.pubkey, "abcd1234"); - } - - /// Agent records WITH an auth_tag round-trip correctly through serde. - #[test] - fn managed_agent_record_with_auth_tag_round_trips() { - let json = r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "auth_tag": "[\"auth\",\"deadbeef\",\"\",\"cafebabe\"]", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#; - - let record: ManagedAgentRecord = - serde_json::from_str(json).expect("record with auth_tag should deserialize"); - - assert_eq!( - record.auth_tag.as_deref(), - Some(r#"["auth","deadbeef","","cafebabe"]"#) - ); - - // Round-trip: serialize and deserialize again. - let serialized = serde_json::to_string(&record).expect("should serialize"); - let record2: ManagedAgentRecord = - serde_json::from_str(&serialized).expect("round-trip should deserialize"); - assert_eq!(record.auth_tag, record2.auth_tag); - } - - // ── Inbound author gate tests ──────────────────────────────────────── - - use super::{validate_respond_to_allowlist, RespondTo}; - - #[test] - fn respond_to_default_is_owner_only() { - assert_eq!(RespondTo::default(), RespondTo::OwnerOnly); - } - - #[test] - fn respond_to_serde_is_kebab_case() { - assert_eq!( - serde_json::to_string(&RespondTo::OwnerOnly).unwrap(), - "\"owner-only\"" - ); - assert_eq!( - serde_json::to_string(&RespondTo::Allowlist).unwrap(), - "\"allowlist\"" - ); - assert_eq!( - serde_json::to_string(&RespondTo::Anyone).unwrap(), - "\"anyone\"" - ); - let parsed: RespondTo = serde_json::from_str("\"owner-only\"").unwrap(); - assert_eq!(parsed, RespondTo::OwnerOnly); - let parsed: RespondTo = serde_json::from_str("\"allowlist\"").unwrap(); - assert_eq!(parsed, RespondTo::Allowlist); - let parsed: RespondTo = serde_json::from_str("\"anyone\"").unwrap(); - assert_eq!(parsed, RespondTo::Anyone); - } - - #[test] - fn respond_to_rejects_unknown_modes() { - // `nobody` is a valid harness mode but intentionally not exposed - // through the desktop request types. - assert!(serde_json::from_str::("\"nobody\"").is_err()); - assert!(serde_json::from_str::("\"OwnerOnly\"").is_err()); - } - - /// Records persisted before this feature must continue to load, - /// defaulting to OwnerOnly (the safe, matches-harness-default value). - #[test] - fn managed_agent_record_without_respond_to_fields_defaults_to_owner_only() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "legacy-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("legacy record without respond_to fields should deserialize"); - assert_eq!(record.respond_to, RespondTo::OwnerOnly); - assert!(record.respond_to_allowlist.is_empty()); - } - - #[test] - fn validate_respond_to_allowlist_accepts_valid_hex_and_lowercases() { - let upper = "A".repeat(64); - let lower = "a".repeat(64); - let result = validate_respond_to_allowlist(std::slice::from_ref(&upper)).unwrap(); - assert_eq!(result, vec![lower.clone()]); - } - - #[test] - fn validate_respond_to_allowlist_dedups_preserving_order() { - let a = "a".repeat(64); - let b = "b".repeat(64); - let a_upper = "A".repeat(64); - let input = vec![a.clone(), b.clone(), a_upper]; - let result = validate_respond_to_allowlist(&input).unwrap(); - assert_eq!(result, vec![a, b]); - } - - #[test] - fn validate_respond_to_allowlist_rejects_wrong_length() { - let too_short = "a".repeat(63); - assert!(validate_respond_to_allowlist(&[too_short]).is_err()); - let too_long = "a".repeat(65); - assert!(validate_respond_to_allowlist(&[too_long]).is_err()); - } - - #[test] - fn validate_respond_to_allowlist_rejects_non_hex() { - let bad = "z".repeat(64); - assert!(validate_respond_to_allowlist(&[bad]).is_err()); - // npub-style strings should not slip through. - let npub = format!("npub1{}", "a".repeat(59)); - assert!(validate_respond_to_allowlist(&[npub]).is_err()); - } - - #[test] - fn validate_respond_to_allowlist_trims_whitespace() { - let padded = format!(" {} ", "a".repeat(64)); - let result = validate_respond_to_allowlist(&[padded]).unwrap(); - assert_eq!(result, vec!["a".repeat(64)]); - } - - #[test] - fn validate_respond_to_allowlist_accepts_empty() { - // Empty is allowed at this layer; the boundary check - // (Allowlist mode requires ≥1 entry) is the caller's job. - let result = validate_respond_to_allowlist(&[]).unwrap(); - assert!(result.is_empty()); - } - - use super::{CreateManagedAgentRequest, RelayMeshConfig}; - - /// Wire-shape test: the create request arrives from TS as camelCase - /// (`relayMesh: { modelRef }`). `rename_all = "camelCase"` on - /// `CreateManagedAgentRequest` does NOT recurse into nested structs, so - /// `RelayMeshConfig` needs its own `alias = "modelRef"`. This test pins - /// the exact JSON the frontend sends; if the alias is dropped, creating - /// a relay-mesh agent fails to deserialize at the Tauri boundary. - #[test] - fn create_request_deserializes_camel_case_relay_mesh() { - let request: CreateManagedAgentRequest = serde_json::from_str( - r#"{ - "name": "mesh-agent", - "relayMesh": { "modelRef": "Qwen3" } - }"#, - ) - .expect("camelCase relayMesh payload from TS should deserialize"); - assert_eq!( - request.relay_mesh, - Some(RelayMeshConfig { - model_ref: "Qwen3".to_string() - }) - ); - } - - /// Persisted records use snake_case; the camelCase alias must not break - /// the stored-record round trip. - #[test] - fn relay_mesh_config_round_trips_snake_case() { - let config = RelayMeshConfig { - model_ref: "Qwen3".to_string(), - }; - let json = serde_json::to_string(&config).unwrap(); - assert_eq!(json, r#"{"model_ref":"Qwen3"}"#); - let back: RelayMeshConfig = serde_json::from_str(&json).unwrap(); - assert_eq!(back, config); - } - - // ── Packs → Teams serde alias backward compatibility ──────────────── - - #[test] - fn persona_record_deserializes_old_source_pack_fields_via_alias() { - let record: PersonaRecord = serde_json::from_str( - r#"{ - "id": "persona-1", - "display_name": "Test", - "avatar_url": null, - "system_prompt": "Prompt", - "source_pack": "com.example.my-pack", - "source_pack_persona_slug": "agent-one", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }"#, - ) - .expect("old-format persona with source_pack should deserialize via alias"); - - assert_eq!(record.source_team.as_deref(), Some("com.example.my-pack")); - assert_eq!( - record.source_team_persona_slug.as_deref(), - Some("agent-one") - ); - } - - #[test] - fn persona_record_serializes_new_field_names() { - let record: PersonaRecord = serde_json::from_str( - r#"{ - "id": "persona-1", - "display_name": "Test", - "avatar_url": null, - "system_prompt": "Prompt", - "source_team": "com.example.my-team", - "source_team_persona_slug": "agent-one", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }"#, - ) - .unwrap(); - - let json = serde_json::to_string(&record).unwrap(); - assert!(json.contains("source_team")); - assert!(json.contains("source_team_persona_slug")); - assert!(!json.contains("source_pack")); - } - - #[test] - fn managed_agent_record_deserializes_old_pack_path_fields_via_alias() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "persona_pack_path": "/path/to/agents/packs/my-pack", - "persona_name_in_pack": "agent-one", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("old-format agent with persona_pack_path should deserialize via alias"); - - assert_eq!( - record.persona_team_dir, - Some(PathBuf::from("/path/to/agents/packs/my-pack")) - ); - assert_eq!(record.persona_name_in_team.as_deref(), Some("agent-one")); - } - - #[test] - fn team_record_deserializes_without_new_fields() { - let record: super::TeamRecord = serde_json::from_str( - r#"{ - "id": "team-1", - "name": "My Team", - "description": null, - "persona_ids": ["p1", "p2"], - "is_builtin": false, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }"#, - ) - .expect("team record without new fields should deserialize with defaults"); - - assert_eq!(record.source_dir, None); - assert!(!record.is_symlink); - assert_eq!(record.symlink_target, None); - assert_eq!(record.version, None); - } -} +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs new file mode 100644 index 0000000000..b7a725762c --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -0,0 +1,429 @@ +use super::{ManagedAgentRecord, PersonaRecord}; +use std::path::PathBuf; + +#[test] +fn persona_record_defaults_active_when_field_is_missing() { + let record: PersonaRecord = serde_json::from_str( + r#"{ + "id": "builtin:fizz", + "display_name": "Fizz", + "avatar_url": null, + "system_prompt": "Prompt", + "created_at": "2026-03-19T00:00:00Z", + "updated_at": "2026-03-19T00:00:00Z" + }"#, + ) + .expect("legacy persona payload should deserialize"); + + assert!(record.is_active); + assert!(!record.is_builtin); + assert_eq!(record.runtime, None); + assert_eq!(record.model, None); + assert!(record.name_pool.is_empty()); +} + +/// Legacy agent records (created before NIP-OA) lack the `auth_tag` field. +/// `#[serde(default)]` must ensure they deserialize with `auth_tag: None`. +#[test] +fn managed_agent_record_without_auth_tag_deserializes() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("legacy agent record without auth_tag should deserialize"); + + assert_eq!(record.auth_tag, None); + assert_eq!(record.avatar_url, None); + assert_eq!(record.pubkey, "abcd1234"); +} + +/// Agent records WITH an auth_tag round-trip correctly through serde. +#[test] +fn managed_agent_record_with_auth_tag_round_trips() { + let json = r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "auth_tag": "[\"auth\",\"deadbeef\",\"\",\"cafebabe\"]", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#; + + let record: ManagedAgentRecord = + serde_json::from_str(json).expect("record with auth_tag should deserialize"); + + assert_eq!( + record.auth_tag.as_deref(), + Some(r#"["auth","deadbeef","","cafebabe"]"#) + ); + + // Round-trip: serialize and deserialize again. + let serialized = serde_json::to_string(&record).expect("should serialize"); + let record2: ManagedAgentRecord = + serde_json::from_str(&serialized).expect("round-trip should deserialize"); + assert_eq!(record.auth_tag, record2.auth_tag); +} + +// ── Inbound author gate tests ──────────────────────────────────────── + +use super::{validate_respond_to_allowlist, RespondTo}; + +#[test] +fn respond_to_default_is_owner_only() { + assert_eq!(RespondTo::default(), RespondTo::OwnerOnly); +} + +#[test] +fn respond_to_serde_is_kebab_case() { + assert_eq!( + serde_json::to_string(&RespondTo::OwnerOnly).unwrap(), + "\"owner-only\"" + ); + assert_eq!( + serde_json::to_string(&RespondTo::Allowlist).unwrap(), + "\"allowlist\"" + ); + assert_eq!( + serde_json::to_string(&RespondTo::Anyone).unwrap(), + "\"anyone\"" + ); + let parsed: RespondTo = serde_json::from_str("\"owner-only\"").unwrap(); + assert_eq!(parsed, RespondTo::OwnerOnly); + let parsed: RespondTo = serde_json::from_str("\"allowlist\"").unwrap(); + assert_eq!(parsed, RespondTo::Allowlist); + let parsed: RespondTo = serde_json::from_str("\"anyone\"").unwrap(); + assert_eq!(parsed, RespondTo::Anyone); +} + +#[test] +fn respond_to_rejects_unknown_modes() { + // `nobody` is a valid harness mode but intentionally not exposed + // through the desktop request types. + assert!(serde_json::from_str::("\"nobody\"").is_err()); + assert!(serde_json::from_str::("\"OwnerOnly\"").is_err()); +} + +/// Records persisted before this feature must continue to load, +/// defaulting to OwnerOnly (the safe, matches-harness-default value). +#[test] +fn managed_agent_record_without_respond_to_fields_defaults_to_owner_only() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "legacy-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("legacy record without respond_to fields should deserialize"); + assert_eq!(record.respond_to, RespondTo::OwnerOnly); + assert!(record.respond_to_allowlist.is_empty()); +} + +#[test] +fn validate_respond_to_allowlist_accepts_valid_hex_and_lowercases() { + let upper = "A".repeat(64); + let lower = "a".repeat(64); + let result = validate_respond_to_allowlist(std::slice::from_ref(&upper)).unwrap(); + assert_eq!(result, vec![lower.clone()]); +} + +#[test] +fn validate_respond_to_allowlist_dedups_preserving_order() { + let a = "a".repeat(64); + let b = "b".repeat(64); + let a_upper = "A".repeat(64); + let input = vec![a.clone(), b.clone(), a_upper]; + let result = validate_respond_to_allowlist(&input).unwrap(); + assert_eq!(result, vec![a, b]); +} + +#[test] +fn validate_respond_to_allowlist_rejects_wrong_length() { + let too_short = "a".repeat(63); + assert!(validate_respond_to_allowlist(&[too_short]).is_err()); + let too_long = "a".repeat(65); + assert!(validate_respond_to_allowlist(&[too_long]).is_err()); +} + +#[test] +fn validate_respond_to_allowlist_rejects_non_hex() { + let bad = "z".repeat(64); + assert!(validate_respond_to_allowlist(&[bad]).is_err()); + // npub-style strings should not slip through. + let npub = format!("npub1{}", "a".repeat(59)); + assert!(validate_respond_to_allowlist(&[npub]).is_err()); +} + +#[test] +fn validate_respond_to_allowlist_trims_whitespace() { + let padded = format!(" {} ", "a".repeat(64)); + let result = validate_respond_to_allowlist(&[padded]).unwrap(); + assert_eq!(result, vec!["a".repeat(64)]); +} + +#[test] +fn validate_respond_to_allowlist_accepts_empty() { + // Empty is allowed at this layer; the boundary check + // (Allowlist mode requires ≥1 entry) is the caller's job. + let result = validate_respond_to_allowlist(&[]).unwrap(); + assert!(result.is_empty()); +} + +use super::{CreateManagedAgentRequest, RelayMeshConfig}; + +/// Wire-shape test: the create request arrives from TS as camelCase +/// (`relayMesh: { modelRef }`). `rename_all = "camelCase"` on +/// `CreateManagedAgentRequest` does NOT recurse into nested structs, so +/// `RelayMeshConfig` needs its own `alias = "modelRef"`. This test pins +/// the exact JSON the frontend sends; if the alias is dropped, creating +/// a relay-mesh agent fails to deserialize at the Tauri boundary. +#[test] +fn create_request_deserializes_camel_case_relay_mesh() { + let request: CreateManagedAgentRequest = serde_json::from_str( + r#"{ + "name": "mesh-agent", + "relayMesh": { "modelRef": "Qwen3" } + }"#, + ) + .expect("camelCase relayMesh payload from TS should deserialize"); + assert_eq!( + request.relay_mesh, + Some(RelayMeshConfig { + model_ref: "Qwen3".to_string() + }) + ); +} + +/// Persisted records use snake_case; the camelCase alias must not break +/// the stored-record round trip. +#[test] +fn relay_mesh_config_round_trips_snake_case() { + let config = RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }; + let json = serde_json::to_string(&config).unwrap(); + assert_eq!(json, r#"{"model_ref":"Qwen3"}"#); + let back: RelayMeshConfig = serde_json::from_str(&json).unwrap(); + assert_eq!(back, config); +} + +// ── Packs → Teams serde alias backward compatibility ──────────────── + +#[test] +fn persona_record_deserializes_old_source_pack_fields_via_alias() { + let record: PersonaRecord = serde_json::from_str( + r#"{ + "id": "persona-1", + "display_name": "Test", + "avatar_url": null, + "system_prompt": "Prompt", + "source_pack": "com.example.my-pack", + "source_pack_persona_slug": "agent-one", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#, + ) + .expect("old-format persona with source_pack should deserialize via alias"); + + assert_eq!(record.source_team.as_deref(), Some("com.example.my-pack")); + assert_eq!( + record.source_team_persona_slug.as_deref(), + Some("agent-one") + ); +} + +#[test] +fn persona_record_serializes_new_field_names() { + let record: PersonaRecord = serde_json::from_str( + r#"{ + "id": "persona-1", + "display_name": "Test", + "avatar_url": null, + "system_prompt": "Prompt", + "source_team": "com.example.my-team", + "source_team_persona_slug": "agent-one", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#, + ) + .unwrap(); + + let json = serde_json::to_string(&record).unwrap(); + assert!(json.contains("source_team")); + assert!(json.contains("source_team_persona_slug")); + assert!(!json.contains("source_pack")); +} + +#[test] +fn managed_agent_record_deserializes_old_pack_path_fields_via_alias() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "persona_pack_path": "/path/to/agents/packs/my-pack", + "persona_name_in_pack": "agent-one", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("old-format agent with persona_pack_path should deserialize via alias"); + + assert_eq!( + record.persona_team_dir, + Some(PathBuf::from("/path/to/agents/packs/my-pack")) + ); + assert_eq!(record.persona_name_in_team.as_deref(), Some("agent-one")); +} + +#[test] +fn team_record_deserializes_without_new_fields() { + let record: super::TeamRecord = serde_json::from_str( + r#"{ + "id": "team-1", + "name": "My Team", + "description": null, + "persona_ids": ["p1", "p2"], + "is_builtin": false, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#, + ) + .expect("team record without new fields should deserialize with defaults"); + + assert_eq!(record.source_dir, None); + assert!(!record.is_symlink); + assert_eq!(record.symlink_target, None); + assert_eq!(record.version, None); +} + +/// A record whose in-memory key was blanked (because it lives in the +/// keyring) must NOT serialize `private_key_nsec` into JSON. +#[test] +fn managed_agent_record_omits_empty_key_from_json() { + let mut record = sample_agent_record(); + record.private_key_nsec = String::new(); + + let json = serde_json::to_string(&record).expect("serialize"); + assert!( + !json.contains("private_key_nsec"), + "blanked key must be skipped from JSON, got: {json}" + ); +} + +/// A record with an inline key (the keyringless `0o600` JSON fallback) +/// serializes the key and round-trips it back. +#[test] +fn managed_agent_record_serializes_inline_key_for_fallback() { + let mut record = sample_agent_record(); + record.private_key_nsec = "nsec1fallback".to_string(); + + let json = serde_json::to_string(&record).expect("serialize"); + assert!(json.contains("nsec1fallback")); + + let back: ManagedAgentRecord = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back.private_key_nsec, "nsec1fallback"); +} + +/// A keyring-backed record on disk lacks `private_key_nsec`; it must +/// deserialize with an empty key (to be hydrated from the keyring). +#[test] +fn managed_agent_record_without_key_deserializes_empty() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("keyring-backed record without inline key should deserialize"); + + assert_eq!(record.private_key_nsec, ""); +} + +fn sample_agent_record() -> ManagedAgentRecord { + serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("sample record") +} diff --git a/desktop/src-tauri/src/migration.rs b/desktop/src-tauri/src/migration.rs index d44245190b..1da537ea9b 100644 --- a/desktop/src-tauri/src/migration.rs +++ b/desktop/src-tauri/src/migration.rs @@ -263,6 +263,11 @@ fn copy_file_over_generated_default(src: &Path, dst: &Path) -> std::io::Result<( /// Read a JSON array of objects from `path`, apply `f` to each object, /// and write back if any mutation returned `true`. +/// +/// Writes back via [`crate::managed_agents::atomic_write_json_restricted`] +/// (owner-only `0o600`): the store files this rewrites can carry plaintext +/// agent nsecs on a keyringless host, so the write must not reopen the umask +/// window SECURITY.md:90 closes. fn patch_json_records( path: &Path, mut f: impl FnMut(&mut serde_json::Map) -> bool, @@ -285,7 +290,7 @@ fn patch_json_records( } if changed { if let Ok(bytes) = serde_json::to_vec_pretty(&records) { - if let Err(e) = crate::managed_agents::atomic_write_json(path, &bytes) { + if let Err(e) = crate::managed_agents::atomic_write_json_restricted(path, &bytes) { eprintln!("buzz-desktop: patch-json-records: {e}"); } } diff --git a/desktop/src-tauri/src/migration_tests.rs b/desktop/src-tauri/src/migration_tests.rs index f60f394fd6..2f013d77a3 100644 --- a/desktop/src-tauri/src/migration_tests.rs +++ b/desktop/src-tauri/src/migration_tests.rs @@ -654,6 +654,36 @@ fn migrate_packs_to_teams_rewrites_agents_json() { assert!(records[0].get("persona_name_in_pack").is_none()); } +/// `patch_json_records` rewrites `managed-agents.json`, which carries plaintext +/// agent nsecs on a keyringless host — the writeback must land `0o600` from the +/// write itself (no post-write `chmod`), or a launch-time reconcile reopens the +/// umask window SECURITY.md:90 closes (Thufir, migration.rs:288). +#[cfg(unix)] +#[test] +fn patch_json_records_rewrites_secret_store_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + write_agents_json( + dir.path(), + &serde_json::json!([{ "private_key_nsec": "nsec1secret", "provider": "goose" }]), + ); + let path = dir.path().join("agents/managed-agents.json"); + + // Mutate so the write actually fires (it only writes back on `changed`). + patch_json_records(&path, |obj| { + let provider = obj.remove("provider").unwrap(); + obj.insert("runtime".to_string(), provider); + true + }); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "secret-bearing rewrite must be owner-only"); + let records = read_agents_json(dir.path()); + assert_eq!(records[0]["private_key_nsec"], "nsec1secret"); + assert_eq!(records[0]["runtime"], "goose"); +} + #[test] fn rename_provider_to_runtime_migrates_field() { let dir = tempfile::tempdir().unwrap(); diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs new file mode 100644 index 0000000000..b19be29553 --- /dev/null +++ b/desktop/src-tauri/src/secret_store.rs @@ -0,0 +1,169 @@ +//! 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. +//! +//! The store is deliberately NOT on any env-read path. `BUZZ_PRIVATE_KEY` +//! resolution for harnessed agents and CI is handled upstream (an env +//! short-circuit for the human key, child-process env injection for agents); +//! adding an env tier here would duplicate that precedence and create a +//! divergent-behavior trap. + +/// 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 +/// resurrect a rotated/stale key). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum KeyringProbe { + /// Keyring is reachable and an entry for the key already exists. + Present, + /// Keyring is reachable but has no entry for the key. + ReachableButEmpty, + /// Keyring backend is unavailable this boot (no Secret Service, dbus + /// failure, etc.). Migration must be skipped. + 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"). +pub struct SecretStore { + service: String, +} + +impl SecretStore { + /// Keyring-backed store under `service`. The active platform backend + /// (apple-native / windows-native / sync-secret-service) is chosen at + /// compile time. + pub fn keyring(service: impl Into) -> Self { + SecretStore { + service: service.into(), + } + } +} + +/// Whether a keyring error string indicates the backend itself is unavailable +/// (vs. a per-entry error like "not found"). Mirrors goose's discriminator +/// (`crates/goose/src/config/base.rs`): treat dbus / Secret Service / platform +/// secure-storage failures as "keyring unavailable, fall back to file". +#[cfg(feature = "system-keyring")] +fn is_keyring_availability_error(error_str: &str) -> bool { + let lower = error_str.to_lowercase(); + lower.contains("keyring") + || lower.contains("dbus") + || lower.contains("org.freedesktop.secrets") + || lower.contains("platform secure storage") + || lower.contains("no secret service") +} + +#[cfg(feature = "system-keyring")] +fn keyring_entry(service: &str, key: &str) -> Result { + keyring::Entry::new(service, key) +} + +impl SecretStore { + /// 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 + } + // A non-availability per-entry error (e.g. bad attributes) + // means the backend is reachable but the entry is unusable. + Err(_) => KeyringProbe::ReachableButEmpty, + }, + Err(e) if is_keyring_availability_error(&e.to_string()) => { + KeyringProbe::Unreachable + } + Err(_) => KeyringProbe::Unreachable, + } + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = key; + 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". + pub fn load(&self, key: &str) -> Result, String> { + #[cfg(feature = "system-keyring")] + { + 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}")), + } + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = key; + Err("system-keyring feature disabled".to_string()) + } + } + + /// 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> { + #[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}")) + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = (key, value); + Err("system-keyring feature disabled".to_string()) + } + } + + /// Delete the secret for `key`. A missing entry is not an error. + pub fn delete(&self, key: &str) -> Result<(), String> { + #[cfg(feature = "system-keyring")] + { + 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}")), + } + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = key; + Err("system-keyring feature disabled".to_string()) + } + } +} + +#[cfg(all(test, feature = "system-keyring"))] +mod tests { + use super::*; + + #[test] + fn availability_error_discriminator() { + assert!(is_keyring_availability_error("dbus connection failed")); + assert!(is_keyring_availability_error( + "org.freedesktop.secrets not provided" + )); + assert!(is_keyring_availability_error("No Secret Service")); + assert!(is_keyring_availability_error( + "Platform secure storage failure" + )); + // A plain "not found" is per-entry, not an availability failure. + assert!(!is_keyring_availability_error("entry not found")); + } +}