From c9d783a8d635a64908f3b1338d567693a7c996c6 Mon Sep 17 00:00:00 2001 From: Yuansheng Wang Date: Thu, 30 Jul 2026 10:53:55 +0800 Subject: [PATCH 1/2] feat(mcp): OAuth 2.1 resource-server discovery surface for /mcp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the inbound-leg half of AISIX-Cloud#1143. When an environment projects the new mcp_auth_settings row (canonical /mcp resource URL) and has at least one enabled oidc_provider, the gateway: - serves the RFC 9728 Protected Resource Metadata document on both /.well-known/oauth-protected-resource and its path-insertion /mcp form, derived entirely from configuration (resource URL, enabled providers' issuers, union of their required_scopes); - attaches WWW-Authenticate challenges to /mcp auth failures: bare resource_metadata on missing credentials, error=invalid_token on rejected credentials, and error=insufficient_scope naming only the currently-required scopes on a scope failure (the scope failure is split into a dedicated JwtInsufficientScope variant that renders byte-identically to JwtClaimsRejected, so /v1 is unchanged; bound_claims policy denials deliberately carry no challenge). Without the settings row the surface is dormant: the well-known routes 404 and no header is attached — every existing environment behaves byte-identically to before. A malformed row (path other than /mcp, query/fragment, non-http scheme) keeps the surface dormant with one process-wide warning. The resources file treats mcp_auth_settings as a singleton: a second entry is a load error. The row is wired through the full config path: schema variant (schemas/resources/mcp_auth_settings.schema.json via dump-schema), snapshot table, etcd loader + watch supervisor (put/delete/clone), declarative filesource and export. --- crates/aisix-core/src/bin/dump-schema.rs | 5 + crates/aisix-core/src/filesource/desugar.rs | 7 + crates/aisix-core/src/filesource/mod.rs | 27 +- crates/aisix-core/src/filesource/tests.rs | 31 ++ .../src/models/mcp_auth_settings.rs | 87 ++++ crates/aisix-core/src/models/mod.rs | 8 +- crates/aisix-core/src/models/schema.rs | 18 + crates/aisix-core/src/models/snapshot.rs | 8 + crates/aisix-etcd/src/loader.rs | 22 +- crates/aisix-etcd/src/supervisor.rs | 36 ++ crates/aisix-proxy/src/error.rs | 142 ++++++- crates/aisix-proxy/src/jwt.rs | 51 ++- crates/aisix-proxy/src/lib.rs | 200 ++++++++- crates/aisix-proxy/src/mcp_auth.rs | 402 ++++++++++++++++++ crates/aisix-server/src/export/document.rs | 15 + .../resources/mcp_auth_settings.schema.json | 16 + 16 files changed, 1038 insertions(+), 37 deletions(-) create mode 100644 crates/aisix-core/src/models/mcp_auth_settings.rs create mode 100644 crates/aisix-proxy/src/mcp_auth.rs create mode 100644 schemas/resources/mcp_auth_settings.schema.json diff --git a/crates/aisix-core/src/bin/dump-schema.rs b/crates/aisix-core/src/bin/dump-schema.rs index 9568e66c..ed2f0fb9 100644 --- a/crates/aisix-core/src/bin/dump-schema.rs +++ b/crates/aisix-core/src/bin/dump-schema.rs @@ -70,6 +70,11 @@ fn main() { "oidc_provider", schema::oidc_provider_root_schema(), ); + dump_value( + &out_dir, + "mcp_auth_settings", + schema::mcp_auth_settings_root_schema(), + ); dump::(&out_dir, "ensemble"); dump::(&out_dir, "rate_limit"); diff --git a/crates/aisix-core/src/filesource/desugar.rs b/crates/aisix-core/src/filesource/desugar.rs index a127bd24..8b629448 100644 --- a/crates/aisix-core/src/filesource/desugar.rs +++ b/crates/aisix-core/src/filesource/desugar.rs @@ -61,6 +61,11 @@ pub(crate) enum IdentityField { /// `name`, with `display_name` accepted as the alternative spelling /// (mcp_servers, a2a_agents — mirroring their schemas). NameOrDisplayName, + /// A fixed constant — the kind is a per-file singleton + /// (mcp_auth_settings) whose entries carry no identity field of + /// their own. Every entry shares the constant, so a second entry + /// trips the pass-1 duplicate check. + Fixed(&'static str), } impl IdentityField { @@ -70,6 +75,7 @@ impl IdentityField { Self::DisplayName => "display_name", Self::Name => "name", Self::NameOrDisplayName => "name (or display_name)", + Self::Fixed(_) => "the singleton identity", } } @@ -86,6 +92,7 @@ impl IdentityField { Self::DisplayName => field("display_name"), Self::Name => field("name"), Self::NameOrDisplayName => field("name").or_else(|| field("display_name")), + Self::Fixed(v) => Some(v.to_string()), } } } diff --git a/crates/aisix-core/src/filesource/mod.rs b/crates/aisix-core/src/filesource/mod.rs index 1779c992..c354dee2 100644 --- a/crates/aisix-core/src/filesource/mod.rs +++ b/crates/aisix-core/src/filesource/mod.rs @@ -46,8 +46,9 @@ use yaml_rust2::{Yaml, YamlLoader}; use crate::models::{ validate_a2a_agent, validate_apikey, validate_cache_policy, validate_guardrail, - validate_mcp_server, validate_model, validate_observability_exporter, validate_oidc_provider, - validate_provider_key, validate_rate_limit_policy, A2aAgent, ApiKey, CachePolicy, Guardrail, + validate_mcp_auth_settings, validate_mcp_server, validate_model, + validate_observability_exporter, validate_oidc_provider, validate_provider_key, + validate_rate_limit_policy, A2aAgent, ApiKey, CachePolicy, Guardrail, McpAuthSettings, McpServer, Model, ObservabilityExporter, OidcProvider, ProviderKey, RateLimitPolicy, SchemaError, }; @@ -130,8 +131,8 @@ pub(crate) fn url_has_credentials(url: &str) -> bool { false } -/// Fixed processing order for the ten resource collections. -const KINDS: [(&str, IdentityField); 10] = [ +/// Fixed processing order for the eleven resource collections. +const KINDS: [(&str, IdentityField); 11] = [ ("provider_keys", IdentityField::DisplayName), ("models", IdentityField::DisplayName), ("api_keys", IdentityField::DisplayName), @@ -142,6 +143,12 @@ const KINDS: [(&str, IdentityField); 10] = [ ("observability_exporters", IdentityField::Name), ("rate_limit_policies", IdentityField::Name), ("oidc_providers", IdentityField::Name), + // Singleton: the fixed identity makes a second entry a duplicate + // at pass 1, enforcing at-most-one row per file. + ( + "mcp_auth_settings", + IdentityField::Fixed("mcp_auth_settings"), + ), ]; /// Load `path` into a fresh [`AisixSnapshot`], resolving `${VAR}` @@ -346,6 +353,7 @@ pub fn load_from_str( let mut observability_exporters: Vec<(String, String, ObservabilityExporter)> = Vec::new(); let mut rate_limit_policies: Vec<(String, String, RateLimitPolicy)> = Vec::new(); let mut oidc_providers: Vec<(String, String, OidcProvider)> = Vec::new(); + let mut mcp_auth_settings: Vec<(String, String, McpAuthSettings)> = Vec::new(); for mut entry in prepared { let id = derive_id(entry.kind, &entry.identity); @@ -436,6 +444,12 @@ pub fn load_from_str( oidc_providers.push((id, scope, t)); } } + "mcp_auth_settings" => { + if let Some(t) = finish(&scope, &entry.doc, validate_mcp_auth_settings, &mut errors) + { + mcp_auth_settings.push((id, scope, t)); + } + } other => unreachable!("kind {other} is not in KINDS"), } } @@ -671,6 +685,11 @@ pub fn load_from_str( .oidc_providers .insert(ResourceEntry::new(id, v, revision)); } + for (id, _, v) in mcp_auth_settings { + snapshot + .mcp_auth_settings + .insert(ResourceEntry::new(id, v, revision)); + } Ok(snapshot) } diff --git a/crates/aisix-core/src/filesource/tests.rs b/crates/aisix-core/src/filesource/tests.rs index 275048b5..255c51c3 100644 --- a/crates/aisix-core/src/filesource/tests.rs +++ b/crates/aisix-core/src/filesource/tests.rs @@ -536,6 +536,37 @@ oidc_providers: ); } +#[test] +fn mcp_auth_settings_singleton_loads() { + let contents = r#" +_format_version: "1" +mcp_auth_settings: + - resource_url: https://gw.example.com/mcp +"#; + let snapshot = load(contents, &env_of(&[])).expect("loads"); + assert_eq!(snapshot.mcp_auth_settings.len(), 1); + let entry = snapshot.mcp_auth_settings.entries().pop().unwrap(); + assert_eq!(entry.value.resource_url, "https://gw.example.com/mcp"); +} + +#[test] +fn duplicate_mcp_auth_settings_is_a_load_error() { + // The kind is a per-environment singleton: its fixed identity makes + // any second entry a pass-1 duplicate (AISIX-Cloud#1143). + let contents = r#" +_format_version: "1" +mcp_auth_settings: + - resource_url: https://gw.example.com/mcp + - resource_url: https://other.example.com/mcp +"#; + let errs = errors_of(load(contents, &env_of(&[]))); + assert_eq!(errs.len(), 1, "{errs:?}"); + assert!( + errs[0].contains("duplicate mcp_auth_settings entry"), + "{errs:?}" + ); +} + #[test] fn oidc_url_with_embedded_credentials_is_a_load_error() { let contents = r#" diff --git a/crates/aisix-core/src/models/mcp_auth_settings.rs b/crates/aisix-core/src/models/mcp_auth_settings.rs new file mode 100644 index 00000000..960c92da --- /dev/null +++ b/crates/aisix-core/src/models/mcp_auth_settings.rs @@ -0,0 +1,87 @@ +//! `McpAuthSettings` entity — the environment's inbound MCP OAuth +//! discovery identity, stored in etcd under `mcp_auth_settings/` +//! (the control plane keys the singleton row by the environment id). +//! +//! Carrying a valid row activates the `/mcp` OAuth 2.1 resource-server +//! discovery surface (AISIX-Cloud#1143) when at least one enabled +//! [`OidcProvider`](super::oidc_provider::OidcProvider) also exists: +//! the RFC 9728 Protected Resource Metadata document is served under +//! `/.well-known/oauth-protected-resource`, and `/mcp` auth failures +//! carry a `WWW-Authenticate` challenge pointing at it. Without the +//! row the surface stays dormant and behavior is unchanged. +//! +//! At most one row exists per environment; the declarative resources +//! file rejects a document carrying more than one entry at load. + +use serde::{Deserialize, Serialize}; + +use crate::resource::Resource; + +#[derive(Debug, Clone, Serialize, Deserialize, schemars::JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct McpAuthSettings { + /// Canonical URI of this environment's `/mcp` endpoint, e.g. + /// `https://gw.example.com/mcp`. Published verbatim as the PRM + /// document's `resource` (never derived from the request Host + /// header) and the value the trust providers' `audiences` must + /// include for OAuth-for-MCP tokens to validate. The URL path must + /// be exactly `/mcp` — the gateway's fixed MCP route. + #[schemars(length(min = 1))] + pub resource_url: String, + + /// etcd-key uuid. Filled by the loader and never included in the + /// JSON payload. + #[serde(skip)] + pub(crate) runtime_id: String, +} + +impl Resource for McpAuthSettings { + fn id(&self) -> &str { + &self.runtime_id + } + + /// Fixed identity: the row is a per-environment singleton, so the + /// by-name index key is a constant rather than a user-chosen label. + fn name(&self) -> &str { + "mcp_auth_settings" + } + + fn kind() -> &'static str { + "mcp_auth_settings" + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn deserialises_minimal_settings() { + let s: McpAuthSettings = + serde_json::from_str(r#"{"resource_url": "https://gw.example.com/mcp"}"#).unwrap(); + assert_eq!(s.resource_url, "https://gw.example.com/mcp"); + } + + #[test] + fn rejects_unknown_fields() { + let r: Result = + serde_json::from_str(r#"{"resource_url": "https://gw.example.com/mcp", "extra": 1}"#); + assert!(r.is_err()); + } + + #[test] + fn rejects_missing_resource_url() { + let r: Result = serde_json::from_str(r#"{}"#); + assert!(r.is_err()); + } + + #[test] + fn resource_trait_uses_fixed_identity() { + assert_eq!(McpAuthSettings::kind(), "mcp_auth_settings"); + let mut s: McpAuthSettings = + serde_json::from_str(r#"{"resource_url": "https://gw.example.com/mcp"}"#).unwrap(); + s.runtime_id = "env-uuid-1".into(); + assert_eq!(s.id(), "env-uuid-1"); + assert_eq!(s.name(), "mcp_auth_settings"); + } +} diff --git a/crates/aisix-core/src/models/mod.rs b/crates/aisix-core/src/models/mod.rs index 03a21d2b..f264c66f 100644 --- a/crates/aisix-core/src/models/mod.rs +++ b/crates/aisix-core/src/models/mod.rs @@ -21,6 +21,7 @@ pub mod cache_policy; pub mod embedding; pub mod ensemble; pub mod guardrail; +pub mod mcp_auth_settings; pub mod mcp_policy; pub mod mcp_server; pub mod model; @@ -47,6 +48,7 @@ pub use guardrail::{ GuardrailScopeType, KeywordConfig, KeywordPattern, LakeraConfig, OpenaiModerationConfig, PiiConfig, PiiCustomPattern, PiiDetectorConfig, PresidioConfig, PresidioEntityConfig, }; +pub use mcp_auth_settings::McpAuthSettings; pub use mcp_policy::{McpAccess, McpAccessMode, McpPolicy, McpPolicyMode, McpPolicyScope}; pub use mcp_server::{McpAuthType, McpServer, McpServerType, McpTransport}; pub use model::{ @@ -66,9 +68,9 @@ pub use rate_limit_policy::{PolicyScope, PolicyWindow, RateLimitPolicy}; pub use routing::{Routing, RoutingStrategy, RoutingTarget, WhenAllUnavailablePolicy}; pub use schema::{ validate_a2a_agent, validate_apikey, validate_cache_policy, validate_guardrail, - validate_guardrail_attachment, validate_mcp_policy, validate_mcp_server, validate_model, - validate_observability_exporter, validate_oidc_provider, validate_provider_key, - validate_rate_limit_policy, SchemaError, + validate_guardrail_attachment, validate_mcp_auth_settings, validate_mcp_policy, + validate_mcp_server, validate_model, validate_observability_exporter, validate_oidc_provider, + validate_provider_key, validate_rate_limit_policy, SchemaError, }; pub use semantic::{ Aggregation, DistanceMetric, EmbeddingFailureMode, OnEmbeddingFailure, Semantic, SemanticMatch, diff --git a/crates/aisix-core/src/models/schema.rs b/crates/aisix-core/src/models/schema.rs index bc3fa9b7..4551733d 100644 --- a/crates/aisix-core/src/models/schema.rs +++ b/crates/aisix-core/src/models/schema.rs @@ -34,6 +34,7 @@ pub struct Schemas { pub mcp_policy: Validator, pub a2a_agent: Validator, pub oidc_provider: Validator, + pub mcp_auth_settings: Validator, } pub static SCHEMAS: Lazy> = Lazy::new(|| Arc::new(Schemas::compile())); @@ -77,6 +78,9 @@ impl Schemas { oidc_provider: jsonschema::options() .build(&oidc_provider_root_schema()) .expect("oidc_provider schema is well-formed"), + mcp_auth_settings: jsonschema::options() + .build(&mcp_auth_settings_root_schema()) + .expect("mcp_auth_settings schema is well-formed"), } } } @@ -155,6 +159,10 @@ pub fn validate_oidc_provider(value: &Value) -> Result<(), SchemaError> { validate(&SCHEMAS.oidc_provider, value) } +pub fn validate_mcp_auth_settings(value: &Value) -> Result<(), SchemaError> { + validate(&SCHEMAS.mcp_auth_settings, value) +} + /// Build a resource's canonical JSON Schema from its struct via `schemars`, /// the single source of field shapes and per-field constraints. /// @@ -415,6 +423,16 @@ pub fn oidc_provider_root_schema() -> Value { schema } +/// Canonical JSON Schema for the `mcp_auth_settings` resource, derived +/// from the [`McpAuthSettings`](crate::models::McpAuthSettings) struct. +/// One required field (`resource_url`); the singleton-per-environment +/// invariant is enforced by the writers (cp-api keys the row by the +/// environment id; the resources file rejects duplicates at load), not +/// by the document schema. +pub fn mcp_auth_settings_root_schema() -> Value { + struct_root_schema::(false) +} + /// Canonical JSON Schema for the `mcp_policy` resource, derived from the /// [`McpPolicy`](crate::models::McpPolicy) struct. Uses the nullable `Option` /// representation (`true`) so `scope_ref` accepts an explicit `null` as diff --git a/crates/aisix-core/src/models/snapshot.rs b/crates/aisix-core/src/models/snapshot.rs index 71c93001..b6ec5886 100644 --- a/crates/aisix-core/src/models/snapshot.rs +++ b/crates/aisix-core/src/models/snapshot.rs @@ -8,6 +8,7 @@ use super::a2a_agent::A2aAgent; use super::apikey::ApiKey; use super::cache_policy::CachePolicy; use super::guardrail::{Guardrail, GuardrailAttachment}; +use super::mcp_auth_settings::McpAuthSettings; use super::mcp_policy::McpPolicy; use super::mcp_server::McpServer; use super::model::Model; @@ -56,6 +57,12 @@ pub struct AisixSnapshot { /// request to the API key whose `jwt_subject` equals the token's /// identity claim. pub oidc_providers: ResourceTable, + /// The environment's inbound MCP OAuth discovery identity: + /// `/aisix//mcp_auth_settings/` — a singleton row + /// (cp-api keys it by the environment id). Together with at least + /// one enabled `oidc_providers` row it activates the `/mcp` + /// RFC 9728 discovery surface (AISIX-Cloud#1143). + pub mcp_auth_settings: ResourceTable, } impl AisixSnapshot { @@ -78,6 +85,7 @@ impl AisixSnapshot { + self.mcp_policies.len() + self.a2a_agents.len() + self.oidc_providers.len() + + self.mcp_auth_settings.len() } } diff --git a/crates/aisix-etcd/src/loader.rs b/crates/aisix-etcd/src/loader.rs index 4870b3a7..6e0cc596 100644 --- a/crates/aisix-etcd/src/loader.rs +++ b/crates/aisix-etcd/src/loader.rs @@ -13,11 +13,11 @@ use aisix_core::models::{ validate_a2a_agent, validate_apikey, validate_cache_policy, validate_guardrail, - validate_guardrail_attachment, validate_mcp_policy, validate_mcp_server, validate_model, - validate_observability_exporter, validate_oidc_provider, validate_provider_key, - validate_rate_limit_policy, A2aAgent, ApiKey, CachePolicy, Guardrail, GuardrailAttachment, - McpPolicy, McpServer, Model, ObservabilityExporter, OidcProvider, ProviderKey, RateLimitPolicy, - SchemaError, + validate_guardrail_attachment, validate_mcp_auth_settings, validate_mcp_policy, + validate_mcp_server, validate_model, validate_observability_exporter, validate_oidc_provider, + validate_provider_key, validate_rate_limit_policy, A2aAgent, ApiKey, CachePolicy, Guardrail, + GuardrailAttachment, McpAuthSettings, McpPolicy, McpServer, Model, ObservabilityExporter, + OidcProvider, ProviderKey, RateLimitPolicy, SchemaError, }; use aisix_core::resource::ResourceEntry; use aisix_core::AisixSnapshot; @@ -294,6 +294,18 @@ pub fn build_snapshot(prefix: &str, entries: &[RawEntry]) -> (AisixSnapshot, Bui snapshot.oidc_providers.insert(entry); } } + "mcp_auth_settings" => { + if let Some(entry) = validate_and_parse::( + &raw.key, + raw.revision, + parsed, + &value, + validate_mcp_auth_settings, + &mut stats, + ) { + snapshot.mcp_auth_settings.insert(entry); + } + } other => { tracing::debug!(key = %raw.key, kind = %other, "unknown etcd kind; skipping"); stats.unknown_kind += 1; diff --git a/crates/aisix-etcd/src/supervisor.rs b/crates/aisix-etcd/src/supervisor.rs index eff991ee..98db422b 100644 --- a/crates/aisix-etcd/src/supervisor.rs +++ b/crates/aisix-etcd/src/supervisor.rs @@ -489,6 +489,9 @@ impl Supervisor

{ for e in tiny.oidc_providers.entries() { new.oidc_providers.insert(clone_entry(&e)); } + for e in tiny.mcp_auth_settings.entries() { + new.mcp_auth_settings.insert(clone_entry(&e)); + } new }); self.remove_rejection_for_key(&entry.key); @@ -548,6 +551,7 @@ impl Supervisor

{ "mcp_policies" => snap.mcp_policies.get_by_id(parsed.id).is_some(), "a2a_agents" => snap.a2a_agents.get_by_id(parsed.id).is_some(), "oidc_providers" => snap.oidc_providers.get_by_id(parsed.id).is_some(), + "mcp_auth_settings" => snap.mcp_auth_settings.get_by_id(parsed.id).is_some(), _ => false, }; let removed_rejection = self.remove_rejection_for_key(key_str); @@ -607,6 +611,9 @@ impl Supervisor

{ "oidc_providers" => { new.oidc_providers.remove(parsed.id); } + "mcp_auth_settings" => { + new.mcp_auth_settings.remove(parsed.id); + } _ => {} } new @@ -860,6 +867,9 @@ fn clone_snapshot(src: &AisixSnapshot) -> AisixSnapshot { for e in src.oidc_providers.entries() { out.oidc_providers.insert(clone_entry(&e)); } + for e in src.mcp_auth_settings.entries() { + out.mcp_auth_settings.insert(clone_entry(&e)); + } out } @@ -884,6 +894,7 @@ fn resource_counts(snap: &AisixSnapshot) -> BTreeMap { ("mcp_policies", snap.mcp_policies.len()), ("a2a_agents", snap.a2a_agents.len()), ("oidc_providers", snap.oidc_providers.len()), + ("mcp_auth_settings", snap.mcp_auth_settings.len()), ] { if n > 0 { counts.insert(kind.to_string(), n); @@ -1035,6 +1046,12 @@ mod tests { "issuer": "https://idp.example.com/realms/agents", "audiences": ["aisix-gateway"] }"#; + // The MCP OAuth discovery settings row (AISIX-Cloud#1143) — + // configuring the resource URL mid-run must activate the + // discovery surface without a resync. + const VALID_MCP_AUTH_SETTINGS: &[u8] = br#"{ + "resource_url": "https://gw.example.com/mcp" + }"#; let provider = Arc::new(FakeProvider::new(vec![], 0)); let sup = Supervisor::new(provider, "/aisix"); @@ -1063,6 +1080,11 @@ mod tests { VALID_OIDC_PROVIDER, "OidcProvider", ), + ( + "/aisix/mcp_auth_settings/env-1", + VALID_MCP_AUTH_SETTINGS, + "McpAuthSettings", + ), ] { assert!( sup.apply_put(&entry(key, body, 2)), @@ -1085,6 +1107,11 @@ mod tests { "ObservabilityExporter not merged" ); assert_eq!(snap.oidc_providers.len(), 1, "OidcProvider not merged"); + assert_eq!( + snap.mcp_auth_settings.len(), + 1, + "McpAuthSettings not merged" + ); } #[tokio::test] @@ -1111,6 +1138,13 @@ mod tests { br#"{"name":"idp","issuer":"https://idp.example.com","audiences":["aisix"]}"#, 1, ), + // AISIX-Cloud#1143: clearing the resource URL projects a + // delete; it must deactivate the discovery surface. + entry( + "/aisix/mcp_auth_settings/env-1", + br#"{"resource_url":"https://gw.example.com/mcp"}"#, + 1, + ), ], 1, )); @@ -1125,6 +1159,8 @@ mod tests { assert!(sup.handle().load().guardrail_attachments.is_empty()); assert!(sup.apply_delete("/aisix/oidc_providers/op-1")); assert!(sup.handle().load().oidc_providers.is_empty()); + assert!(sup.apply_delete("/aisix/mcp_auth_settings/env-1")); + assert!(sup.handle().load().mcp_auth_settings.is_empty()); } #[tokio::test] diff --git a/crates/aisix-proxy/src/error.rs b/crates/aisix-proxy/src/error.rs index df276283..b909bdd2 100644 --- a/crates/aisix-proxy/src/error.rs +++ b/crates/aisix-proxy/src/error.rs @@ -171,10 +171,23 @@ pub enum ProxyError { #[error("JWT has expired")] JwtExpired, /// The JWT verified but does not satisfy the trust provider's - /// `required_scopes` / `bound_claims` requirements. 403: the - /// caller is authenticated, just not entitled. + /// `bound_claims` requirements. 403: the caller is authenticated, + /// just not entitled. (Scope failures are the sibling + /// [`Self::JwtInsufficientScope`]; both render identically.) #[error("JWT does not satisfy the required scopes or claims")] JwtClaimsRejected, + /// The JWT verified but its granted scopes do not cover the trust + /// provider's `required_scopes`. Split from + /// [`Self::JwtClaimsRejected`] so the active `/mcp` OAuth surface + /// can attach an RFC 6750 `insufficient_scope` challenge naming the + /// scopes this operation needs (AISIX-Cloud#1143) — a scope failure + /// is curable by re-consenting, a `bound_claims` policy denial is + /// not, so only the former may carry a challenge. Renders + /// byte-identically to `JwtClaimsRejected` on the wire (same 403, + /// `permission_denied` type, `jwt_claims_rejected` code): the `/v1` + /// contract is unchanged. + #[error("JWT does not satisfy the required scopes or claims")] + JwtInsufficientScope { required_scopes: Vec }, /// The JWT verified but no API key carries a `jwt_subject` equal /// to its identity claim. Named explicitly (not a generic invalid /// credential) because the fix — binding a key to the identity — @@ -280,7 +293,9 @@ impl ProxyError { | ProxyError::JwtInvalid | ProxyError::JwtExpired | ProxyError::JwtIdentityUnmapped => StatusCode::UNAUTHORIZED, - ProxyError::JwtClaimsRejected => StatusCode::FORBIDDEN, + ProxyError::JwtClaimsRejected | ProxyError::JwtInsufficientScope { .. } => { + StatusCode::FORBIDDEN + } ProxyError::JwksUnavailable => StatusCode::SERVICE_UNAVAILABLE, ProxyError::ModelForbidden(_) => StatusCode::FORBIDDEN, ProxyError::ModelIpRestricted(_) => StatusCode::FORBIDDEN, @@ -308,7 +323,9 @@ impl ProxyError { | ProxyError::JwtInvalid | ProxyError::JwtExpired | ProxyError::JwtIdentityUnmapped => "invalid_api_key", - ProxyError::JwtClaimsRejected => "permission_denied", + ProxyError::JwtClaimsRejected | ProxyError::JwtInsufficientScope { .. } => { + "permission_denied" + } // Auth infrastructure fault, not a credential judgment — the // generic server-fault family, like a 5xx from dispatch. ProxyError::JwksUnavailable => "api_error", @@ -407,7 +424,9 @@ impl ProxyError { // (`jwt_identity_unmapped`). ProxyError::JwtInvalid => env.with_code("jwt_invalid"), ProxyError::JwtExpired => env.with_code("jwt_expired"), - ProxyError::JwtClaimsRejected => env.with_code("jwt_claims_rejected"), + ProxyError::JwtClaimsRejected | ProxyError::JwtInsufficientScope { .. } => { + env.with_code("jwt_claims_rejected") + } ProxyError::JwtIdentityUnmapped => env.with_code("jwt_identity_unmapped"), ProxyError::JwksUnavailable => env.with_code("jwks_unavailable"), _ => env, @@ -455,8 +474,53 @@ fn render_bridge_upstream_envelope( ErrorEnvelope::new(safe_message, "upstream_error") } +/// Auth-failure classification attached to the response as an extension +/// so the `/mcp` challenge middleware (`crate::mcp_auth`) can add the +/// RFC 9728 `WWW-Authenticate` header. Classification must happen here: +/// `IntoResponse` has no access to route or snapshot state, and only the +/// active `/mcp` surface actually renders the header. +#[derive(Debug, Clone)] +pub(crate) enum AuthChallenge { + /// No usable credential was presented — bare `resource_metadata`. + MissingCredentials, + /// A credential was presented and rejected — `error="invalid_token"`. + InvalidToken, + /// A valid token lacks the scopes this operation requires — + /// `error="insufficient_scope"` plus the currently-required scopes + /// (SEP-2350 semantics: only what this operation needs, never an + /// echo of previously granted scopes). + InsufficientScope { required_scopes: Vec }, +} + +impl ProxyError { + /// The challenge classification for this error, when it is an + /// auth failure a Bearer challenge can describe. `JwtClaimsRejected` + /// (`bound_claims` policy denial) deliberately maps to `None`: + /// re-authenticating cannot cure it, so a challenge would only send + /// clients into a retry loop. `JwksUnavailable` is a gateway-side + /// fault, not a credential judgment. + fn auth_challenge(&self) -> Option { + match self { + ProxyError::MissingAuth => Some(AuthChallenge::MissingCredentials), + ProxyError::InvalidApiKey + | ProxyError::ApiKeyExpired + | ProxyError::ApiKeyDisabled + | ProxyError::JwtInvalid + | ProxyError::JwtExpired + | ProxyError::JwtIdentityUnmapped => Some(AuthChallenge::InvalidToken), + ProxyError::JwtInsufficientScope { required_scopes } => { + Some(AuthChallenge::InsufficientScope { + required_scopes: required_scopes.clone(), + }) + } + _ => None, + } + } +} + impl IntoResponse for ProxyError { fn into_response(self) -> Response { + let challenge = self.auth_challenge(); let status = self.status(); let retry_after = self.retry_after_secs(); let body = self.envelope(); @@ -466,6 +530,9 @@ impl IntoResponse for ProxyError { response.headers_mut().insert("retry-after", value); } } + if let Some(challenge) = challenge { + response.extensions_mut().insert(challenge); + } response } } @@ -659,6 +726,71 @@ mod tests { assert_eq!(e.kind(), "invalid_api_key"); } + /// AISIX-Cloud#1143: the scope-failure variant split out of + /// `JwtClaimsRejected` must stay byte-identical on the wire — same + /// 403, `permission_denied` type, `jwt_claims_rejected` code and + /// message — so `/v1` callers cannot observe the split. The only + /// difference is the challenge classification for the `/mcp` + /// middleware. + #[test] + fn insufficient_scope_renders_identically_to_claims_rejected() { + let scope_err = ProxyError::JwtInsufficientScope { + required_scopes: vec!["mcp:tools".into()], + }; + let policy_err = ProxyError::JwtClaimsRejected; + assert_eq!(scope_err.status(), policy_err.status()); + assert_eq!(scope_err.kind(), policy_err.kind()); + let a = serde_json::to_value(scope_err.envelope()).unwrap(); + let b = serde_json::to_value(policy_err.envelope()).unwrap(); + assert_eq!(a, b, "wire envelopes must not diverge"); + } + + #[test] + fn auth_challenge_classification_covers_exactly_the_curable_failures() { + assert!(matches!( + ProxyError::MissingAuth.auth_challenge(), + Some(AuthChallenge::MissingCredentials) + )); + for e in [ + ProxyError::InvalidApiKey, + ProxyError::ApiKeyExpired, + ProxyError::ApiKeyDisabled, + ProxyError::JwtInvalid, + ProxyError::JwtExpired, + ProxyError::JwtIdentityUnmapped, + ] { + assert!(matches!( + e.auth_challenge(), + Some(AuthChallenge::InvalidToken) + )); + } + let scope_err = ProxyError::JwtInsufficientScope { + required_scopes: vec!["mcp:tools".into()], + }; + match scope_err.auth_challenge() { + Some(AuthChallenge::InsufficientScope { required_scopes }) => { + assert_eq!(required_scopes, vec!["mcp:tools".to_string()]); + } + other => panic!("expected InsufficientScope, got {other:?}"), + } + // A bound_claims policy denial is not curable by re-consenting: + // no challenge, ever. Same for the gateway-side JWKS fault. + assert!(ProxyError::JwtClaimsRejected.auth_challenge().is_none()); + assert!(ProxyError::JwksUnavailable.auth_challenge().is_none()); + } + + #[tokio::test] + async fn into_response_attaches_the_challenge_extension() { + let resp = ProxyError::MissingAuth.into_response(); + assert!(matches!( + resp.extensions().get::(), + Some(AuthChallenge::MissingCredentials) + )); + // Non-auth errors carry no marker. + let resp = ProxyError::ModelNotFound("m".into()).into_response(); + assert!(resp.extensions().get::().is_none()); + } + #[test] fn model_forbidden_is_403_permission_denied() { let e = ProxyError::ModelForbidden("gpt-4o".into()); diff --git a/crates/aisix-proxy/src/jwt.rs b/crates/aisix-proxy/src/jwt.rs index 8f5078fd..75ef17e1 100644 --- a/crates/aisix-proxy/src/jwt.rs +++ b/crates/aisix-proxy/src/jwt.rs @@ -299,14 +299,23 @@ pub(crate) async fn authenticate_jwt( }; // ── Provider claim requirements ────────────────────────────────── - if let Err(reason) = check_provider_claims(&claims, prov) { - return Err(deny( - state, - reason, - &iss, - kid.as_deref(), - ProxyError::JwtClaimsRejected, - )); + // Scope and bound-claim failures map to distinct errors: a scope + // failure may carry the `/mcp` `insufficient_scope` challenge + // (AISIX-Cloud#1143), a policy denial never does. Both render the + // same 403 on the wire. + if let Err(rejection) = check_provider_claims(&claims, prov) { + let (reason, err) = match rejection { + ClaimsRejection::Scope => ( + "jwt_scope_missing", + ProxyError::JwtInsufficientScope { + required_scopes: prov.required_scopes.clone(), + }, + ), + ClaimsRejection::BoundClaim => { + ("jwt_bound_claim_mismatch", ProxyError::JwtClaimsRejected) + } + }; + return Err(deny(state, reason, &iss, kid.as_deref(), err)); } // ── Identity mapping ───────────────────────────────────────────── @@ -477,12 +486,22 @@ fn validate_with_keys( Err((reason, err)) } +/// Which provider requirement a verified token failed. The two map to +/// different [`ProxyError`]s: a missing scope is curable by requesting +/// a broader grant (so the `/mcp` surface may challenge for it), a +/// `bound_claims` mismatch is an operator policy denial. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ClaimsRejection { + Scope, + BoundClaim, +} + /// Enforce the provider's `required_scopes` and `bound_claims`. Returns -/// the denial reason class on the first unmet requirement. +/// the rejection class of the first unmet requirement. fn check_provider_claims( claims: &serde_json::Value, prov: &OidcProvider, -) -> Result<(), &'static str> { +) -> Result<(), ClaimsRejection> { if !prov.required_scopes.is_empty() { let scopes = token_scopes(claims); if !prov @@ -490,7 +509,7 @@ fn check_provider_claims( .iter() .all(|req| scopes.iter().any(|s| s == req)) { - return Err("jwt_scope_missing"); + return Err(ClaimsRejection::Scope); } } if let Some(bound) = &prov.bound_claims { @@ -498,7 +517,7 @@ fn check_provider_claims( let matched = nested_claim(claims, path) .is_some_and(|actual| bound_claim_matches(actual, expect)); if !matched { - return Err("jwt_bound_claim_mismatch"); + return Err(ClaimsRejection::BoundClaim); } } } @@ -1177,14 +1196,14 @@ jyxumGxNpoIV8LlzsMsaWQ== no_scope["scope"] = serde_json::json!("openid"); assert_eq!( check_provider_claims(&no_scope, &prov), - Err("jwt_scope_missing") + Err(ClaimsRejection::Scope) ); let mut wrong_dept = good.clone(); wrong_dept["department"] = serde_json::json!("finance"); assert_eq!( check_provider_claims(&wrong_dept, &prov), - Err("jwt_bound_claim_mismatch") + Err(ClaimsRejection::BoundClaim) ); // A missing bound claim denies — never a silent pass. @@ -1192,7 +1211,7 @@ jyxumGxNpoIV8LlzsMsaWQ== missing.as_object_mut().unwrap().remove("department"); assert_eq!( check_provider_claims(&missing, &prov), - Err("jwt_bound_claim_mismatch") + Err(ClaimsRejection::BoundClaim) ); // Non-string claim shapes never match. @@ -1200,7 +1219,7 @@ jyxumGxNpoIV8LlzsMsaWQ== numeric["department"] = serde_json::json!(7); assert_eq!( check_provider_claims(&numeric, &prov), - Err("jwt_bound_claim_mismatch") + Err(ClaimsRejection::BoundClaim) ); } diff --git a/crates/aisix-proxy/src/lib.rs b/crates/aisix-proxy/src/lib.rs index 742a60cc..7bd07fb6 100644 --- a/crates/aisix-proxy/src/lib.rs +++ b/crates/aisix-proxy/src/lib.rs @@ -46,6 +46,7 @@ mod images; mod jobs; mod jwt; mod mcp; +mod mcp_auth; mod messages; mod model_resolve; mod models; @@ -155,10 +156,34 @@ pub fn build_router(state: ProxyState) -> Router { "/passthrough/:provider/*rest", any(passthrough::passthrough), ) - // Downstream-facing MCP gateway. Authentication (AISIX API key) is - // enforced inside the handler via the `AuthenticatedKey` extractor. - .route("/mcp", any(mcp::mcp_endpoint)) - .route("/mcp/", any(mcp::mcp_endpoint)) + // RFC 9728 Protected Resource Metadata for the /mcp OAuth surface + // (AISIX-Cloud#1143). Unauthenticated by design — discovery must + // precede auth; 404 while the surface is dormant. Both the root + // and the path-insertion form are served: spec-strict clients + // resolve the latter, while several mainstream clients ignore + // path segments and fetch the former. + .route( + "/.well-known/oauth-protected-resource", + get(mcp_auth::protected_resource_metadata), + ) + .route( + "/.well-known/oauth-protected-resource/mcp", + get(mcp_auth::protected_resource_metadata), + ) + // Downstream-facing MCP gateway. Authentication (AISIX API key or, + // with trust providers configured, an IdP-issued JWT) is enforced + // inside the handler via the `AuthenticatedKey` extractor. The + // nested router scopes the OAuth challenge middleware to the /mcp + // surface only (AISIX-Cloud#1143). + .merge( + Router::new() + .route("/mcp", any(mcp::mcp_endpoint)) + .route("/mcp/", any(mcp::mcp_endpoint)) + .route_layer(middleware::from_fn_with_state( + state.clone(), + mcp_auth::challenge_middleware, + )), + ) // Downstream-facing A2A gateway. One route per registered agent; the // agent's card (with the service URL rewritten to the gateway) is served // at the RFC 8615 well-known path under it. Authentication (AISIX API @@ -727,6 +752,173 @@ mod tests { assert_eq!(v["error"]["type"], "invalid_api_key"); } + // ─── /mcp OAuth discovery surface (AISIX-Cloud#1143) ───────────── + + /// Activate the discovery surface on a snapshot: a valid + /// mcp_auth_settings row plus one enabled trust provider. + fn seed_oauth_discovery(snap: &AisixSnapshot) { + let settings: aisix_core::models::McpAuthSettings = + serde_json::from_str(r#"{"resource_url": "https://gw.example.com/mcp"}"#).unwrap(); + snap.mcp_auth_settings + .insert(ResourceEntry::new("env-1", settings, 1)); + let provider: aisix_core::models::OidcProvider = serde_json::from_str( + r#"{"name":"corp","issuer":"https://sso.example.com/realms/agents", + "audiences":["https://gw.example.com/mcp"], + "required_scopes":["mcp:tools"]}"#, + ) + .unwrap(); + snap.oidc_providers + .insert(ResourceEntry::new("op-1", provider, 1)); + } + + #[tokio::test] + async fn prm_endpoints_serve_the_document_when_active() { + let hub = Arc::new(Hub::new()); + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + seed_oauth_discovery(&snap); + let app = build_router(build_state(snap, hub)); + + for path in [ + "/.well-known/oauth-protected-resource", + "/.well-known/oauth-protected-resource/mcp", + ] { + let req = Request::builder() + .method("GET") + .uri(path) + .body(Body::empty()) + .unwrap(); + let resp = run(app.clone(), req).await; + assert_eq!(resp.status(), StatusCode::OK, "{path}"); + let bytes = to_bytes(resp.into_body(), 4096).await.unwrap(); + let v: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(v["resource"], "https://gw.example.com/mcp", "{path}"); + assert_eq!( + v["authorization_servers"], + serde_json::json!(["https://sso.example.com/realms/agents"]), + "{path}" + ); + assert_eq!(v["scopes_supported"], serde_json::json!(["mcp:tools"])); + } + } + + #[tokio::test] + async fn prm_endpoints_404_while_dormant() { + let hub = Arc::new(Hub::new()); + // No mcp_auth_settings row: pre-#1143 state, byte-identical. + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + let app = build_router(build_state(snap, hub)); + + for path in [ + "/.well-known/oauth-protected-resource", + "/.well-known/oauth-protected-resource/mcp", + ] { + let req = Request::builder() + .method("GET") + .uri(path) + .body(Body::empty()) + .unwrap(); + let resp = run(app.clone(), req).await; + assert_eq!(resp.status(), StatusCode::NOT_FOUND, "{path}"); + } + } + + #[tokio::test] + async fn mcp_auth_failures_carry_the_challenge_when_active() { + let hub = Arc::new(Hub::new()); + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + seed_oauth_discovery(&snap); + let app = build_router(build_state(snap, hub)); + + // No credentials: bare resource_metadata. + let req = Request::builder() + .method("POST") + .uri("/mcp") + .header("content-type", "application/json") + .body(Body::from( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/list"}"#, + )) + .unwrap(); + let resp = run(app.clone(), req).await; + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + let challenge = resp + .headers() + .get("www-authenticate") + .expect("401 on active /mcp must carry WWW-Authenticate") + .to_str() + .unwrap() + .to_string(); + assert_eq!( + challenge, + "Bearer resource_metadata=\ + \"https://gw.example.com/.well-known/oauth-protected-resource/mcp\"" + ); + + // A presented-but-unknown credential adds error="invalid_token". + let req = Request::builder() + .method("POST") + .uri("/mcp") + .header("authorization", "Bearer sk-wrong") + .header("content-type", "application/json") + .body(Body::from( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/list"}"#, + )) + .unwrap(); + let resp = run(app, req).await; + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + let challenge = resp + .headers() + .get("www-authenticate") + .expect("rejected credential must carry WWW-Authenticate") + .to_str() + .unwrap() + .to_string(); + assert!(challenge.contains("error=\"invalid_token\""), "{challenge}"); + assert!(challenge.contains("resource_metadata="), "{challenge}"); + } + + #[tokio::test] + async fn mcp_401_has_no_challenge_while_dormant() { + let hub = Arc::new(Hub::new()); + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + let app = build_router(build_state(snap, hub)); + + let req = Request::builder() + .method("POST") + .uri("/mcp") + .header("content-type", "application/json") + .body(Body::from( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/list"}"#, + )) + .unwrap(); + let resp = run(app, req).await; + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert!( + resp.headers().get("www-authenticate").is_none(), + "dormant /mcp must stay byte-identical to pre-#1143" + ); + } + + #[tokio::test] + async fn v1_auth_failures_never_carry_the_challenge() { + let hub = Arc::new(Hub::new()); + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + seed_oauth_discovery(&snap); + let app = build_router(build_state(snap, hub)); + + let req = Request::builder() + .method("POST") + .uri("/v1/chat/completions") + .header("content-type", "application/json") + .body(Body::from(r#"{"model":"my-gpt4","messages":[]}"#)) + .unwrap(); + let resp = run(app, req).await; + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert!( + resp.headers().get("www-authenticate").is_none(), + "the challenge is scoped to /mcp; /v1 is unchanged" + ); + } + #[tokio::test] async fn livez_reports_plain_ok_by_default() { let hub = Arc::new(Hub::new()); diff --git a/crates/aisix-proxy/src/mcp_auth.rs b/crates/aisix-proxy/src/mcp_auth.rs new file mode 100644 index 00000000..991918ab --- /dev/null +++ b/crates/aisix-proxy/src/mcp_auth.rs @@ -0,0 +1,402 @@ +//! Inbound MCP OAuth 2.1 discovery surface (AISIX-Cloud#1143). +//! +//! Makes `/mcp` a spec-compliant OAuth 2.1 resource server per the MCP +//! authorization spec (2025-11-25): an RFC 9728 Protected Resource +//! Metadata document under `/.well-known/oauth-protected-resource` +//! (both the root and the path-insertion `/mcp` form), and RFC 6750 +//! `WWW-Authenticate` challenges on `/mcp` auth failures so a standard +//! MCP client can discover the authorization server from a bare 401. +//! +//! The surface is ACTIVE only when the environment projects a valid +//! [`McpAuthSettings`] row (the canonical `/mcp` resource URL) AND at +//! least one enabled `oidc_providers` row exists. Dormant otherwise: +//! the well-known routes 404 and no challenge header is attached, so +//! every pre-#1143 environment is byte-identical to before. +//! +//! Token validation itself is unchanged — `crate::jwt` already enforces +//! signature/`iss`/`exp`/`aud` (inclusion semantics) and maps the +//! identity onto a bound API key. This module only adds the discovery +//! face in front of it. + +use std::collections::BTreeSet; +use std::sync::Once; + +use aisix_core::models::McpAuthSettings; +use aisix_core::AisixSnapshot; +use axum::extract::{Request, State}; +use axum::http::{header, HeaderValue, StatusCode}; +use axum::middleware::Next; +use axum::response::{IntoResponse, Response}; +use axum::Json; +use serde_json::json; + +use crate::error::AuthChallenge; +use crate::state::ProxyState; + +/// The resolved discovery identity of an active environment. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct DiscoveryIdentity { + /// The canonical `/mcp` resource URI, published verbatim as the PRM + /// `resource` (never derived from the request Host header). + pub resource_url: String, + /// Absolute URL of the path-insertion PRM endpoint, carried in + /// every challenge's `resource_metadata` attribute. Derived from + /// `resource_url`'s origin, never from the request. + pub challenge_url: String, +} + +/// Resolve the discovery identity, or `None` while the surface is +/// dormant (no settings row, no enabled provider, or a malformed row). +/// +/// A malformed row (unparseable URL, non-http(s) scheme, query or +/// fragment, path ≠ `/mcp`) keeps the surface dormant and logs one +/// process-wide warning — never per request. The absent-row state is a +/// legitimate steady state (every pre-#1143 environment) and logs +/// nothing. +pub(crate) fn discovery_identity(snapshot: &AisixSnapshot) -> Option { + let mut entries = snapshot.mcp_auth_settings.entries(); + if entries.is_empty() { + return None; + } + if entries.len() > 1 { + // The CP keys the row by the environment id, so a second row + // should be impossible; pick deterministically and say so once. + static MULTI_WARN: Once = Once::new(); + MULTI_WARN.call_once(|| { + tracing::warn!( + target: "aisix::mcp_auth", + count = entries.len(), + "multiple mcp_auth_settings rows in snapshot; using the smallest id", + ); + }); + entries.sort_by(|a, b| a.id.cmp(&b.id)); + } + let settings = &entries[0]; + + let Some(identity) = validate_resource_url(&settings.value) else { + static MALFORMED_WARN: Once = Once::new(); + MALFORMED_WARN.call_once(|| { + tracing::warn!( + target: "aisix::mcp_auth", + "mcp_auth_settings.resource_url is not an absolute http(s) URL with path \ + exactly /mcp and no query or fragment; the OAuth discovery surface \ + stays dormant", + ); + }); + return None; + }; + + if !crate::jwt::any_enabled_provider(snapshot) { + return None; + } + Some(identity) +} + +/// Parse and validate the configured resource URL: absolute `http`/ +/// `https`, no query, no fragment, path exactly `/mcp` (the gateway's +/// fixed MCP route — the CP rejects anything else at write time; this +/// is defense in depth on the projected row). +fn validate_resource_url(settings: &McpAuthSettings) -> Option { + let parsed = url::Url::parse(&settings.resource_url).ok()?; + if !matches!(parsed.scheme(), "http" | "https") { + return None; + } + if parsed.query().is_some() || parsed.fragment().is_some() { + return None; + } + if parsed.path() != "/mcp" { + return None; + } + // `Origin::ascii_serialization` renders `scheme://host[:port]` with + // default ports elided — exactly the base the well-known routes are + // reachable under. + let origin = parsed.origin().ascii_serialization(); + Some(DiscoveryIdentity { + resource_url: settings.resource_url.clone(), + challenge_url: format!("{origin}/.well-known/oauth-protected-resource/mcp"), + }) +} + +/// The RFC 9728 Protected Resource Metadata document, derived entirely +/// from configuration: `authorization_servers` lists the enabled trust +/// providers' issuers, `scopes_supported` the union of their +/// `required_scopes` (omitted when empty). +fn prm_document(snapshot: &AisixSnapshot, identity: &DiscoveryIdentity) -> serde_json::Value { + let mut issuers: Vec = Vec::new(); + let mut scopes: BTreeSet = BTreeSet::new(); + for entry in snapshot.oidc_providers.entries() { + if !entry.value.enabled { + continue; + } + issuers.push(entry.value.issuer.clone()); + scopes.extend(entry.value.required_scopes.iter().cloned()); + } + issuers.sort(); + issuers.dedup(); + + let mut doc = json!({ + "resource": identity.resource_url, + "authorization_servers": issuers, + "bearer_methods_supported": ["header"], + }); + if !scopes.is_empty() { + doc["scopes_supported"] = json!(scopes.into_iter().collect::>()); + } + doc +} + +/// `GET /.well-known/oauth-protected-resource` and its RFC 9728 +/// path-insertion sibling `…/mcp`. Unauthenticated by design (discovery +/// must precede auth); 404 with an empty body while the surface is +/// dormant — indistinguishable from the route not existing. +pub(crate) async fn protected_resource_metadata(State(state): State) -> Response { + let snapshot = state.snapshot.load(); + let Some(identity) = discovery_identity(&snapshot) else { + return StatusCode::NOT_FOUND.into_response(); + }; + Json(prm_document(&snapshot, &identity)).into_response() +} + +/// The `WWW-Authenticate` value for one challenge classification. +/// Values interpolated into the header are defensively stripped of +/// `"` and `\` so a config value can never break out of the quoted +/// attribute (the CP validates both upstream; this is depth). +fn challenge_header_value(challenge: &AuthChallenge, challenge_url: &str) -> Option { + let quote = |s: &str| -> String { s.chars().filter(|c| *c != '"' && *c != '\\').collect() }; + let url = quote(challenge_url); + let value = match challenge { + AuthChallenge::MissingCredentials => { + format!("Bearer resource_metadata=\"{url}\"") + } + AuthChallenge::InvalidToken => { + format!("Bearer error=\"invalid_token\", resource_metadata=\"{url}\"") + } + AuthChallenge::InsufficientScope { required_scopes } => { + let scope = required_scopes + .iter() + .map(|s| quote(s)) + .collect::>() + .join(" "); + format!( + "Bearer error=\"insufficient_scope\", scope=\"{scope}\", \ + resource_metadata=\"{url}\"" + ) + } + }; + HeaderValue::from_str(&value).ok() +} + +/// Route-scoped middleware on the `/mcp` routes only: when the response +/// carries an [`AuthChallenge`] marker (attached by `ProxyError`'s +/// renderer) and the surface is active, attach the `WWW-Authenticate` +/// header. Everything else — inactive environments, non-auth errors, +/// success responses — passes through untouched. +pub(crate) async fn challenge_middleware( + State(state): State, + request: Request, + next: Next, +) -> Response { + let mut response = next.run(request).await; + let Some(challenge) = response.extensions().get::().cloned() else { + return response; + }; + let snapshot = state.snapshot.load(); + let Some(identity) = discovery_identity(&snapshot) else { + return response; + }; + if let Some(value) = challenge_header_value(&challenge, &identity.challenge_url) { + response + .headers_mut() + .insert(header::WWW_AUTHENTICATE, value); + } + response +} + +#[cfg(test)] +mod tests { + use super::*; + use aisix_core::models::OidcProvider; + use aisix_core::resource::ResourceEntry; + + fn settings_entry(id: &str, resource_url: &str) -> ResourceEntry { + let s: McpAuthSettings = + serde_json::from_str(&format!(r#"{{"resource_url": "{resource_url}"}}"#)).unwrap(); + ResourceEntry::new(id, s, 1) + } + + fn provider_entry(id: &str, json: &str) -> ResourceEntry { + let p: OidcProvider = serde_json::from_str(json).unwrap(); + ResourceEntry::new(id, p, 1) + } + + fn active_snapshot() -> AisixSnapshot { + let snap = AisixSnapshot::new(); + snap.mcp_auth_settings + .insert(settings_entry("env-1", "https://gw.example.com/mcp")); + snap.oidc_providers.insert(provider_entry( + "op-1", + r#"{"name":"corp","issuer":"https://sso.example.com/realms/agents", + "audiences":["https://gw.example.com/mcp"], + "required_scopes":["mcp:tools"]}"#, + )); + snap + } + + #[test] + fn dormant_without_settings_row() { + let snap = AisixSnapshot::new(); + snap.oidc_providers.insert(provider_entry( + "op-1", + r#"{"name":"corp","issuer":"https://sso.example.com","audiences":["a"]}"#, + )); + assert_eq!(discovery_identity(&snap), None); + } + + #[test] + fn dormant_without_enabled_provider() { + let snap = AisixSnapshot::new(); + snap.mcp_auth_settings + .insert(settings_entry("env-1", "https://gw.example.com/mcp")); + assert_eq!(discovery_identity(&snap), None); + // A disabled provider does not activate the surface either. + snap.oidc_providers.insert(provider_entry( + "op-1", + r#"{"name":"corp","issuer":"https://sso.example.com","audiences":["a"], + "enabled": false}"#, + )); + assert_eq!(discovery_identity(&snap), None); + } + + #[test] + fn dormant_on_malformed_resource_url() { + for bad in [ + "not a url", + "ftp://gw.example.com/mcp", + "https://gw.example.com/other", + "https://gw.example.com/mcp?x=1", + "https://gw.example.com/mcp#frag", + "https://gw.example.com/", + ] { + let snap = AisixSnapshot::new(); + snap.mcp_auth_settings.insert(settings_entry("env-1", bad)); + snap.oidc_providers.insert(provider_entry( + "op-1", + r#"{"name":"corp","issuer":"https://sso.example.com","audiences":["a"]}"#, + )); + assert_eq!(discovery_identity(&snap), None, "must stay dormant: {bad}"); + } + } + + #[test] + fn active_identity_derives_challenge_url_from_origin() { + let identity = discovery_identity(&active_snapshot()).expect("active"); + assert_eq!(identity.resource_url, "https://gw.example.com/mcp"); + assert_eq!( + identity.challenge_url, + "https://gw.example.com/.well-known/oauth-protected-resource/mcp" + ); + } + + #[test] + fn non_default_port_survives_in_challenge_url() { + let snap = AisixSnapshot::new(); + snap.mcp_auth_settings + .insert(settings_entry("env-1", "http://gw.internal:8443/mcp")); + snap.oidc_providers.insert(provider_entry( + "op-1", + r#"{"name":"corp","issuer":"https://sso.example.com","audiences":["a"]}"#, + )); + let identity = discovery_identity(&snap).expect("active"); + assert_eq!( + identity.challenge_url, + "http://gw.internal:8443/.well-known/oauth-protected-resource/mcp" + ); + } + + #[test] + fn prm_document_lists_enabled_issuers_and_scope_union() { + let snap = active_snapshot(); + // A second enabled provider with another scope, and a disabled + // one that must not appear. + snap.oidc_providers.insert(provider_entry( + "op-2", + r#"{"name":"partner","issuer":"https://partner.example.com", + "audiences":["https://gw.example.com/mcp"], + "required_scopes":["mcp:read","mcp:tools"]}"#, + )); + snap.oidc_providers.insert(provider_entry( + "op-3", + r#"{"name":"dormant","issuer":"https://off.example.com", + "audiences":["a"],"enabled":false}"#, + )); + let identity = discovery_identity(&snap).unwrap(); + let doc = prm_document(&snap, &identity); + assert_eq!(doc["resource"], "https://gw.example.com/mcp"); + assert_eq!( + doc["authorization_servers"], + json!([ + "https://partner.example.com", + "https://sso.example.com/realms/agents" + ]) + ); + assert_eq!(doc["bearer_methods_supported"], json!(["header"])); + assert_eq!(doc["scopes_supported"], json!(["mcp:read", "mcp:tools"])); + } + + #[test] + fn prm_document_omits_empty_scopes_supported() { + let snap = AisixSnapshot::new(); + snap.mcp_auth_settings + .insert(settings_entry("env-1", "https://gw.example.com/mcp")); + snap.oidc_providers.insert(provider_entry( + "op-1", + r#"{"name":"corp","issuer":"https://sso.example.com","audiences":["a"]}"#, + )); + let identity = discovery_identity(&snap).unwrap(); + let doc = prm_document(&snap, &identity); + assert!(doc.get("scopes_supported").is_none()); + } + + #[test] + fn challenge_values_per_classification() { + let url = "https://gw.example.com/.well-known/oauth-protected-resource/mcp"; + let h = challenge_header_value(&AuthChallenge::MissingCredentials, url).unwrap(); + assert_eq!( + h.to_str().unwrap(), + format!("Bearer resource_metadata=\"{url}\"") + ); + + let h = challenge_header_value(&AuthChallenge::InvalidToken, url).unwrap(); + assert_eq!( + h.to_str().unwrap(), + format!("Bearer error=\"invalid_token\", resource_metadata=\"{url}\"") + ); + + let h = challenge_header_value( + &AuthChallenge::InsufficientScope { + required_scopes: vec!["mcp:tools".into(), "mcp:read".into()], + }, + url, + ) + .unwrap(); + assert_eq!( + h.to_str().unwrap(), + format!( + "Bearer error=\"insufficient_scope\", scope=\"mcp:tools mcp:read\", \ + resource_metadata=\"{url}\"" + ) + ); + } + + #[test] + fn challenge_value_strips_quote_breakouts() { + let h = challenge_header_value( + &AuthChallenge::InsufficientScope { + required_scopes: vec!["a\"b\\c".into()], + }, + "https://gw.example.com/.well-known/oauth-protected-resource/mcp", + ) + .unwrap(); + assert!(h.to_str().unwrap().contains("scope=\"abc\"")); + } +} diff --git a/crates/aisix-server/src/export/document.rs b/crates/aisix-server/src/export/document.rs index dc80ed66..b160ef0a 100644 --- a/crates/aisix-server/src/export/document.rs +++ b/crates/aisix-server/src/export/document.rs @@ -318,6 +318,21 @@ pub fn build_export_document(snapshot: &AisixSnapshot, reveal_secrets: bool) -> ), ); + // mcp_auth_settings — singleton per environment with a fixed + // identity; no secrets (the resource URL is public discovery data). + push_kind( + &mut collections, + "mcp_auth_settings", + emit_entries( + &snapshot.mcp_auth_settings, + |_| "mcp_auth_settings".to_string(), + "mcp_auth_settings", + &mut diag, + |_, _, _| {}, + |_, _| {}, + ), + ); + // guardrail_attachments are consumed above to decide which guardrails // are gateway-wide; they are not a file collection of their own. diff --git a/schemas/resources/mcp_auth_settings.schema.json b/schemas/resources/mcp_auth_settings.schema.json new file mode 100644 index 00000000..ea7e0be0 --- /dev/null +++ b/schemas/resources/mcp_auth_settings.schema.json @@ -0,0 +1,16 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "additionalProperties": false, + "properties": { + "resource_url": { + "description": "Canonical URI of this environment's `/mcp` endpoint, e.g. `https://gw.example.com/mcp`. Published verbatim as the PRM document's `resource` (never derived from the request Host header) and the value the trust providers' `audiences` must include for OAuth-for-MCP tokens to validate. The URL path must be exactly `/mcp` — the gateway's fixed MCP route.", + "minLength": 1, + "type": "string" + } + }, + "required": [ + "resource_url" + ], + "title": "McpAuthSettings", + "type": "object" +} From e1b4b114b409978bdfe487d54892d647f7f961c5 Mon Sep 17 00:00:00 2001 From: Yuansheng Wang Date: Thu, 30 Jul 2026 21:57:46 +0800 Subject: [PATCH 2/2] fix(mcp): audit follow-ups on the discovery surface Addresses the independent pre-merge audit of #859: - reject userinfo in resource_url (validate_resource_url + a filesource load error mirroring the OIDC issuer/jwks_uri rule): the URL is published verbatim on the unauthenticated protected-resource-metadata endpoint, so an embedded credential must never activate the surface. - extract claims_rejection_error() and unit-test both arms, pinning the single construction site of JwtInsufficientScope (scope failures must carry the provider's required scopes; bound-claims denials must keep the challenge-less variant). - register the well-known routes with any() and gate GET/HEAD inside the handler, so a dormant environment answers the pre-existing bare 404 for every method (previously non-GET flipped to 405 even while dormant); active non-GET/HEAD now 405s with an Allow header. Tests pin both. - sanitize challenge-header interpolations to RFC 6750 NQCHAR: a space can no longer corrupt the scope list and a control byte loses one character instead of silently dropping the whole WWW-Authenticate header. - deterministic multi-row pick test, and a comment on the middleware's second snapshot load (accepted eventual consistency). --- crates/aisix-core/src/filesource/mod.rs | 15 +++++ crates/aisix-core/src/filesource/tests.rs | 18 ++++++ crates/aisix-proxy/src/jwt.rs | 62 ++++++++++++++---- crates/aisix-proxy/src/lib.rs | 47 +++++++++++--- crates/aisix-proxy/src/mcp_auth.rs | 77 ++++++++++++++++++++--- 5 files changed, 191 insertions(+), 28 deletions(-) diff --git a/crates/aisix-core/src/filesource/mod.rs b/crates/aisix-core/src/filesource/mod.rs index c354dee2..0ae7eead 100644 --- a/crates/aisix-core/src/filesource/mod.rs +++ b/crates/aisix-core/src/filesource/mod.rs @@ -609,6 +609,21 @@ pub fn load_from_str( } } + // Same rule for the MCP OAuth resource URL: it is published verbatim + // on the unauthenticated protected-resource-metadata endpoint, so an + // embedded credential would be world-readable. + for (_, scope, settings) in &mcp_auth_settings { + if url_has_credentials(&settings.resource_url) { + errors.push(LoadError { + scope: scope.clone(), + message: "mcp_auth_settings resource_url must not embed credentials (user \ + info or a token query parameter) — protected resource metadata \ + is public" + .into(), + }); + } + } + // An explicit `provider_key_id` must also resolve: in file mode every // provider-key id is derived from its name, so any other value is // guaranteed dangling and would only surface per-request. diff --git a/crates/aisix-core/src/filesource/tests.rs b/crates/aisix-core/src/filesource/tests.rs index 255c51c3..50048848 100644 --- a/crates/aisix-core/src/filesource/tests.rs +++ b/crates/aisix-core/src/filesource/tests.rs @@ -549,6 +549,24 @@ mcp_auth_settings: assert_eq!(entry.value.resource_url, "https://gw.example.com/mcp"); } +#[test] +fn mcp_auth_settings_resource_url_with_credentials_is_a_load_error() { + // The resource URL is served verbatim on the unauthenticated PRM + // endpoint, so embedded credentials must fail the load — same rule + // as OIDC issuer/jwks_uri. + let contents = r#" +_format_version: "1" +mcp_auth_settings: + - resource_url: https://user:s3cret@gw.example.com/mcp +"#; + let errs = errors_of(load(contents, &env_of(&[]))); + assert!( + errs.iter() + .any(|e| e.contains("must not embed credentials")), + "{errs:?}" + ); +} + #[test] fn duplicate_mcp_auth_settings_is_a_load_error() { // The kind is a per-environment singleton: its fixed identity makes diff --git a/crates/aisix-proxy/src/jwt.rs b/crates/aisix-proxy/src/jwt.rs index 75ef17e1..046bbb72 100644 --- a/crates/aisix-proxy/src/jwt.rs +++ b/crates/aisix-proxy/src/jwt.rs @@ -304,17 +304,7 @@ pub(crate) async fn authenticate_jwt( // (AISIX-Cloud#1143), a policy denial never does. Both render the // same 403 on the wire. if let Err(rejection) = check_provider_claims(&claims, prov) { - let (reason, err) = match rejection { - ClaimsRejection::Scope => ( - "jwt_scope_missing", - ProxyError::JwtInsufficientScope { - required_scopes: prov.required_scopes.clone(), - }, - ), - ClaimsRejection::BoundClaim => { - ("jwt_bound_claim_mismatch", ProxyError::JwtClaimsRejected) - } - }; + let (reason, err) = claims_rejection_error(rejection, prov); return Err(deny(state, reason, &iss, kid.as_deref(), err)); } @@ -496,6 +486,26 @@ enum ClaimsRejection { BoundClaim, } +/// Map a claims-rejection class onto its deny-reason string and +/// caller-visible error. Split out of `authenticate_jwt` so the single +/// construction site of [`ProxyError::JwtInsufficientScope`] — the +/// error that carries the `/mcp` `insufficient_scope` challenge — is +/// unit-testable (audit finding on #859). +fn claims_rejection_error( + rejection: ClaimsRejection, + prov: &OidcProvider, +) -> (&'static str, ProxyError) { + match rejection { + ClaimsRejection::Scope => ( + "jwt_scope_missing", + ProxyError::JwtInsufficientScope { + required_scopes: prov.required_scopes.clone(), + }, + ), + ClaimsRejection::BoundClaim => ("jwt_bound_claim_mismatch", ProxyError::JwtClaimsRejected), + } +} + /// Enforce the provider's `required_scopes` and `bound_claims`. Returns /// the rejection class of the first unmet requirement. fn check_provider_claims( @@ -1166,6 +1176,36 @@ jyxumGxNpoIV8LlzsMsaWQ== assert!(validate_with_keys(&sign(&c), Algorithm::RS256, &prov, &decoding_keys()).is_ok()); } + #[test] + fn claims_rejection_error_maps_scope_to_insufficient_scope_with_provider_scopes() { + let prov = test_provider( + r#"{ + "name": "test-idp", + "issuer": "https://idp.test/realms/agents", + "audiences": ["aisix"], + "required_scopes": ["ai.access", "mcp:tools"] + }"#, + ); + + let (reason, err) = claims_rejection_error(ClaimsRejection::Scope, &prov); + assert_eq!(reason, "jwt_scope_missing"); + match err { + ProxyError::JwtInsufficientScope { required_scopes } => { + assert_eq!( + required_scopes, + vec!["ai.access".to_string(), "mcp:tools".to_string()], + "the challenge must name the provider's required scopes", + ); + } + other => panic!("scope failure must map to JwtInsufficientScope, got {other:?}"), + } + + // A bound-claims policy denial keeps the challenge-less variant. + let (reason, err) = claims_rejection_error(ClaimsRejection::BoundClaim, &prov); + assert_eq!(reason, "jwt_bound_claim_mismatch"); + assert!(matches!(err, ProxyError::JwtClaimsRejected)); + } + #[test] fn scope_and_bound_claim_checks() { let prov = test_provider( diff --git a/crates/aisix-proxy/src/lib.rs b/crates/aisix-proxy/src/lib.rs index 7bd07fb6..e3ea82b1 100644 --- a/crates/aisix-proxy/src/lib.rs +++ b/crates/aisix-proxy/src/lib.rs @@ -161,14 +161,17 @@ pub fn build_router(state: ProxyState) -> Router { // precede auth; 404 while the surface is dormant. Both the root // and the path-insertion form are served: spec-strict clients // resolve the latter, while several mainstream clients ignore - // path segments and fetch the former. + // path segments and fetch the former. `any(...)` — not `get` — + // so a dormant environment keeps answering the bare 404 axum's + // fallback produced before these routes existed, for every + // method; the handler does the GET/HEAD gate itself. .route( "/.well-known/oauth-protected-resource", - get(mcp_auth::protected_resource_metadata), + any(mcp_auth::protected_resource_metadata), ) .route( "/.well-known/oauth-protected-resource/mcp", - get(mcp_auth::protected_resource_metadata), + any(mcp_auth::protected_resource_metadata), ) // Downstream-facing MCP gateway. Authentication (AISIX API key or, // with trust providers configured, an IdP-issued JWT) is enforced @@ -812,16 +815,40 @@ mod tests { "/.well-known/oauth-protected-resource", "/.well-known/oauth-protected-resource/mcp", ] { - let req = Request::builder() - .method("GET") - .uri(path) - .body(Body::empty()) - .unwrap(); - let resp = run(app.clone(), req).await; - assert_eq!(resp.status(), StatusCode::NOT_FOUND, "{path}"); + // Every method — not just GET — must keep answering the bare + // 404 the axum fallback produced before these routes existed. + for method in ["GET", "POST", "PUT", "DELETE"] { + let req = Request::builder() + .method(method) + .uri(path) + .body(Body::empty()) + .unwrap(); + let resp = run(app.clone(), req).await; + assert_eq!(resp.status(), StatusCode::NOT_FOUND, "{method} {path}"); + } } } + #[tokio::test] + async fn prm_endpoints_reject_non_get_methods_when_active() { + let hub = Arc::new(Hub::new()); + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + seed_oauth_discovery(&snap); + let app = build_router(build_state(snap, hub)); + + let req = Request::builder() + .method("POST") + .uri("/.well-known/oauth-protected-resource") + .body(Body::empty()) + .unwrap(); + let resp = run(app, req).await; + assert_eq!(resp.status(), StatusCode::METHOD_NOT_ALLOWED); + assert_eq!( + resp.headers().get("allow").and_then(|v| v.to_str().ok()), + Some("GET, HEAD") + ); + } + #[tokio::test] async fn mcp_auth_failures_carry_the_challenge_when_active() { let hub = Arc::new(Hub::new()); diff --git a/crates/aisix-proxy/src/mcp_auth.rs b/crates/aisix-proxy/src/mcp_auth.rs index 991918ab..4f0f96c7 100644 --- a/crates/aisix-proxy/src/mcp_auth.rs +++ b/crates/aisix-proxy/src/mcp_auth.rs @@ -101,6 +101,12 @@ fn validate_resource_url(settings: &McpAuthSettings) -> Option serde /// `GET /.well-known/oauth-protected-resource` and its RFC 9728 /// path-insertion sibling `…/mcp`. Unauthenticated by design (discovery -/// must precede auth); 404 with an empty body while the surface is -/// dormant — indistinguishable from the route not existing. -pub(crate) async fn protected_resource_metadata(State(state): State) -> Response { +/// must precede auth). Registered with `any(...)` and the method check +/// done here, AFTER the dormancy check: a dormant environment answers a +/// bare empty 404 for EVERY method — byte-identical to the route not +/// existing (axum's pre-#1143 fallback) — while an active one answers +/// 405 + `Allow` for non-GET/HEAD. +pub(crate) async fn protected_resource_metadata( + method: axum::http::Method, + State(state): State, +) -> Response { let snapshot = state.snapshot.load(); let Some(identity) = discovery_identity(&snapshot) else { return StatusCode::NOT_FOUND.into_response(); }; + if method != axum::http::Method::GET && method != axum::http::Method::HEAD { + let mut response = StatusCode::METHOD_NOT_ALLOWED.into_response(); + response + .headers_mut() + .insert(header::ALLOW, HeaderValue::from_static("GET, HEAD")); + return response; + } Json(prm_document(&snapshot, &identity)).into_response() } /// The `WWW-Authenticate` value for one challenge classification. -/// Values interpolated into the header are defensively stripped of -/// `"` and `\` so a config value can never break out of the quoted -/// attribute (the CP validates both upstream; this is depth). +/// Values interpolated into the header are filtered to RFC 6750 NQCHAR +/// (visible ASCII minus `"` and `\`) so a config value can neither +/// break out of the quoted attribute nor corrupt the space-separated +/// scope list, and `HeaderValue::from_str` cannot fail on them — a +/// stray control byte must lose one character, never the whole header +/// (the CP validates upstream; this is depth). fn challenge_header_value(challenge: &AuthChallenge, challenge_url: &str) -> Option { - let quote = |s: &str| -> String { s.chars().filter(|c| *c != '"' && *c != '\\').collect() }; + let quote = |s: &str| -> String { + s.chars() + .filter(|c| matches!(*c, '\x21' | '\x23'..='\x5B' | '\x5D'..='\x7E')) + .collect() + }; let url = quote(challenge_url); let value = match challenge { AuthChallenge::MissingCredentials => { @@ -191,6 +217,11 @@ fn challenge_header_value(challenge: &AuthChallenge, challenge_url: &str) -> Opt /// renderer) and the surface is active, attach the `WWW-Authenticate` /// header. Everything else — inactive environments, non-auth errors, /// success responses — passes through untouched. +/// +/// The snapshot is loaded here a second time (the auth decision loaded +/// its own copy), so a config swap mid-request can attach a challenge +/// reflecting newer config than the 401 was produced under. Accepted +/// eventual consistency: the client re-runs discovery and self-heals. pub(crate) async fn challenge_middleware( State(state): State, request: Request, @@ -276,6 +307,10 @@ mod tests { "https://gw.example.com/mcp?x=1", "https://gw.example.com/mcp#frag", "https://gw.example.com/", + // Userinfo would be served verbatim on the unauthenticated + // PRM endpoint — never activate on it. + "https://user:s3cret@gw.example.com/mcp", + "https://user@gw.example.com/mcp", ] { let snap = AisixSnapshot::new(); snap.mcp_auth_settings.insert(settings_entry("env-1", bad)); @@ -297,6 +332,18 @@ mod tests { ); } + #[test] + fn multiple_rows_pick_the_smallest_id_deterministically() { + // The CP keys the row by the environment id so a second row + // should be impossible; if one ever appears the pick must be + // stable across processes, not insertion-ordered. + let snap = active_snapshot(); // row id "env-1" + snap.mcp_auth_settings + .insert(settings_entry("env-0", "http://first.example.com/mcp")); + let identity = discovery_identity(&snap).expect("active"); + assert_eq!(identity.resource_url, "http://first.example.com/mcp"); + } + #[test] fn non_default_port_survives_in_challenge_url() { let snap = AisixSnapshot::new(); @@ -399,4 +446,20 @@ mod tests { .unwrap(); assert!(h.to_str().unwrap().contains("scope=\"abc\"")); } + + #[test] + fn challenge_value_survives_space_and_control_bytes_in_scopes() { + // A space would corrupt the RFC 6750 space-separated scope list + // and a control byte would previously make HeaderValue::from_str + // fail — dropping the WHOLE header, resource_metadata included. + // The NQCHAR filter must lose only the offending characters. + let h = challenge_header_value( + &AuthChallenge::InsufficientScope { + required_scopes: vec!["mcp tools".into(), "a\u{7}b".into()], + }, + "https://gw.example.com/.well-known/oauth-protected-resource/mcp", + ) + .expect("header must survive hostile scope values"); + assert!(h.to_str().unwrap().contains("scope=\"mcptools ab\"")); + } }