From b4ad33d1a3c10c9652dd57f7afd7a949f8f26895 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Thu, 22 Oct 2026 04:51:37 +0000 Subject: [PATCH] Cache decrypted Codex auth and managed secrets (#56081) ## Why Repeated reads of Codex auth and managed secrets repeat expensive scrypt decryption even when the encrypted file and keyring passphrase are unchanged. ## What changed Extend the decrypted snapshot cache to `CodexAuth` and `ManagedSecrets`, keeping at most one snapshot per namespace. Reuse snapshots only when the file path, ciphertext, and passphrase match, and invalidate matching snapshots on writes. Every read still checks the file and keyring before reusing plaintext. ## Testing Extend cache tests to cover concurrent reuse, writes, external file replacement, file removal, key rotation, keyring errors, invalid ciphertext, and isolation across homes and namespaces. Prime the auth cache before verifying fallback when the keyring fails. GitOrigin-RevId: eaf802ef4cfdbe482b56423807a1fe0fe3def2cd --- .../login/src/auth/storage_policy_tests.rs | 1 + codex-rs/secrets/src/lib.rs | 5 + codex-rs/secrets/src/local.rs | 222 ++++++++++-------- codex-rs/secrets/src/local_cache_tests.rs | 101 ++++++++ 4 files changed, 234 insertions(+), 95 deletions(-) create mode 100644 codex-rs/secrets/src/local_cache_tests.rs 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(()) +}