mirror of
https://github.com/openai/codex.git
synced 2026-11-14 21:40:39 +00:00
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
This commit is contained in:
@@ -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",
|
||||
|
||||
+3
-4
@@ -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"
|
||||
|
||||
+1
-2
@@ -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]
|
||||
|
||||
@@ -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<Mutex<Option<oneshot::Sender<()>>>>,
|
||||
release: Arc<Mutex<mpsc::Receiver<ReadOutcome>>>,
|
||||
}
|
||||
|
||||
impl KeyringStore for GatedKeyringStore {
|
||||
fn load(&self, service: &str, account: &str) -> Result<Option<String>, 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<bool, CredentialStoreError> {
|
||||
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));
|
||||
|
||||
@@ -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<K: KeyringStore + Clone + 'static>(
|
||||
.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<K: KeyringStore + Clone + 'static>(
|
||||
keyring_store: &K,
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
) -> std::result::Result<Option<StoredOAuthTokens>, 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<K: KeyringStore + Clone + 'static>(
|
||||
keyring_store: &K,
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
guard: impl std::borrow::Borrow<OAuthStoreLock>,
|
||||
) -> std::result::Result<Option<StoredOAuthTokens>, 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")?
|
||||
{
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<Mutex<Option<oneshot::Sender<()>>>>,
|
||||
release: Arc<Mutex<mpsc::Receiver<ReadOutcome>>>,
|
||||
}
|
||||
|
||||
impl GatedKeyringStore {
|
||||
pub(crate) fn new(
|
||||
inner: MockKeyringStore,
|
||||
) -> (Self, oneshot::Receiver<()>, mpsc::Sender<ReadOutcome>) {
|
||||
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<Option<String>, 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<bool, CredentialStoreError> {
|
||||
self.inner.delete(service, account)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -78,6 +78,30 @@ struct CachedSecrets {
|
||||
file: Arc<SecretsFile>,
|
||||
}
|
||||
|
||||
/// 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<u8>,
|
||||
}
|
||||
|
||||
impl LocalSecretsSnapshot<'_> {
|
||||
/// Looks up a value in the captured version, using the current keyring entry.
|
||||
pub fn get(self, scope: &SecretScope, name: &SecretName) -> Result<Option<String>> {
|
||||
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<Option<String>> {
|
||||
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<bool> {
|
||||
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<Vec<SecretListEntry>> {
|
||||
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<SecretsFile> {
|
||||
/// 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<Option<LocalSecretsSnapshot<'_>>> {
|
||||
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<SecretsFile> {
|
||||
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<SecretString> {
|
||||
fn load_passphrase(&self) -> Result<Option<SecretString>> {
|
||||
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<SecretString> {
|
||||
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"),
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user