mirror of
https://github.com/openai/codex.git
synced 2026-10-21 05:22:21 +00:00
Speed up MCP OAuth credential reads (#37860)
## Why Refreshing MCP connection identities should not stall the async executor while another process holds the OAuth credential-store lock. ## What changed - Probe file and secrets credential stores without waiting during runtime refreshes, retaining the last known credentials when the store is contended. - Reuse unchanged authentication failures while credentials are temporarily unavailable instead of restarting the connection. - Run blocking credential loads on a blocking worker during MCP client construction. - Allow OAuth fallback when a configured authorization header is missing, empty, or invalid. GitOrigin-RevId: 38dad5606da9a1ceb30fe945000ec9151a5058fa
This commit is contained in:
@@ -64,6 +64,7 @@ use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::McpStartupCompleteEvent;
|
||||
use codex_protocol::protocol::McpStartupFailure;
|
||||
use codex_protocol::protocol::McpStartupFailureReason;
|
||||
use codex_protocol::protocol::McpStartupStatus;
|
||||
use codex_protocol::protocol::McpStartupUpdateEvent;
|
||||
use codex_rmcp_client::determine_streamable_http_auth_status_from_credentials;
|
||||
@@ -98,6 +99,16 @@ impl McpServerConnection {
|
||||
if client.client.is_closed().await {
|
||||
return None;
|
||||
}
|
||||
if current == desired {
|
||||
if matches!(desired.oauth_credentials(), Ok(None))
|
||||
&& tokio::time::timeout(Duration::ZERO, client.client.managed_oauth_credentials())
|
||||
.await
|
||||
.is_ok_and(|credentials| matches!(credentials, Some(Some(_))))
|
||||
{
|
||||
return None;
|
||||
}
|
||||
return Some(client);
|
||||
}
|
||||
let Ok(desired_credentials) = desired.oauth_credentials() else {
|
||||
return Some(client);
|
||||
};
|
||||
@@ -167,6 +178,7 @@ impl McpServerView {
|
||||
/// A published view over a set of running MCP server connections.
|
||||
pub(crate) struct McpConnectionSet {
|
||||
servers: HashMap<String, McpServerView>,
|
||||
protocol_mode: crate::McpProtocolMode,
|
||||
required_servers: Vec<String>,
|
||||
optional_startup_deadline: OnceLock<tokio::time::Instant>,
|
||||
tool_catalog_revision: Arc<RwLock<u64>>,
|
||||
@@ -336,6 +348,9 @@ impl McpConnectionSet {
|
||||
.then(|| (codex_home.clone(), codex_apps_tools_cache_key.clone())),
|
||||
client_elicitation_capability.clone(),
|
||||
client_mcp_extensions.clone(),
|
||||
previous
|
||||
.and_then(|previous| previous.servers.get(&server_name))
|
||||
.and_then(|view| view.connection.identity.as_ref()),
|
||||
);
|
||||
let expected_protocol_mode = match &configured_config.transport {
|
||||
McpServerTransportConfig::StreamableHttp { .. } => Some(protocol_mode),
|
||||
@@ -361,14 +376,31 @@ impl McpConnectionSet {
|
||||
reusable_previous.and_then(|previous| previous.servers.get(&server_name))
|
||||
{
|
||||
let connection = Arc::clone(&previous_view.connection);
|
||||
if connection
|
||||
.reusable_client(&connection_identity)
|
||||
.await
|
||||
.is_some_and(|client| {
|
||||
previous_view.catalog_item_limit == catalog_item_limit
|
||||
&& expected_protocol_mode
|
||||
.is_some_and(|expected| client.client.protocol_mode() == expected)
|
||||
})
|
||||
let unchanged_auth_failure = if connection.identity.as_ref()
|
||||
== Some(&connection_identity)
|
||||
&& connection_identity.oauth_store_was_contended
|
||||
&& reusable_previous
|
||||
.is_some_and(|previous| previous.protocol_mode == protocol_mode)
|
||||
&& connection.client.startup_complete.load(Ordering::Acquire)
|
||||
{
|
||||
connection
|
||||
.client()
|
||||
.await
|
||||
.err()
|
||||
.filter(StartupOutcomeError::is_authentication_required)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
if unchanged_auth_failure.is_some()
|
||||
|| connection
|
||||
.reusable_client(&connection_identity)
|
||||
.await
|
||||
.is_some_and(|client| {
|
||||
previous_view.catalog_item_limit == catalog_item_limit
|
||||
&& expected_protocol_mode.is_some_and(|expected| {
|
||||
client.client.protocol_mode() == expected
|
||||
})
|
||||
})
|
||||
{
|
||||
servers.insert(
|
||||
server_name.clone(),
|
||||
@@ -380,7 +412,44 @@ impl McpConnectionSet {
|
||||
catalog_item_limit,
|
||||
},
|
||||
);
|
||||
reused_ready.push(server_name);
|
||||
if let Some(error) = unchanged_auth_failure {
|
||||
let status = McpStartupStatus::Failed {
|
||||
error: mcp_init_error_display(
|
||||
&server_name,
|
||||
Some(&configured_config),
|
||||
&error,
|
||||
),
|
||||
reason: connection_identity
|
||||
.oauth_credentials()
|
||||
.ok()
|
||||
.flatten()
|
||||
.map(|_| McpStartupFailureReason::ReauthenticationRequired),
|
||||
};
|
||||
let tx_event = tx_event.clone();
|
||||
let submit_id = startup_submit_id.clone();
|
||||
let publication_gate = publication_gate.clone();
|
||||
join_set.spawn(async move {
|
||||
if !publication_gate.wait().await {
|
||||
return (server_name, Err(StartupOutcomeError::Cancelled));
|
||||
}
|
||||
if let Some(tx_event) = tx_event.as_ref() {
|
||||
for status in [McpStartupStatus::Starting, status] {
|
||||
let _ = emit_update(
|
||||
submit_id.as_str(),
|
||||
tx_event,
|
||||
McpStartupUpdateEvent {
|
||||
server: server_name.clone(),
|
||||
status,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
(server_name, Err(error))
|
||||
});
|
||||
} else {
|
||||
reused_ready.push(server_name);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
}
|
||||
@@ -567,6 +636,7 @@ impl McpConnectionSet {
|
||||
}
|
||||
let manager = Self {
|
||||
servers,
|
||||
protocol_mode,
|
||||
required_servers,
|
||||
optional_startup_deadline: OnceLock::new(),
|
||||
tool_catalog_revision: Arc::new(RwLock::new(0)),
|
||||
@@ -625,6 +695,7 @@ impl McpConnectionSet {
|
||||
pub fn empty(prefix_mcp_tool_names: bool) -> Self {
|
||||
Self {
|
||||
servers: HashMap::new(),
|
||||
protocol_mode: crate::McpProtocolMode::Legacy,
|
||||
required_servers: Vec::new(),
|
||||
optional_startup_deadline: OnceLock::new(),
|
||||
tool_catalog_revision: Arc::new(RwLock::new(0)),
|
||||
|
||||
@@ -88,6 +88,7 @@ impl McpConnectionSet {
|
||||
) -> Self {
|
||||
Self {
|
||||
servers: HashMap::new(),
|
||||
protocol_mode: crate::McpProtocolMode::Legacy,
|
||||
required_servers: Vec::new(),
|
||||
optional_startup_deadline: OnceLock::new(),
|
||||
tool_catalog_revision: Arc::new(RwLock::new(0)),
|
||||
@@ -3034,6 +3035,7 @@ async fn executor_owned_chatgpt_mcp_accepts_only_safe_explicit_authorization() -
|
||||
/*codex_apps_cache_identity*/ None,
|
||||
ElicitationCapability::default(),
|
||||
ClientMcpExtensions::default(),
|
||||
/*previous_identity*/ None,
|
||||
)
|
||||
};
|
||||
let direct_keyring_identity = connection_identity(AuthKeyringBackendKind::Direct);
|
||||
@@ -3486,6 +3488,7 @@ fn reusable_server_identity(
|
||||
/*codex_apps_cache_identity*/ None,
|
||||
ElicitationCapability::default(),
|
||||
ClientMcpExtensions::default(),
|
||||
/*previous_identity*/ None,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -4021,6 +4024,7 @@ async fn connection_identity_distinguishes_accounts_with_the_same_token() -> any
|
||||
/*codex_apps_cache_identity*/ None,
|
||||
ElicitationCapability::default(),
|
||||
ClientMcpExtensions::default(),
|
||||
/*previous_identity*/ None,
|
||||
)
|
||||
};
|
||||
|
||||
@@ -4072,6 +4076,7 @@ async fn connection_identity_distinguishes_agent_account_runtime_and_task() -> a
|
||||
/*codex_apps_cache_identity*/ None,
|
||||
ElicitationCapability::default(),
|
||||
ClientMcpExtensions::default(),
|
||||
/*previous_identity*/ None,
|
||||
)
|
||||
};
|
||||
let previous_identity = connection_identity(&previous_auth);
|
||||
|
||||
@@ -17,8 +17,6 @@ use codex_login::CodexAuth;
|
||||
use codex_protocol::mcp::ClientMcpExtensions;
|
||||
use codex_rmcp_client::StoredOAuthCredentialSnapshot;
|
||||
use codex_rmcp_client::StoredOAuthTokens;
|
||||
use codex_rmcp_client::stored_oauth_credential_snapshot;
|
||||
use codex_rmcp_client::stored_oauth_credentials;
|
||||
use rmcp::model::ElicitationCapability;
|
||||
use tracing::warn;
|
||||
|
||||
@@ -99,6 +97,7 @@ pub(crate) struct McpServerConnectionIdentity {
|
||||
environment_id: String,
|
||||
oauth_store: Option<(OAuthCredentialsStoreMode, AuthKeyringBackendKind)>,
|
||||
oauth_credentials: Result<Option<StoredOAuthCredentialSnapshot>, String>,
|
||||
pub(crate) oauth_store_was_contended: bool,
|
||||
resolved_environment: Result<Option<Arc<Environment>>, String>,
|
||||
local_stdio_fallback_cwd: Option<PathBuf>,
|
||||
referenced_environment_variables: Vec<(String, Option<OsString>)>,
|
||||
@@ -124,8 +123,14 @@ impl McpServerConnectionIdentity {
|
||||
codex_apps_cache_identity: Option<(PathBuf, ConnectorRuntimeContextKey)>,
|
||||
client_elicitation_capability: ElicitationCapability,
|
||||
client_mcp_extensions: ClientMcpExtensions,
|
||||
previous_identity: Option<&Self>,
|
||||
) -> Self {
|
||||
let config = server.config();
|
||||
let valid_http_header_value = |value: &str| {
|
||||
value
|
||||
.bytes()
|
||||
.all(|byte| byte == b'\t' || (byte >= b' ' && byte != 0x7f))
|
||||
};
|
||||
let stored_oauth_url = if runtime_auth_provider.is_none()
|
||||
&& (!matches!(config.auth, McpServerAuth::ChatGpt) || config.is_local_environment())
|
||||
{
|
||||
@@ -133,20 +138,44 @@ impl McpServerConnectionIdentity {
|
||||
McpServerTransportConfig::StreamableHttp {
|
||||
url,
|
||||
bearer_token_env_var: None,
|
||||
..
|
||||
} => Some(url),
|
||||
McpServerTransportConfig::StreamableHttp {
|
||||
bearer_token_env_var: Some(_),
|
||||
..
|
||||
http_headers,
|
||||
env_http_headers,
|
||||
} if !http_headers.as_ref().is_some_and(|headers| {
|
||||
headers.iter().any(|(name, value)| {
|
||||
name.eq_ignore_ascii_case("authorization") && valid_http_header_value(value)
|
||||
})
|
||||
}) && !env_http_headers.as_ref().is_some_and(|headers| {
|
||||
headers.iter().any(|(name, env_var)| {
|
||||
name.eq_ignore_ascii_case("authorization")
|
||||
&& std::env::var(env_var).is_ok_and(|value| {
|
||||
!value.trim().is_empty() && valid_http_header_value(&value)
|
||||
})
|
||||
})
|
||||
}) =>
|
||||
{
|
||||
Some(url)
|
||||
}
|
||||
McpServerTransportConfig::StreamableHttp { .. }
|
||||
| McpServerTransportConfig::Stdio { .. } => None,
|
||||
}
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let oauth_credentials = stored_oauth_url.map_or(Ok(None), |url| {
|
||||
stored_oauth_credential_snapshot(
|
||||
config.oauth_credential_name(server_name).as_ref(),
|
||||
let credential_name = config.oauth_credential_name(server_name);
|
||||
StoredOAuthCredentialSnapshot::for_runtime_refresh(
|
||||
previous_identity.and_then(|previous_identity| {
|
||||
previous_identity
|
||||
.oauth_credentials
|
||||
.as_ref()
|
||||
.ok()
|
||||
.and_then(Option::as_ref)
|
||||
.filter(|_| {
|
||||
previous_identity.oauth_store
|
||||
== Some((store_mode, keyring_backend_kind))
|
||||
})
|
||||
}),
|
||||
credential_name.as_ref(),
|
||||
url,
|
||||
store_mode,
|
||||
keyring_backend_kind,
|
||||
@@ -165,6 +194,11 @@ impl McpServerConnectionIdentity {
|
||||
let referenced_environment_variables = referenced_environment_variables(config);
|
||||
let runtime_auth = runtime_auth_provider.and(auth).cloned();
|
||||
let runtime_auth_token = runtime_auth.as_ref().and_then(|auth| auth.get_token().ok());
|
||||
let oauth_store_was_contended = oauth_credentials
|
||||
.as_ref()
|
||||
.ok()
|
||||
.and_then(Option::as_ref)
|
||||
.is_some_and(StoredOAuthCredentialSnapshot::store_was_contended);
|
||||
|
||||
Self {
|
||||
transport: config.transport.clone(),
|
||||
@@ -173,6 +207,7 @@ impl McpServerConnectionIdentity {
|
||||
.is_some()
|
||||
.then_some((store_mode, keyring_backend_kind)),
|
||||
oauth_credentials,
|
||||
oauth_store_was_contended,
|
||||
resolved_environment: resolved_environment.clone(),
|
||||
local_stdio_fallback_cwd,
|
||||
referenced_environment_variables,
|
||||
@@ -241,12 +276,14 @@ impl McpServerConnectionIdentity {
|
||||
store_mode,
|
||||
keyring_backend_kind,
|
||||
),
|
||||
Ok(None) | Err(_) => stored_oauth_credentials(
|
||||
Ok(None) | Err(_) => StoredOAuthCredentialSnapshot::for_runtime_refresh(
|
||||
/*previous*/ None,
|
||||
credential_name.as_ref(),
|
||||
url,
|
||||
store_mode,
|
||||
keyring_backend_kind,
|
||||
),
|
||||
)
|
||||
.map(|snapshot| snapshot.map(|snapshot| snapshot.credentials().clone())),
|
||||
};
|
||||
|
||||
match current_credentials {
|
||||
|
||||
@@ -72,6 +72,7 @@ use codex_utils_home_dir::find_codex_home;
|
||||
pub(crate) use self::resolved_store::ResolvedOAuthCredentialStore;
|
||||
pub(crate) use self::resolved_store::ResolvedOAuthTokens;
|
||||
pub(crate) use self::resolved_store::resolve_oauth_tokens_from_store_policy;
|
||||
use self::resolved_store::try_resolve_oauth_tokens_from_store_policy;
|
||||
|
||||
const KEYRING_SERVICE: &str = "Codex MCP Credentials";
|
||||
const MCP_OAUTH_SECRET_PREFIX: &str = "MCP_OAUTH";
|
||||
@@ -88,10 +89,17 @@ pub struct StoredOAuthTokens {
|
||||
}
|
||||
|
||||
/// OAuth credentials paired with the concrete store selected for their client lifecycle.
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct StoredOAuthCredentialSnapshot {
|
||||
credentials: StoredOAuthTokens,
|
||||
store: ResolvedOAuthCredentialStore,
|
||||
store_was_contended: bool,
|
||||
}
|
||||
|
||||
impl PartialEq for StoredOAuthCredentialSnapshot {
|
||||
fn eq(&self, other: &Self) -> bool {
|
||||
self.credentials == other.credentials && self.store == other.store
|
||||
}
|
||||
}
|
||||
|
||||
impl StoredOAuthCredentialSnapshot {
|
||||
@@ -100,7 +108,49 @@ impl StoredOAuthCredentialSnapshot {
|
||||
&self.credentials
|
||||
}
|
||||
|
||||
/// Rereads the selected authority, allowing Auto login to migrate File credentials to keyring.
|
||||
/// Returns whether this snapshot was retained because its store could not be read.
|
||||
pub fn store_was_contended(&self) -> bool {
|
||||
self.store_was_contended
|
||||
}
|
||||
|
||||
/// Refreshes a runtime snapshot without waiting or discarding its last known authority.
|
||||
pub fn for_runtime_refresh(
|
||||
previous: Option<&Self>,
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
store_mode: OAuthCredentialsStoreMode,
|
||||
keyring_backend_kind: AuthKeyringBackendKind,
|
||||
) -> Result<Option<Self>> {
|
||||
match try_resolve_oauth_tokens_from_store_policy(
|
||||
&DefaultKeyringStore,
|
||||
server_name,
|
||||
url,
|
||||
store_mode,
|
||||
keyring_backend_kind,
|
||||
) {
|
||||
Ok(Some(mut resolved)) => {
|
||||
resolved.tokens.token_response.0.set_expires_in(None);
|
||||
Ok(Some(Self {
|
||||
credentials: resolved.tokens,
|
||||
store: resolved.store,
|
||||
store_was_contended: false,
|
||||
}))
|
||||
}
|
||||
Ok(None) => Ok(None),
|
||||
Err(error) if oauth_store_is_contended(&error) => Ok(previous
|
||||
.filter(|previous| {
|
||||
previous.credentials.server_name == server_name
|
||||
&& previous.credentials.url == url
|
||||
})
|
||||
.map(|previous| Self {
|
||||
store_was_contended: true,
|
||||
..previous.clone()
|
||||
})),
|
||||
Err(error) => Err(error),
|
||||
}
|
||||
}
|
||||
|
||||
/// Rereads the selected authority without waiting for a contended credential-store lock.
|
||||
pub fn reload(
|
||||
&self,
|
||||
server_name: &str,
|
||||
@@ -111,10 +161,21 @@ impl StoredOAuthCredentialSnapshot {
|
||||
if self.store == ResolvedOAuthCredentialStore::File
|
||||
&& store_mode == OAuthCredentialsStoreMode::Auto
|
||||
{
|
||||
return stored_oauth_credentials(server_name, url, store_mode, keyring_backend_kind);
|
||||
return Self::for_runtime_refresh(
|
||||
/*previous*/ None,
|
||||
server_name,
|
||||
url,
|
||||
store_mode,
|
||||
keyring_backend_kind,
|
||||
)
|
||||
.map(|snapshot| snapshot.map(|snapshot| snapshot.credentials));
|
||||
}
|
||||
|
||||
let credentials = self.store.load(&DefaultKeyringStore, server_name, url)?;
|
||||
let credentials = match self.store.try_load(&DefaultKeyringStore, server_name, url) {
|
||||
Ok(credentials) => credentials,
|
||||
Err(error) if oauth_store_is_contended(&error) => return Ok(None),
|
||||
Err(error) => return Err(error),
|
||||
};
|
||||
Ok(normalized_oauth_credentials(credentials.as_ref()))
|
||||
}
|
||||
}
|
||||
@@ -194,9 +255,18 @@ pub fn stored_oauth_credential_snapshot(
|
||||
Ok(Some(StoredOAuthCredentialSnapshot {
|
||||
credentials,
|
||||
store: resolved.store,
|
||||
store_was_contended: false,
|
||||
}))
|
||||
}
|
||||
|
||||
fn oauth_store_is_contended(error: &Error) -> bool {
|
||||
matches!(
|
||||
error.downcast_ref::<OAuthStoreLockFailure>(),
|
||||
Some(OAuthStoreLockFailure::Timeout { acquire_timeout, .. })
|
||||
if acquire_timeout.is_zero()
|
||||
)
|
||||
}
|
||||
|
||||
fn normalized_oauth_credentials(tokens: Option<&StoredOAuthTokens>) -> Option<StoredOAuthTokens> {
|
||||
tokens.map(|tokens| {
|
||||
let mut tokens = tokens.clone();
|
||||
@@ -283,6 +353,14 @@ fn load_oauth_tokens_from_secrets_keyring<K: KeyringStore + Clone + 'static>(
|
||||
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,
|
||||
) -> 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(
|
||||
codex_home.to_path_buf(),
|
||||
@@ -682,6 +760,13 @@ struct FallbackTokenEntry {
|
||||
|
||||
fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result<Option<StoredOAuthTokens>> {
|
||||
let _store_lock = OAuthStoreLock::acquire_for_read(OAuthStore::File)?;
|
||||
load_oauth_tokens_from_file_with_lock_held(server_name, url)
|
||||
}
|
||||
|
||||
fn load_oauth_tokens_from_file_with_lock_held(
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
) -> Result<Option<StoredOAuthTokens>> {
|
||||
let Some(store) = read_fallback_file_unlocked()? else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
@@ -8,13 +8,18 @@ use codex_keyring_store::KeyringStore;
|
||||
use tracing::warn;
|
||||
|
||||
use super::OAuthKeyringLoadError;
|
||||
use super::OAuthStore;
|
||||
use super::OAuthStoreLock;
|
||||
use super::OAuthStoreLockFailure;
|
||||
use super::StoredOAuthTokens;
|
||||
use super::compute_store_key;
|
||||
use super::delete_oauth_tokens_from_direct_keyring;
|
||||
use super::delete_oauth_tokens_from_file;
|
||||
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::save_oauth_tokens_to_file;
|
||||
use super::save_oauth_tokens_with_keyring;
|
||||
|
||||
@@ -57,6 +62,34 @@ impl ResolvedOAuthCredentialStore {
|
||||
}
|
||||
}
|
||||
|
||||
/// Reads the selected authority without waiting for its aggregate-store lock.
|
||||
pub(crate) fn try_load<K: KeyringStore + Clone + 'static>(
|
||||
self,
|
||||
keyring_store: &K,
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
) -> Result<Option<StoredOAuthTokens>> {
|
||||
match self {
|
||||
Self::File => {
|
||||
let _store_lock = OAuthStoreLock::try_acquire_for_read(OAuthStore::File)?;
|
||||
load_oauth_tokens_from_file_with_lock_held(server_name, url)
|
||||
.context("failed to probe OAuth tokens from resolved file storage")
|
||||
}
|
||||
Self::Keyring(AuthKeyringBackendKind::Direct) => {
|
||||
self.load(keyring_store, server_name, url)
|
||||
}
|
||||
Self::Keyring(AuthKeyringBackendKind::Secrets) => {
|
||||
let _store_lock = OAuthStoreLock::try_acquire_for_read(OAuthStore::Secrets)?;
|
||||
load_oauth_tokens_from_secrets_keyring_with_lock_held(
|
||||
keyring_store,
|
||||
server_name,
|
||||
url,
|
||||
)
|
||||
.map_err(anyhow::Error::from)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Saves credentials only to this already-resolved authority.
|
||||
pub(crate) fn save<K: KeyringStore + Clone + 'static>(
|
||||
self,
|
||||
@@ -174,3 +207,32 @@ pub(crate) fn resolve_oauth_tokens_from_store_policy<K: KeyringStore + Clone + '
|
||||
})),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn try_resolve_oauth_tokens_from_store_policy<K: KeyringStore + Clone + 'static>(
|
||||
keyring_store: &K,
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
store_mode: OAuthCredentialsStoreMode,
|
||||
keyring_backend_kind: AuthKeyringBackendKind,
|
||||
) -> Result<Option<ResolvedOAuthTokens>> {
|
||||
let load = |store: ResolvedOAuthCredentialStore| {
|
||||
store
|
||||
.try_load(keyring_store, server_name, url)
|
||||
.map(|tokens| tokens.map(|tokens| ResolvedOAuthTokens { tokens, store }))
|
||||
};
|
||||
let keyring = ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind);
|
||||
match store_mode {
|
||||
OAuthCredentialsStoreMode::File => load(ResolvedOAuthCredentialStore::File),
|
||||
OAuthCredentialsStoreMode::Keyring => load(keyring),
|
||||
OAuthCredentialsStoreMode::Auto => match load(keyring) {
|
||||
Ok(Some(tokens)) => Ok(Some(tokens)),
|
||||
Ok(None) => load(ResolvedOAuthCredentialStore::File),
|
||||
Err(error) if error.downcast_ref::<OAuthStoreLockFailure>().is_some() => Err(error),
|
||||
Err(error) => {
|
||||
warn!("failed to read OAuth tokens from keyring: {error}");
|
||||
load(ResolvedOAuthCredentialStore::File)
|
||||
.with_context(|| format!("failed to read OAuth tokens from keyring: {error}"))
|
||||
}
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -59,15 +59,28 @@ enum OAuthStoreLockMode {
|
||||
|
||||
impl OAuthStoreLock {
|
||||
pub(super) fn acquire_for_write(store: OAuthStore) -> Result<Self, OAuthStoreLockFailure> {
|
||||
Self::acquire_with_mode(store, OAuthStoreLockMode::Exclusive)
|
||||
Self::acquire_with_timeout(
|
||||
store,
|
||||
STORE_LOCK_ACQUIRE_TIMEOUT,
|
||||
OAuthStoreLockMode::Exclusive,
|
||||
)
|
||||
}
|
||||
|
||||
pub(super) fn acquire_for_read(store: OAuthStore) -> Result<Self, OAuthStoreLockFailure> {
|
||||
Self::acquire_with_mode(store, OAuthStoreLockMode::Shared)
|
||||
Self::acquire_with_timeout(
|
||||
store,
|
||||
STORE_LOCK_ACQUIRE_TIMEOUT,
|
||||
OAuthStoreLockMode::Shared,
|
||||
)
|
||||
}
|
||||
|
||||
fn acquire_with_mode(
|
||||
pub(super) fn try_acquire_for_read(store: OAuthStore) -> Result<Self, OAuthStoreLockFailure> {
|
||||
Self::acquire_with_timeout(store, Duration::ZERO, OAuthStoreLockMode::Shared)
|
||||
}
|
||||
|
||||
fn acquire_with_timeout(
|
||||
store: OAuthStore,
|
||||
acquire_timeout: Duration,
|
||||
mode: OAuthStoreLockMode,
|
||||
) -> Result<Self, OAuthStoreLockFailure> {
|
||||
// This lock intentionally follows the existing local File/Secrets credential-store
|
||||
@@ -77,7 +90,7 @@ impl OAuthStoreLock {
|
||||
// provide its own matching lock authority instead of using this local path.
|
||||
let codex_home = find_codex_home()
|
||||
.map_err(|source| OAuthStoreLockFailure::CodexHome { store, source })?;
|
||||
Self::acquire_in_with_mode(&codex_home, store, STORE_LOCK_ACQUIRE_TIMEOUT, mode)
|
||||
Self::acquire_in_with_mode(&codex_home, store, acquire_timeout, mode)
|
||||
}
|
||||
|
||||
fn acquire_in_with_mode(
|
||||
|
||||
@@ -1025,19 +1025,27 @@ impl RmcpClient {
|
||||
&& auth_provider.is_none()
|
||||
&& !default_headers.contains_key(AUTHORIZATION)
|
||||
{
|
||||
if let Some(store) = pinned_credential_store.get().copied() {
|
||||
// Rebuilds reread the source selected during first construction. Only the
|
||||
// initial construction below evaluates configured store policy.
|
||||
store
|
||||
.load(&DefaultKeyringStore, server_name, url)?
|
||||
.map(|tokens| ResolvedOAuthTokens { tokens, store })
|
||||
} else {
|
||||
let oauth_server_name = server_name.clone();
|
||||
let oauth_url = url.clone();
|
||||
let oauth_store_mode = *store_mode;
|
||||
let oauth_keyring_backend_kind = *keyring_backend_kind;
|
||||
let pinned_credential_store = Arc::clone(pinned_credential_store);
|
||||
|
||||
tokio::task::spawn_blocking(move || -> Result<Option<ResolvedOAuthTokens>> {
|
||||
if let Some(store) = pinned_credential_store.get().copied() {
|
||||
// Rebuilds reread the source selected during first construction. Only
|
||||
// initial construction below evaluates configured store policy.
|
||||
return store
|
||||
.load(&DefaultKeyringStore, &oauth_server_name, &oauth_url)
|
||||
.map(|tokens| tokens.map(|tokens| ResolvedOAuthTokens { tokens, store }));
|
||||
}
|
||||
|
||||
match resolve_oauth_tokens_from_store_policy(
|
||||
&DefaultKeyringStore,
|
||||
server_name,
|
||||
url,
|
||||
*store_mode,
|
||||
*keyring_backend_kind,
|
||||
&oauth_server_name,
|
||||
&oauth_url,
|
||||
oauth_store_mode,
|
||||
oauth_keyring_backend_kind,
|
||||
) {
|
||||
Ok(tokens) => {
|
||||
if let Some(resolved) = tokens.as_ref() {
|
||||
@@ -1045,18 +1053,22 @@ impl RmcpClient {
|
||||
// first concrete source so Auto is not reevaluated mid-client.
|
||||
pinned_credential_store.set(resolved.store).map_err(|_| {
|
||||
anyhow!(
|
||||
"OAuth credential store pinned concurrently for MCP server `{server_name}`"
|
||||
"OAuth credential store pinned concurrently for MCP server `{oauth_server_name}`"
|
||||
)
|
||||
})?;
|
||||
}
|
||||
tokens
|
||||
Ok(tokens)
|
||||
}
|
||||
Err(err) => {
|
||||
warn!("failed to read tokens for server `{server_name}`: {err}");
|
||||
None
|
||||
warn!(
|
||||
"failed to read tokens for server `{oauth_server_name}`: {err}"
|
||||
);
|
||||
Ok(None)
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
.await
|
||||
.map_err(|error| anyhow!("OAuth credential loading task failed: {error}"))??
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user