diff --git a/crates/animus-mcp-oauth/src/config.rs b/crates/animus-mcp-oauth/src/config.rs index 0c04a488..44c8849c 100644 --- a/crates/animus-mcp-oauth/src/config.rs +++ b/crates/animus-mcp-oauth/src/config.rs @@ -32,6 +32,10 @@ pub enum ServerResolutionError { WorkflowConfig(String), #[error("failed to load project config: {0}")] ProjectConfig(String), + /// Resolving the pinned confidential client secret (`client_secret_env`) + /// from the process env / project keychain failed. + #[error("failed to resolve OAuth client secret for MCP server `{0}`: {1}")] + ClientSecret(String, String), } /// Outcome of resolving a server's OAuth shape from config. @@ -45,6 +49,10 @@ pub struct ServerResolution { pub scopes: Vec, /// Pre-registered client id, if the config pins one (skips DCR). pub client_id: Option, + /// Resolved confidential client secret for a pinned pre-registered app, + /// when the `authorization_code` oauth block sets `client_secret_env`. + /// `None` for public (PKCE-only) pinned clients and for DCR. + pub client_secret: Option, /// True when the resolved server's oauth block is an /// `authorization_code` flow (vs absent / a machine-to-machine flow). pub is_authorization_code: bool, @@ -130,7 +138,7 @@ pub fn resolve_server_url( } }; if let Some(def) = loaded.as_ref().and_then(|loaded| loaded.config.mcp_servers.get(server)) { - return finalize(server, url_override, def.url.clone(), def.oauth.clone()); + return finalize(project_root, server, url_override, def.url.clone(), def.oauth.clone()); } // Project config mcp_servers next. @@ -138,7 +146,7 @@ pub fn resolve_server_url( .map_err(|err| ServerResolutionError::ProjectConfig(err.to_string()))?; if let Some(entry) = project_config.mcp_servers.get(server) { let oauth = entry.oauth.as_ref().and_then(|value| serde_json::from_value::(value.clone()).ok()); - return finalize(server, url_override, entry.url.clone(), oauth); + return finalize(project_root, server, url_override, entry.url.clone(), oauth); } // Not in config: only proceed if the user passed an explicit URL. @@ -147,6 +155,7 @@ pub fn resolve_server_url( url: url.to_string(), scopes: Vec::new(), client_id: None, + client_secret: None, is_authorization_code: true, broker_oauth: None, }), @@ -155,6 +164,7 @@ pub fn resolve_server_url( } fn finalize( + project_root: &Path, server: &str, url_override: Option<&str>, config_url: Option, @@ -165,17 +175,22 @@ fn finalize( .or(config_url) .ok_or_else(|| ServerResolutionError::MissingUrl(server.to_string()))?; match oauth { - Some(cfg) if cfg.flow == OauthFlow::AuthorizationCode => Ok(ServerResolution { - url, - scopes: cfg.scopes, - client_id: cfg.client_id, - is_authorization_code: true, - broker_oauth: None, - }), + Some(cfg) if cfg.flow == OauthFlow::AuthorizationCode => { + let client_secret = resolve_client_secret(project_root, server, &cfg)?; + Ok(ServerResolution { + url, + scopes: cfg.scopes, + client_id: cfg.client_id, + client_secret, + is_authorization_code: true, + broker_oauth: None, + }) + } Some(cfg) => Ok(ServerResolution { url, scopes: Vec::new(), client_id: None, + client_secret: None, is_authorization_code: false, broker_oauth: Some(cfg), }), @@ -183,12 +198,39 @@ fn finalize( url, scopes: Vec::new(), client_id: None, + client_secret: None, is_authorization_code: false, broker_oauth: None, }), } } +/// Resolve the confidential pre-registered client secret for an +/// `authorization_code` server, if its oauth block pins one via +/// `client_secret_env`. The name is looked up in the process environment first +/// (explicit parent env wins, matching the `${VAR}`/keychain precedence used +/// elsewhere), then the project keychain the `animus secret` surface writes to. +/// Returns `None` when no `client_secret_env` is set or it resolves to empty — +/// a public (PKCE-only) pinned client or DCR. +fn resolve_client_secret( + project_root: &Path, + server: &str, + cfg: &OauthConfig, +) -> Result, ServerResolutionError> { + let Some(name) = cfg.client_secret_env.as_deref().map(str::trim).filter(|s| !s.is_empty()) else { + return Ok(None); + }; + if let Ok(value) = std::env::var(name) { + if !value.is_empty() { + return Ok(Some(value)); + } + } + let store = build_secret_store(project_root)?; + let value = + store.get(name).map_err(|err| ServerResolutionError::ClientSecret(server.to_string(), err.to_string()))?; + Ok(value.filter(|v| !v.is_empty())) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/animus-mcp-oauth/src/flow.rs b/crates/animus-mcp-oauth/src/flow.rs index 5f96f050..b3ca6c0f 100644 --- a/crates/animus-mcp-oauth/src/flow.rs +++ b/crates/animus-mcp-oauth/src/flow.rs @@ -448,7 +448,14 @@ pub async fn run_auth(project_root: &Path, server: &str, opts: RunAuthOptions<'_ // pinned id, failing on servers that don't support DCR. // - No pinned id: `AuthorizationSession::new` performs DCR. let (auth_url, csrf_state, exchange) = if let Some(client_id) = pinned_client_id { - let config = OAuthClientConfig::new(client_id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone()); + let mut config = + OAuthClientConfig::new(client_id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone()); + // Confidential pre-registered app: attach the resolved client_secret so + // the token exchange authenticates as that app (public/PKCE clients + // leave this unset). + if let Some(secret) = resolution.client_secret.as_deref() { + config = config.with_client_secret(secret.to_string()); + } manager .configure_client(config) .map_err(|err| anyhow!("failed to configure pinned client_id for `{server}`: {err}"))?; @@ -608,11 +615,18 @@ pub async fn begin_auth(project_root: &Path, server: &str, opts: BeginOptions<'_ // still return a confidential client; carry the secret so completion can // authenticate the exchange with the same client begin registered. let (client_id, client_secret) = if let Some(id) = pinned_client_id { - let config = OAuthClientConfig::new(id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone()); + let mut config = OAuthClientConfig::new(id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone()); + // Confidential pre-registered app: carry the resolved client_secret into + // the pending record so `complete_auth` authenticates the exchange as the + // same app. Public/PKCE pinned clients leave it `None`. + let pinned_secret = resolution.client_secret.clone(); + if let Some(secret) = pinned_secret.as_deref() { + config = config.with_client_secret(secret.to_string()); + } manager .configure_client(config) .map_err(|err| anyhow!("failed to configure pinned client_id for `{server}`: {err}"))?; - (id.to_string(), None) + (id.to_string(), pinned_secret) } else { // register_client runs DCR and internally calls configure_client. let config = manager diff --git a/crates/orchestrator-config/src/workflow_config/tests.rs b/crates/orchestrator-config/src/workflow_config/tests.rs index 1c6c9a0d..8f422e08 100644 --- a/crates/orchestrator-config/src/workflow_config/tests.rs +++ b/crates/orchestrator-config/src/workflow_config/tests.rs @@ -3411,6 +3411,80 @@ fn validation_rejects_authorization_code_with_blank_client_id() { assert!(err.to_string().contains("client_id"), "should reject blank client_id: {err}"); } +#[test] +fn validation_accepts_authorization_code_with_pinned_client_and_secret_env() { + // A confidential pre-registered app (REQ-044 Phase 2): a pinned client_id + // plus a client_secret_env is the supported confidential shape. + let mut config = builtin_workflow_config(); + config.mcp_servers.insert( + "hubspot".to_string(), + http_oauth_server(OauthConfig { + flow: OauthFlow::AuthorizationCode, + token_url: None, + client_id_env: None, + client_secret_env: Some("HUBSPOT_CLIENT_SECRET".to_string()), + refresh_token_env: None, + bearer_env: None, + scopes: vec![], + audience: None, + cache: true, + client_id: Some("app-123".to_string()), + }), + ); + validate_workflow_config(&config) + .expect("authorization_code with a pinned client_id + client_secret_env should validate"); +} + +#[test] +fn validation_rejects_authorization_code_secret_env_without_client_id() { + // A client_secret_env with no pinned client_id makes no sense: DCR mints the + // client, so there is nothing to attach the secret to. + let mut config = builtin_workflow_config(); + config.mcp_servers.insert( + "hubspot".to_string(), + http_oauth_server(OauthConfig { + flow: OauthFlow::AuthorizationCode, + token_url: None, + client_id_env: None, + client_secret_env: Some("HUBSPOT_CLIENT_SECRET".to_string()), + refresh_token_env: None, + bearer_env: None, + scopes: vec![], + audience: None, + cache: true, + client_id: None, + }), + ); + let err = validate_workflow_config(&config) + .expect_err("client_secret_env without a pinned client_id must fail validation"); + let msg = err.to_string(); + assert!(msg.contains("client_secret_env"), "should name client_secret_env: {msg}"); + assert!(msg.contains("client_id"), "should require a pinned client_id: {msg}"); +} + +#[test] +fn validation_rejects_authorization_code_blank_secret_env() { + // A blank client_secret_env name is a typo — reject it even with a client_id. + let mut config = builtin_workflow_config(); + config.mcp_servers.insert( + "hubspot".to_string(), + http_oauth_server(OauthConfig { + flow: OauthFlow::AuthorizationCode, + token_url: None, + client_id_env: None, + client_secret_env: Some(" ".to_string()), + refresh_token_env: None, + bearer_env: None, + scopes: vec![], + audience: None, + cache: true, + client_id: Some("app-123".to_string()), + }), + ); + let err = validate_workflow_config(&config).expect_err("blank client_secret_env must fail validation"); + assert!(err.to_string().contains("client_secret_env"), "should reject blank client_secret_env: {err}"); +} + #[test] fn validation_accepts_oauth_client_credentials() { let mut config = builtin_workflow_config(); diff --git a/crates/orchestrator-config/src/workflow_config/validation.rs b/crates/orchestrator-config/src/workflow_config/validation.rs index 841bd0a0..d3dc2a86 100644 --- a/crates/orchestrator-config/src/workflow_config/validation.rs +++ b/crates/orchestrator-config/src/workflow_config/validation.rs @@ -427,13 +427,13 @@ fn validate_oauth_config(server_name: &str, transport: Option<&str>, oauth: &Oau } } OauthFlow::AuthorizationCode => { - // Discovery + DCR fill in the token/authorization endpoints, so - // none of the machine-to-machine `*_env` fields apply. Reject - // them so a misplaced credential pointer surfaces at validation - // time rather than being silently ignored. + // Discovery + DCR fill in the token/authorization endpoints, so the + // machine-to-machine `*_env` fields don't apply — EXCEPT + // `client_secret_env`, which pins the secret of a CONFIDENTIAL + // pre-registered app (handled below). Reject the rest so a misplaced + // credential pointer surfaces at validation time. for (value, name) in [ (&oauth.client_id_env, "client_id_env"), - (&oauth.client_secret_env, "client_secret_env"), (&oauth.refresh_token_env, "refresh_token_env"), (&oauth.bearer_env, "bearer_env"), (&oauth.token_url, "token_url"), @@ -447,9 +447,30 @@ fn validate_oauth_config(server_name: &str, transport: Option<&str>, oauth: &Oau // skip Dynamic Client Registration with an empty id, failing later // at the browser/token-exchange step. Reject it here. Omit the key // entirely to use DCR. + let has_pinned_client_id = oauth.client_id.as_ref().is_some_and(|id| !id.trim().is_empty()); if oauth.client_id.as_ref().is_some_and(|id| id.trim().is_empty()) { errors.push(format!("{} must not be blank for flow=\"authorization_code\"", field("client_id"))); } + // `client_secret_env` names the keychain/env entry holding the secret + // of a CONFIDENTIAL pre-registered app (providers like HubSpot that + // require a `client_secret` at token exchange). It only makes sense + // alongside a pinned `client_id` — there is no client to attach a + // secret to under DCR — and the name itself must be non-blank. + if let Some(secret_env) = oauth.client_secret_env.as_ref() { + if secret_env.trim().is_empty() { + errors.push(format!( + "{} must not be blank for flow=\"authorization_code\"", + field("client_secret_env") + )); + } + if !has_pinned_client_id { + errors.push(format!( + "{} requires a pinned {} for flow=\"authorization_code\"", + field("client_secret_env"), + field("client_id") + )); + } + } } } } diff --git a/docs/reference/mcp-oauth.md b/docs/reference/mcp-oauth.md index 7c7e9e34..e97676aa 100644 --- a/docs/reference/mcp-oauth.md +++ b/docs/reference/mcp-oauth.md @@ -52,6 +52,8 @@ mcp_servers: - repo - read:user client_id: my-pre-registered-client # skip Dynamic Client Registration + # optional — only for a CONFIDENTIAL pre-registered app (requires client_id): + client_secret_env: MY_APP_CLIENT_SECRET # names a keychain/env secret ``` Fields: @@ -62,11 +64,14 @@ Fields: | `url` | yes | Upstream HTTP MCP endpoint; also the OAuth `resource` indicator (RFC 8707), the discovery seed (RFC 9728 protected-resource metadata → authorization server), and the proxy target | | `scopes` | no | Requested at authorization. **When omitted, Animus auto-detects the server's advertised `scopes_supported`** from discovery metadata and requests those (see "Scope posture" below); if the server advertises none, nothing is requested and it applies its own minimal default. Set this to pin a specific (narrower) scope set, or `[none]` to force no scopes (opt out of auto-detection). | | `client_id` | no | Pre-registered client id; when omitted, Dynamic Client Registration (RFC 7591) is used | - -The machine-to-machine credential pointers (`token_url`, `client_id_env`, -`client_secret_env`, `refresh_token_env`, `bearer_env`) must **not** be set on -an `authorization_code` server — discovery fills those endpoints in, and -validation rejects them if present. +| `client_secret_env` | no | For a **confidential** pre-registered app (e.g. HubSpot) that requires a `client_secret` at token exchange. Names the env var / keychain entry (`animus secret set `) holding the secret — resolved from the process env first, then the project keychain. Requires a pinned `client_id`; leave unset for public (PKCE-only) clients and DCR. | + +The remaining machine-to-machine credential pointers (`token_url`, +`client_id_env`, `refresh_token_env`, `bearer_env`) must **not** be set on an +`authorization_code` server — discovery fills those endpoints in, and validation +rejects them if present. `client_secret_env` is the one exception (see above): +it pins the secret of a confidential pre-registered app and is only valid +alongside a `client_id`. ## CLI