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

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");

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(

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<Option<CachedSecrets>> = Mutex::new(None);
+static CODEX_AUTH_CACHE: Mutex<Option<CachedSecrets>> = Mutex::new(None);
+static MANAGED_SECRETS_CACHE: Mutex<Option<CachedSecrets>> = 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<Option<CachedSecrets>>> {
+    /// 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<Option<CachedSecrets>> {
         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;

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(())
+}