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