From 14578ad4fcce911c6fdbcc412ade8a2dec3a4457 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Tue, 7 Jul 2026 04:29:04 +0000 Subject: [PATCH] Restack OAuth Auto-store drift diagnostics --- codex-rs/Cargo.lock | 2 + codex-rs/rmcp-client/Cargo.toml | 2 + codex-rs/rmcp-client/src/oauth.rs | 72 +++-- .../rmcp-client/src/oauth/resolution_state.rs | 253 ++++++++++++++++++ .../src/oauth/resolution_state_tests.rs | 131 +++++++++ .../rmcp-client/src/oauth/resolved_store.rs | 107 +++++--- codex-rs/rmcp-client/src/oauth/store_lock.rs | 11 +- 7 files changed, 519 insertions(+), 59 deletions(-) create mode 100644 codex-rs/rmcp-client/src/oauth/resolution_state.rs create mode 100644 codex-rs/rmcp-client/src/oauth/resolution_state_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 0d4d3c33bfc7..edb1b24a42bd 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3795,10 +3795,12 @@ dependencies = [ "codex-config", "codex-exec-server", "codex-keyring-store", + "codex-otel", "codex-protocol", "codex-secrets", "codex-utils-cargo-bin", "codex-utils-home-dir", + "codex-utils-path", "codex-utils-path-uri", "codex-utils-pty", "futures", diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index a8464f3054d9..5d1e11f99290 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -19,8 +19,10 @@ codex-api = { workspace = true } codex-config = { workspace = true } codex-exec-server = { workspace = true } codex-keyring-store = { workspace = true } +codex-otel = { workspace = true } codex-protocol = { workspace = true } codex-secrets = { workspace = true } +codex-utils-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-pty = { workspace = true } codex-utils-home-dir = { workspace = true } diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 11682a5fdc9d..bf6f3558b5d8 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -18,6 +18,7 @@ mod refresh_lock; mod refresh_transaction; +mod resolution_state; mod resolved_store; mod store_lock; @@ -58,6 +59,8 @@ use std::time::SystemTime; use std::time::UNIX_EPOCH; use tracing::warn; +use self::resolution_state::StoreResolutionReason; +use self::resolution_state::record_store_resolution; use self::store_lock::OAuthStore; use self::store_lock::OAuthStoreLock; use self::store_lock::OAuthStoreLockFailure; @@ -282,21 +285,53 @@ fn save_oauth_tokens_with_keyring_store( store_mode: OAuthCredentialsStoreMode, keyring_backend_kind: AuthKeyringBackendKind, ) -> Result<()> { - match store_mode { - OAuthCredentialsStoreMode::Auto => save_oauth_tokens_with_keyring_with_fallback_to_file( - keyring_store, - keyring_backend_kind, - server_name, - tokens, - ), - OAuthCredentialsStoreMode::File => save_oauth_tokens_to_file(tokens), - OAuthCredentialsStoreMode::Keyring => save_oauth_tokens_with_keyring_and_cleanup_file( - keyring_store, - keyring_backend_kind, - server_name, - tokens, - ), - } + let (resolved_store, reason) = match store_mode { + OAuthCredentialsStoreMode::Auto => { + let resolved_store = save_oauth_tokens_with_keyring_with_fallback_to_file( + keyring_store, + keyring_backend_kind, + server_name, + tokens, + )?; + let reason = match resolved_store { + ResolvedOAuthCredentialStore::File => { + StoreResolutionReason::AutoSaveToFileAfterKeyringError + } + ResolvedOAuthCredentialStore::Keyring(_) => { + StoreResolutionReason::AutoSaveToKeyring + } + }; + (resolved_store, reason) + } + OAuthCredentialsStoreMode::File => { + save_oauth_tokens_to_file(tokens)?; + ( + ResolvedOAuthCredentialStore::File, + StoreResolutionReason::ConfiguredSave, + ) + } + OAuthCredentialsStoreMode::Keyring => { + save_oauth_tokens_with_keyring_and_cleanup_file( + keyring_store, + keyring_backend_kind, + server_name, + tokens, + )?; + ( + ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), + StoreResolutionReason::ConfiguredSave, + ) + } + }; + record_store_resolution( + server_name, + &tokens.url, + store_mode, + keyring_backend_kind, + resolved_store, + reason, + ); + Ok(()) } fn save_oauth_tokens_with_keyring( @@ -399,14 +434,14 @@ fn save_oauth_tokens_with_keyring_with_fallback_to_file Result<()> { +) -> Result { match save_oauth_tokens_with_keyring_and_cleanup_file( keyring_store, keyring_backend_kind, server_name, tokens, ) { - Ok(()) => Ok(()), + Ok(()) => Ok(ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind)), // As on load, a store lock failure is a coordination failure rather than evidence that // the keyring backend is unavailable. Falling back could leave a newer File token hidden // behind a stale Secrets entry. @@ -415,7 +450,8 @@ fn save_oauth_tokens_with_keyring_with_fallback_to_file &'static str { + match self { + Self::AutoLoadFromKeyring => "auto_load_keyring", + Self::AutoLoadFromFileAfterMissingKeyring => "auto_load_file_keyring_missing", + Self::AutoLoadFromFileAfterKeyringError => "auto_load_file_keyring_error", + Self::AutoSaveToKeyring => "auto_save_keyring", + Self::AutoSaveToFileAfterKeyringError => "auto_save_file_keyring_error", + Self::ConfiguredLoad => "configured_load", + Self::ConfiguredSave => "configured_save", + } + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +enum ObservedStore { + File, + Keyring, +} + +impl ObservedStore { + fn as_str(self) -> &'static str { + match self { + Self::File => "file", + Self::Keyring => "keyring", + } + } +} + +impl From for ObservedStore { + fn from(store: ResolvedOAuthCredentialStore) -> Self { + match store { + ResolvedOAuthCredentialStore::File => Self::File, + ResolvedOAuthCredentialStore::Keyring(_) => Self::Keyring, + } + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +struct StoreResolution { + store_mode: OAuthCredentialsStoreMode, + keyring_backend: AuthKeyringBackendKind, + resolved_store: ObservedStore, +} + +#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] +struct ResolutionState { + #[serde(default = "resolution_state_version")] + version: u32, + #[serde(default)] + resolutions: BTreeMap, +} + +impl Default for ResolutionState { + fn default() -> Self { + Self { + version: RESOLUTION_STATE_VERSION, + resolutions: BTreeMap::new(), + } + } +} + +fn resolution_state_version() -> u32 { + RESOLUTION_STATE_VERSION +} + +#[derive(Debug, PartialEq, Eq)] +struct StoreResolutionChange { + previous: ObservedStore, + current: ObservedStore, +} + +pub(super) fn record_store_resolution( + server_name: &str, + url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend: AuthKeyringBackendKind, + resolved_store: ResolvedOAuthCredentialStore, + reason: StoreResolutionReason, +) { + let result = match find_codex_home() { + Ok(codex_home) => record_store_resolution_in( + &codex_home, + server_name, + url, + store_mode, + keyring_backend, + resolved_store, + ), + Err(error) => Err(error.into()), + }; + + match result { + Ok(Some(change)) => { + warn!( + server_name, + previous_store = change.previous.as_str(), + resolved_store = change.current.as_str(), + resolution_reason = reason.as_str(), + "MCP OAuth Auto storage resolved differently than its previous use" + ); + if let Some(metrics) = codex_otel::global() { + let _ = metrics.counter( + RESOLUTION_CHANGED_METRIC, + /*inc*/ 1, + &[ + ("previous_store", change.previous.as_str()), + ("resolved_store", change.current.as_str()), + ("reason", reason.as_str()), + ], + ); + } + } + Ok(None) => {} + Err(error) => { + warn!( + server_name, + resolution_reason = reason.as_str(), + error = %error, + "failed to update MCP OAuth store resolution diagnostics" + ); + } + } +} + +fn record_store_resolution_in( + codex_home: &Path, + server_name: &str, + url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend: AuthKeyringBackendKind, + resolved_store: ResolvedOAuthCredentialStore, +) -> Result> { + // This short, best-effort lock serializes only the diagnostic map. It must not make an OAuth + // operation wait behind the 60-second credential-store lock budget. + let _lock = OAuthStoreLock::acquire_in( + codex_home, + OAuthStore::ResolutionState, + RESOLUTION_STATE_LOCK_TIMEOUT, + )?; + let path = codex_home.join(RESOLUTION_STATE_FILENAME); + let mut state = read_resolution_state(&path)?; + anyhow::ensure!( + state.version == RESOLUTION_STATE_VERSION, + "unsupported MCP OAuth store resolution state version {}", + state.version + ); + + let store_key = compute_store_key(server_name, url)?; + let current = StoreResolution { + store_mode, + keyring_backend, + resolved_store: resolved_store.into(), + }; + let previous = state.resolutions.get(&store_key).copied(); + if previous == Some(current) { + return Ok(None); + } + + state.resolutions.insert(store_key, current); + let serialized = serde_json::to_string(&state) + .context("failed to serialize MCP OAuth store resolution diagnostics")?; + write_atomically(&path, &serialized).with_context(|| { + format!( + "failed to write MCP OAuth store resolution diagnostics at {}", + path.display() + ) + })?; + + // Explicit modes and keyring-backend changes are intentional authority changes, so they reset + // the comparison baseline. Only repeated Auto resolution under one backend indicates the + // availability drift this diagnostic is meant to surface. + Ok(previous.and_then(|previous| { + (previous.store_mode == OAuthCredentialsStoreMode::Auto + && current.store_mode == OAuthCredentialsStoreMode::Auto + && previous.keyring_backend == current.keyring_backend + && previous.resolved_store != current.resolved_store) + .then_some(StoreResolutionChange { + previous: previous.resolved_store, + current: current.resolved_store, + }) + })) +} + +fn read_resolution_state(path: &Path) -> Result { + let contents = match fs::read_to_string(path) { + Ok(contents) => contents, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(ResolutionState::default()), + Err(error) => { + return Err(error).with_context(|| { + format!( + "failed to read MCP OAuth store resolution diagnostics at {}", + path.display() + ) + }); + } + }; + serde_json::from_str(&contents).with_context(|| { + format!( + "failed to parse MCP OAuth store resolution diagnostics at {}", + path.display() + ) + }) +} + +#[cfg(test)] +#[path = "resolution_state_tests.rs"] +mod tests; diff --git a/codex-rs/rmcp-client/src/oauth/resolution_state_tests.rs b/codex-rs/rmcp-client/src/oauth/resolution_state_tests.rs new file mode 100644 index 000000000000..876747777e99 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth/resolution_state_tests.rs @@ -0,0 +1,131 @@ +use codex_config::types::AuthKeyringBackendKind; +use codex_config::types::OAuthCredentialsStoreMode; +use pretty_assertions::assert_eq; + +use super::ObservedStore; +use super::ResolutionState; +use super::StoreResolution; +use super::StoreResolutionChange; +use super::record_store_resolution_in; +use crate::oauth::ResolvedOAuthCredentialStore; +use crate::oauth::compute_store_key; + +#[test] +fn auto_resolution_change_is_reported_and_persisted() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let server_name = "test-server"; + let url = "https://example.test/mcp"; + + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Direct, + ResolvedOAuthCredentialStore::Keyring(AuthKeyringBackendKind::Direct), + )?, + None + ); + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Direct, + ResolvedOAuthCredentialStore::File, + )?, + Some(StoreResolutionChange { + previous: ObservedStore::Keyring, + current: ObservedStore::File, + }) + ); + + let state: ResolutionState = serde_json::from_str(&std::fs::read_to_string( + codex_home.path().join(super::RESOLUTION_STATE_FILENAME), + )?)?; + assert_eq!( + state, + ResolutionState { + version: super::RESOLUTION_STATE_VERSION, + resolutions: [( + compute_store_key(server_name, url)?, + StoreResolution { + store_mode: OAuthCredentialsStoreMode::Auto, + keyring_backend: AuthKeyringBackendKind::Direct, + resolved_store: ObservedStore::File, + }, + )] + .into(), + } + ); + Ok(()) +} + +#[test] +fn intentional_configuration_changes_reset_the_auto_comparison() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let server_name = "test-server"; + let url = "https://example.test/mcp"; + + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Direct, + ResolvedOAuthCredentialStore::Keyring(AuthKeyringBackendKind::Direct), + )?, + None + ); + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::File, + AuthKeyringBackendKind::Direct, + ResolvedOAuthCredentialStore::File, + )?, + None + ); + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Direct, + ResolvedOAuthCredentialStore::File, + )?, + None + ); + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Secrets, + ResolvedOAuthCredentialStore::Keyring(AuthKeyringBackendKind::Secrets), + )?, + None + ); + assert_eq!( + record_store_resolution_in( + codex_home.path(), + server_name, + url, + OAuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Secrets, + ResolvedOAuthCredentialStore::File, + )?, + Some(StoreResolutionChange { + previous: ObservedStore::Keyring, + current: ObservedStore::File, + }) + ); + Ok(()) +} diff --git a/codex-rs/rmcp-client/src/oauth/resolved_store.rs b/codex-rs/rmcp-client/src/oauth/resolved_store.rs index 937fa51e6df2..66fc5ff8a635 100644 --- a/codex-rs/rmcp-client/src/oauth/resolved_store.rs +++ b/codex-rs/rmcp-client/src/oauth/resolved_store.rs @@ -11,15 +11,17 @@ use super::OAuthKeyringLoadError; use super::StoredOAuthTokens; use super::load_oauth_tokens_from_file; use super::load_oauth_tokens_from_keyring; +use super::resolution_state::StoreResolutionReason; +use super::resolution_state::record_store_resolution; use super::save_oauth_tokens_to_file; use super::save_oauth_tokens_with_keyring; /// Concrete credential store resolved for one MCP OAuth client lifecycle. /// -/// This is intentionally not durable. `Auto` may resolve differently in a later process, but a -/// client that loaded credentials from one store must reread, refresh, persist, and remove only -/// through that store. A mid-lifecycle backend failure is unexpected and must return an error -/// rather than falling back to another possibly stale refresh token. +/// This selection is intentionally not persisted as credential authority. `Auto` may resolve +/// differently in a later process, but a client that loaded credentials from one store must +/// reread, refresh, persist, and remove only through that store. A separate best-effort diagnostic +/// record warns when later Auto resolution changes; it never influences this selection. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum ResolvedOAuthCredentialStore { File, @@ -85,67 +87,98 @@ pub(crate) fn resolve_oauth_tokens_from_store_policy Result> { - match store_mode { + let (resolved, reason) = match store_mode { OAuthCredentialsStoreMode::Auto => { // Auto remains keyring-first at lifecycle startup. The returned source is then pinned // by the client transport recipe and OAuth persistor so retries, recovery, and // refresh work cannot hot-switch stores. - // TODO(stevenlee): Different processes can still resolve Auto to different stores - // when keyring availability differs. Solving that safely requires durable backend - // selection or reconciliation of legacy entries and is intentionally outside this - // stack. + // Different processes can still resolve Auto differently when keyring availability + // changes. The token-free observation below makes that drift visible without turning + // the sidecar into another source of credential authority. match load_oauth_tokens_from_keyring( keyring_store, keyring_backend_kind, server_name, url, ) { - Ok(Some(tokens)) => Ok(Some(ResolvedOAuthTokens { - tokens, - store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), - })), - Ok(None) => Ok( + Ok(Some(tokens)) => ( + Some(ResolvedOAuthTokens { + tokens, + store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), + }), + StoreResolutionReason::AutoLoadFromKeyring, + ), + Ok(None) => ( load_oauth_tokens_from_file(server_name, url)?.map(|tokens| { ResolvedOAuthTokens { tokens, store: ResolvedOAuthCredentialStore::File, } }), + StoreResolutionReason::AutoLoadFromFileAfterMissingKeyring, ), // Auto may fall back when the keyring backend is unavailable, but a Secrets // aggregate-lock failure means authority may be changing. Consulting File in // that state could replay credentials hidden behind a newer Secrets entry. - Err(OAuthKeyringLoadError::StoreLock(error)) => Err(error.into()), + Err(OAuthKeyringLoadError::StoreLock(error)) => { + return Err(error.into()); + } Err(error) => { warn!("failed to read OAuth tokens from keyring: {error}"); - Ok(load_oauth_tokens_from_file(server_name, url) - .with_context(|| { - format!("failed to read OAuth tokens from keyring: {error}") - })? - .map(|tokens| ResolvedOAuthTokens { - tokens, - store: ResolvedOAuthCredentialStore::File, - })) + ( + load_oauth_tokens_from_file(server_name, url) + .with_context(|| { + format!("failed to read OAuth tokens from keyring: {error}") + })? + .map(|tokens| ResolvedOAuthTokens { + tokens, + store: ResolvedOAuthCredentialStore::File, + }), + StoreResolutionReason::AutoLoadFromFileAfterKeyringError, + ) } } } - OAuthCredentialsStoreMode::File => Ok(load_oauth_tokens_from_file(server_name, url)?.map( - |tokens| ResolvedOAuthTokens { + OAuthCredentialsStoreMode::File => ( + load_oauth_tokens_from_file(server_name, url)?.map(|tokens| ResolvedOAuthTokens { tokens, store: ResolvedOAuthCredentialStore::File, - }, - )), - OAuthCredentialsStoreMode::Keyring => Ok(load_oauth_tokens_from_keyring( - keyring_store, - keyring_backend_kind, + }), + StoreResolutionReason::ConfiguredLoad, + ), + OAuthCredentialsStoreMode::Keyring => ( + load_oauth_tokens_from_keyring(keyring_store, keyring_backend_kind, server_name, url) + .map_err(anyhow::Error::from) + .context("failed to read OAuth tokens from keyring")? + .map(|tokens| ResolvedOAuthTokens { + tokens, + store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), + }), + StoreResolutionReason::ConfiguredLoad, + ), + }; + + // Explicit modes select their store even when no credential exists. Auto has no concrete + // selection until one store returns credentials or login successfully persists them. + let resolved_store = resolved + .as_ref() + .map(|resolved| resolved.store) + .or(match store_mode { + OAuthCredentialsStoreMode::Auto => None, + OAuthCredentialsStoreMode::File => Some(ResolvedOAuthCredentialStore::File), + OAuthCredentialsStoreMode::Keyring => { + Some(ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind)) + } + }); + if let Some(resolved_store) = resolved_store { + record_store_resolution( server_name, url, - ) - .map_err(anyhow::Error::from) - .context("failed to read OAuth tokens from keyring")? - .map(|tokens| ResolvedOAuthTokens { - tokens, - store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), - })), + store_mode, + keyring_backend_kind, + resolved_store, + reason, + ); } + Ok(resolved) } diff --git a/codex-rs/rmcp-client/src/oauth/store_lock.rs b/codex-rs/rmcp-client/src/oauth/store_lock.rs index b053f4e5ae25..4f4505176273 100644 --- a/codex-rs/rmcp-client/src/oauth/store_lock.rs +++ b/codex-rs/rmcp-client/src/oauth/store_lock.rs @@ -1,8 +1,8 @@ -//! Cross-process serialization for MCP OAuth stores shared by multiple credentials. +//! Cross-process serialization for MCP OAuth state shared by multiple credentials. //! -//! File and Secrets each keep credentials for multiple MCP servers in one aggregate document. -//! Their lock therefore protects the complete read-modify-write operation. Direct keyring entries -//! are already stored independently per credential and do not use this lock. +//! File, Secrets, and the token-free resolution diagnostic each keep entries for multiple MCP +//! servers in one aggregate document. Their locks protect complete read-modify-write operations. +//! Direct keyring entries are already independent per credential and do not use this lock. use std::fs; use std::fs::File; @@ -25,6 +25,7 @@ const LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock pub(super) enum OAuthStore { File, Secrets, + ResolutionState, } impl OAuthStore { @@ -32,6 +33,7 @@ impl OAuthStore { match self { Self::File => "file-store.lock", Self::Secrets => "secrets-store.lock", + Self::ResolutionState => "resolution-state.lock", } } } @@ -41,6 +43,7 @@ impl std::fmt::Display for OAuthStore { match self { Self::File => f.write_str("fallback file"), Self::Secrets => f.write_str("encrypted secrets"), + Self::ResolutionState => f.write_str("store resolution diagnostics"), } } }