From 8c25ab98ba6e9ef777c5c536938b895205336712 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 06:28:48 +0200 Subject: [PATCH 1/8] fix(agent): store Windows secrets as encrypted files keyed from Credential Manager Windows Credential Manager limits a credential to 2560 bytes, and OAuth credentials with refresh tokens exceed it, so enrolling the Windows service failed (#107). Store each secret as an AES-256-GCM file in the daemon's private state directory and keep only its 32-byte random key in Credential Manager, as suggested in #108. - The key is created once per secret and reused, so replacing the file atomically is the only step that changes a stored secret. - The file is bound to its service and account (AAD), so a file moved to another entry's path does not open. - Values stored directly in Credential Manager by earlier versions are still read, and are replaced on the next write. Delete removes such a value first, so a delete that stops halfway never brings an old value back. - The envelope logic sits behind a small key-store trait and is tested on every platform with an in-memory store and injected failures; one Windows test round-trips a 10,000-character secret through Credential Manager. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + crates/agent/Cargo.toml | 1 + crates/agent/src/secret_store.rs | 555 +++++++++++++++++++++++++++++-- 3 files changed, 530 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d8ba3125..526b49ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -51,6 +51,7 @@ dependencies = [ "anyhow", "apple-native-keyring-store", "async-trait", + "aws-lc-rs", "axum", "base64 0.23.1", "bytes", diff --git a/crates/agent/Cargo.toml b/crates/agent/Cargo.toml index 695324a8..693e4440 100644 --- a/crates/agent/Cargo.toml +++ b/crates/agent/Cargo.toml @@ -27,6 +27,7 @@ agentdesktop-proto.workspace = true agentdesktop-usage.workspace = true anyhow.workspace = true axum.workspace = true +aws-lc-rs.workspace = true base64.workspace = true bytes.workspace = true clap.workspace = true diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 9eeb3bb8..3afad7f4 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -1,17 +1,22 @@ use std::path::Path; -#[cfg(target_os = "linux")] +#[cfg(any(target_os = "linux", windows))] use std::path::PathBuf; use anyhow::Context; #[cfg(target_os = "macos")] use apple_native_keyring_store::keychain::{Cred, MacKeychainDomain}; -#[cfg(target_os = "linux")] +#[cfg(any(target_os = "linux", windows))] use crate::secure_fs; /// Platform-native durable storage for daemon secrets. +/// +/// On Windows a secret is stored as an encrypted file in the daemon's private +/// state directory, and only its small random key goes into Credential +/// Manager, which limits a credential to 2560 bytes. OAuth credentials with +/// refresh tokens exceed that. pub struct SecretStore { - #[cfg(target_os = "linux")] + #[cfg(any(target_os = "linux", windows))] directory: PathBuf, #[cfg(target_os = "macos")] keychain: MacKeychainDomain, @@ -38,8 +43,9 @@ impl SecretStore { #[cfg(windows)] { - let _ = state_dir; - Ok(Self {}) + let directory = state_dir.join("secrets"); + secure_fs::ensure_private_dir(&directory)?; + Ok(Self { directory }) } } @@ -78,15 +84,12 @@ impl SecretStore { #[cfg(windows)] { - let entry = keyring::Entry::new(service, account) - .context("open operating system credential store")?; - match entry.get_password() { - Ok(secret) => Ok(Some(secret)), - Err(keyring::Error::NoEntry) => Ok(None), - Err(error) => { - Err(error).context("read secret from operating system credential store") - } - } + envelope::get( + &self.entry_path(service, account), + &CredentialManager, + service, + account, + ) } } @@ -108,10 +111,13 @@ impl SecretStore { #[cfg(windows)] { - keyring::Entry::new(service, account) - .context("open operating system credential store")? - .set_password(secret) - .context("store secret in operating system credential store") + envelope::set( + &self.entry_path(service, account), + &CredentialManager, + service, + account, + secret, + ) } } @@ -142,18 +148,16 @@ impl SecretStore { #[cfg(windows)] { - let entry = keyring::Entry::new(service, account) - .context("open operating system credential store")?; - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), - Err(error) => { - Err(error).context("delete secret from operating system credential store") - } - } + envelope::delete( + &self.entry_path(service, account), + &CredentialManager, + service, + account, + ) } } - #[cfg(target_os = "linux")] + #[cfg(any(target_os = "linux", windows))] fn entry_path(&self, service: &str, account: &str) -> PathBuf { use sha2::{Digest, Sha256}; @@ -170,6 +174,475 @@ impl SecretStore { } } +/// Windows Credential Manager entries, used for the per-secret file keys and +/// for reading secrets stored before the files existed. +#[cfg(windows)] +struct CredentialManager; + +#[cfg(windows)] +impl envelope::KeyBackend for CredentialManager { + fn get(&self, service: &str, account: &str) -> anyhow::Result> { + let entry = keyring::Entry::new(service, account) + .context("open operating system credential store")?; + match entry.get_password() { + Ok(value) => Ok(Some(value)), + Err(keyring::Error::NoEntry) => Ok(None), + Err(error) => Err(error).context("read from operating system credential store"), + } + } + + fn set(&self, service: &str, account: &str, value: &str) -> anyhow::Result<()> { + keyring::Entry::new(service, account) + .context("open operating system credential store")? + .set_password(value) + .context("store in operating system credential store") + } + + fn delete(&self, service: &str, account: &str) -> anyhow::Result<()> { + let entry = keyring::Entry::new(service, account) + .context("open operating system credential store")?; + match entry.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), + Err(error) => Err(error).context("delete from operating system credential store"), + } + } +} + +/// Secrets of any size kept as AES-256-GCM files whose keys live in a small +/// key store. Compiled on every platform for tests; used on Windows. +#[cfg(any(windows, test))] +mod envelope { + use std::{ + fs, + io::ErrorKind, + path::Path, + sync::{Mutex, PoisonError}, + }; + + use anyhow::{Context, bail}; + use aws_lc_rs::aead::{AES_256_GCM, Aad, NONCE_LEN, Nonce, RandomizedNonceKey}; + use base64::{Engine, engine::general_purpose::STANDARD}; + + use crate::secure_fs; + + const KEY_LEN: usize = 32; + const TAG_LEN: usize = 16; + const FORMAT_VERSION: u8 = 1; + /// Appended to the secret's service to name the entry that holds its key. + /// The secret's own service and account keep any value stored before the + /// encrypted files existed, so it can still be read until it is replaced. + const KEY_SERVICE_SUFFIX: &str = ".file-key"; + + /// Small values in the operating system credential store. Deleting an + /// entry that does not exist succeeds. + pub(super) trait KeyBackend { + fn get(&self, service: &str, account: &str) -> anyhow::Result>; + fn set(&self, service: &str, account: &str, value: &str) -> anyhow::Result<()>; + fn delete(&self, service: &str, account: &str) -> anyhow::Result<()>; + } + + /// Serializes writes so that two first writes of one secret cannot leave + /// one write's key next to the other's file. + static WRITES: Mutex<()> = Mutex::new(()); + + pub(super) fn get( + path: &Path, + keys: &impl KeyBackend, + service: &str, + account: &str, + ) -> anyhow::Result> { + let sealed = match fs::read(path) { + Ok(sealed) => sealed, + // A secret stored directly in the credential store by an earlier + // version, or none at all. + Err(error) if error.kind() == ErrorKind::NotFound => return keys.get(service, account), + Err(error) => { + return Err(error).with_context(|| format!("read secret from {}", path.display())); + } + }; + let key = read_key(keys, service, account)? + .context("the key of a stored secret is missing")? + .context("the key of a stored secret is malformed")?; + open(&key, service, account, sealed).map(Some) + } + + pub(super) fn set( + path: &Path, + keys: &impl KeyBackend, + service: &str, + account: &str, + secret: &str, + ) -> anyhow::Result<()> { + let _writes = WRITES.lock().unwrap_or_else(PoisonError::into_inner); + // The key is created once and reused, so replacing the file is the only + // step that changes the stored secret. A key that cannot be decoded + // cannot open the current file either, so it is replaced. + let key = match read_key(keys, service, account)? { + Some(Some(key)) => key, + Some(None) | None => { + let mut key = [0; KEY_LEN]; + aws_lc_rs::rand::fill(&mut key) + .map_err(|_| anyhow::anyhow!("generate secret file key"))?; + keys.set(&key_service(service), account, &STANDARD.encode(key)) + .context("store secret file key")?; + key + } + }; + secure_fs::atomic_write(path, &seal(&key, service, account, secret)?, 0o600) + .with_context(|| format!("store secret in {}", path.display()))?; + // The file now shadows a value an earlier version stored directly. + if let Err(error) = keys.delete(service, account) { + tracing::warn!(%error, service, "could not remove the replaced credential store entry"); + } + Ok(()) + } + + pub(super) fn delete( + path: &Path, + keys: &impl KeyBackend, + service: &str, + account: &str, + ) -> anyhow::Result<()> { + let _writes = WRITES.lock().unwrap_or_else(PoisonError::into_inner); + // An earlier version's value goes first: it would be read again once + // the file is gone, so a delete that stops halfway must not leave it. + keys.delete(service, account) + .context("delete secret from operating system credential store")?; + match fs::remove_file(path) { + Ok(()) => {} + Err(error) if error.kind() == ErrorKind::NotFound => {} + Err(error) => { + return Err(error).with_context(|| format!("remove secret {}", path.display())); + } + } + keys.delete(&key_service(service), account) + .context("delete secret file key") + } + + fn key_service(service: &str) -> String { + format!("{service}{KEY_SERVICE_SUFFIX}") + } + + /// `None` when there is no key, `Some(None)` when the stored key cannot be + /// decoded. + fn read_key( + keys: &impl KeyBackend, + service: &str, + account: &str, + ) -> anyhow::Result>> { + let encoded = keys + .get(&key_service(service), account) + .context("read secret file key")?; + Ok(encoded.map(|encoded| { + STANDARD + .decode(encoded) + .ok() + .and_then(|key| key.try_into().ok()) + })) + } + + /// Binds the file to its entry, so a file moved to another entry's path + /// does not open. + fn aad(service: &str, account: &str) -> Vec { + let mut aad = b"agentdesktop secret v1\0".to_vec(); + aad.extend_from_slice(service.as_bytes()); + aad.push(0); + aad.extend_from_slice(account.as_bytes()); + aad + } + + /// `version || nonce || ciphertext || tag` + fn seal( + key: &[u8; KEY_LEN], + service: &str, + account: &str, + secret: &str, + ) -> anyhow::Result> { + let key = RandomizedNonceKey::new(&AES_256_GCM, key) + .map_err(|_| anyhow::anyhow!("load secret file key"))?; + let mut ciphertext = secret.as_bytes().to_vec(); + let nonce = key + .seal_in_place_append_tag(Aad::from(aad(service, account)), &mut ciphertext) + .map_err(|_| anyhow::anyhow!("encrypt secret"))?; + let mut sealed = Vec::with_capacity(1 + NONCE_LEN + ciphertext.len()); + sealed.push(FORMAT_VERSION); + sealed.extend_from_slice(nonce.as_ref()); + sealed.extend_from_slice(&ciphertext); + Ok(sealed) + } + + fn open( + key: &[u8; KEY_LEN], + service: &str, + account: &str, + mut sealed: Vec, + ) -> anyhow::Result { + if sealed.len() < 1 + NONCE_LEN + TAG_LEN { + bail!("stored secret is truncated"); + } + if sealed[0] != FORMAT_VERSION { + bail!("stored secret has unknown format version {}", sealed[0]); + } + let nonce = Nonce::try_assume_unique_for_key(&sealed[1..1 + NONCE_LEN]) + .map_err(|_| anyhow::anyhow!("read stored secret nonce"))?; + let key = RandomizedNonceKey::new(&AES_256_GCM, key) + .map_err(|_| anyhow::anyhow!("load secret file key"))?; + let plaintext = key + .open_in_place( + nonce, + Aad::from(aad(service, account)), + &mut sealed[1 + NONCE_LEN..], + ) + .map_err(|_| anyhow::anyhow!("stored secret does not match its key"))?; + String::from_utf8(plaintext.to_vec()).context("stored secret is not UTF-8") + } + + #[cfg(test)] + mod tests { + use std::{ + cell::{Cell, RefCell}, + collections::BTreeMap, + fs, + path::PathBuf, + }; + + use super::{KeyBackend, delete, get, key_service, set}; + + const SERVICE: &str = "dev.agentdesktop.test"; + const ACCOUNT: &str = "device"; + + /// In-memory credential store with injectable failures. + #[derive(Default)] + struct FakeKeys { + entries: RefCell>, + fail_sets: Cell, + fail_deletes_of: RefCell>, + } + + impl FakeKeys { + fn value(&self, service: &str) -> Option { + self.entries + .borrow() + .get(&(service.to_owned(), ACCOUNT.to_owned())) + .cloned() + } + + fn insert(&self, service: &str, value: &str) { + self.entries + .borrow_mut() + .insert((service.to_owned(), ACCOUNT.to_owned()), value.to_owned()); + } + } + + impl KeyBackend for FakeKeys { + fn get(&self, service: &str, account: &str) -> anyhow::Result> { + Ok(self + .entries + .borrow() + .get(&(service.to_owned(), account.to_owned())) + .cloned()) + } + + fn set(&self, service: &str, account: &str, value: &str) -> anyhow::Result<()> { + if self.fail_sets.get() { + anyhow::bail!("injected set failure"); + } + // Windows Credential Manager limit. + assert!(value.len() <= 2560, "credential too large: {}", value.len()); + self.entries + .borrow_mut() + .insert((service.to_owned(), account.to_owned()), value.to_owned()); + Ok(()) + } + + fn delete(&self, service: &str, account: &str) -> anyhow::Result<()> { + if self.fail_deletes_of.borrow().as_deref() == Some(service) { + anyhow::bail!("injected delete failure"); + } + self.entries + .borrow_mut() + .remove(&(service.to_owned(), account.to_owned())); + Ok(()) + } + } + + fn entry(directory: &tempfile::TempDir, name: &str) -> PathBuf { + directory.path().join(name) + } + + #[test] + fn a_secret_larger_than_a_credential_round_trips() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + let secret = "x".repeat(20_000); + + set(&path, &keys, SERVICE, ACCOUNT, &secret).unwrap(); + assert_eq!(get(&path, &keys, SERVICE, ACCOUNT).unwrap(), Some(secret)); + assert!( + !fs::read(&path) + .unwrap() + .windows(16) + .any(|w| w == b"xxxxxxxxxxxxxxxx") + ); + assert_eq!(keys.entries.borrow().len(), 1); + + set(&path, &keys, SERVICE, ACCOUNT, "second").unwrap(); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("second") + ); + } + + #[test] + fn the_key_is_created_once_and_reused() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + + set(&path, &keys, SERVICE, ACCOUNT, "first").unwrap(); + let key = keys.value(&key_service(SERVICE)).unwrap(); + set(&path, &keys, SERVICE, ACCOUNT, "second").unwrap(); + assert_eq!(keys.value(&key_service(SERVICE)).unwrap(), key); + } + + #[test] + fn a_value_stored_by_an_earlier_version_is_read_and_replaced() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + keys.insert(SERVICE, "legacy"); + + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("legacy") + ); + set(&path, &keys, SERVICE, ACCOUNT, "new").unwrap(); + assert_eq!(keys.value(SERVICE), None); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("new") + ); + } + + #[test] + fn a_failed_key_write_keeps_the_current_value() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + keys.insert(SERVICE, "legacy"); + keys.fail_sets.set(true); + + assert!(set(&path, &keys, SERVICE, ACCOUNT, "new").is_err()); + assert!(!path.exists()); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("legacy") + ); + } + + #[test] + fn a_failed_file_write_keeps_the_current_value() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + set(&path, &keys, SERVICE, ACCOUNT, "current").unwrap(); + + // Writing into a directory that does not exist fails. + let missing = directory.path().join("missing").join("entry"); + assert!(set(&missing, &keys, SERVICE, ACCOUNT, "new").is_err()); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("current") + ); + } + + #[test] + fn a_failed_legacy_cleanup_still_stores_the_secret() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + keys.insert(SERVICE, "legacy"); + *keys.fail_deletes_of.borrow_mut() = Some(SERVICE.to_owned()); + + set(&path, &keys, SERVICE, ACCOUNT, "new").unwrap(); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("new") + ); + } + + #[test] + fn a_file_does_not_open_under_another_entry() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let other = entry(&directory, "other"); + let keys = FakeKeys::default(); + set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); + set(&other, &keys, "dev.agentdesktop.other", ACCOUNT, "other").unwrap(); + + fs::copy(&path, &other).unwrap(); + assert!(get(&other, &keys, "dev.agentdesktop.other", ACCOUNT).is_err()); + } + + #[test] + fn a_tampered_file_or_missing_key_is_an_error_that_the_next_write_repairs() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); + + let mut sealed = fs::read(&path).unwrap(); + *sealed.last_mut().unwrap() ^= 1; + fs::write(&path, sealed).unwrap(); + assert!(get(&path, &keys, SERVICE, ACCOUNT).is_err()); + set(&path, &keys, SERVICE, ACCOUNT, "repaired").unwrap(); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("repaired") + ); + + keys.entries.borrow_mut().clear(); + assert!(get(&path, &keys, SERVICE, ACCOUNT).is_err()); + set(&path, &keys, SERVICE, ACCOUNT, "rekeyed").unwrap(); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("rekeyed") + ); + + keys.insert(&key_service(SERVICE), "not base64!"); + assert!(get(&path, &keys, SERVICE, ACCOUNT).is_err()); + set(&path, &keys, SERVICE, ACCOUNT, "fixed").unwrap(); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("fixed") + ); + } + + #[test] + fn delete_removes_everything_and_a_partial_delete_never_resurrects_an_old_value() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + keys.insert(SERVICE, "legacy"); + *keys.fail_deletes_of.borrow_mut() = Some(SERVICE.to_owned()); + set(&path, &keys, SERVICE, ACCOUNT, "new").unwrap(); + *keys.fail_deletes_of.borrow_mut() = None; + + // Stops after the file: the secret reads as gone, a retry finishes. + *keys.fail_deletes_of.borrow_mut() = Some(key_service(SERVICE)); + assert!(delete(&path, &keys, SERVICE, ACCOUNT).is_err()); + assert_eq!(get(&path, &keys, SERVICE, ACCOUNT).unwrap(), None); + *keys.fail_deletes_of.borrow_mut() = None; + delete(&path, &keys, SERVICE, ACCOUNT).unwrap(); + + assert!(!path.exists()); + assert!(keys.entries.borrow().is_empty()); + delete(&path, &keys, SERVICE, ACCOUNT).unwrap(); + } + } +} + #[cfg(target_os = "macos")] fn macos_keychain_domain(effective_uid: u32) -> MacKeychainDomain { if effective_uid == 0 { @@ -192,6 +665,34 @@ mod macos_tests { } } +#[cfg(all(test, windows))] +mod windows_tests { + use super::SecretStore; + + #[test] + fn a_secret_larger_than_a_credential_round_trips_through_credential_manager() { + let directory = tempfile::tempdir().unwrap(); + let store = SecretStore::new(directory.path()).unwrap(); + let account = format!("test-{}", rand::random::()); + let secret = "x".repeat(10_000); + + store + .set("dev.agentdesktop.test", &account, &secret) + .unwrap(); + assert_eq!( + store.get("dev.agentdesktop.test", &account).unwrap(), + secret + ); + store.delete("dev.agentdesktop.test", &account).unwrap(); + assert_eq!( + store + .get_optional("dev.agentdesktop.test", &account) + .unwrap(), + None + ); + } +} + #[cfg(all(test, target_os = "linux"))] mod tests { use std::{fs, os::unix::fs::PermissionsExt}; From e62ce2680b76442143ed6b2820cf6f88d7bae3e6 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 06:43:15 +0200 Subject: [PATCH 2/8] fix: address review on Windows secret envelope - A secret file whose key is missing reads as absent (with a warning), so the GitHub, Anthropic and gateway OIDC flows sign in again instead of failing on every request. A malformed key or tampered file stays an error. - Only the first write removes an earlier version's Credential Manager value, so later writes skip that call. - The test credential store checks the limit in UTF-16 bytes, as keyring stores it. - aws-lc-rs is a Windows and dev dependency only. - One SecretStore::new branch for Linux and Windows; the key lookup returns an enum instead of a nested Option. Co-Authored-By: Claude Opus 5.5 --- crates/agent/Cargo.toml | 4 +- crates/agent/src/secret_store.rs | 96 ++++++++++++++++++++------------ 2 files changed, 64 insertions(+), 36 deletions(-) diff --git a/crates/agent/Cargo.toml b/crates/agent/Cargo.toml index 693e4440..b32cfa21 100644 --- a/crates/agent/Cargo.toml +++ b/crates/agent/Cargo.toml @@ -27,7 +27,6 @@ agentdesktop-proto.workspace = true agentdesktop-usage.workspace = true anyhow.workspace = true axum.workspace = true -aws-lc-rs.workspace = true base64.workspace = true bytes.workspace = true clap.workspace = true @@ -59,6 +58,8 @@ toml.workspace = true url.workspace = true [dev-dependencies] +# The Windows secret envelope (secret_store.rs) is tested on every platform. +aws-lc-rs.workspace = true futures-util = "0.3" # Hand-rolled RFC 6455 handshake for the vscode-copilot-capi WebSocket # tunnel tests (llm_proxy.rs): Sec-WebSocket-Accept is @@ -79,6 +80,7 @@ keyring-core.workspace = true plist = "1.10" [target.'cfg(windows)'.dependencies] +aws-lc-rs.workspace = true keyring.workspace = true windows-service = "0.8.1" windows-sys = { version = "0.61", features = [ diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 3afad7f4..47dfa35e 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -24,7 +24,7 @@ pub struct SecretStore { impl SecretStore { pub fn new(state_dir: &Path) -> anyhow::Result { - #[cfg(target_os = "linux")] + #[cfg(any(target_os = "linux", windows))] { let directory = state_dir.join("secrets"); secure_fs::ensure_private_dir(&directory)?; @@ -40,13 +40,6 @@ impl SecretStore { keychain: macos_keychain_domain(effective_uid), }) } - - #[cfg(windows)] - { - let directory = state_dir.join("secrets"); - secure_fs::ensure_private_dir(&directory)?; - Ok(Self { directory }) - } } pub fn get(&self, service: &str, account: &str) -> anyhow::Result { @@ -260,10 +253,22 @@ mod envelope { return Err(error).with_context(|| format!("read secret from {}", path.display())); } }; - let key = read_key(keys, service, account)? - .context("the key of a stored secret is missing")? - .context("the key of a stored secret is malformed")?; - open(&key, service, account, sealed).map(Some) + match read_key(keys, service, account)? { + StoredKey::Key(key) => open(&key, service, account, sealed).map(Some), + // Nothing can open the file any more (the credential store was + // reset, or the state directory restored without it), so the secret + // is gone. Reporting it as absent lets callers sign in again; the + // next write replaces the file. + StoredKey::Missing => { + tracing::warn!( + service, + path = %path.display(), + "the key of a stored secret is missing; treating the secret as absent" + ); + Ok(None) + } + StoredKey::Malformed => bail!("the key of a stored secret is malformed"), + } } pub(super) fn set( @@ -278,8 +283,8 @@ mod envelope { // step that changes the stored secret. A key that cannot be decoded // cannot open the current file either, so it is replaced. let key = match read_key(keys, service, account)? { - Some(Some(key)) => key, - Some(None) | None => { + StoredKey::Key(key) => key, + StoredKey::Missing | StoredKey::Malformed => { let mut key = [0; KEY_LEN]; aws_lc_rs::rand::fill(&mut key) .map_err(|_| anyhow::anyhow!("generate secret file key"))?; @@ -288,10 +293,13 @@ mod envelope { key } }; + // Only the first write can replace a value an earlier version stored + // directly; later writes skip the credential store lookup. + let first_write = !path.try_exists().unwrap_or(true); secure_fs::atomic_write(path, &seal(&key, service, account, secret)?, 0o600) .with_context(|| format!("store secret in {}", path.display()))?; - // The file now shadows a value an earlier version stored directly. - if let Err(error) = keys.delete(service, account) { + // The file now shadows that value; remove it so no stale copy is left. + if first_write && let Err(error) = keys.delete(service, account) { tracing::warn!(%error, service, "could not remove the replaced credential store entry"); } Ok(()) @@ -323,22 +331,24 @@ mod envelope { format!("{service}{KEY_SERVICE_SUFFIX}") } - /// `None` when there is no key, `Some(None)` when the stored key cannot be - /// decoded. - fn read_key( - keys: &impl KeyBackend, - service: &str, - account: &str, - ) -> anyhow::Result>> { - let encoded = keys + enum StoredKey { + Missing, + Malformed, + Key([u8; KEY_LEN]), + } + + fn read_key(keys: &impl KeyBackend, service: &str, account: &str) -> anyhow::Result { + let Some(encoded) = keys .get(&key_service(service), account) - .context("read secret file key")?; - Ok(encoded.map(|encoded| { - STANDARD - .decode(encoded) - .ok() - .and_then(|key| key.try_into().ok()) - })) + .context("read secret file key")? + else { + return Ok(StoredKey::Missing); + }; + Ok(STANDARD + .decode(encoded) + .ok() + .and_then(|key| key.try_into().ok()) + .map_or(StoredKey::Malformed, StoredKey::Key)) } /// Binds the file to its entry, so a file moved to another entry's path @@ -417,6 +427,7 @@ mod envelope { entries: RefCell>, fail_sets: Cell, fail_deletes_of: RefCell>, + deletes: Cell, } impl FakeKeys { @@ -447,8 +458,9 @@ mod envelope { if self.fail_sets.get() { anyhow::bail!("injected set failure"); } - // Windows Credential Manager limit. - assert!(value.len() <= 2560, "credential too large: {}", value.len()); + // Windows Credential Manager limit; keyring stores UTF-16. + let size = value.encode_utf16().count() * 2; + assert!(size <= 2560, "credential too large: {size} bytes"); self.entries .borrow_mut() .insert((service.to_owned(), account.to_owned()), value.to_owned()); @@ -456,6 +468,7 @@ mod envelope { } fn delete(&self, service: &str, account: &str) -> anyhow::Result<()> { + self.deletes.set(self.deletes.get() + 1); if self.fail_deletes_of.borrow().as_deref() == Some(service) { anyhow::bail!("injected delete failure"); } @@ -525,6 +538,19 @@ mod envelope { ); } + #[test] + fn only_the_first_write_removes_an_earlier_versions_value() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + keys.insert(SERVICE, "legacy"); + + set(&path, &keys, SERVICE, ACCOUNT, "first").unwrap(); + assert_eq!(keys.deletes.get(), 1); + set(&path, &keys, SERVICE, ACCOUNT, "second").unwrap(); + assert_eq!(keys.deletes.get(), 1); + } + #[test] fn a_failed_key_write_keeps_the_current_value() { let directory = tempfile::tempdir().unwrap(); @@ -586,7 +612,7 @@ mod envelope { } #[test] - fn a_tampered_file_or_missing_key_is_an_error_that_the_next_write_repairs() { + fn a_tampered_file_or_malformed_key_is_an_error_and_a_missing_key_reads_as_absent() { let directory = tempfile::tempdir().unwrap(); let path = entry(&directory, "entry"); let keys = FakeKeys::default(); @@ -603,7 +629,7 @@ mod envelope { ); keys.entries.borrow_mut().clear(); - assert!(get(&path, &keys, SERVICE, ACCOUNT).is_err()); + assert_eq!(get(&path, &keys, SERVICE, ACCOUNT).unwrap(), None); set(&path, &keys, SERVICE, ACCOUNT, "rekeyed").unwrap(); assert_eq!( get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), From a0fe00f4ddfd84f968866192d54e2f0a1e017bb7 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 06:49:09 +0200 Subject: [PATCH 3/8] fix: address Copilot review on Windows secret envelope - A read that misses the file and then finds no earlier-version value looks at the file again: a concurrent first write stores the file before it removes that value, so the read no longer reports a secret that existed throughout as absent. Regression test for that interleaving. - The entry-binding test gives both entries the same key, so only the AAD binding can reject the copied file. Co-Authored-By: Claude Opus 5.5 --- crates/agent/src/secret_store.rs | 65 +++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 47dfa35e..5084ad75 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -244,14 +244,20 @@ mod envelope { service: &str, account: &str, ) -> anyhow::Result> { - let sealed = match fs::read(path) { - Ok(sealed) => sealed, + let sealed = match read_sealed(path)? { + Some(sealed) => sealed, // A secret stored directly in the credential store by an earlier // version, or none at all. - Err(error) if error.kind() == ErrorKind::NotFound => return keys.get(service, account), - Err(error) => { - return Err(error).with_context(|| format!("read secret from {}", path.display())); - } + None => match keys.get(service, account)? { + Some(value) => return Ok(Some(value)), + // A first write may have stored the file and removed that value + // since the file was looked up. It writes the file first, so the + // file is there now if that happened. + None => match read_sealed(path)? { + Some(sealed) => sealed, + None => return Ok(None), + }, + }, }; match read_key(keys, service, account)? { StoredKey::Key(key) => open(&key, service, account, sealed).map(Some), @@ -327,6 +333,16 @@ mod envelope { .context("delete secret file key") } + fn read_sealed(path: &Path) -> anyhow::Result>> { + match fs::read(path) { + Ok(sealed) => Ok(Some(sealed)), + Err(error) if error.kind() == ErrorKind::NotFound => Ok(None), + Err(error) => { + Err(error).with_context(|| format!("read secret from {}", path.display())) + } + } + } + fn key_service(service: &str) -> String { format!("{service}{KEY_SERVICE_SUFFIX}") } @@ -428,6 +444,9 @@ mod envelope { fail_sets: Cell, fail_deletes_of: RefCell>, deletes: Cell, + /// Runs a concurrent first write (installs the file, removes the + /// earlier version's value) when that value is looked up. + first_write_during_lookup: RefCell>, } impl FakeKeys { @@ -447,6 +466,14 @@ mod envelope { impl KeyBackend for FakeKeys { fn get(&self, service: &str, account: &str) -> anyhow::Result> { + if service == SERVICE + && let Some((staged, path)) = self.first_write_during_lookup.take() + { + fs::rename(staged, path).unwrap(); + self.entries + .borrow_mut() + .remove(&(service.to_owned(), account.to_owned())); + } Ok(self .entries .borrow() @@ -538,6 +565,25 @@ mod envelope { ); } + #[test] + fn a_read_racing_the_first_write_still_finds_the_secret() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let staged = entry(&directory, "staged"); + let keys = FakeKeys::default(); + set(&path, &keys, SERVICE, ACCOUNT, "migrated").unwrap(); + fs::rename(&path, &staged).unwrap(); + keys.insert(SERVICE, "legacy"); + + // The read misses the file, then the write lands before the + // fallback lookup. + *keys.first_write_during_lookup.borrow_mut() = Some((staged, path.clone())); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("migrated") + ); + } + #[test] fn only_the_first_write_removes_an_earlier_versions_value() { let directory = tempfile::tempdir().unwrap(); @@ -605,7 +651,14 @@ mod envelope { let other = entry(&directory, "other"); let keys = FakeKeys::default(); set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); + // Same key for both entries, so only the binding can reject the copy. + let key = keys.value(&key_service(SERVICE)).unwrap(); + keys.insert(&key_service("dev.agentdesktop.other"), &key); set(&other, &keys, "dev.agentdesktop.other", ACCOUNT, "other").unwrap(); + assert_eq!( + keys.value(&key_service("dev.agentdesktop.other")).unwrap(), + key + ); fs::copy(&path, &other).unwrap(); assert!(get(&other, &keys, "dev.agentdesktop.other", ACCOUNT).is_err()); From d1efec1d1506bb7d986869edb76571c319ccb9f0 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 09:38:11 +0200 Subject: [PATCH 4/8] fix: serialize Windows secret writes across processes The in-process mutex did not cover a second daemon instance, which runs secret-using tasks before it finds the first one on the named pipe. Two competing first writes could then leave one write's file next to the other's key. Writes and deletes now hold an exclusive lock on `secrets/.lock` (std File::lock, LockFileEx on Windows), which serializes threads and processes alike. Regression test with competing first writes, each on its own lock handle; the failed-file-write test now fails at the file replace rather than at a missing directory. Co-Authored-By: Claude Opus 5.5 --- crates/agent/src/secret_store.rs | 92 +++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 14 deletions(-) diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 5084ad75..54cc8535 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -205,12 +205,7 @@ impl envelope::KeyBackend for CredentialManager { /// key store. Compiled on every platform for tests; used on Windows. #[cfg(any(windows, test))] mod envelope { - use std::{ - fs, - io::ErrorKind, - path::Path, - sync::{Mutex, PoisonError}, - }; + use std::{fs, io::ErrorKind, path::Path}; use anyhow::{Context, bail}; use aws_lc_rs::aead::{AES_256_GCM, Aad, NONCE_LEN, Nonce, RandomizedNonceKey}; @@ -234,9 +229,25 @@ mod envelope { fn delete(&self, service: &str, account: &str) -> anyhow::Result<()>; } - /// Serializes writes so that two first writes of one secret cannot leave - /// one write's key next to the other's file. - static WRITES: Mutex<()> = Mutex::new(()); + /// Serializes writes, across threads and processes (a second daemon + /// instance runs secret-using tasks before it finds the first one), so + /// two first writes of one secret cannot leave one write's key next to + /// the other's file. Released when the returned file is dropped. + fn lock_writes(path: &Path) -> anyhow::Result { + let lock_path = path + .parent() + .context("secret path has no parent")? + .join(".lock"); + let lock = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&lock_path) + .with_context(|| format!("open secret lock {}", lock_path.display()))?; + lock.lock() + .with_context(|| format!("lock {}", lock_path.display()))?; + Ok(lock) + } pub(super) fn get( path: &Path, @@ -284,7 +295,7 @@ mod envelope { account: &str, secret: &str, ) -> anyhow::Result<()> { - let _writes = WRITES.lock().unwrap_or_else(PoisonError::into_inner); + let _writes = lock_writes(path)?; // The key is created once and reused, so replacing the file is the only // step that changes the stored secret. A key that cannot be decoded // cannot open the current file either, so it is replaced. @@ -317,7 +328,7 @@ mod envelope { service: &str, account: &str, ) -> anyhow::Result<()> { - let _writes = WRITES.lock().unwrap_or_else(PoisonError::into_inner); + let _writes = lock_writes(path)?; // An earlier version's value goes first: it would be read again once // the file is gone, so a delete that stops halfway must not leave it. keys.delete(service, account) @@ -584,6 +595,57 @@ mod envelope { ); } + /// Thread-safe store whose key writes are slow, so unserialized first + /// writes would interleave. + #[derive(Default)] + struct SlowKeys(std::sync::Mutex>); + + impl KeyBackend for SlowKeys { + fn get(&self, service: &str, account: &str) -> anyhow::Result> { + Ok(self + .0 + .lock() + .unwrap() + .get(&(service.to_owned(), account.to_owned())) + .cloned()) + } + + fn set(&self, service: &str, account: &str, value: &str) -> anyhow::Result<()> { + std::thread::sleep(std::time::Duration::from_millis(20)); + self.0 + .lock() + .unwrap() + .insert((service.to_owned(), account.to_owned()), value.to_owned()); + Ok(()) + } + + fn delete(&self, service: &str, account: &str) -> anyhow::Result<()> { + self.0 + .lock() + .unwrap() + .remove(&(service.to_owned(), account.to_owned())); + Ok(()) + } + } + + #[test] + fn competing_first_writes_leave_a_readable_secret() { + // Each write opens its own lock handle, as a second process would. + for _ in 0..10 { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = SlowKeys::default(); + std::thread::scope(|scope| { + for writer in ["one", "two", "three"] { + let (path, keys) = (&path, &keys); + scope.spawn(move || set(path, keys, SERVICE, ACCOUNT, writer).unwrap()); + } + }); + let stored = get(&path, &keys, SERVICE, ACCOUNT).unwrap().unwrap(); + assert!(["one", "two", "three"].contains(&stored.as_str())); + } + } + #[test] fn only_the_first_write_removes_an_earlier_versions_value() { let directory = tempfile::tempdir().unwrap(); @@ -620,9 +682,11 @@ mod envelope { let keys = FakeKeys::default(); set(&path, &keys, SERVICE, ACCOUNT, "current").unwrap(); - // Writing into a directory that does not exist fails. - let missing = directory.path().join("missing").join("entry"); - assert!(set(&missing, &keys, SERVICE, ACCOUNT, "new").is_err()); + // Replacing a non-empty directory with the new file fails. + let blocked = entry(&directory, "blocked"); + fs::create_dir(&blocked).unwrap(); + fs::write(blocked.join("occupied"), "").unwrap(); + assert!(set(&blocked, &keys, SERVICE, ACCOUNT, "new").is_err()); assert_eq!( get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), Some("current") From b24e6045336713b1e593f5c666d14018c98d7de1 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 09:52:22 +0200 Subject: [PATCH 5/8] fix: scope Windows file keys to their store and never pair a new key with an old file - Credential Manager is shared by everything running as one Windows account, while the files and the lock are per state directory. Each secrets directory now has a random identity (`secrets/.store-id`), and file keys live at `.file-key` / `/`, so two state directories no longer overwrite or delete each other's keys. - When a write has to create a new key (missing or malformed), it removes the old file first: nothing can open it, and a later failure then leaves a secret that reads as absent instead of a file next to the wrong key. - Regression tests for both, with two directories on one key store and a check that no file exists when a new key is stored. Co-Authored-By: Claude Opus 5.5 --- crates/agent/src/secret_store.rs | 191 ++++++++++++++++++++++++++----- 1 file changed, 165 insertions(+), 26 deletions(-) diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 54cc8535..33fe90c4 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -216,9 +216,10 @@ mod envelope { const KEY_LEN: usize = 32; const TAG_LEN: usize = 16; const FORMAT_VERSION: u8 = 1; - /// Appended to the secret's service to name the entry that holds its key. - /// The secret's own service and account keep any value stored before the - /// encrypted files existed, so it can still be read until it is replaced. + /// Appended to the secret's service to name the entry that holds its key + /// (its account is `/`). The secret's own service and + /// account keep any value stored before the encrypted files existed, so it + /// can still be read until it is replaced. const KEY_SERVICE_SUFFIX: &str = ".file-key"; /// Small values in the operating system credential store. Deleting an @@ -270,7 +271,11 @@ mod envelope { }, }, }; - match read_key(keys, service, account)? { + let stored_key = match store_id(path)? { + Some(id) => read_key(keys, &key_entry(&id, service, account))?, + None => StoredKey::Missing, + }; + match stored_key { StoredKey::Key(key) => open(&key, service, account, sealed).map(Some), // Nothing can open the file any more (the credential store was // reset, or the state directory restored without it), so the secret @@ -296,16 +301,21 @@ mod envelope { secret: &str, ) -> anyhow::Result<()> { let _writes = lock_writes(path)?; + let entry = key_entry(&create_store_id(path)?, service, account); // The key is created once and reused, so replacing the file is the only // step that changes the stored secret. A key that cannot be decoded // cannot open the current file either, so it is replaced. - let key = match read_key(keys, service, account)? { + let key = match read_key(keys, &entry)? { StoredKey::Key(key) => key, StoredKey::Missing | StoredKey::Malformed => { + // Nothing can open a file left from before, so remove it first: + // if a later step fails, the secret reads as absent rather than + // as a file next to the wrong key. + remove_file(path)?; let mut key = [0; KEY_LEN]; aws_lc_rs::rand::fill(&mut key) .map_err(|_| anyhow::anyhow!("generate secret file key"))?; - keys.set(&key_service(service), account, &STANDARD.encode(key)) + keys.set(&entry.service, &entry.account, &STANDARD.encode(key)) .context("store secret file key")?; key } @@ -333,15 +343,64 @@ mod envelope { // the file is gone, so a delete that stops halfway must not leave it. keys.delete(service, account) .context("delete secret from operating system credential store")?; + remove_file(path)?; + let Some(id) = store_id(path)? else { + return Ok(()); + }; + let entry = key_entry(&id, service, account); + keys.delete(&entry.service, &entry.account) + .context("delete secret file key") + } + + fn remove_file(path: &Path) -> anyhow::Result<()> { match fs::remove_file(path) { - Ok(()) => {} - Err(error) if error.kind() == ErrorKind::NotFound => {} - Err(error) => { - return Err(error).with_context(|| format!("remove secret {}", path.display())); - } + Ok(()) => Ok(()), + Err(error) if error.kind() == ErrorKind::NotFound => Ok(()), + Err(error) => Err(error).with_context(|| format!("remove secret {}", path.display())), } - keys.delete(&key_service(service), account) - .context("delete secret file key") + } + + /// Identifies the secrets directory. Credential Manager entries are shared + /// by everything running as one Windows account, the files are per state + /// directory, so the key entries are scoped to this identity. + fn store_id(path: &Path) -> anyhow::Result> { + let id_path = store_id_path(path)?; + match fs::read_to_string(&id_path) { + Ok(id) if is_store_id(&id) => Ok(Some(id)), + Ok(_) => Ok(None), + Err(error) if error.kind() == ErrorKind::NotFound => Ok(None), + Err(error) => Err(error).with_context(|| format!("read {}", id_path.display())), + } + } + + /// Called with the write lock held. A malformed identity is replaced; the + /// files it keyed then read as absent and are rewritten on the next write. + fn create_store_id(path: &Path) -> anyhow::Result { + if let Some(id) = store_id(path)? { + return Ok(id); + } + let mut bytes = [0; 16]; + aws_lc_rs::rand::fill(&mut bytes) + .map_err(|_| anyhow::anyhow!("generate secret store identity"))?; + let id = bytes + .iter() + .map(|byte| format!("{byte:02x}")) + .collect::(); + let id_path = store_id_path(path)?; + secure_fs::atomic_write(&id_path, id.as_bytes(), 0o600) + .with_context(|| format!("store {}", id_path.display()))?; + Ok(id) + } + + fn store_id_path(path: &Path) -> anyhow::Result { + Ok(path + .parent() + .context("secret path has no parent")? + .join(".store-id")) + } + + fn is_store_id(id: &str) -> bool { + id.len() == 32 && id.bytes().all(|byte| byte.is_ascii_hexdigit()) } fn read_sealed(path: &Path) -> anyhow::Result>> { @@ -354,6 +413,19 @@ mod envelope { } } + /// The credential store entry that holds one secret's file key. + struct KeyEntry { + service: String, + account: String, + } + + fn key_entry(store_id: &str, service: &str, account: &str) -> KeyEntry { + KeyEntry { + service: key_service(service), + account: format!("{store_id}/{account}"), + } + } + fn key_service(service: &str) -> String { format!("{service}{KEY_SERVICE_SUFFIX}") } @@ -364,9 +436,9 @@ mod envelope { Key([u8; KEY_LEN]), } - fn read_key(keys: &impl KeyBackend, service: &str, account: &str) -> anyhow::Result { + fn read_key(keys: &impl KeyBackend, entry: &KeyEntry) -> anyhow::Result { let Some(encoded) = keys - .get(&key_service(service), account) + .get(&entry.service, &entry.account) .context("read secret file key")? else { return Ok(StoredKey::Missing); @@ -440,10 +512,12 @@ mod envelope { cell::{Cell, RefCell}, collections::BTreeMap, fs, - path::PathBuf, + path::{Path, PathBuf}, }; - use super::{KeyBackend, delete, get, key_service, set}; + use super::{ + KEY_SERVICE_SUFFIX, KeyBackend, delete, get, key_entry, key_service, set, store_id, + }; const SERVICE: &str = "dev.agentdesktop.test"; const ACCOUNT: &str = "device"; @@ -458,9 +532,27 @@ mod envelope { /// Runs a concurrent first write (installs the file, removes the /// earlier version's value) when that value is looked up. first_write_during_lookup: RefCell>, + /// Records whether this file exists when a new key is stored. + file_at_key_write: RefCell>, } impl FakeKeys { + /// The file key of the secret at `path`. + fn key_of(&self, path: &Path, service: &str) -> Option { + let entry = key_entry(&store_id(path).unwrap().unwrap(), service, ACCOUNT); + self.entries + .borrow() + .get(&(entry.service, entry.account)) + .cloned() + } + + fn put_key(&self, path: &Path, service: &str, value: &str) { + let entry = key_entry(&store_id(path).unwrap().unwrap(), service, ACCOUNT); + self.entries + .borrow_mut() + .insert((entry.service, entry.account), value.to_owned()); + } + fn value(&self, service: &str) -> Option { self.entries .borrow() @@ -496,6 +588,11 @@ mod envelope { if self.fail_sets.get() { anyhow::bail!("injected set failure"); } + if service.ends_with(KEY_SERVICE_SUFFIX) + && let Some((path, seen)) = self.file_at_key_write.borrow_mut().as_mut() + { + *seen = path.exists(); + } // Windows Credential Manager limit; keyring stores UTF-16. let size = value.encode_utf16().count() * 2; assert!(size <= 2560, "credential too large: {size} bytes"); @@ -552,9 +649,9 @@ mod envelope { let keys = FakeKeys::default(); set(&path, &keys, SERVICE, ACCOUNT, "first").unwrap(); - let key = keys.value(&key_service(SERVICE)).unwrap(); + let key = keys.key_of(&path, SERVICE).unwrap(); set(&path, &keys, SERVICE, ACCOUNT, "second").unwrap(); - assert_eq!(keys.value(&key_service(SERVICE)).unwrap(), key); + assert_eq!(keys.key_of(&path, SERVICE).unwrap(), key); } #[test] @@ -646,6 +743,51 @@ mod envelope { } } + #[test] + fn a_new_key_is_never_stored_next_to_an_old_file() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); + + // The key is lost; the file survives. Had the new key been stored + // first, a failed file write would leave the old file next to it, + // and reads would fail instead of reporting the secret absent. + keys.entries.borrow_mut().clear(); + *keys.file_at_key_write.borrow_mut() = Some((path.clone(), true)); + set(&path, &keys, SERVICE, ACCOUNT, "new").unwrap(); + assert!(!keys.file_at_key_write.borrow().as_ref().unwrap().1); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("new") + ); + } + + #[test] + fn stores_in_two_directories_keep_their_own_keys() { + let (first, second) = (tempfile::tempdir().unwrap(), tempfile::tempdir().unwrap()); + let (first, second) = (entry(&first, "entry"), entry(&second, "entry")); + let keys = FakeKeys::default(); + + set(&first, &keys, SERVICE, ACCOUNT, "first").unwrap(); + set(&second, &keys, SERVICE, ACCOUNT, "second").unwrap(); + assert_eq!( + get(&first, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("first") + ); + assert_eq!( + get(&second, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("second") + ); + + delete(&first, &keys, SERVICE, ACCOUNT).unwrap(); + assert_eq!(get(&first, &keys, SERVICE, ACCOUNT).unwrap(), None); + assert_eq!( + get(&second, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("second") + ); + } + #[test] fn only_the_first_write_removes_an_earlier_versions_value() { let directory = tempfile::tempdir().unwrap(); @@ -716,13 +858,10 @@ mod envelope { let keys = FakeKeys::default(); set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); // Same key for both entries, so only the binding can reject the copy. - let key = keys.value(&key_service(SERVICE)).unwrap(); - keys.insert(&key_service("dev.agentdesktop.other"), &key); + let key = keys.key_of(&path, SERVICE).unwrap(); + keys.put_key(&other, "dev.agentdesktop.other", &key); set(&other, &keys, "dev.agentdesktop.other", ACCOUNT, "other").unwrap(); - assert_eq!( - keys.value(&key_service("dev.agentdesktop.other")).unwrap(), - key - ); + assert_eq!(keys.key_of(&other, "dev.agentdesktop.other").unwrap(), key); fs::copy(&path, &other).unwrap(); assert!(get(&other, &keys, "dev.agentdesktop.other", ACCOUNT).is_err()); @@ -753,7 +892,7 @@ mod envelope { Some("rekeyed") ); - keys.insert(&key_service(SERVICE), "not base64!"); + keys.put_key(&path, SERVICE, "not base64!"); assert!(get(&path, &keys, SERVICE, ACCOUNT).is_err()); set(&path, &keys, SERVICE, ACCOUNT, "fixed").unwrap(); assert_eq!( From 4d5cfb5719f0c74b242d7d3ee5d53dd9ceb84050 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 09:59:23 +0200 Subject: [PATCH 6/8] fix: treat a non-UTF-8 store identity as malformed `read_to_string` failed with InvalidData on such a file, which blocked every write until the file was removed by hand. Read the bytes and treat anything that is not a valid identity as malformed, so the next write replaces it. Other filesystem errors still propagate. Co-Authored-By: Claude Opus 5.5 --- crates/agent/src/secret_store.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 33fe90c4..34d4b6f6 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -365,9 +365,9 @@ mod envelope { /// directory, so the key entries are scoped to this identity. fn store_id(path: &Path) -> anyhow::Result> { let id_path = store_id_path(path)?; - match fs::read_to_string(&id_path) { - Ok(id) if is_store_id(&id) => Ok(Some(id)), - Ok(_) => Ok(None), + match fs::read(&id_path) { + // Anything else, including bytes that are not UTF-8, is malformed. + Ok(id) => Ok(String::from_utf8(id).ok().filter(|id| is_store_id(id))), Err(error) if error.kind() == ErrorKind::NotFound => Ok(None), Err(error) => Err(error).with_context(|| format!("read {}", id_path.display())), } @@ -788,6 +788,24 @@ mod envelope { ); } + #[test] + fn a_malformed_store_identity_is_replaced_by_the_next_write() { + let directory = tempfile::tempdir().unwrap(); + let path = entry(&directory, "entry"); + let keys = FakeKeys::default(); + set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); + + for malformed in [&b"\xff\xfe not utf-8"[..], b"too short"] { + fs::write(directory.path().join(".store-id"), malformed).unwrap(); + assert_eq!(get(&path, &keys, SERVICE, ACCOUNT).unwrap(), None); + set(&path, &keys, SERVICE, ACCOUNT, "rewritten").unwrap(); + assert_eq!( + get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), + Some("rewritten") + ); + } + } + #[test] fn only_the_first_write_removes_an_earlier_versions_value() { let directory = tempfile::tempdir().unwrap(); From c64cc7e1d68526522a794a304ffdce6a1808f591 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 10:13:23 +0200 Subject: [PATCH 7/8] fix: take the store lock shared for reads A read could load a file, then a delete and a new write could replace its key before the read looked the key up; the read then reported the old file as corrupt, which identity::load turns into IdentityUnreadable. Reads now hold the store lock shared (writes and deletes hold it exclusively), so reads still run in parallel but never span a replace. This also covers the first-write race, so the re-read of the file in get() is gone. Test: a writer on its own handle cannot take the lock while a read looks up the key. Co-Authored-By: Claude Opus 5.5 --- crates/agent/src/secret_store.rs | 88 ++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 34d4b6f6..08aed0bc 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -230,11 +230,13 @@ mod envelope { fn delete(&self, service: &str, account: &str) -> anyhow::Result<()>; } - /// Serializes writes, across threads and processes (a second daemon - /// instance runs secret-using tasks before it finds the first one), so - /// two first writes of one secret cannot leave one write's key next to - /// the other's file. Released when the returned file is dropped. - fn lock_writes(path: &Path) -> anyhow::Result { + /// Locks the store across threads and processes (a second daemon instance + /// runs secret-using tasks before it finds the first one). Writes and + /// deletes take it exclusively, so two first writes cannot leave one + /// write's key next to the other's file; reads take it shared, so a read + /// never pairs a file with a key that a delete and a new write replaced + /// in between. Released when the returned file is dropped. + fn lock_store(path: &Path, exclusive: bool) -> anyhow::Result { let lock_path = path .parent() .context("secret path has no parent")? @@ -245,8 +247,12 @@ mod envelope { .truncate(false) .open(&lock_path) .with_context(|| format!("open secret lock {}", lock_path.display()))?; - lock.lock() - .with_context(|| format!("lock {}", lock_path.display()))?; + if exclusive { + lock.lock() + } else { + lock.lock_shared() + } + .with_context(|| format!("lock {}", lock_path.display()))?; Ok(lock) } @@ -256,20 +262,11 @@ mod envelope { service: &str, account: &str, ) -> anyhow::Result> { - let sealed = match read_sealed(path)? { - Some(sealed) => sealed, + let _reading = lock_store(path, false)?; + let Some(sealed) = read_sealed(path)? else { // A secret stored directly in the credential store by an earlier // version, or none at all. - None => match keys.get(service, account)? { - Some(value) => return Ok(Some(value)), - // A first write may have stored the file and removed that value - // since the file was looked up. It writes the file first, so the - // file is there now if that happened. - None => match read_sealed(path)? { - Some(sealed) => sealed, - None => return Ok(None), - }, - }, + return keys.get(service, account); }; let stored_key = match store_id(path)? { Some(id) => read_key(keys, &key_entry(&id, service, account))?, @@ -300,7 +297,7 @@ mod envelope { account: &str, secret: &str, ) -> anyhow::Result<()> { - let _writes = lock_writes(path)?; + let _writes = lock_store(path, true)?; let entry = key_entry(&create_store_id(path)?, service, account); // The key is created once and reused, so replacing the file is the only // step that changes the stored secret. A key that cannot be decoded @@ -338,7 +335,7 @@ mod envelope { service: &str, account: &str, ) -> anyhow::Result<()> { - let _writes = lock_writes(path)?; + let _writes = lock_store(path, true)?; // An earlier version's value goes first: it would be read again once // the file is gone, so a delete that stops halfway must not leave it. keys.delete(service, account) @@ -529,9 +526,9 @@ mod envelope { fail_sets: Cell, fail_deletes_of: RefCell>, deletes: Cell, - /// Runs a concurrent first write (installs the file, removes the - /// earlier version's value) when that value is looked up. - first_write_during_lookup: RefCell>, + /// When a file key is looked up, records whether a writer could take + /// the lock in this directory right then. + writer_could_lock_during_key_read: RefCell)>>, /// Records whether this file exists when a new key is stored. file_at_key_write: RefCell>, } @@ -569,13 +566,20 @@ mod envelope { impl KeyBackend for FakeKeys { fn get(&self, service: &str, account: &str) -> anyhow::Result> { - if service == SERVICE - && let Some((staged, path)) = self.first_write_during_lookup.take() + if service.ends_with(KEY_SERVICE_SUFFIX) + && let Some((directory, could_lock)) = + self.writer_could_lock_during_key_read.borrow_mut().as_mut() { - fs::rename(staged, path).unwrap(); - self.entries - .borrow_mut() - .remove(&(service.to_owned(), account.to_owned())); + // A separate handle, as another process's writer would use. + let lock = fs::OpenOptions::new() + .write(true) + .open(directory.join(".lock")) + .unwrap(); + *could_lock = Some(match lock.try_lock() { + Ok(()) => true, + Err(fs::TryLockError::WouldBlock) => false, + Err(fs::TryLockError::Error(error)) => panic!("{error}"), + }); } Ok(self .entries @@ -674,21 +678,27 @@ mod envelope { } #[test] - fn a_read_racing_the_first_write_still_finds_the_secret() { + fn a_read_holds_off_writers_until_it_has_the_key() { + // Otherwise a delete and a new write between reading the file and + // reading its key would pair the old file with the new key. let directory = tempfile::tempdir().unwrap(); let path = entry(&directory, "entry"); - let staged = entry(&directory, "staged"); let keys = FakeKeys::default(); - set(&path, &keys, SERVICE, ACCOUNT, "migrated").unwrap(); - fs::rename(&path, &staged).unwrap(); - keys.insert(SERVICE, "legacy"); + set(&path, &keys, SERVICE, ACCOUNT, "secret").unwrap(); - // The read misses the file, then the write lands before the - // fallback lookup. - *keys.first_write_during_lookup.borrow_mut() = Some((staged, path.clone())); + *keys.writer_could_lock_during_key_read.borrow_mut() = + Some((directory.path().to_owned(), None)); assert_eq!( get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), - Some("migrated") + Some("secret") + ); + assert_eq!( + keys.writer_could_lock_during_key_read + .borrow() + .as_ref() + .unwrap() + .1, + Some(false) ); } From 5a949aeaf5a41dc39b920bc6c616911c1ec0b6a3 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Thu, 15 Oct 2026 11:11:46 +0200 Subject: [PATCH 8/8] test: fail the file write of the secret being updated The failed-write test failed a write to another path, so it would pass even if a failed update destroyed its own file. A test-only switch now fails the next file write; the test updates the stored secret itself and checks that the file and value are unchanged (it fails if set() removes the file before writing). Co-Authored-By: Claude Opus 5.5 --- crates/agent/src/secret_store.rs | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/crates/agent/src/secret_store.rs b/crates/agent/src/secret_store.rs index 08aed0bc..7d59e3a9 100644 --- a/crates/agent/src/secret_store.rs +++ b/crates/agent/src/secret_store.rs @@ -320,8 +320,7 @@ mod envelope { // Only the first write can replace a value an earlier version stored // directly; later writes skip the credential store lookup. let first_write = !path.try_exists().unwrap_or(true); - secure_fs::atomic_write(path, &seal(&key, service, account, secret)?, 0o600) - .with_context(|| format!("store secret in {}", path.display()))?; + write_sealed(path, &seal(&key, service, account, secret)?)?; // The file now shadows that value; remove it so no stale copy is left. if first_write && let Err(error) = keys.delete(service, account) { tracing::warn!(%error, service, "could not remove the replaced credential store entry"); @@ -349,6 +348,21 @@ mod envelope { .context("delete secret file key") } + #[cfg(test)] + thread_local! { + /// Makes the next secret file write on this thread fail. + static FAIL_NEXT_FILE_WRITE: std::cell::Cell = const { std::cell::Cell::new(false) }; + } + + fn write_sealed(path: &Path, sealed: &[u8]) -> anyhow::Result<()> { + #[cfg(test)] + if FAIL_NEXT_FILE_WRITE.with(|fail| fail.replace(false)) { + bail!("injected file write failure"); + } + secure_fs::atomic_write(path, sealed, 0o600) + .with_context(|| format!("store secret in {}", path.display())) + } + fn remove_file(path: &Path) -> anyhow::Result<()> { match fs::remove_file(path) { Ok(()) => Ok(()), @@ -513,7 +527,8 @@ mod envelope { }; use super::{ - KEY_SERVICE_SUFFIX, KeyBackend, delete, get, key_entry, key_service, set, store_id, + FAIL_NEXT_FILE_WRITE, KEY_SERVICE_SUFFIX, KeyBackend, delete, get, key_entry, + key_service, set, store_id, }; const SERVICE: &str = "dev.agentdesktop.test"; @@ -851,12 +866,11 @@ mod envelope { let path = entry(&directory, "entry"); let keys = FakeKeys::default(); set(&path, &keys, SERVICE, ACCOUNT, "current").unwrap(); + let before = fs::read(&path).unwrap(); - // Replacing a non-empty directory with the new file fails. - let blocked = entry(&directory, "blocked"); - fs::create_dir(&blocked).unwrap(); - fs::write(blocked.join("occupied"), "").unwrap(); - assert!(set(&blocked, &keys, SERVICE, ACCOUNT, "new").is_err()); + FAIL_NEXT_FILE_WRITE.with(|fail| fail.set(true)); + assert!(set(&path, &keys, SERVICE, ACCOUNT, "new").is_err()); + assert_eq!(fs::read(&path).unwrap(), before); assert_eq!( get(&path, &keys, SERVICE, ACCOUNT).unwrap().as_deref(), Some("current")