From 43ca762a59b19ec7c1e82dab6992f4467dca923c Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Fri, 13 Nov 2026 22:37:45 +0000 Subject: [PATCH] Release MCP OAuth read locks before keyring access (#61401) ## Why Encrypted MCP OAuth reads hold a shared file lock while accessing the keyring, blocking writers until the keyring lookup and decryption finish. ## What changed - Add `LocalSecretsSnapshot` to capture ciphertext under the file lock, then release shared read locks before keyring access and decryption. Keep exclusive locks across conditional credential updates. - Reuse the loaded passphrase when updating existing secrets, avoiding a second keyring lookup. Return an error when an existing encrypted file has no keyring passphrase instead of generating a replacement. - Update login storage error fixtures to use directory reads that fail consistently across platforms. ## Testing Add regression coverage for releasing read locks during keyring access, updating secrets with one keyring lookup, rejecting missing passphrases on cached reads, and reading captured snapshots after later writes. GitOrigin-RevId: add8f60dba52f71c8f8bc10ae8b8ae2f3379ccac --- codex-rs/cli/src/login_storage_tests.rs | 35 +++--- ...er_records_sanitized_storage_fallback.snap | 7 +- ...rage_error_displays_context_and_cause.snap | 3 +- .../rmcp-client/src/ema_identity_tests.rs | 81 +------------- codex-rs/rmcp-client/src/oauth.rs | 28 ++--- .../rmcp-client/src/oauth/resolved_store.rs | 45 ++++---- .../rmcp-client/src/oauth/test_support.rs | 88 +++++++++++++++ .../src/oauth/tests/store_lock_tests.rs | 57 ++++++++++ codex-rs/secrets/src/lib.rs | 1 + codex-rs/secrets/src/local.rs | 101 +++++++++++++----- codex-rs/secrets/src/local_cache_tests.rs | 14 ++- 11 files changed, 295 insertions(+), 165 deletions(-) diff --git a/codex-rs/cli/src/login_storage_tests.rs b/codex-rs/cli/src/login_storage_tests.rs index ded91dcfa1..9809e9161a 100644 --- a/codex-rs/cli/src/login_storage_tests.rs +++ b/codex-rs/cli/src/login_storage_tests.rs @@ -11,8 +11,9 @@ use tracing_subscriber::EnvFilter; #[test] fn login_default_filter_records_sanitized_storage_fallback() -> anyhow::Result<()> { let home = tempdir()?; - // Fail before accessing the real keyring, then allow the plaintext fallback to succeed. - std::fs::write(home.path().join("secrets"), "not a directory")?; + let secrets_path = home.path().join("secrets").join("codex_auth.age"); + // Reading a directory fails before accessing the real keyring on every platform. + std::fs::create_dir_all(&secrets_path)?; let log_path = home.path().join("login.log"); let subscriber = tracing_subscriber::fmt() .without_time() @@ -36,7 +37,7 @@ fn login_default_filter_records_sanitized_storage_fallback() -> anyhow::Result<( failure.contains("failed to save auth to keyring, falling back to file storage"), "{logs}" ); - assert!(failure.contains("failed to create secrets dir"), "{logs}"); + assert!(failure.contains("failed to read secrets file"), "{logs}"); assert!( logs.contains("credential storage file fallback completed"), "{logs}" @@ -46,15 +47,19 @@ fn login_default_filter_records_sanitized_storage_fallback() -> anyhow::Result<( assert!(!logs.contains("private-api-key")); let completion = logs.lines().last().unwrap(); assert!(!completion.contains(home.path().to_str().unwrap())); - let cause = std::fs::create_dir_all(home.path().join("secrets")) - .unwrap_err() - .to_string(); + let cause = std::fs::read(&secrets_path).unwrap_err().to_string(); + // Windows reports access denied for directory reads; Unix reports a directory error. insta::assert_snapshot!( - logs.replace( - home.path().join("secrets").to_str().unwrap(), - "[SECRETS_PATH]" - ) - .replace(&cause, "[OS_ERROR]") + logs.replace(secrets_path.to_str().unwrap(), "[SECRETS_PATH]") + .replace(&cause, "[OS_ERROR]") + .replace( + "secure_error=\"access_denied\"", + "secure_error=\"[OS_ERROR_CATEGORY]\"" + ) + .replace( + "secure_error=\"other\"", + "secure_error=\"[OS_ERROR_CATEGORY]\"" + ) ); Ok(()) } @@ -62,11 +67,9 @@ fn login_default_filter_records_sanitized_storage_fallback() -> anyhow::Result<( #[test] fn login_storage_error_displays_context_and_cause() -> anyhow::Result<()> { let home = tempdir()?; - let secrets_path = home.path().join("secrets"); - std::fs::write(&secrets_path, "not a directory")?; - let cause = std::fs::create_dir_all(&secrets_path) - .unwrap_err() - .to_string(); + let secrets_path = home.path().join("secrets").join("codex_auth.age"); + std::fs::create_dir_all(&secrets_path)?; + let cause = std::fs::read(&secrets_path).unwrap_err().to_string(); let error = login_with_api_key( home.path(), "private-api-key", diff --git a/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap index 8536486dcc..56b6cf0960 100644 --- a/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap +++ b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap @@ -1,7 +1,6 @@ --- source: cli/src/login_storage_tests.rs -assertion_line: 47 -expression: "logs.replace(home.path().join(\"secrets\").to_str().unwrap(),\n\"[SECRETS_PATH]\").replace(&cause, \"[OS_ERROR]\")" +expression: "logs.replace(secrets_path.to_str().unwrap(),\n\"[SECRETS_PATH]\").replace(&cause,\n\"[OS_ERROR]\").replace(\"secure_error=\\\"access_denied\\\"\",\n\"secure_error=\\\"[OS_ERROR_CATEGORY]\\\"\").replace(\"secure_error=\\\"other\\\"\",\n\"secure_error=\\\"[OS_ERROR_CATEGORY]\\\"\")" --- - WARN codex_login::auth::storage: failed to save auth to keyring, falling back to file storage: failed to write OAuth tokens to encrypted auth storage: failed to create secrets dir [SECRETS_PATH]: [OS_ERROR] - WARN codex_otel::auth_storage: credential storage file fallback completed credential_kind="codex" store_mode="auto" selected_store="secrets" actual_store="file" operation="save" secure_outcome="error" outcome="success" fallback_reason="secure_error" secure_error="other" storage_phase="policy" originator="none" + WARN codex_login::auth::storage: failed to save auth to keyring, falling back to file storage: failed to write OAuth tokens to encrypted auth storage: failed to read secrets file at [SECRETS_PATH]: [OS_ERROR] + WARN codex_otel::auth_storage: credential storage file fallback completed credential_kind="codex" store_mode="auto" selected_store="secrets" actual_store="file" operation="save" secure_outcome="error" outcome="success" fallback_reason="secure_error" secure_error="[OS_ERROR_CATEGORY]" storage_phase="policy" originator="none" diff --git a/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap index f4f7250e38..7374e2d557 100644 --- a/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap +++ b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap @@ -1,6 +1,5 @@ --- source: cli/src/login_storage_tests.rs -assertion_line: 58 expression: "message.replace(secrets_path.to_str().unwrap(),\n\"[SECRETS_PATH]\").replace(&cause, \"[OS_ERROR]\")" --- -failed to write OAuth tokens to encrypted auth storage: failed to create secrets dir [SECRETS_PATH]: [OS_ERROR] +failed to write OAuth tokens to encrypted auth storage: failed to read secrets file at [SECRETS_PATH]: [OS_ERROR] diff --git a/codex-rs/rmcp-client/src/ema_identity_tests.rs b/codex-rs/rmcp-client/src/ema_identity_tests.rs index ddfc428748..113134852a 100644 --- a/codex-rs/rmcp-client/src/ema_identity_tests.rs +++ b/codex-rs/rmcp-client/src/ema_identity_tests.rs @@ -1,8 +1,4 @@ use std::io; -use std::sync::Mutex; -use std::sync::PoisonError; -use std::sync::mpsc; -use std::thread; use std::time::Duration; use std::time::SystemTime; use std::time::UNIX_EPOCH; @@ -13,13 +9,10 @@ use codex_config::types::AuthKeyringBackendKind; use codex_exec_server::RouteAwareHttpClient; use codex_http_client::HttpClientFactory; use codex_http_client::OutboundProxyPolicy; -use codex_keyring_store::CredentialStoreError; use codex_keyring_store::tests::MockKeyringStore; use futures::FutureExt; use pretty_assertions::assert_eq; -use pretty_assertions::assert_ne; use serde_json::json; -use tokio::sync::oneshot; use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -32,6 +25,9 @@ use crate::WrappedOAuthTokenResponse; use crate::oauth::RefreshCredentialLock; use crate::oauth::ResolvedOAuthCredentialStore; use crate::oauth::StoredOAuthCredentialSnapshot; +use crate::oauth::test_support::GatedKeyringStore; +use crate::oauth::test_support::REREAD_TEST_TIMEOUT; +use crate::oauth::test_support::ReadOutcome; use crate::oauth::test_support::TempCodexHome; fn credentials(issuer: &str, subject: &str, expires_at: u64) -> StoredOAuthTokens { @@ -180,67 +176,6 @@ async fn enterprise_discovery_shares_idp_checks_but_keeps_login_endpoint_policy( Ok(()) } -const REREAD_TEST_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 5); -const REREAD_PANIC_SENTINEL: &str = "credential-panic-payload-sentinel"; - -#[derive(Clone, Copy, Debug)] -enum ReadOutcome { - Stored, - BackendError, - Panic, -} - -#[derive(Clone, Debug)] -struct GatedKeyringStore { - inner: MockKeyringStore, - executor_thread: thread::ThreadId, - entered: Arc>>>, - release: Arc>>, -} - -impl KeyringStore for GatedKeyringStore { - fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { - // Fail before blocking if a regression puts this read on the current-thread executor. - assert_ne!(thread::current().id(), self.executor_thread); - if let Some(entered) = self - .entered - .lock() - .unwrap_or_else(PoisonError::into_inner) - .take() - { - let _ = entered.send(()); - } - let outcome = match self - .release - .lock() - .unwrap_or_else(PoisonError::into_inner) - .recv_timeout(REREAD_TEST_TIMEOUT) - { - Ok(outcome) => outcome, - Err(mpsc::RecvTimeoutError::Disconnected) => ReadOutcome::Stored, - Err(mpsc::RecvTimeoutError::Timeout) => panic!("credential reread gate timed out"), - }; - match outcome { - ReadOutcome::Stored => self.inner.load(service, account), - ReadOutcome::BackendError => Err(CredentialStoreError::new( - keyring::Error::PlatformFailure(Box::new(io::Error::new( - io::ErrorKind::PermissionDenied, - "credential backend unavailable", - ))), - )), - ReadOutcome::Panic => panic!("{REREAD_PANIC_SENTINEL}"), - } - } - - fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { - self.inner.save(service, account, value) - } - - fn delete(&self, service: &str, account: &str) -> Result { - self.inner.delete(service, account) - } -} - #[tokio::test(flavor = "current_thread")] async fn refresh_subject_reread_is_cancellable_and_releases_guard_on_failure() -> Result<()> { let _home = TempCodexHome::new(); @@ -255,15 +190,7 @@ async fn refresh_subject_reread_is_cancellable_and_releases_guard_on_failure() - ] { let inner = MockKeyringStore::default(); store.save(&inner, &stored.server_name, &stored)?; - let (entered_tx, entered_rx) = oneshot::channel(); - // Dropping the sole sender releases the worker on any early return or panic. - let (release_tx, release_rx) = mpsc::channel(); - let keyring = GatedKeyringStore { - inner, - executor_thread: thread::current().id(), - entered: Arc::new(Mutex::new(Some(entered_tx))), - release: Arc::new(Mutex::new(release_rx)), - }; + let (keyring, entered_rx, release_tx) = GatedKeyringStore::new(inner); let request = request(&issuer, &snapshot); crate::oauth::test_support::warm_http_client(request.http_client.as_ref()).await?; let mut identity = Box::pin(resolve_ema_idp_identity_in(request, &keyring)); diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index d14245fc89..910811b912 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -39,6 +39,7 @@ use codex_config::types::AuthKeyringBackendKind; use codex_config::types::OAuthCredentialsStoreMode; use codex_otel::auth_storage::Operation; use codex_otel::auth_storage::Store; +use codex_secrets::LocalSecretsBackend; use codex_secrets::LocalSecretsNamespace; use codex_secrets::SecretName; use codex_secrets::SecretScope; @@ -407,7 +408,8 @@ fn load_oauth_tokens_from_keyring( .map_err(OAuthKeyringLoadError::Backend) } AuthKeyringBackendKind::Secrets => { - load_oauth_tokens_from_secrets_keyring(keyring_store, server_name, url) + let guard = OAuthStoreLock::acquire_for_read(OAuthStore::Secrets)?; + load_oauth_tokens_from_secrets_keyring(keyring_store, server_name, url, guard) } } } @@ -434,25 +436,25 @@ fn load_oauth_tokens_from_secrets_keyring( keyring_store: &K, server_name: &str, url: &str, -) -> std::result::Result, OAuthKeyringLoadError> { - let _store_lock = OAuthStoreLock::acquire_for_read(OAuthStore::Secrets)?; - load_oauth_tokens_from_secrets_keyring_with_lock_held(keyring_store, server_name, url) -} - -fn load_oauth_tokens_from_secrets_keyring_with_lock_held( - keyring_store: &K, - server_name: &str, - url: &str, + guard: impl std::borrow::Borrow, ) -> std::result::Result, OAuthKeyringLoadError> { let codex_home = find_codex_home().map_err(anyhow::Error::from)?; - let manager = SecretsManager::new_with_keyring_store_and_namespace( + let backend = LocalSecretsBackend::new_with_namespace( codex_home.to_path_buf(), - SecretsBackendKind::Local, Arc::new(keyring_store.clone()), LocalSecretsNamespace::McpOAuth, ); + let snapshot = backend + .read_snapshot() + .context("failed to load MCP OAuth tokens from encrypted storage")?; + // Readers own their shared guard. Conditional writers lend their exclusive + // guard so it remains held across the comparison and the subsequent write. + drop(guard); + let Some(snapshot) = snapshot else { + return Ok(None); + }; let secret_name = compute_secret_name(server_name, url)?; - match manager + match snapshot .get(&SecretScope::Global, &secret_name) .context("failed to load MCP OAuth tokens from encrypted storage")? { diff --git a/codex-rs/rmcp-client/src/oauth/resolved_store.rs b/codex-rs/rmcp-client/src/oauth/resolved_store.rs index b30bf5e31f..95c473b588 100644 --- a/codex-rs/rmcp-client/src/oauth/resolved_store.rs +++ b/codex-rs/rmcp-client/src/oauth/resolved_store.rs @@ -25,7 +25,7 @@ use super::delete_oauth_tokens_from_secrets_keyring; use super::load_oauth_tokens_from_file; use super::load_oauth_tokens_from_file_with_lock_held; use super::load_oauth_tokens_from_keyring; -use super::load_oauth_tokens_from_secrets_keyring_with_lock_held; +use super::load_oauth_tokens_from_secrets_keyring; use super::normalized_file_oauth_credentials; use super::normalized_oauth_credentials; use super::read_fallback_file_unlocked; @@ -146,11 +146,12 @@ impl ResolvedOAuthCredentialStore { .context("failed to reread OAuth tokens from resolved keyring storage; refusing file fallback") } Backend::Keyring(AuthKeyringBackendKind::Secrets) => { - let _store_lock = OAuthStoreLock::try_acquire_for_read(OAuthStore::Secrets)?; - load_oauth_tokens_from_secrets_keyring_with_lock_held( + let guard = OAuthStoreLock::try_acquire_for_read(OAuthStore::Secrets)?; + load_oauth_tokens_from_secrets_keyring( keyring_store, server_name, url, + guard, ) .map_err(anyhow::Error::from) } @@ -250,32 +251,26 @@ impl ResolvedOAuthCredentialStore { expected: &StoredOAuthTokens, ) -> Result<()> { let mut observation = telemetry::resolved(self, Operation::Load); - let mut _store_lock = None; + let mut store_lock = None; let mut file_key = None; let current = (|| -> Result<_> { - _store_lock = match self.backend { - Backend::File => Some(OAuthStoreLock::acquire_for_write(OAuthStore::File)?), - Backend::Keyring(AuthKeyringBackendKind::Secrets) => { - Some(OAuthStoreLock::acquire_for_write(OAuthStore::Secrets)?) - } - Backend::Keyring(AuthKeyringBackendKind::Direct) => None, - }; match self.backend { - Backend::File => Ok( - load_oauth_tokens_from_file_with_lock_held(server_name, url)?.map( - |(key, tokens)| { - file_key = Some(key); - tokens - }, - ), - ), - Backend::Keyring(AuthKeyringBackendKind::Secrets) => { - load_oauth_tokens_from_secrets_keyring_with_lock_held( - keyring_store, - server_name, - url, + Backend::File => { + store_lock = Some(OAuthStoreLock::acquire_for_write(OAuthStore::File)?); + Ok( + load_oauth_tokens_from_file_with_lock_held(server_name, url)?.map( + |(key, tokens)| { + file_key = Some(key); + tokens + }, + ), ) - .map_err(anyhow::Error::from) + } + Backend::Keyring(AuthKeyringBackendKind::Secrets) => { + let guard = + store_lock.insert(OAuthStoreLock::acquire_for_write(OAuthStore::Secrets)?); + load_oauth_tokens_from_secrets_keyring(keyring_store, server_name, url, guard) + .map_err(anyhow::Error::from) } Backend::Keyring(AuthKeyringBackendKind::Direct) => load_oauth_tokens_from_keyring( keyring_store, diff --git a/codex-rs/rmcp-client/src/oauth/test_support.rs b/codex-rs/rmcp-client/src/oauth/test_support.rs index 208d11b458..3e1b481e25 100644 --- a/codex-rs/rmcp-client/src/oauth/test_support.rs +++ b/codex-rs/rmcp-client/src/oauth/test_support.rs @@ -1,8 +1,18 @@ +use codex_keyring_store::CredentialStoreError; +use codex_keyring_store::KeyringStore; +use codex_keyring_store::tests::MockKeyringStore; +use pretty_assertions::assert_ne; +use std::io; use std::path::Path; +use std::sync::Arc; use std::sync::Mutex; use std::sync::MutexGuard; use std::sync::OnceLock; use std::sync::PoisonError; +use std::sync::mpsc; +use std::thread; +use std::time::Duration; +use tokio::sync::oneshot; use tempfile::tempdir; @@ -67,3 +77,81 @@ impl Drop for TempCodexHome { } } } + +pub(crate) const REREAD_TEST_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 5); +const REREAD_PANIC_SENTINEL: &str = "credential-panic-payload-sentinel"; + +#[derive(Clone, Copy, Debug)] +pub(crate) enum ReadOutcome { + Stored, + BackendError, + Panic, +} + +#[derive(Clone, Debug)] +pub(crate) struct GatedKeyringStore { + inner: MockKeyringStore, + executor_thread: thread::ThreadId, + entered: Arc>>>, + release: Arc>>, +} + +impl GatedKeyringStore { + pub(crate) fn new( + inner: MockKeyringStore, + ) -> (Self, oneshot::Receiver<()>, mpsc::Sender) { + let (entered_tx, entered_rx) = oneshot::channel(); + // Dropping the sender also releases the worker on early return or panic. + let (release_tx, release_rx) = mpsc::channel(); + let keyring = Self { + inner, + executor_thread: thread::current().id(), + entered: Arc::new(Mutex::new(Some(entered_tx))), + release: Arc::new(Mutex::new(release_rx)), + }; + (keyring, entered_rx, release_tx) + } +} + +impl KeyringStore for GatedKeyringStore { + fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { + // Fail before blocking if a regression puts this read on the current-thread executor. + assert_ne!(thread::current().id(), self.executor_thread); + if let Some(entered) = self + .entered + .lock() + .unwrap_or_else(PoisonError::into_inner) + .take() + { + let _ = entered.send(()); + } + let outcome = match self + .release + .lock() + .unwrap_or_else(PoisonError::into_inner) + .recv_timeout(REREAD_TEST_TIMEOUT) + { + Ok(outcome) => outcome, + Err(mpsc::RecvTimeoutError::Disconnected) => ReadOutcome::Stored, + Err(mpsc::RecvTimeoutError::Timeout) => panic!("credential reread gate timed out"), + }; + match outcome { + ReadOutcome::Stored => self.inner.load(service, account), + ReadOutcome::BackendError => Err(CredentialStoreError::new( + keyring::Error::PlatformFailure(Box::new(io::Error::new( + io::ErrorKind::PermissionDenied, + "credential backend unavailable", + ))), + )), + ReadOutcome::Panic => panic!("{REREAD_PANIC_SENTINEL}"), + } + } + + fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { + self.inner.save(service, account, value) + } + + fn delete(&self, service: &str, account: &str) -> Result { + self.inner.delete(service, account) + } +} diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index 0a3513bfaa..f9f6b0f185 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -28,6 +28,7 @@ use super::OAuthStore; use super::OAuthStoreLock; use super::OAuthStoreLockFailure; use super::OAuthStoreLockMode; +use crate::oauth::ResolvedOAuthCredentialStore; use crate::oauth::StoredOAuthCredentialSnapshot; use crate::oauth::StoredOAuthTokens; use crate::oauth::WrappedOAuthTokenResponse; @@ -41,6 +42,8 @@ use crate::oauth::save_oauth_tokens_to_secrets_keyring_with_lock_held; use crate::oauth::save_oauth_tokens_with_keyring; use crate::oauth::save_oauth_tokens_with_keyring_with_fallback_to_file; use crate::oauth::stored_oauth_credential_snapshot; +use crate::oauth::test_support::GatedKeyringStore; +use crate::oauth::test_support::ReadOutcome; use crate::oauth::test_support::TempCodexHome; use codex_config::types::OAuthCredentialsStoreMode; @@ -597,6 +600,60 @@ fn file_store_load_and_delete_observe_aggregate_lock() -> Result<()> { Ok(()) } +#[tokio::test] +async fn secrets_reads_release_the_file_lock_before_keyring_access() -> Result<()> { + let home = TempCodexHome::new(); + let keyring = MockKeyringStore::default(); + let store = ResolvedOAuthCredentialStore::keyring(AuthKeyringBackendKind::Secrets); + let tokens = sample_tokens(); + store.save(&keyring, &tokens.server_name, &tokens)?; + + for load in [ + ResolvedOAuthCredentialStore::load, + ResolvedOAuthCredentialStore::try_load, + ] { + let (gated, entered, release) = GatedKeyringStore::new(keyring.clone()); + let expected = tokens.clone(); + let reader = tokio::task::spawn_blocking(move || { + load(store, &gated, &expected.server_name, &expected.url) + }); + tokio::time::timeout(STORE_LOCK_CONTENTION_EVENT_TIMEOUT, entered).await??; + let _writer = OAuthStoreLock::acquire_in_with_mode( + home.path(), + OAuthStore::Secrets, + Duration::ZERO, + OAuthStoreLockMode::Exclusive, + )?; + release.send(ReadOutcome::Stored)?; + let result = reader.await??.expect("read captured the stored credential"); + assert_tokens_match_without_expiry(&result, &tokens); + } + Ok(()) +} + +#[tokio::test] +async fn updating_an_existing_secret_requires_one_keyring_lookup() -> Result<()> { + let _home = TempCodexHome::new(); + let keyring = MockKeyringStore::default(); + let store = ResolvedOAuthCredentialStore::keyring(AuthKeyringBackendKind::Secrets); + let mut updated = sample_tokens(); + store.save(&keyring, &updated.server_name, &updated)?; + updated + .token_response + .0 + .set_access_token(AccessToken::new("updated".into())); + let (gated, _entered, release) = GatedKeyringStore::new(keyring.clone()); + release.send(ReadOutcome::Stored)?; + release.send(ReadOutcome::BackendError)?; + let write = updated.clone(); + tokio::task::spawn_blocking(move || store.save(&gated, &write.server_name, &write)).await??; + let stored = store + .load(&keyring, &updated.server_name, &updated.url)? + .expect("updated credential was persisted"); + assert_tokens_match_without_expiry(&stored, &updated); + Ok(()) +} + #[test] fn secrets_store_lock_preserves_updates_for_different_servers() -> Result<()> { let env = TempCodexHome::new(); diff --git a/codex-rs/secrets/src/lib.rs b/codex-rs/secrets/src/lib.rs index d934101900..9ef92a03e6 100644 --- a/codex-rs/secrets/src/lib.rs +++ b/codex-rs/secrets/src/lib.rs @@ -18,6 +18,7 @@ mod sanitizer; pub use local::LocalSecretsBackend; pub use local::LocalSecretsNamespace; +pub use local::LocalSecretsSnapshot; pub use sanitizer::redact_secrets; const KEYRING_SERVICE: &str = "codex"; diff --git a/codex-rs/secrets/src/local.rs b/codex-rs/secrets/src/local.rs index 6a7daedcf2..38278386e8 100644 --- a/codex-rs/secrets/src/local.rs +++ b/codex-rs/secrets/src/local.rs @@ -78,6 +78,30 @@ struct CachedSecrets { file: Arc, } +/// A point-in-time copy of the encrypted file. Reading a value still requires +/// the current keyring passphrase, even when the decrypted contents are cached. +pub struct LocalSecretsSnapshot<'a> { + backend: &'a LocalSecretsBackend, + ciphertext: Vec, +} + +impl LocalSecretsSnapshot<'_> { + /// Looks up a value in the captured version, using the current keyring entry. + pub fn get(self, scope: &SecretScope, name: &SecretName) -> Result> { + let (mut file, _) = self.open()?; + Ok(file.secrets.remove(&scope.canonical_key(name))) + } + + fn open(self) -> Result<(SecretsFile, SecretString)> { + let passphrase = self + .backend + .load_passphrase()? + .context("missing keyring passphrase for the existing encrypted secrets file")?; + let file = self.backend.decrypt_file(&self.ciphertext, &passphrase)?; + Ok((file, passphrase)) + } +} + impl SecretsFile { fn new_empty() -> Self { Self { @@ -130,29 +154,39 @@ impl LocalSecretsBackend { pub fn set(&self, scope: &SecretScope, name: &SecretName, value: &str) -> Result<()> { anyhow::ensure!(!value.is_empty(), "secret value must not be empty"); let canonical_key = scope.canonical_key(name); - let mut file = self.load_file()?; + let (mut file, passphrase) = match self.read_snapshot()? { + Some(snapshot) => snapshot.open()?, + None => (SecretsFile::new_empty(), self.load_or_create_passphrase()?), + }; file.secrets.insert(canonical_key, value.to_string()); - self.save_file(&file) + self.save_file(&file, &passphrase) } pub fn get(&self, scope: &SecretScope, name: &SecretName) -> Result> { - let canonical_key = scope.canonical_key(name); - let file = self.load_file()?; - Ok(file.secrets.get(&canonical_key).cloned()) + let Some(snapshot) = self.read_snapshot()? else { + return Ok(None); + }; + snapshot.get(scope, name) } pub fn delete(&self, scope: &SecretScope, name: &SecretName) -> Result { let canonical_key = scope.canonical_key(name); - let mut file = self.load_file()?; + let Some(snapshot) = self.read_snapshot()? else { + return Ok(false); + }; + let (mut file, passphrase) = snapshot.open()?; let removed = file.secrets.remove(&canonical_key).is_some(); if removed { - self.save_file(&file)?; + self.save_file(&file, &passphrase)?; } Ok(removed) } pub fn list(&self, scope_filter: Option<&SecretScope>) -> Result> { - let file = self.load_file()?; + let Some(snapshot) = self.read_snapshot()? else { + return Ok(Vec::new()); + }; + let (file, _) = snapshot.open()?; let mut entries = Vec::new(); for canonical_key in file.secrets.keys() { let Some(entry) = parse_canonical_key(canonical_key) else { @@ -194,16 +228,24 @@ impl LocalSecretsBackend { } } - fn load_file(&self) -> Result { + /// Copies the ciphertext while the caller holds any required file lock. + /// The returned snapshot can be opened after releasing a shared read lock. + pub fn read_snapshot(&self) -> Result>> { let path = self.secrets_path(); - if !path.exists() { - return Ok(SecretsFile::new_empty()); + match fs::read(&path) { + Ok(ciphertext) => Ok(Some(LocalSecretsSnapshot { + backend: self, + ciphertext, + })), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(error) => Err(error) + .with_context(|| format!("failed to read secrets file at {}", path.display())), } + } - let ciphertext = fs::read(&path) - .with_context(|| format!("failed to read secrets file at {}", path.display()))?; - let passphrase = self.load_or_create_passphrase()?; - let ciphertext_hash: [u8; 32] = Sha256::digest(&ciphertext).into(); + fn decrypt_file(&self, ciphertext: &[u8], passphrase: &SecretString) -> Result { + let path = self.secrets_path(); + 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 @@ -219,7 +261,7 @@ impl LocalSecretsBackend { { return Ok(cached.file.as_ref().clone()); } - let plaintext = decrypt_with_passphrase(&ciphertext, &passphrase)?; + let plaintext = decrypt_with_passphrase(ciphertext, passphrase)?; let mut parsed: SecretsFile = serde_json::from_slice(&plaintext).with_context(|| { format!( "failed to deserialize decrypted secrets file at {}", @@ -244,14 +286,13 @@ impl LocalSecretsBackend { Ok(parsed) } - fn save_file(&self, file: &SecretsFile) -> Result<()> { + fn save_file(&self, file: &SecretsFile, passphrase: &SecretString) -> Result<()> { let dir = self.secrets_dir(); fs::create_dir_all(&dir) .with_context(|| format!("failed to create secrets dir {}", dir.display()))?; - let passphrase = self.load_or_create_passphrase()?; let plaintext = serde_json::to_vec(file).context("failed to serialize secrets file")?; - let ciphertext = encrypt_with_passphrase(&plaintext, &passphrase)?; + let ciphertext = encrypt_with_passphrase(&plaintext, passphrase)?; let path = self.secrets_path(); write_file_atomically(&path, &ciphertext)?; let mut cache = self @@ -274,20 +315,24 @@ impl LocalSecretsBackend { Ok(()) } - fn load_or_create_passphrase(&self) -> Result { + fn load_passphrase(&self) -> Result> { let account = compute_keyring_account(&self.codex_home, self.namespace); - let loaded = self - .keyring_store + self.keyring_store .load(keyring_service(), &account) + .map(|loaded| loaded.map(SecretString::from)) .map_err(std::io::Error::from) - .with_context(|| format!("failed to load secrets key from keyring for {account}"))?; - match loaded { - Some(existing) => Ok(SecretString::from(existing)), + .with_context(|| format!("failed to load secrets key from keyring for {account}")) + } + + fn load_or_create_passphrase(&self) -> Result { + match self.load_passphrase()? { + Some(existing) => Ok(existing), None => { // Generate a high-entropy key and persist it in the OS keyring. // This keeps secrets out of plaintext config while remaining // fully local/offline for the MVP. let generated = generate_passphrase()?; + let account = compute_keyring_account(&self.codex_home, self.namespace); self.keyring_store .save(keyring_service(), &account, generated.expose_secret()) .map_err(std::io::Error::from) @@ -474,10 +519,12 @@ mod tests { version: SECRETS_VERSION + 1, secrets: BTreeMap::new(), }; - backend.save_file(&file)?; + backend.save_file(&file, &backend.load_or_create_passphrase()?)?; let error = backend - .load_file() + .read_snapshot()? + .expect("file was saved") + .open() .expect_err("must reject newer schema version"); assert!( error.to_string().contains("newer than supported version"), diff --git a/codex-rs/secrets/src/local_cache_tests.rs b/codex-rs/secrets/src/local_cache_tests.rs index a66d480404..f812d3b2b1 100644 --- a/codex-rs/secrets/src/local_cache_tests.rs +++ b/codex-rs/secrets/src/local_cache_tests.rs @@ -40,6 +40,9 @@ fn cached_reads_check_file_presence_key_rotation_and_keyring_errors() -> Result< assert_eq!(backend.get(&scope, &name)?, Some("original".to_string())); // Neither a changed encryption key nor a keyring error may reuse plaintext. + keyring.delete(keyring_service(), &account)?; + assert!(backend.get(&scope, &name).is_err()); + assert_eq!(keyring.load(keyring_service(), &account)?, None); let replacement_key = generate_passphrase()?; keyring.save(keyring_service(), &account, replacement_key.expose_secret())?; assert!(backend.get(&scope, &name).is_err()); @@ -58,6 +61,11 @@ fn cached_reads_check_file_presence_key_rotation_and_keyring_errors() -> Result< assert!(backend.get(&scope, &name).is_err()); fs::write(backend.secrets_path(), &ciphertext)?; assert_eq!(backend.get(&scope, &name)?, Some("original".to_string())); + + let snapshot = backend.read_snapshot()?.expect("stored ciphertext"); + backend.set(&scope, &name, "replacement")?; + assert_eq!(snapshot.get(&scope, &name)?, Some("original".to_string())); + assert_eq!(backend.get(&scope, &name)?, Some("replacement".to_string())); } Ok(()) } @@ -152,7 +160,11 @@ fn saves_cache_committed_contents_and_preserve_them_after_a_failed_commit() -> R fs::rename(&path, &backup)?; fs::create_dir(&path)?; let attempted = SecretsFile::new_empty(); - assert!(backend.save_file(&attempted).is_err()); + assert!( + backend + .save_file(&attempted, &backend.load_or_create_passphrase()?) + .is_err() + ); assert!(Arc::ptr_eq( &committed, &backend