diff --git a/codex-rs/login/src/auth/storage_policy_tests.rs b/codex-rs/login/src/auth/storage_policy_tests.rs index 9375b59be1..17e4ab035a 100644 --- a/codex-rs/login/src/auth/storage_policy_tests.rs +++ b/codex-rs/login/src/auth/storage_policy_tests.rs @@ -292,6 +292,7 @@ fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()> let encrypted = auth_with_prefix("encrypted"); seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &encrypted)?; + assert_eq!(storage.load()?, Some(encrypted.clone())); mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); let expected = auth_with_prefix("fallback"); diff --git a/codex-rs/secrets/src/lib.rs b/codex-rs/secrets/src/lib.rs index de7e07e3cc..d934101900 100644 --- a/codex-rs/secrets/src/lib.rs +++ b/codex-rs/secrets/src/lib.rs @@ -210,8 +210,10 @@ pub(crate) fn keyring_service() -> &'static str { #[cfg(test)] mod tests { use super::*; + use crate::local::SECRETS_CACHE_TEST_LOCK; use codex_keyring_store::tests::MockKeyringStore; use pretty_assertions::assert_eq; + use std::sync::PoisonError; #[test] fn environment_id_fallback_has_cwd_prefix() { @@ -233,6 +235,9 @@ mod tests { #[test] fn manager_round_trips_local_backend() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK + .lock() + .unwrap_or_else(PoisonError::into_inner); let codex_home = tempfile::tempdir().expect("tempdir"); let keyring = Arc::new(MockKeyringStore::default()); let manager = SecretsManager::new_with_keyring_store( diff --git a/codex-rs/secrets/src/local.rs b/codex-rs/secrets/src/local.rs index 7aebad1547..801b20b261 100644 --- a/codex-rs/secrets/src/local.rs +++ b/codex-rs/secrets/src/local.rs @@ -43,6 +43,13 @@ const CODEX_AUTH_SECRETS_FILENAME: &str = "codex_auth.age"; const MCP_OAUTH_SECRETS_FILENAME: &str = "mcp_oauth.age"; const GATEWAY_OAUTH_SECRETS_FILENAME: &str = "gateway_oauth.age"; static MCP_OAUTH_CACHE: Mutex> = Mutex::new(None); +static CODEX_AUTH_CACHE: Mutex> = Mutex::new(None); +static MANAGED_SECRETS_CACHE: Mutex> = Mutex::new(None); + +// Tests that access the shared caches must hold this lock so another test's +// temporary home cannot evict a snapshot while cache reuse is being asserted. +#[cfg(test)] +pub(super) static SECRETS_CACHE_TEST_LOCK: Mutex<()> = Mutex::new(()); /// Selects the local encrypted file used by a `LocalSecretsBackend`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -176,13 +183,14 @@ impl LocalSecretsBackend { self.secrets_dir().join(filename) } - /// Gateway readers share one bounded cache per backend; independent managers - /// still reread the ciphertext and keyring before reusing decrypted credentials. - fn decrypted_cache(&self) -> Option<&Mutex>> { + /// Each namespace keeps at most one snapshot; gateway snapshots are scoped to + /// a backend and its clones. Every read still checks the file and keyring. + fn decrypted_cache(&self) -> &Mutex> { match self.namespace { - LocalSecretsNamespace::McpOAuth => Some(&MCP_OAUTH_CACHE), - LocalSecretsNamespace::GatewayOAuth => Some(&self.gateway_cache), - LocalSecretsNamespace::ManagedSecrets | LocalSecretsNamespace::CodexAuth => None, + LocalSecretsNamespace::ManagedSecrets => &MANAGED_SECRETS_CACHE, + LocalSecretsNamespace::CodexAuth => &CODEX_AUTH_CACHE, + LocalSecretsNamespace::McpOAuth => &MCP_OAUTH_CACHE, + LocalSecretsNamespace::GatewayOAuth => &self.gateway_cache, } } @@ -195,18 +203,19 @@ impl LocalSecretsBackend { let ciphertext = fs::read(&path) .with_context(|| format!("failed to read secrets file at {}", path.display()))?; let passphrase = self.load_or_create_passphrase()?; - let cache = self.decrypted_cache().map(|cache| { - let ciphertext_hash: [u8; 32] = Sha256::digest(&ciphertext).into(); - let passphrase_hash: [u8; 32] = - Sha256::digest(passphrase.expose_secret().as_bytes()).into(); - let cache = cache.lock().unwrap_or_else(PoisonError::into_inner); - (cache, ciphertext_hash, passphrase_hash) - }); - if let Some((cache, ciphertext_hash, passphrase_hash)) = cache.as_ref() - && let Some(cached) = cache.as_ref() + let ciphertext_hash: [u8; 32] = Sha256::digest(&ciphertext).into(); + let passphrase_hash: [u8; 32] = + Sha256::digest(passphrase.expose_secret().as_bytes()).into(); + // Hold the namespace lock through decryption so concurrent misses share + // one decrypted snapshot instead of repeating the expensive scrypt work. + let mut cache = self + .decrypted_cache() + .lock() + .unwrap_or_else(PoisonError::into_inner); + if let Some(cached) = cache.as_ref() && cached.path == path - && cached.ciphertext_hash == *ciphertext_hash - && cached.passphrase_hash == *passphrase_hash + && cached.ciphertext_hash == ciphertext_hash + && cached.passphrase_hash == passphrase_hash { return Ok(cached.file.as_ref().clone()); } @@ -226,14 +235,12 @@ impl LocalSecretsBackend { parsed.version, SECRETS_VERSION ); - if let Some((mut cache, ciphertext_hash, passphrase_hash)) = cache { - *cache = Some(CachedSecrets { - path, - ciphertext_hash, - passphrase_hash, - file: Arc::new(parsed.clone()), - }); - } + *cache = Some(CachedSecrets { + path, + ciphertext_hash, + passphrase_hash, + file: Arc::new(parsed.clone()), + }); Ok(parsed) } @@ -247,11 +254,12 @@ impl LocalSecretsBackend { let ciphertext = encrypt_with_passphrase(&plaintext, &passphrase)?; let path = self.secrets_path(); write_file_atomically(&path, &ciphertext)?; - if let Some(cache) = self.decrypted_cache() { - let mut cache = cache.lock().unwrap_or_else(PoisonError::into_inner); - if cache.as_ref().is_some_and(|cached| cached.path == path) { - *cache = None; - } + let mut cache = self + .decrypted_cache() + .lock() + .unwrap_or_else(PoisonError::into_inner); + if cache.as_ref().is_some_and(|cached| cached.path == path) { + *cache = None; } Ok(()) } @@ -443,10 +451,11 @@ mod tests { use keyring::Error as KeyringError; use pretty_assertions::assert_eq; - static MCP_OAUTH_CACHE_TEST_LOCK: Mutex<()> = Mutex::new(()); - #[test] fn load_file_rejects_newer_schema_versions() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK + .lock() + .unwrap_or_else(PoisonError::into_inner); let codex_home = tempfile::tempdir().expect("tempdir"); let keyring = Arc::new(MockKeyringStore::default()); let backend = LocalSecretsBackend::new(codex_home.path().to_path_buf(), keyring); @@ -469,6 +478,9 @@ mod tests { #[test] fn set_fails_when_keyring_is_unavailable() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK + .lock() + .unwrap_or_else(PoisonError::into_inner); let codex_home = tempfile::tempdir().expect("tempdir"); let keyring = Arc::new(MockKeyringStore::default()); let account = @@ -495,6 +507,9 @@ mod tests { #[test] fn save_file_does_not_leave_temp_files() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK + .lock() + .unwrap_or_else(PoisonError::into_inner); let codex_home = tempfile::tempdir().expect("tempdir"); let keyring = Arc::new(MockKeyringStore::default()); let backend = LocalSecretsBackend::new(codex_home.path().to_path_buf(), keyring); @@ -528,7 +543,7 @@ mod tests { #[test] fn local_namespaces_write_separate_files() -> Result<()> { - let _cache_lock = MCP_OAUTH_CACHE_TEST_LOCK + let _cache_lock = SECRETS_CACHE_TEST_LOCK .lock() .unwrap_or_else(PoisonError::into_inner); let codex_home = tempfile::tempdir().expect("tempdir"); @@ -606,76 +621,93 @@ mod tests { } #[test] - fn mcp_oauth_cache_reuses_plaintext_and_invalidates_when_ciphertext_changes() -> Result<()> { - let _cache_lock = MCP_OAUTH_CACHE_TEST_LOCK + fn shared_namespace_caches_reuse_plaintext_and_detect_ciphertext_changes() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK .lock() .unwrap_or_else(PoisonError::into_inner); - let codex_home = tempfile::tempdir().expect("tempdir"); - let keyring = Arc::new(MockKeyringStore::default()); - let first = LocalSecretsBackend::new_with_namespace( - codex_home.path().to_path_buf(), - keyring.clone(), + for namespace in [ + LocalSecretsNamespace::ManagedSecrets, + LocalSecretsNamespace::CodexAuth, LocalSecretsNamespace::McpOAuth, - ); - let second = LocalSecretsBackend::new_with_namespace( - codex_home.path().to_path_buf(), - keyring, - LocalSecretsNamespace::McpOAuth, - ); - let scope = SecretScope::Global; - let name = SecretName::new("TEST_SECRET")?; - let cached_file = || { - Arc::clone( - &MCP_OAUTH_CACHE + ] { + let codex_home = tempfile::tempdir().expect("tempdir"); + let keyring = Arc::new(MockKeyringStore::default()); + let first = LocalSecretsBackend::new_with_namespace( + codex_home.path().to_path_buf(), + keyring.clone(), + namespace, + ); + let second = LocalSecretsBackend::new_with_namespace( + codex_home.path().to_path_buf(), + keyring, + namespace, + ); + let scope = SecretScope::Global; + let name = SecretName::new("TEST_SECRET")?; + let cached_file = || { + Arc::clone( + &first + .decrypted_cache() + .lock() + .unwrap_or_else(PoisonError::into_inner) + .as_ref() + .expect("credentials should be cached") + .file, + ) + }; + + first.set(&scope, &name, "one")?; + let original_ciphertext = fs::read(first.secrets_path())?; + let (first_cached, second_cached) = std::thread::scope(|threads| { + let first_reader = threads.spawn(|| { + assert_eq!(first.get(&scope, &name)?, Some("one".to_string())); + Ok::<_, anyhow::Error>(cached_file()) + }); + let second_reader = threads.spawn(|| { + assert_eq!(second.get(&scope, &name)?, Some("one".to_string())); + Ok::<_, anyhow::Error>(cached_file()) + }); + Ok::<_, anyhow::Error>(( + first_reader.join().expect("first credential reader")?, + second_reader.join().expect("second credential reader")?, + )) + })?; + assert!(Arc::ptr_eq(&first_cached, &second_cached)); + + assert_eq!(second.get(&scope, &name)?, Some("one".to_string())); + assert!(Arc::ptr_eq(&first_cached, &cached_file())); + + first.set(&scope, &name, "two")?; + assert!( + first + .decrypted_cache() .lock() .unwrap_or_else(PoisonError::into_inner) .as_ref() - .expect("MCP OAuth credentials should be cached") - .file, - ) - }; + .is_none_or(|cached| cached.path != first.secrets_path()) + ); + assert_eq!(second.get(&scope, &name)?, Some("two".to_string())); + assert!(!Arc::ptr_eq(&first_cached, &cached_file())); - first.set(&scope, &name, "one")?; - let (first_cached, second_cached) = std::thread::scope(|threads| { - let first_reader = threads.spawn(|| { - assert_eq!(first.get(&scope, &name)?, Some("one".to_string())); - Ok::<_, anyhow::Error>(cached_file()) - }); - let second_reader = threads.spawn(|| { - assert_eq!(second.get(&scope, &name)?, Some("one".to_string())); - Ok::<_, anyhow::Error>(cached_file()) - }); - Ok::<_, anyhow::Error>(( - first_reader.join().expect("first credential reader")?, - second_reader.join().expect("second credential reader")?, - )) - })?; - assert!(Arc::ptr_eq(&first_cached, &second_cached)); - - assert_eq!(second.get(&scope, &name)?, Some("one".to_string())); - assert!(Arc::ptr_eq(&first_cached, &cached_file())); - - first.set(&scope, &name, "two")?; - assert!( - MCP_OAUTH_CACHE - .lock() - .unwrap_or_else(PoisonError::into_inner) - .as_ref() - .is_none_or(|cached| cached.path != first.secrets_path()) - ); - assert_eq!(second.get(&scope, &name)?, Some("two".to_string())); - assert!(!Arc::ptr_eq(&first_cached, &cached_file())); - - assert!(first.delete(&scope, &name)?); - assert!( - MCP_OAUTH_CACHE - .lock() - .unwrap_or_else(PoisonError::into_inner) - .as_ref() - .is_none_or(|cached| cached.path != first.secrets_path()) - ); - assert_eq!(second.get(&scope, &name)?, None); + // Another process can replace the file without invalidating this cache. + fs::write(first.secrets_path(), &original_ciphertext)?; + assert_eq!(second.get(&scope, &name)?, Some("one".to_string())); + assert!(first.delete(&scope, &name)?); + assert!( + first + .decrypted_cache() + .lock() + .unwrap_or_else(PoisonError::into_inner) + .as_ref() + .is_none_or(|cached| cached.path != first.secrets_path()) + ); + assert_eq!(second.get(&scope, &name)?, None); + } Ok(()) } } + +#[cfg(test)] +#[path = "local_cache_tests.rs"] +mod cache_tests; diff --git a/codex-rs/secrets/src/local_cache_tests.rs b/codex-rs/secrets/src/local_cache_tests.rs new file mode 100644 index 0000000000..c653e8f219 --- /dev/null +++ b/codex-rs/secrets/src/local_cache_tests.rs @@ -0,0 +1,101 @@ +//! Cached reads must observe current filesystem and keyring authority. + +use super::*; +use codex_keyring_store::tests::MockKeyringStore; +use keyring::Error as KeyringError; +use pretty_assertions::assert_eq; + +#[test] +fn cached_reads_check_file_presence_key_rotation_and_keyring_errors() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK + .lock() + .unwrap_or_else(PoisonError::into_inner); + for namespace in [ + LocalSecretsNamespace::ManagedSecrets, + LocalSecretsNamespace::CodexAuth, + ] { + let home = tempfile::tempdir()?; + let keyring = Arc::new(MockKeyringStore::default()); + let backend = LocalSecretsBackend::new_with_namespace( + home.path().to_path_buf(), + keyring.clone(), + namespace, + ); + let scope = SecretScope::Global; + let name = SecretName::new("TEST_SECRET")?; + backend.set(&scope, &name, "original")?; + assert_eq!(backend.get(&scope, &name)?, Some("original".to_string())); + let ciphertext = fs::read(backend.secrets_path())?; + let account = compute_keyring_account(home.path(), namespace); + let original_key = keyring + .load(keyring_service(), &account)? + .expect("persisted encryption key"); + + // A cached snapshot cannot make a removed file appear to exist. + fs::remove_file(backend.secrets_path())?; + assert_eq!(backend.get(&scope, &name)?, None); + assert_eq!(backend.list(/*scope_filter*/ None)?, Vec::new()); + fs::write(backend.secrets_path(), &ciphertext)?; + assert_eq!(backend.get(&scope, &name)?, Some("original".to_string())); + + // Neither a changed encryption key nor a keyring error may reuse plaintext. + let replacement_key = generate_passphrase()?; + keyring.save(keyring_service(), &account, replacement_key.expose_secret())?; + assert!(backend.get(&scope, &name).is_err()); + keyring.save(keyring_service(), &account, &original_key)?; + assert_eq!(backend.get(&scope, &name)?, Some("original".to_string())); + keyring.set_error( + &account, + KeyringError::PlatformFailure(Box::new(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "keyring is locked", + ))), + ); + assert!(backend.get(&scope, &name).is_err()); + + fs::write(backend.secrets_path(), b"invalid ciphertext")?; + assert!(backend.get(&scope, &name).is_err()); + fs::write(backend.secrets_path(), &ciphertext)?; + assert_eq!(backend.get(&scope, &name)?, Some("original".to_string())); + } + Ok(()) +} + +#[test] +fn shared_caches_keep_homes_and_namespaces_isolated() -> Result<()> { + let _cache_lock = SECRETS_CACHE_TEST_LOCK + .lock() + .unwrap_or_else(PoisonError::into_inner); + let homes = [tempfile::tempdir()?, tempfile::tempdir()?]; + let keyring = Arc::new(MockKeyringStore::default()); + let scope = SecretScope::Global; + let name = SecretName::new("TEST_SECRET")?; + let mut backends = Vec::new(); + for (index, home) in homes.iter().enumerate() { + for namespace in [ + LocalSecretsNamespace::ManagedSecrets, + LocalSecretsNamespace::CodexAuth, + LocalSecretsNamespace::McpOAuth, + ] { + let backend = LocalSecretsBackend::new_with_namespace( + home.path().to_path_buf(), + keyring.clone(), + namespace, + ); + let value = format!("{index}-{namespace:?}"); + backend.set(&scope, &name, &value)?; + backends.push((backend, value)); + } + } + for (backend, value) in backends.iter().rev().chain(backends.iter()) { + assert_eq!(backend.get(&scope, &name)?, Some(value.clone())); + assert_eq!( + backend.list(Some(&scope))?, + vec![SecretListEntry { + scope: scope.clone(), + name: name.clone(), + }] + ); + } + Ok(()) +}