diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 5553aca947..79f5bf0ce9 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -230,6 +230,8 @@ impl McpRequestProcessor { credential_name: &credential_name, issuer: &idp.issuer, client_id: &idp.client_id, + client_secret: idp.client_secret.as_ref().map(|secret| secret.as_str()), + token_endpoint_auth_method: idp.token_endpoint_auth_method, keyring_backend_kind: mcp_config.auth_keyring_backend_kind, callback_port: mcp_config.mcp_oauth_callback_port, callback_url: mcp_config.mcp_oauth_callback_url.as_deref(), diff --git a/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs b/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs index 5a1ba15d78..3881be3e72 100644 --- a/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs +++ b/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs @@ -252,7 +252,7 @@ async fn enterprise_login_attempt_lifecycle() -> Result<()> { "issuer": format!("{origin}/as"), "token_endpoint": format!("{origin}/as/token"), "grant_types_supported": ["urn:ietf:params:oauth:grant-type:jwt-bearer"], "authorization_grant_profiles_supported": ["urn:ietf:params:oauth:grant-profile:id-jag"], - "token_endpoint_auth_methods_supported": ["none"], + "token_endpoint_auth_methods_supported": ["none", "client_secret_basic", "client_secret_post"], }))) .mount(&server) .await; @@ -283,11 +283,11 @@ async fn enterprise_login_attempt_lifecycle() -> Result<()> { .respond_with(move |request: &wiremock::Request| { let fields = url::form_urlencoded::parse(&request.body).into_owned().collect::>(); // EMA section 5: the resource AS gets the ID-JAG, never the IdP credential. - assert!(request.headers.get("authorization").is_none()); + let expected = format!("Basic {}", base64::engine::general_purpose::STANDARD.encode("mcp-client:resource-secret")); + assert_eq!(request.headers.get("authorization").unwrap().to_str().unwrap(), expected); assert_eq!(serde_json::to_value(fields).unwrap(), json!({ "grant_type": "urn:ietf:params:oauth:grant-type:jwt-bearer", "assertion": expected_jag, - "client_id": "mcp-client", })); if let Some((path, auth)) = account_refresh.lock().unwrap().pop_front() { // Another process refreshes the account while the resource exchange runs. @@ -322,9 +322,10 @@ async fn enterprise_login_attempt_lifecycle() -> Result<()> { "grant-type%3Atoken-exchange", )) .respond_with(move |request: &wiremock::Request| { - let fields = url::form_urlencoded::parse(&request.body) + let mut fields = url::form_urlencoded::parse(&request.body) .into_owned() .collect::>(); + fields.remove("client_secret"); let subject = fields.get("subject_token").map(String::as_str); assert!(matches!( subject, @@ -379,7 +380,7 @@ async fn enterprise_login_attempt_lifecycle() -> Result<()> { "issuer": issuer, "authorization_endpoint": format!("{issuer}/authorize"), "token_endpoint": format!("{issuer}/token"), - "token_endpoint_auth_methods_supported": ["none"], + "token_endpoint_auth_methods_supported": ["none", "client_secret_basic", "client_secret_post"], }); Mock::given(method("GET")) .and(path("/.well-known/oauth-authorization-server/idp")) @@ -427,12 +428,14 @@ analytics = {{ enabled = false }}"# r#"[mcp_enterprise_managed_auth.idp] issuer = "{issuer}" client_id = "idp-client" +client_secret = "idp-secret" +token_endpoint_auth_method = "client_secret_post" [mcp_servers.enterprise] url = "{resource}" auth = "ema_auth" scopes = ["files.read"] -oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" }} +oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_endpoint_auth_method = "client_secret_basic", authorization_server_issuer = "{origin}/as" }} [mcp_servers.ordinary] url = "{resource}" @@ -442,7 +445,7 @@ http_headers = {{ Authorization = "Bearer ordinary-access" }} url = "{origin}/mcp" enabled = false auth = "ema_auth" -oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" }}"# +oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_endpoint_auth_method = "client_secret_basic", authorization_server_issuer = "{origin}/as" }}"# )) .write(home.path())?; write_chatgpt_auth( @@ -544,6 +547,8 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" ); let idp = McpServerIdpOAuthConfig { issuer: issuer.clone(), + client_secret: None, + token_endpoint_auth_method: None, client_id: "idp-client".into(), }; let initial_scope = McpEmaAuthScope::new("enterprise-user".into(), WORKSPACE_ID_INITIAL.into()) @@ -574,6 +579,16 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" .plan_type("business"), AuthCredentialsStoreMode::File, )?; + // Same issuer, different client: the dedicated confidential grant must win. + let auth_path = home.path().join("auth.json"); + let mut account_auth: Value = serde_json::from_slice(&std::fs::read(&auth_path)?)?; + account_auth["tokens"]["oauth_provenance"] = + json!({"issuer":issuer,"client_id":"account-client"}); + account_auth["tokens"]["id_token"] = json!(format!("{}.{}.signature", + URL_SAFE_NO_PAD.encode(r#"{"alg":"ES256"}"#), + URL_SAFE_NO_PAD.encode(json!({"iss":issuer,"aud":"account-client","sub":"enterprise-user","exp":now + 3600, + "https://api.openai.com/auth":{"chatgpt_user_id":"enterprise-user","chatgpt_account_id":WORKSPACE_ID_REFRESHED,"chatgpt_plan_type":"business"}}).to_string()))); + std::fs::write(&auth_path, serde_json::to_vec(&account_auth)?)?; let browser = HttpClientBuilder::new() .without_request_logging() .without_redirects() @@ -777,7 +792,7 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" statuses.iter().find(|status| status["name"] == "ordinary"), ); } - std::fs::write(config_path, saved_config)?; + std::fs::write(&config_path, &saved_config)?; let handshakes = server.received_requests().await.context("MCP handshakes")?; let enterprise_discovery = handshakes .iter() @@ -847,6 +862,19 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" 3 ); + // Login and both renewals used the dedicated IdP registration, even though + // the primary account belongs to a different client at the same issuer. + assert!( + idp_server + .received_requests() + .await + .context("IdP requests")? + .iter() + .filter(|request| request.url.path() == "/idp/token") + .all(|request| url::form_urlencoded::parse(&request.body) + .any(|(name, value)| name == "client_secret" && value == "idp-secret")) + ); + // Replacing the enterprise grant fences old threads, including cached bearers. let relogin = enterprise_rpc( &client, @@ -1001,6 +1029,16 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" json!({"name":"enterprise","threadId":thread_id,"loginId":recovery["loginId"],"success":true}) ); + // Switch the IdP registration to public for the account-grant recovery checks. + // The earlier dedicated login, startup, and renewal used separate IdP/AS secrets. + let mut config: toml::Value = toml::from_str(&saved_config)?; + let idp_config = config["mcp_enterprise_managed_auth"]["idp"] + .as_table_mut() + .context("IdP config")?; + idp_config.remove("client_secret"); + idp_config.remove("token_endpoint_auth_method"); + std::fs::write(&config_path, toml::to_string(&config)?)?; + // An eligible account login uses its own OAuth client, without a second sign-in. let auth_path = home.path().join("auth.json"); let mut account_auth: Value = serde_json::from_slice(&std::fs::read(&auth_path)?)?; @@ -1307,10 +1345,11 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" ); } + // Use the thread admitted after switching the IdP registration to public. let stalled = enterprise_rpc( &client, "mcpServer/oauth/login", - json!({"name":"enterprise","threadId":thread_id}), + json!({"name":"enterprise","threadId":recovered["thread"]["id"]}), ) .await?; let mut unfinished = tokio::net::TcpStream::connect(("127.0.0.1", callback_port)).await?; @@ -1364,7 +1403,7 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" .await??; assert_eq!( login_completion.context("enterprise login completion")?, - json!({"name":"enterprise","threadId":thread_id,"loginId":stalled["loginId"],"success":false,"error":"Enterprise sign-in failed."}) + json!({"name":"enterprise","threadId":recovered["thread"]["id"],"loginId":stalled["loginId"],"success":false,"error":"Enterprise sign-in failed."}) ); assert_eq!( resource_exchanges.load(std::sync::atomic::Ordering::SeqCst), @@ -1729,6 +1768,8 @@ oauth = {{ client_id = "mcp-client", authorization_server_issuer = "{origin}/as" ); let idp = McpServerIdpOAuthConfig { issuer: issuer.clone(), + client_secret: None, + token_endpoint_auth_method: None, client_id: "idp-client".into(), }; let scope = diff --git a/codex-rs/codex-mcp/src/catalog_tests.rs b/codex-rs/codex-mcp/src/catalog_tests.rs index eaf50e79b8..14dcedc750 100644 --- a/codex-rs/codex-mcp/src/catalog_tests.rs +++ b/codex-rs/codex-mcp/src/catalog_tests.rs @@ -226,6 +226,8 @@ fn plugin_host_root_is_retained_in_catalog_identity() { fn ema_policy_survives_rebuilds_and_rebinds_materialized_servers() { let idp = McpServerIdpOAuthConfig { issuer: "https://idp.example".to_string(), + client_secret: None, + token_endpoint_auth_method: None, client_id: "enterprise-client".to_string(), }; let mut builder = ResolvedMcpCatalog::builder(); diff --git a/codex-rs/codex-mcp/src/ema_account.rs b/codex-rs/codex-mcp/src/ema_account.rs index 9abd5b1d07..3227e7f4c6 100644 --- a/codex-rs/codex-mcp/src/ema_account.rs +++ b/codex-rs/codex-mcp/src/ema_account.rs @@ -1,5 +1,5 @@ //! Account-grant eligibility and registration-specific sign-in requirements. -//! The MCP service owns this state across threads; it retains no credentials. +//! The MCP service owns this state across threads; it retains registration policy but no user tokens. use std::sync::Mutex; @@ -66,6 +66,9 @@ impl EmaState { pub(crate) fn account_grant_is_usable(tokens: &TokenData, idp: &McpServerIdpOAuthConfig) -> bool { tokens.oauth_provenance.as_ref().is_some_and(|provenance| { provenance.issuer == idp.issuer + // A provisioned secret belongs to the configured client, not another + // primary login client at the same IdP. Use dedicated login in that case. + && (idp.client_secret.is_none() || provenance.client_id == idp.client_id) && tokens .login_owner_id .as_ref() diff --git a/codex-rs/codex-mcp/src/ema_token_source.rs b/codex-rs/codex-mcp/src/ema_token_source.rs index d9ef85eb1c..37fc6ec47e 100644 --- a/codex-rs/codex-mcp/src/ema_token_source.rs +++ b/codex-rs/codex-mcp/src/ema_token_source.rs @@ -181,7 +181,7 @@ impl EnterpriseTokenSource { let tokens = grant.tokens(); // Metadata discovery must not block ordinary account refresh. let identity = resolve_ema_account_idp_identity( - &idp.issuer, + idp, client_id, &tokens.id_token.raw_jwt, tokens.refresh_token.clone(), @@ -218,6 +218,8 @@ impl EnterpriseTokenSource { resolve_ema_idp_identity(EmaIdpIdentityRequest { issuer: &idp.issuer, client_id, + client_secret: idp.client_secret.as_ref().map(|secret| secret.as_str()), + token_endpoint_auth_method: idp.token_endpoint_auth_method, credentials: snapshot.acquire_current_ema_credentials().await?, http_client: Arc::clone(&self.http_client), redirect_mode: StreamableHttpRedirectMode::Legacy, @@ -233,6 +235,8 @@ impl EnterpriseTokenSource { resource: self.registration.resource(), scopes: self.registration.scopes(), mcp_client_id: self.registration.client_id(), + mcp_client_secret: self.registration.client_secret(), + token_endpoint_auth_method: self.registration.token_endpoint_auth_method(), expected_authorization_server_issuer: self .registration .authorization_server_issuer(), diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index b464628d10..acd97b9e4a 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -186,6 +186,8 @@ fn ema_catalog_allows_only_configured_servers_without_widening_policy() { let mut config = test_mcp_config(PathBuf::new()); let idp = codex_config::McpServerIdpOAuthConfig { issuer: "https://idp.example".into(), + client_secret: None, + token_endpoint_auth_method: None, client_id: "enterprise-client".into(), }; let deny_all = codex_protocol::mcp_policy::EnvironmentMcpPolicy { diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 30e7cf132a..9b0ac2f2de 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -180,6 +180,7 @@ pub use marketplace_edit::remove_user_marketplace_config; pub use mcp_edit::load_global_mcp_servers; pub use mcp_ema::McpEmaAuthScope; pub use mcp_ema::McpEmaRegistration; +pub use mcp_ema::McpEmaTokenEndpointAuthMethod; pub use mcp_ema::McpEnterpriseManagedAuthConfig; pub use mcp_ema::McpServerIdpOAuthConfig; pub use mcp_types::AppToolApproval; diff --git a/codex-rs/config/src/mcp_ema.rs b/codex-rs/config/src/mcp_ema.rs index da354b23ed..f53ab45c7c 100644 --- a/codex-rs/config/src/mcp_ema.rs +++ b/codex-rs/config/src/mcp_ema.rs @@ -14,19 +14,46 @@ use sha2::Digest; use sha2::Sha256; use codex_features::Feature; +use codex_utils_redacted_string::RedactedString; use crate::ConfigLayerSource; use crate::ConfigLayerStack; use crate::McpServerConfig; use crate::McpServerTransportConfig; +/// Token endpoint authentication selected by a confidential EMA client registration. +#[derive(Serialize, Deserialize, Debug, Clone, Copy, Default, PartialEq, Eq, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum McpEmaTokenEndpointAuthMethod { + #[default] + ClientSecretBasic, + ClientSecretPost, +} + +impl McpEmaTokenEndpointAuthMethod { + pub fn as_str(self) -> &'static str { + match self { + Self::ClientSecretBasic => "client_secret_basic", + Self::ClientSecretPost => "client_secret_post", + } + } +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] #[serde(deny_unknown_fields)] pub struct McpServerIdpOAuthConfig { /// Issuer used for enterprise OAuth discovery and identity validation. pub issuer: String, - /// Public OAuth client registered with that enterprise IdP. + /// OAuth client registered with that enterprise IdP. pub client_id: String, + /// Provisioned client secret for confidential IdP login and token exchange. + /// Omitted for a public client. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub client_secret: Option, + /// Registered EMA client authentication method. Defaults to client_secret_basic + /// when a secret is configured; requires a client secret. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub token_endpoint_auth_method: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] @@ -44,6 +71,8 @@ pub struct McpEmaRegistration { server_url: String, resource: Option, client_id: Option, + client_secret: Option, + token_endpoint_auth_method: Option, authorization_server_issuer: Option, scopes: Vec, } @@ -65,6 +94,14 @@ impl McpEmaRegistration { self.client_id.as_deref() } + pub fn client_secret(&self) -> Option<&str> { + self.client_secret.as_ref().map(|secret| secret.as_str()) + } + + pub fn token_endpoint_auth_method(&self) -> Option { + self.token_endpoint_auth_method + } + pub fn authorization_server_issuer(&self) -> Option<&str> { self.authorization_server_issuer.as_deref() } @@ -118,7 +155,29 @@ impl McpEnterpriseManagedAuthConfig { ) -> io::Result> { validate_ema_auth_sources(stack, servers)?; validate_xaa_opt_in_source(stack, xaa_enabled)?; - Self::from_config_layers(stack, fallback) + let profile = Self::from_config_layers(stack, fallback)?; + if let Some(profile) = &profile { + if profile + .idp + .client_secret + .as_ref() + .is_some_and(|secret| secret.trim().is_empty()) + { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "enterprise IdP client_secret must not be empty", + )); + } + if profile.idp.token_endpoint_auth_method.is_some() + && profile.idp.client_secret.is_none() + { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "enterprise IdP token_endpoint_auth_method requires client_secret", + )); + } + } + Ok(profile) } /// Selects one complete trusted registration rather than merging issuer/client pairs. @@ -196,6 +255,8 @@ impl McpServerConfig { server_url: url.clone(), resource: self.oauth_resource.clone(), client_id: oauth.client_id.clone(), + client_secret: oauth.client_secret.clone(), + token_endpoint_auth_method: oauth.token_endpoint_auth_method, authorization_server_issuer: oauth.authorization_server_issuer.clone(), scopes: self.scopes.clone().unwrap_or_default(), }); @@ -207,8 +268,18 @@ impl McpServerConfig { if !self.is_local_environment() { return Err("ema_auth requires a host-owned MCP connection"); } - if self.oauth_client_secret().is_some() { - return Err("ema_auth cannot be combined with oauth.client_secret"); + if let Some(secret) = self.oauth_client_secret() { + if secret.trim().is_empty() { + return Err("ema_auth oauth.client_secret must not be empty"); + } + if self.oauth.as_ref().is_none_or(|oauth| { + oauth + .authorization_server_issuer + .as_ref() + .is_none_or(|issuer| issuer.trim().is_empty()) + }) { + return Err("confidential ema_auth requires oauth.authorization_server_issuer"); + } } let McpServerTransportConfig::StreamableHttp { bearer_token_env_var, diff --git a/codex-rs/config/src/mcp_ema_tests.rs b/codex-rs/config/src/mcp_ema_tests.rs index e196db41b5..257acfe614 100644 --- a/codex-rs/config/src/mcp_ema_tests.rs +++ b/codex-rs/config/src/mcp_ema_tests.rs @@ -57,6 +57,8 @@ fn ema_profiles_and_auth_modes_preserve_non_project_authority() { let expected = McpEnterpriseManagedAuthConfig { idp: McpServerIdpOAuthConfig { issuer: "https://idp.example".into(), + client_secret: None, + token_endpoint_auth_method: None, client_id: "enterprise".into(), }, }; @@ -225,6 +227,8 @@ fn ema_credential_names_bind_user_workspace_issuer_and_client() { let scope = McpEmaAuthScope::new(user.into(), workspace.into()).unwrap(); let idp = McpServerIdpOAuthConfig { issuer: issuer.into(), + client_secret: None, + token_endpoint_auth_method: None, client_id: client.into(), }; assert!(names.insert(idp.credential_name(&scope))); @@ -243,6 +247,8 @@ fn ema_credential_names_bind_user_workspace_issuer_and_client() { let scope = McpEmaAuthScope::new(identifiers[0].into(), identifiers[1].into()).unwrap(); let name = McpServerIdpOAuthConfig { issuer: identifiers[2].into(), + client_secret: None, + token_endpoint_auth_method: None, client_id: identifiers[3].into(), } .credential_name(&scope); @@ -274,3 +280,48 @@ fn ema_rejects_alternate_credentials_and_executor_custody() { toml::from_str("url='https://resource.example'\nauth='ema_auth'\nhttp_headers={}").unwrap(); assert_eq!(server.validate_ema_auth_transport(), Ok(())); } + +#[test] +fn confidential_ema_registration_binds_secrets_to_trusted_issuers() { + let (system, _, project) = local_sources("ema-confidential"); + let trusted = "[mcp_enterprise_managed_auth.idp]\nissuer='https://idp.example'\nclient_id='idp'\nclient_secret='idp-secret'\ntoken_endpoint_auth_method='client_secret_post'\n[mcp_servers.enterprise]\nurl='https://resource.example'\nauth='ema_auth'\noauth={client_id='client', client_secret='resource-secret', token_endpoint_auth_method='client_secret_basic', authorization_server_issuer='https://as.example'}"; + assert!(valid_ema_layers(vec![(system.clone(), trusted)])); + let layers = stack(vec![ + (system.clone(), trusted), + ( + project.clone(), + "[mcp_enterprise_managed_auth.idp]\nclient_secret='project-secret'", + ), + ]); + let profile = + McpEnterpriseManagedAuthConfig::from_config_layers(&layers, /*fallback*/ None) + .unwrap() + .unwrap(); + assert_eq!(profile.idp.client_secret, Some("idp-secret".into())); + + for invalid in [ + trusted.replace("idp-secret", " "), + trusted.replace("client_secret='idp-secret'\n", ""), + ] { + assert!( + McpEnterpriseManagedAuthConfig::resolve( + &stack(vec![(system.clone(), &invalid)]), + /*fallback*/ None, + &Default::default(), + /*xaa_enabled*/ false, + ) + .is_err() + ); + } + + for override_config in [ + "[mcp_servers.enterprise.oauth]\nauthorization_server_issuer='https://attacker.example'", + "[mcp_servers.enterprise.oauth]\nclient_secret='project-secret'", + "[mcp_servers.enterprise.oauth]\ntoken_endpoint_auth_method='client_secret_post'", + ] { + assert!(!valid_ema_layers(vec![ + (system.clone(), trusted), + (project.clone(), override_config) + ])); + } +} diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index c85e7c99b8..b6243ab801 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -18,6 +18,7 @@ use serde::Serialize; use serde::de::Error as SerdeError; use crate::McpEmaRegistration; +use crate::McpEmaTokenEndpointAuthMethod; use crate::RequirementSource; /// Effective MCP environment id when config omits `environment_id`. @@ -164,6 +165,11 @@ pub struct McpServerOAuthConfig { #[serde(default, skip_serializing_if = "Option::is_none")] pub client_secret: Option, + /// Registered EMA client authentication method. Defaults to client_secret_basic + /// when a secret is configured; requires a client secret. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub token_endpoint_auth_method: Option, + /// Registered callback URL associated with this OAuth client. #[serde(default, skip_serializing_if = "Option::is_none")] pub callback_url: Option, @@ -560,6 +566,20 @@ impl TryFrom for McpServerConfig { return Err("oauth.client_secret requires oauth.client_id".to_string()); } } + if let Some(oauth) = &oauth + && oauth.token_endpoint_auth_method.is_some() + { + if !matches!(auth, McpServerAuth::EmaAuth) { + return Err( + "oauth.token_endpoint_auth_method requires auth = \"ema_auth\"".to_string(), + ); + } + if oauth.client_secret.is_none() { + return Err( + "oauth.token_endpoint_auth_method requires oauth.client_secret".to_string(), + ); + } + } if !matches!(auth, McpServerAuth::EmaAuth) && oauth .as_ref() diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 83be6f9f06..3a86639981 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2492,6 +2492,14 @@ }, "type": "object" }, + "McpEmaTokenEndpointAuthMethod": { + "description": "Token endpoint authentication selected by a confidential EMA client registration.", + "enum": [ + "client_secret_basic", + "client_secret_post" + ], + "type": "string" + }, "McpEnterpriseManagedAuthConfig": { "additionalProperties": false, "properties": { @@ -2561,12 +2569,24 @@ "additionalProperties": false, "properties": { "client_id": { - "description": "Public OAuth client registered with that enterprise IdP.", + "description": "OAuth client registered with that enterprise IdP.", + "type": "string" + }, + "client_secret": { + "description": "Provisioned client secret for confidential IdP login and token exchange. Omitted for a public client.", "type": "string" }, "issuer": { "description": "Issuer used for enterprise OAuth discovery and identity validation.", "type": "string" + }, + "token_endpoint_auth_method": { + "allOf": [ + { + "$ref": "#/definitions/McpEmaTokenEndpointAuthMethod" + } + ], + "description": "Registered EMA client authentication method. Defaults to client_secret_basic when a secret is configured; requires a client secret." } }, "required": [ @@ -2600,6 +2620,14 @@ "client_secret": { "description": "OAuth client secret used for token exchange with a pre-registered client.", "type": "string" + }, + "token_endpoint_auth_method": { + "allOf": [ + { + "$ref": "#/definitions/McpEmaTokenEndpointAuthMethod" + } + ], + "description": "Registered EMA client authentication method. Defaults to client_secret_basic when a secret is configured; requires a client secret." } }, "type": "object" diff --git a/codex-rs/core/src/config/edit/document_helpers.rs b/codex-rs/core/src/config/edit/document_helpers.rs index 2fcf6203e4..d05db955b8 100644 --- a/codex-rs/core/src/config/edit/document_helpers.rs +++ b/codex-rs/core/src/config/edit/document_helpers.rs @@ -169,6 +169,9 @@ fn serialize_mcp_server_table(config: &McpServerConfig) -> anyhow::Result { pub resource: Option<&'a str>, pub scopes: &'a [String], pub mcp_client_id: Option<&'a str>, + pub mcp_client_secret: Option<&'a str>, + pub token_endpoint_auth_method: Option, pub expected_authorization_server_issuer: Option<&'a str>, pub idp_issuer: &'a str, pub idp_client_id: &'a str, @@ -79,8 +83,19 @@ pub async fn exchange_ema_auth_token( "ema_auth requires the OAuth client ID registered with the MCP authorization server" ) })?; - let (metadata, resource) = - discover_ema_authorization_metadata(EmaDiscoveryRequest::from(&request)).await?; + if request.mcp_client_secret.is_some() + && request + .expected_authorization_server_issuer + .is_none_or(|issuer| issuer.trim().is_empty()) + { + bail!("confidential ema_auth requires a configured authorization server issuer"); + } + let (metadata, resource, resource_client_authentication) = discover_ema_authorization_metadata( + EmaDiscoveryRequest::from(&request), + request.mcp_client_secret, + request.token_endpoint_auth_method, + ) + .await?; let issuer = metadata .issuer .as_deref() @@ -89,6 +104,7 @@ pub async fn exchange_ema_auth_token( token_endpoint, refresh_token, credentials, + client_authentication: idp_client_authentication, } = request.idp_identity.await?; exchange_id_jag( EmaIdJagExchangeRequest { @@ -100,6 +116,8 @@ pub async fn exchange_ema_auth_token( idp_token_endpoint: &token_endpoint, idp_issuer: request.idp_issuer, idp_client_id: request.idp_client_id, + idp_client_authentication, + resource_client_authentication, refresh_token, idp_http_client: request.idp_http_client, resource_http_client: request.resource_http_client, @@ -112,7 +130,9 @@ pub async fn exchange_ema_auth_token( async fn discover_ema_authorization_metadata( request: EmaDiscoveryRequest<'_>, -) -> Result<(EmaAuthorizationMetadata, String)> { + client_secret: Option<&str>, + token_endpoint_auth_method: Option, +) -> Result<(EmaAuthorizationMetadata, String, EmaClientAuthentication)> { let expected_issuer = request.expected_issuer; let EmaResource { server_url: server, @@ -187,20 +207,22 @@ async fn discover_ema_authorization_metadata( if profile == Some(false) || grant != Some(true) { break; } - match validate_ema_public_client_auth( + match resolve_ema_client_auth( metadata .additional_fields .get("token_endpoint_auth_methods_supported"), + client_secret, + token_endpoint_auth_method, "MCP authorization server", ) { - Ok(()) => return Ok((metadata, resource)), + Ok(authentication) => return Ok((metadata, resource, authentication)), Err(error) if expected_issuer.is_some() => return Err(error), Err(_) => break, } } } bail!( - "enterprise MCP discovery found no authorization server advertising public-client JWT bearer support for ID-JAG" + "enterprise MCP discovery found no authorization server supporting the configured client authentication and ID-JAG JWT bearer grant" ) } diff --git a/codex-rs/rmcp-client/src/ema_auth_policy.rs b/codex-rs/rmcp-client/src/ema_auth_policy.rs index fad58e81aa..71fe5240b1 100644 --- a/codex-rs/rmcp-client/src/ema_auth_policy.rs +++ b/codex-rs/rmcp-client/src/ema_auth_policy.rs @@ -4,6 +4,7 @@ use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use anyhow::bail; +use codex_config::McpEmaTokenEndpointAuthMethod; use serde_json::Value; use url::Host; use url::Url; @@ -163,10 +164,48 @@ pub(crate) fn ema_reauthentication_required(message: &'static str) -> anyhow::Er anyhow::Error::new(EmaAuthFailure::ReauthenticationRequired).context(message) } -pub(crate) fn validate_ema_public_client_auth( +pub(crate) fn resolve_ema_client_auth( advertised_methods: Option<&Value>, + client_secret: Option<&str>, + token_endpoint_auth_method: Option, issuer_description: &str, -) -> Result<()> { +) -> Result { + use oauth2::ClientSecret; + use rmcp::transport::auth::enterprise::EmaClientAuthentication; + + if let Some(secret) = client_secret { + if secret.trim().is_empty() { + bail!("{issuer_description} client secret must not be empty"); + } + let registered_method = token_endpoint_auth_method.unwrap_or_default(); + // RFC 8414 defaults absent server capabilities to Basic. Capabilities + // validate the registration; they do not select its authentication method. + let supported = match advertised_methods { + None => registered_method == McpEmaTokenEndpointAuthMethod::ClientSecretBasic, + Some(methods) => methods.as_array().is_some_and(|methods| { + methods + .iter() + .any(|method| method.as_str() == Some(registered_method.as_str())) + }), + }; + if !supported { + bail!( + "{issuer_description} does not support the registered token endpoint authentication method" + ); + } + let secret = ClientSecret::new(secret.to_owned()); + return Ok(match registered_method { + McpEmaTokenEndpointAuthMethod::ClientSecretBasic => { + EmaClientAuthentication::ClientSecretBasic(secret) + } + McpEmaTokenEndpointAuthMethod::ClientSecretPost => { + EmaClientAuthentication::ClientSecretPost(secret) + } + }); + } + if token_endpoint_auth_method.is_some() { + bail!("{issuer_description} token endpoint authentication method requires a client secret"); + } let advertised_methods = advertised_methods.ok_or_else(|| { anyhow!( "{issuer_description} does not explicitly advertise public-client token endpoint authentication" @@ -178,7 +217,7 @@ pub(crate) fn validate_ema_public_client_auth( if !methods.iter().any(|method| method.as_str() == Some("none")) { bail!("{issuer_description} does not support public-client token endpoint authentication"); } - Ok(()) + Ok(EmaClientAuthentication::None) } pub(crate) fn validate_ema_oauth_endpoint(endpoint: &str, description: &str) -> Result<()> { diff --git a/codex-rs/rmcp-client/src/ema_auth_policy_tests.rs b/codex-rs/rmcp-client/src/ema_auth_policy_tests.rs index 3bdaed367d..de707d5897 100644 --- a/codex-rs/rmcp-client/src/ema_auth_policy_tests.rs +++ b/codex-rs/rmcp-client/src/ema_auth_policy_tests.rs @@ -44,7 +44,29 @@ fn public_clients_require_an_explicitly_advertised_auth_method() { (Some(json!("none")), false), ] { assert_eq!( - validate_ema_public_client_auth(advertised.as_ref(), "IdP").is_ok(), + resolve_ema_client_auth( + advertised.as_ref(), + /*client_secret*/ None, + /*token_endpoint_auth_method*/ None, + "IdP" + ) + .is_ok(), + valid + ); + } + use McpEmaTokenEndpointAuthMethod::ClientSecretPost; + for (advertised, registered, valid) in [ + (None, None, true), + (None, Some(ClientSecretPost), false), + ( + Some(json!(["client_secret_basic"])), + Some(ClientSecretPost), + false, + ), + (Some(json!(["client_secret_post"])), None, false), + ] { + assert_eq!( + resolve_ema_client_auth(advertised.as_ref(), Some("secret"), registered, "IdP").is_ok(), valid ); } diff --git a/codex-rs/rmcp-client/src/ema_auth_tests.rs b/codex-rs/rmcp-client/src/ema_auth_tests.rs index 740f290e3c..ceeaefe39d 100644 --- a/codex-rs/rmcp-client/src/ema_auth_tests.rs +++ b/codex-rs/rmcp-client/src/ema_auth_tests.rs @@ -31,6 +31,8 @@ fn request<'a>(resource: &'a str, issuer: Option<&'a str>) -> EmaAuthTokenExchan OutboundProxyPolicy::ReqwestDefault, ))); EmaAuthTokenExchangeRequest { + mcp_client_secret: None, + token_endpoint_auth_method: None, server_url: resource, resource: Some(resource), scopes: &[], @@ -208,8 +210,12 @@ async fn modern_challenge_confines_metadata_redirects() -> Result<()> { .mount(&authorization) .await; let exchange = request(&resource, Some(&issuer)); - let (metadata, actual_resource) = - discover_ema_authorization_metadata(EmaDiscoveryRequest::from(&exchange)).await?; + let (metadata, actual_resource, _) = discover_ema_authorization_metadata( + EmaDiscoveryRequest::from(&exchange), + /*client_secret*/ None, + /*token_endpoint_auth_method*/ None, + ) + .await?; assert_eq!( (metadata.issuer, metadata.token_endpoint, actual_resource), ( @@ -227,10 +233,14 @@ async fn modern_challenge_confines_metadata_redirects() -> Result<()> { .with_priority(1) .mount(&authorization) .await; - let error = discover_ema_authorization_metadata(EmaDiscoveryRequest::from(&exchange)) - .await - .err() - .context("cross-origin redirect must fail")?; + let error = discover_ema_authorization_metadata( + EmaDiscoveryRequest::from(&exchange), + /*client_secret*/ None, + /*token_endpoint_auth_method*/ None, + ) + .await + .err() + .context("cross-origin redirect must fail")?; assert!(error.to_string().contains("cross-origin redirect")); assert!( attacker @@ -447,6 +457,7 @@ async fn rejected_grants_preserve_their_source_and_release_the_credential_lease( exchange.idp_issuer = &idp; exchange.idp_identity = Box::pin(async { Ok(EmaIdpIdentity { + client_authentication: EmaClientAuthentication::None, token_endpoint: format!("{idp}/token"), refresh_token: "old-grant".to_string(), credentials: crate::EmaCredentialLease { credentials }, diff --git a/codex-rs/rmcp-client/src/ema_exchange.rs b/codex-rs/rmcp-client/src/ema_exchange.rs index 25350f27c1..8f21e9f75c 100644 --- a/codex-rs/rmcp-client/src/ema_exchange.rs +++ b/codex-rs/rmcp-client/src/ema_exchange.rs @@ -7,6 +7,7 @@ use anyhow::Result; use codex_exec_server::HttpClient; use oauth2::RefreshToken; use rmcp::transport::auth::enterprise::EmaAuthorizationServer; +use rmcp::transport::auth::enterprise::EmaClientAuthentication; use rmcp::transport::auth::enterprise::EmaError; use rmcp::transport::auth::enterprise::EmaExchangeRequest; use rmcp::transport::auth::enterprise::EmaExchangeStage; @@ -52,6 +53,8 @@ pub(crate) struct EmaIdJagExchangeRequest<'a> { pub idp_token_endpoint: &'a str, pub idp_issuer: &'a str, pub idp_client_id: &'a str, + pub idp_client_authentication: EmaClientAuthentication, + pub resource_client_authentication: EmaClientAuthentication, pub refresh_token: String, pub idp_http_client: Arc, pub resource_http_client: Arc, @@ -78,19 +81,21 @@ pub(crate) async fn exchange_id_jag( StreamableHttpRedirectMode::Legacy, )?; let refresh_token = RefreshToken::new(request.refresh_token); - // Discovery has already verified both public-client registrations. The SDK owns + // Discovery has already verified each registration's authentication method. The SDK owns // token validation and the two exchanges; Codex retains routing and credential policy. let assertion = EmaExchangeRequest::new( EmaAuthorizationServer::new( request.idp_issuer, request.idp_token_endpoint, request.idp_client_id, - ), + ) + .with_client_authentication(request.idp_client_authentication), EmaAuthorizationServer::new( request.authorization_server_issuer, request.authorization_server_token_endpoint, request.mcp_client_id, - ), + ) + .with_client_authentication(request.resource_client_authentication), request.resource, &refresh_token, ) diff --git a/codex-rs/rmcp-client/src/ema_exchange_tests.rs b/codex-rs/rmcp-client/src/ema_exchange_tests.rs index 0b3a24c4f6..c7a27ae751 100644 --- a/codex-rs/rmcp-client/src/ema_exchange_tests.rs +++ b/codex-rs/rmcp-client/src/ema_exchange_tests.rs @@ -77,6 +77,8 @@ async fn exchange(server: &MockServer, refresh_token: &str) -> Result Result Result<()> { +async fn client_round_trip_preserves_signed_narrowing() -> Result<()> { let requested_scopes = ["files.read".to_string(), "files.write".to_string()]; let client = http_client(); - for (scopes, echo_scope, refresh_token) in [ - (requested_scopes.as_slice(), true, "opaque-refresh-token"), - (&[], true, "opaque-refresh-token"), - (&[], false, "opaque-refresh-token"), - (requested_scopes.as_slice(), true, ""), - (requested_scopes.as_slice(), true, " \t"), + use codex_config::McpEmaTokenEndpointAuthMethod::ClientSecretBasic; + use codex_config::McpEmaTokenEndpointAuthMethod::ClientSecretPost; + for (scopes, echo_scope, refresh_token, idp_method) in [ + ( + requested_scopes.as_slice(), + true, + "opaque-refresh-token", + None, + ), + (&[], true, "opaque-refresh-token", Some(ClientSecretBasic)), + (&[], false, "opaque-refresh-token", Some(ClientSecretPost)), + (requested_scopes.as_slice(), true, "", None), + (requested_scopes.as_slice(), true, " \t", None), ] { let server = MockServer::start().await; let issuer = format!("{}/idp", server.uri()); @@ -127,8 +136,28 @@ async fn public_client_round_trip_preserves_signed_narrowing() -> Result<()> { .await; } let expected_subject = refresh_token.to_string(); + let resource_method = idp_method.map(|method| match method { + ClientSecretBasic => ClientSecretPost, + ClientSecretPost => ClientSecretBasic, + }); + let authentication = |method: Option<_>, secret| { + crate::ema_auth_policy::resolve_ema_client_auth( + Some(&json!([ + "none", + "client_secret_basic", + "client_secret_post" + ])), + method.map(|_| secret), + method, + "test issuer", + ) + }; + let idp_secret = "idp:+ /%secret"; + let resource_secret = "resource:+ /%secret"; let result = exchange_id_jag( EmaIdJagExchangeRequest { + idp_client_authentication: authentication(idp_method, idp_secret)?, + resource_client_authentication: authentication(resource_method, resource_secret)?, resource: &resource, scopes, mcp_client_id: "mcp-client", @@ -169,15 +198,41 @@ async fn public_client_round_trip_preserves_signed_narrowing() -> Result<()> { ); let requests = server.received_requests().await.expect("requests"); assert_eq!(requests.len(), 2); - assert!( - requests - .iter() - .all(|request| request.headers.get("authorization").is_none()) - ); let mut forms = requests .iter() .map(|request| unique_form_fields(&request.body)) .collect::>(); + for (index, (method, client_id, secret)) in [ + (idp_method, "idp-client", idp_secret), + (resource_method, "mcp-client", resource_secret), + ] + .into_iter() + .enumerate() + { + let authorization = requests[index] + .headers + .get("authorization") + .map(|value| value.to_str().unwrap()); + let encoded = url::form_urlencoded::byte_serialize(secret.as_bytes()) + .collect::() + .replace("%20", "+"); + let basic = format!( + "Basic {}", + base64::engine::general_purpose::STANDARD.encode(format!("{client_id}:{encoded}")) + ); + assert_eq!( + authorization, + (method == Some(ClientSecretBasic)).then_some(basic.as_str()) + ); + assert_eq!( + forms[index].remove("client_secret").as_deref(), + (method == Some(ClientSecretPost)).then_some(secret) + ); + assert_eq!( + forms[index].remove("client_id").as_deref(), + (method != Some(ClientSecretBasic)).then_some(client_id) + ); + } assert_eq!( forms[0].remove("scope"), (!scopes.is_empty()).then(|| scopes.join(" ")) @@ -200,7 +255,6 @@ async fn public_client_round_trip_preserves_signed_narrowing() -> Result<()> { ), ("audience".to_string(), audience.clone()), ("resource".to_string(), resource.clone()), - ("client_id".to_string(), "idp-client".to_string()), ]) ); assert_eq!( @@ -211,7 +265,6 @@ async fn public_client_round_trip_preserves_signed_narrowing() -> Result<()> { "assertion".to_string(), jag["access_token"].as_str().expect("JAG").to_string() ), - ("client_id".to_string(), "mcp-client".to_string()), ]) ); } diff --git a/codex-rs/rmcp-client/src/ema_identity.rs b/codex-rs/rmcp-client/src/ema_identity.rs index 8bd7fdb51a..60332d9541 100644 --- a/codex-rs/rmcp-client/src/ema_identity.rs +++ b/codex-rs/rmcp-client/src/ema_identity.rs @@ -7,6 +7,7 @@ use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use anyhow::bail; +use codex_config::McpEmaTokenEndpointAuthMethod; use codex_exec_server::HttpClient; use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; @@ -19,8 +20,8 @@ use tracing::instrument::WithSubscriber; use crate::EmaCredentialLease; use crate::ema_auth_policy::advertised_capability; use crate::ema_auth_policy::ema_reauthentication_required; +use crate::ema_auth_policy::resolve_ema_client_auth; use crate::ema_auth_policy::validate_ema_oauth_endpoint; -use crate::ema_auth_policy::validate_ema_public_client_auth; use crate::ema_claims::ID_JAG_TOKEN_TYPE; use crate::ema_exchange::TOKEN_EXCHANGE_GRANT_TYPE; use crate::http_client_adapter::StreamableHttpRedirectMode; @@ -29,10 +30,13 @@ use crate::oauth::StoredOAuthTokens; use crate::oauth::stored_oidc_identity; use crate::oauth_http_client::OAuthHttpClientAdapter; use crate::utils::build_default_headers; +use rmcp::transport::auth::enterprise::EmaClientAuthentication; pub struct EmaIdpIdentityRequest<'a> { pub issuer: &'a str, pub client_id: &'a str, + pub client_secret: Option<&'a str>, + pub token_endpoint_auth_method: Option, pub credentials: EmaCredentialLease, pub http_client: Arc, pub redirect_mode: StreamableHttpRedirectMode, @@ -43,6 +47,7 @@ pub struct EmaIdpIdentity { pub(crate) token_endpoint: String, pub(crate) refresh_token: String, pub(crate) credentials: L, + pub(crate) client_authentication: EmaClientAuthentication, } impl EmaIdpIdentity { @@ -53,6 +58,7 @@ impl EmaIdpIdentity { token_endpoint: self.token_endpoint, refresh_token: self.refresh_token, credentials: map(self.credentials), + client_authentication: self.client_authentication, } } } @@ -64,7 +70,7 @@ impl EmaIdpIdentity { /// ID-token claims corroborate that record; this does not verify a JWT signature or /// authenticate an expired ID token. Only the refresh token is exchanged with the IdP. pub async fn resolve_ema_account_idp_identity( - issuer: &str, + idp: &codex_config::McpServerIdpOAuthConfig, client_id: &str, id_token: &str, refresh_token: String, @@ -77,12 +83,25 @@ pub async fn resolve_ema_account_idp_identity( "Codex account has no refresh token; sign in again", )); } + if idp.client_secret.is_some() && client_id != idp.client_id { + bail!("enterprise IdP client secret does not belong to the account OAuth client"); + } + let issuer = &idp.issuer; + let client_secret = idp.client_secret.as_ref().map(|secret| secret.as_str()); crate::ema_claims::oidc_identity(id_token, issuer, client_id)?; - let token_endpoint = resolve_ema_idp_token_endpoint(issuer, http_client, redirect_mode).await?; + let (token_endpoint, client_authentication) = resolve_ema_idp_token_endpoint( + issuer, + client_secret, + idp.token_endpoint_auth_method, + http_client, + redirect_mode, + ) + .await?; Ok(EmaIdpIdentity { token_endpoint, refresh_token, credentials, + client_authentication, }) } @@ -112,7 +131,13 @@ pub fn stored_ema_identity_is_usable( pub(crate) async fn resolve_ema_idp_authorization_manager( issuer: &str, http_client: Arc, -) -> Result<(AuthorizationManager, AuthorizationMetadata)> { + client_secret: Option<&str>, + token_endpoint_auth_method: Option, +) -> Result<( + AuthorizationManager, + AuthorizationMetadata, + EmaClientAuthentication, +)> { validate_ema_oauth_endpoint(issuer, "enterprise IdP issuer")?; let mut manager = AuthorizationManager::new_with_oauth_http_client(issuer, http_client) .await @@ -130,13 +155,16 @@ pub(crate) async fn resolve_ema_idp_authorization_manager( bail!("enterprise IdP authorization metadata issuer does not match configuration"); } validate_ema_oauth_endpoint(&metadata.token_endpoint, "enterprise IdP token endpoint")?; - validate_ema_public_client_auth( + + let authentication = resolve_ema_client_auth( metadata .additional_fields .get("token_endpoint_auth_methods_supported"), + client_secret, + token_endpoint_auth_method, "enterprise IdP", )?; - Ok((manager, metadata)) + Ok((manager, metadata, authentication)) } /// Resolve a stored refresh-token subject against the configured enterprise IdP metadata. @@ -161,9 +189,14 @@ async fn resolve_ema_idp_identity_in( bail!("stored enterprise IdP credentials do not match the configured issuer and client"); } stored_oidc_identity(credentials)?; - let token_endpoint = - resolve_ema_idp_token_endpoint(request.issuer, request.http_client, request.redirect_mode) - .await?; + let (token_endpoint, client_authentication) = resolve_ema_idp_token_endpoint( + request.issuer, + request.client_secret, + request.token_endpoint_auth_method, + request.http_client, + request.redirect_mode, + ) + .await?; let credentials = request.credentials; let credential_lock = RefreshCredentialLock::acquire_for_server( &credentials.credentials.credentials().server_name, @@ -193,6 +226,7 @@ async fn resolve_ema_idp_identity_in( token_endpoint, refresh_token: refresh_token.secret().to_string(), credentials, + client_authentication, }) }) .await @@ -201,9 +235,11 @@ async fn resolve_ema_idp_identity_in( async fn resolve_ema_idp_token_endpoint( issuer: &str, + client_secret: Option<&str>, + token_endpoint_auth_method: Option, http_client: Arc, redirect_mode: StreamableHttpRedirectMode, -) -> Result { +) -> Result<(String, EmaClientAuthentication)> { let client = OAuthHttpClientAdapter::new_with_redirect_mode( http_client, build_default_headers(/*http_headers*/ None, /*env_http_headers*/ None)?, @@ -211,7 +247,13 @@ async fn resolve_ema_idp_token_endpoint( /*has_configured_headers*/ false, redirect_mode, )?; - let (_, metadata) = resolve_ema_idp_authorization_manager(issuer, Arc::new(client)).await?; + let (_, metadata, authentication) = resolve_ema_idp_authorization_manager( + issuer, + Arc::new(client), + client_secret, + token_endpoint_auth_method, + ) + .await?; for (name, expected) in [ ( "identity_chaining_requested_token_types_supported", @@ -227,7 +269,7 @@ async fn resolve_ema_idp_token_endpoint( ); } } - Ok(metadata.token_endpoint) + Ok((metadata.token_endpoint, authentication)) } #[cfg(test)] diff --git a/codex-rs/rmcp-client/src/ema_identity_tests.rs b/codex-rs/rmcp-client/src/ema_identity_tests.rs index 21e57e2b5b..ddfc428748 100644 --- a/codex-rs/rmcp-client/src/ema_identity_tests.rs +++ b/codex-rs/rmcp-client/src/ema_identity_tests.rs @@ -66,6 +66,8 @@ fn request<'a>( credentials: &'a StoredOAuthCredentialSnapshot, ) -> EmaIdpIdentityRequest<'a> { EmaIdpIdentityRequest { + client_secret: None, + token_endpoint_auth_method: None, issuer, client_id: "idp-client", credentials: EmaCredentialLease { @@ -146,9 +148,15 @@ async fn enterprise_discovery_shares_idp_checks_but_keeps_login_endpoint_policy( /*has_configured_headers*/ false, StreamableHttpRedirectMode::Legacy, )?); - let shared = resolve_ema_idp_authorization_manager(&issuer, client.clone()).await; + let shared = resolve_ema_idp_authorization_manager( + &issuer, + client.clone(), + /*client_secret*/ None, + /*token_endpoint_auth_method*/ None, + ) + .await; let login = crate::enterprise_oauth_login::resolve_enterprise_authorization_manager( - &issuer, client, + &issuer, client, /*client_secret*/ None, /*token_endpoint_auth_method*/ None, ) .await; assert_eq!( diff --git a/codex-rs/rmcp-client/src/enterprise_oauth_login.rs b/codex-rs/rmcp-client/src/enterprise_oauth_login.rs index b4a5de157e..e74381cf93 100644 --- a/codex-rs/rmcp-client/src/enterprise_oauth_login.rs +++ b/codex-rs/rmcp-client/src/enterprise_oauth_login.rs @@ -9,6 +9,7 @@ use std::sync::Arc; use anyhow::Result; use anyhow::anyhow; use anyhow::bail; +use codex_config::McpEmaTokenEndpointAuthMethod; use codex_config::types::AuthKeyringBackendKind; use codex_config::types::OAuthCredentialsStoreMode; use codex_exec_server::HttpClient; @@ -225,6 +226,8 @@ pub async fn perform_enterprise_oauth_login_return_url( &["openid".to_string(), "offline_access".to_string()], Some(&codex_config::McpServerOAuthConfig { client_id: Some(request.client_id.to_string()), + client_secret: request.client_secret.map(Into::into), + token_endpoint_auth_method: request.token_endpoint_auth_method, ..Default::default() }), OAuthLoginPurpose::EnterpriseIdp, @@ -332,12 +335,24 @@ fn validate_enterprise_credentials(stored: &StoredOAuthTokens) -> Result<()> { pub(crate) async fn resolve_enterprise_authorization_manager( issuer: &str, http_client: Arc, + client_secret: Option<&str>, + token_endpoint_auth_method: Option, ) -> Result<(AuthorizationManager, AuthorizationMetadata)> { - let (manager, metadata) = resolve_ema_idp_authorization_manager( + let (manager, mut metadata, _) = resolve_ema_idp_authorization_manager( issuer, Arc::new(EnterpriseOAuthHttpClient(http_client)), + client_secret, + token_endpoint_auth_method, ) .await?; + if client_secret.is_some() { + // The SDK chooses Basic when both methods are advertised. Its login client + // must use the validated registration method, just like the EMA exchanges. + metadata.additional_fields.insert( + "token_endpoint_auth_methods_supported".to_owned(), + serde_json::json!([token_endpoint_auth_method.unwrap_or_default().as_str()]), + ); + } validate_authorization_server_endpoints(&metadata)?; validate_ema_oauth_endpoint( &metadata.authorization_endpoint, @@ -399,6 +414,8 @@ pub struct EnterpriseOAuthLoginRequest<'a> { pub credential_name: &'a str, pub issuer: &'a str, pub client_id: &'a str, + pub client_secret: Option<&'a str>, + pub token_endpoint_auth_method: Option, pub keyring_backend_kind: AuthKeyringBackendKind, pub callback_port: Option, pub callback_url: Option<&'a str>, diff --git a/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs b/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs index 78b383e307..3e0ed3956c 100644 --- a/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs +++ b/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs @@ -1,4 +1,4 @@ -//! Public enterprise login exercises the real callback and keyring adapter in an isolated process. +//! Enterprise login exercises the real callback and keyring adapter in an isolated process. use std::any::Any; use std::collections::HashMap; @@ -33,6 +33,7 @@ use wiremock::matchers::path; use super::*; const SECRET: &str = "enterprise-secret-sentinel"; +const CLIENT_SECRET: &str = "client-secret-sentinel"; const CREDENTIAL_NAME: &str = "ema-idp:enterprise-secret-sentinel"; #[path = "enterprise_oauth_logout_tests.rs"] @@ -60,8 +61,14 @@ async fn isolated_process(test_name: &str) -> Result { Ok(true) } -async fn login(issuer: &str, callback_url: Option<&str>) -> Result { +async fn login( + issuer: &str, + callback_url: Option<&str>, + auth_method: Option, +) -> Result { perform_enterprise_oauth_login_return_url(EnterpriseOAuthLoginRequest { + client_secret: auth_method.map(|_| CLIENT_SECRET), + token_endpoint_auth_method: auth_method, credential_name: CREDENTIAL_NAME, issuer, client_id: "enterprise-client", @@ -78,7 +85,10 @@ async fn login(issuer: &str, callback_url: Option<&str>) -> Result Result { - let handle = login(issuer, /*callback_url*/ None).await?; + let handle = login( + issuer, /*callback_url*/ None, /*auth_method*/ None, + ) + .await?; callback( &handle.authorization_url(), issuer, @@ -92,7 +102,7 @@ async fn metadata(server: &MockServer, issuer: &str) { Mock::given(method("GET")).and(path("/.well-known/oauth-authorization-server/idp")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "issuer": issuer, "authorization_endpoint": format!("{issuer}/authorize?prompt=none"), - "token_endpoint": format!("{issuer}/token"), "token_endpoint_auth_methods_supported": ["none"], + "token_endpoint": format!("{issuer}/token"), "token_endpoint_auth_methods_supported": ["none", "client_secret_basic", "client_secret_post"], }))).mount(server).await; } @@ -160,7 +170,10 @@ async fn enterprise_callback_errors_and_sdk_logs_exclude_credentials() -> Result .expect(u64::from(!callback_error)) .mount(&server) .await; - let login = login(&issuer, /*callback_url*/ None).await?; + let login = login( + &issuer, /*callback_url*/ None, /*auth_method*/ None, + ) + .await?; callback(&login.authorization_url(), &issuer, callback_error).await?; tracing::trace!(target: "rmcp::transport::auth", "SDK trace capture enabled"); let error = login.wait().await.err().expect("reject provider response"); @@ -257,12 +270,20 @@ async fn enterprise_public_api_storage_and_privacy() -> Result<()> { .await; // Callback, exchange and DefaultKeyringStore all run through public entrypoints. - for (login_index, callback_url) in ["http://localhost/callback", "http://127.0.0.1/callback"] - .into_iter() - .enumerate() + use McpEmaTokenEndpointAuthMethod::ClientSecretBasic; + use McpEmaTokenEndpointAuthMethod::ClientSecretPost; + for (login_index, (callback_url, auth_method)) in [ + ("http://localhost/callback", None), + ("http://127.0.0.1/callback", None), + ("http://localhost/callback", Some(ClientSecretBasic)), + ("http://127.0.0.1/callback", Some(ClientSecretPost)), + ] + .into_iter() + .enumerate() { - let login = login(&issuer, Some(callback_url)).await?; + let login = login(&issuer, Some(callback_url), auth_method).await?; let authorization_url = login.authorization_url(); + assert!(!authorization_url.contains(CLIENT_SECRET)); let authorization_query = Url::parse(&authorization_url)? .query_pairs() .into_owned() @@ -280,10 +301,30 @@ async fn enterprise_public_api_storage_and_privacy() -> Result<()> { .collect::>(); assert_eq!(token_requests.len(), login_index + 1); let token_request = &token_requests[login_index]; - assert!(!token_request.headers.contains_key("authorization")); let mut token_form = url::form_urlencoded::parse(&token_request.body) .into_owned() .collect::>(); + let authorization = token_request + .headers + .get("authorization") + .map(|value| value.to_str().unwrap()); + let expected_basic = format!( + "Basic {}", + base64::engine::general_purpose::STANDARD + .encode(format!("enterprise-client:{CLIENT_SECRET}")) + ); + assert_eq!( + authorization, + (auth_method == Some(ClientSecretBasic)).then_some(expected_basic.as_str()) + ); + assert_eq!( + token_form.remove("client_secret").as_deref(), + (auth_method == Some(ClientSecretPost)).then_some(CLIENT_SECRET) + ); + assert_eq!( + token_form.remove("client_id").as_deref(), + (auth_method != Some(ClientSecretBasic)).then_some("enterprise-client") + ); let verifier = oauth2::PkceCodeVerifier::new( token_form.remove("code_verifier").expect("PKCE verifier"), ); @@ -297,7 +338,6 @@ async fn enterprise_public_api_storage_and_privacy() -> Result<()> { HashMap::from([ ("grant_type".to_string(), "authorization_code".to_string()), ("code".to_string(), SECRET.to_string()), - ("client_id".to_string(), "enterprise-client".to_string()), ( "redirect_uri".to_string(), authorization_query["redirect_uri"].clone() @@ -336,6 +376,8 @@ async fn enterprise_public_api_storage_and_privacy() -> Result<()> { }, )? .expect("stored grant"); + assert!(!serde_json::to_string(&stored)?.contains(CLIENT_SECRET)); + assert!(!logs_contain(CLIENT_SECRET)); assert_eq!( stored .token_response @@ -440,9 +482,13 @@ async fn enterprise_public_api_storage_and_privacy() -> Result<()> { // Failed replacement setup leaves the existing attempt valid. let retained = complete_login(&issuer).await?; assert!( - login(&issuer, Some(&format!("{}/callback", server.uri()))) - .await - .is_err(), + login( + &issuer, + Some(&format!("{}/callback", server.uri())), + /*auth_method*/ None + ) + .await + .is_err(), "an occupied registered callback port must not silently move" ); retained.commit_if(|| async { Some(()) }).await?; diff --git a/codex-rs/rmcp-client/src/enterprise_oauth_logout_tests.rs b/codex-rs/rmcp-client/src/enterprise_oauth_logout_tests.rs index 3916c01b41..4f88d27028 100644 --- a/codex-rs/rmcp-client/src/enterprise_oauth_logout_tests.rs +++ b/codex-rs/rmcp-client/src/enterprise_oauth_logout_tests.rs @@ -151,6 +151,8 @@ async fn logout_invalidates_pending_login_across_processes() -> Result<()> { let starting_issuer = issuer.clone(); let starting = tokio::spawn(async move { perform_enterprise_oauth_login_return_url(EnterpriseOAuthLoginRequest { + client_secret: None, + token_endpoint_auth_method: None, credential_name: CREDENTIAL_NAME, issuer: &starting_issuer, client_id: "enterprise-client", @@ -183,7 +185,10 @@ async fn logout_invalidates_pending_login_across_processes() -> Result<()> { // The first logout has no grant to delete. The second deletes the fresh grant // from the previous iteration. Both must invalidate pending callbacks and staged grants. for _ in 0..2 { - let pending = login(&issuer, /*callback_url*/ None).await?; + let pending = login( + &issuer, /*callback_url*/ None, /*auth_method*/ None, + ) + .await?; let staged = complete_login(&issuer).await?; let output = tokio::process::Command::new(std::env::current_exe()?) .args(["--exact", TEST, "--nocapture"]) @@ -271,7 +276,13 @@ async fn logout_invalidates_pending_login_across_processes() -> Result<()> { .expect("persistent generation"); fs::write(&generation_path, b"incomplete write")?; assert!(stale.commit_if(|| async { Some(()) }).await.is_err()); - assert!(login(&issuer, /*callback_url*/ None).await.is_err()); + assert!( + login( + &issuer, /*callback_url*/ None, /*auth_method*/ None + ) + .await + .is_err() + ); // A lost marker never revives a pre-logout attempt or a pre-reset generation. fs::remove_file(&generation_path)?; diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index a9053f6b7f..737e86bb68 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -9,6 +9,7 @@ use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use anyhow::bail; +use codex_config::McpEmaTokenEndpointAuthMethod; use codex_config::McpServerOAuthConfig; use codex_exec_server::HttpClient; use codex_otel::auth_storage::AuthStorageOriginator; @@ -609,8 +610,17 @@ impl OauthLoginFlow { )?); let registered_authorization = if oauth_client_id.is_some() { Some( - resolve_authorization_manager(server_url, Arc::clone(&oauth_http_client), purpose) - .await?, + resolve_authorization_manager( + server_url, + Arc::clone(&oauth_http_client), + purpose, + credentials + .client_secret + .as_ref() + .map(|secret| secret.secret().as_str()), + oauth_config.and_then(|config| config.token_endpoint_auth_method), + ) + .await?, ) } else { None @@ -865,9 +875,17 @@ async fn resolve_authorization_manager( server_url: &str, http_client: Arc, purpose: OAuthLoginPurpose, + client_secret: Option<&str>, + token_endpoint_auth_method: Option, ) -> Result<(AuthorizationManager, AuthorizationMetadata)> { if matches!(purpose, OAuthLoginPurpose::EnterpriseIdp) { - return resolve_enterprise_authorization_manager(server_url, http_client).await; + return resolve_enterprise_authorization_manager( + server_url, + http_client, + client_secret, + token_endpoint_auth_method, + ) + .await; } let mut auth_manager = AuthorizationManager::new_with_oauth_http_client(server_url, http_client).await?; @@ -1182,6 +1200,8 @@ mod tests { &format!("{base_url}/mcp"), )), OAuthLoginPurpose::Mcp, + /*client_secret*/ None, + /*token_endpoint_auth_method*/ None, ) .await .expect("resolve pre-registered OAuth metadata"); @@ -1292,6 +1312,8 @@ mod tests { &format!("{issuer}/mcp"), )), OAuthLoginPurpose::Mcp, + /*client_secret*/ None, + /*token_endpoint_auth_method*/ None, ) .await .expect("resolve issuer-aware authorization metadata");