diff --git a/misc/shlib/shlib.bash b/misc/shlib/shlib.bash index 5674c5db05b50..9bc7a8dbd8134 100644 --- a/misc/shlib/shlib.bash +++ b/misc/shlib/shlib.bash @@ -281,6 +281,8 @@ trufflehog_jq_filter_common() { .Raw != "jdbc:postgresql://%s:%s/materialize" and .Raw != "postgres://postgres:$MATERIALIZE_PROD_SANDBOX_RDS_PASSWORD@$MATERIALIZE_PROD_SANDBOX_RDS_HOSTNAME:5432" and .Raw != "http://user:pass@example.com" and + .Raw != "https://user:pass@issuer.example.com" and + .Raw != "https://user@issuer.example.com" and .Raw != "-----BEGIN PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDDC5MP3v1BHOgI\n5SsmrW8mjxzQGOz0IlC5jp1muW/kpEoE9TG317TEnO5Uye6zZudkFCP8YGEiN3Mc\nFbTM7eX6PjAPdnGU7khuUt/20ZM+NX5kWZPrmPTh4WQaDCL7ah1LqzBaUAMaSXq8\niuy7LGJNF8wdx8L5BjDiGTTxZXOg0Haxknc7Mbiwc9z8eb7omvzQzsOwyqocrF2u\nz86TzX1jtHP48i5CxoRHKxE94De3tNxjT/Y3OZlS4QS7iekAOQ04DVV3GIHvRUXN\n2H8ayy4+yOdhHn6ER5Jn3lti1Q5XSrxkrYn7L1Vcj6IwZQhhF5vc+ovxOYb+8ert\nEo97tIkLAgMBAAECggEAQteHHRPKz9Mzs8Sxvo4GPv0hnzFDl0DhUE4PJCKdtYoV\n8dADq2DJiu3LAZS4cJPt7Y63bGitMRg2oyPPM8G9pD5Goy3wq9zjRqexKDlXUCTt\n/T7zofRny7c94m1RWb7ablGq/vBXt90BqnajvVtvDsN+iKAqccQM4ZdI3QdrEmt1\ncHex924itzG/mqbFTAfAmVj1ZsRnJp55Txy2gqq7jX00xDM8+H49SRvUu49N64LQ\n6BUWCgWCJePRtgjSHjboAzPqSkMdaTE/WDY2zgGF3Qfq4f6JCHKfm4QylCH4gYUU\n1Kf7ttmhu9NoZO+hczobKkxP9RtXfyTRH2bsJXy2HQKBgQDhHgavxk/ln5mdMGGw\nrQud2vF9n7UwFiysYxocIC5/CWD0GAhnawchjPypbW/7vKM5Z9zhW3eH1U9P13sa\n2xHfrU5BZ16rxoBbKNpcr7VeEbUBAsDoGV24xjoecp7rB2hZ+mGik5/5Ig1Rk1KH\ndcvYy2KSi1h4Sm+mXwimmA4VDQKBgQDdzW+5FPbdM2sUB2gLMQtn3ICjDSu6IQ+k\nd0p3WlTIT51RUsPXXKkk96O5anUbeB3syY8tSKPGggsaXaeL3o09yIamtERgCnn3\nd9IS+4VKPWQlFUICU1KrD+TO7IYIX04iXBuVE5ihv0q3mslhDotmX4kS38NtKEFF\njLjA2RvAdwKBgAFkIxxw+Ett+hALnX7vAtRd5wIku4TpjisejanA1Si50RyRDXQ+\nKBQf/+u4HmoK12Nibe4Cl7GCMvRGW59l3S1pr8MdtWsQVfi6Puc1usQzDdBMyQ5m\nIbsjlnZbtPm02QM9Vd8gVGvAtx5a77aglrrnPtuy+r/7jccUbURCSkv9AoGAH9m3\nWGmVRZBzqO2jWDATxjdY1ZE3nUPQHjrvG5KCKD2ehqYO72cj9uYEwcRyyp4GFhGf\nmM4cjo3wEDowrBoqSBv6kgfC5dO7TfkL1qP9sPp93gFeeD0E2wGuRrSaTqt46eA2\nKcMloNx6W0FD98cB55KCeY5eXtdwAA/EHBVRMeMCgYAd3n6PcL6rVXyE3+wRTKK4\n+zvx5sjTAnljr5ttbEnpZafzrYIfDpB8NNjexy83AeC0O13LvSHIFoTwP8sywJRO\nRxbPMjhEBdVZ5NxlxYer7yKN+h5OBJfrLswPku7y4vdFYK3x/lMuNQO61hb1VFHc\nT2BDTbF0QSlPxFsv18B9zg==\n-----END PRIVATE KEY-----\n" and .Raw != "postgres://materialize:materialize@environmentd:6875" and .Raw != "postgres://MATERIALIZE_USERNAME:APP_SPECIFIC_PASSWORD@MATERIALIZE_HOST:6875" and diff --git a/src/environmentd/src/http.rs b/src/environmentd/src/http.rs index b39776ee8ddfc..5caeb44a78631 100644 --- a/src/environmentd/src/http.rs +++ b/src/environmentd/src/http.rs @@ -127,6 +127,7 @@ mod memory; mod metrics; mod metrics_public; mod metrics_viz; +pub(crate) mod oauth_metadata; mod probe; mod prometheus; mod root; @@ -157,10 +158,24 @@ pub struct HttpConfig { pub allowed_origin_list: Vec, pub active_connection_counter: ConnectionCounter, pub helm_chart_version: Option, + /// Externally-visible host name for this environment (without scheme). + /// + /// Used as the canonical host when constructing absolute URLs that the + /// server needs to publish (e.g. the OAuth Protected Resource Metadata + /// `resource` field, RFC 9728). When `None`, callers fall back to the + /// request's `Host` header, which is correct for unproxied dev setups + /// but loses fidelity behind a load balancer that rewrites Host. + /// + /// We deliberately do NOT consult `X-Forwarded-Host` or + /// `X-Forwarded-Proto`: there is no proxy-trust model in environmentd + /// today, and an attacker reaching the server directly can otherwise + /// poison the published metadata URLs. + pub http_host_name: Option, pub concurrent_webhook_req: Arc, pub metrics: Metrics, pub metrics_registry: MetricsRegistry, pub mcp_metrics: mcp_metrics::McpMetrics, + pub oauth_metadata_metrics: oauth_metadata::OauthMetadataMetrics, pub allowed_roles: AllowedRoles, pub internal_route_config: Arc, pub routes_enabled: HttpRoutesEnabled, @@ -213,10 +228,12 @@ impl HttpServer { allowed_origin_list, active_connection_counter, helm_chart_version, + http_host_name, concurrent_webhook_req, metrics, metrics_registry, mcp_metrics, + oauth_metadata_metrics, allowed_roles, internal_route_config, routes_enabled, @@ -238,10 +255,12 @@ impl HttpServer { let frontegg_middleware = frontegg.clone(); let oidc_middleware_rx = oidc_rx.clone(); let adapter_client_middleware_rx = adapter_client_rx.clone(); + let http_host_name_middleware = http_host_name.clone(); let auth_middleware = middleware::from_fn(move |req, next| { let frontegg = frontegg_middleware.clone(); let oidc_rx = oidc_middleware_rx.clone(); let adapter_client_rx = adapter_client_middleware_rx.clone(); + let http_host_name = http_host_name_middleware.clone(); async move { http_auth( req, @@ -252,6 +271,7 @@ impl HttpServer { oidc_rx, adapter_client_rx, allowed_roles, + http_host_name, ) .await } @@ -504,6 +524,39 @@ impl HttpServer { if routes_enabled.mcp_agent || routes_enabled.mcp_developer { use tracing::info; + // RFC 9728 Protected Resource Metadata. Public route — MCP + // clients fetch it before they have a token. Sits on its own + // router so the auth middleware never runs on it. The handler + // returns 404 when no OAuth authorization server is configured + // (`oidc_issuer` dyncfg unset) OR when the listener uses the + // `None` authenticator (no token would ever be validated), so + // it is safe to enable unconditionally whenever MCP is enabled. + // RFC 9728 §3.1 lets clients look up per-resource metadata + // via a path-suffixed well-known URI before falling back to + // the bare one. The MCP endpoints share an identical + // metadata view today, so we serve the same handler at all + // three paths. + let oauth_metadata_router = Router::new() + .route( + oauth_metadata::PROTECTED_RESOURCE_METADATA_PATH, + routing::get(oauth_metadata::handle_protected_resource_metadata), + ) + .route( + oauth_metadata::PROTECTED_RESOURCE_METADATA_PATH_AGENT, + routing::get(oauth_metadata::handle_protected_resource_metadata), + ) + .route( + oauth_metadata::PROTECTED_RESOURCE_METADATA_PATH_DEVELOPER, + routing::get(oauth_metadata::handle_protected_resource_metadata), + ) + .layer(Extension(adapter_client_rx.clone())) + .layer(Extension(oauth_metadata::DiscoveryConfig { + http_host_name: http_host_name.clone(), + authenticator_kind, + })) + .layer(Extension(oauth_metadata_metrics.clone())); + router = router.merge(oauth_metadata_router); + let mut mcp_router = Router::new(); if routes_enabled.mcp_agent { @@ -717,7 +770,7 @@ async fn x_materialize_user_header_auth( Ok(next.run(req).await) } -type Delayed = Shared>; +pub(crate) type Delayed = Shared>; #[derive(Clone)] enum ConnProtocol { @@ -881,6 +934,30 @@ where } } +/// Per-request decision about which `WWW-Authenticate` challenges to emit +/// on a 401, computed by the auth middleware based on the request path. +/// +/// Carries both the `Basic` toggle (today's behavior, kept for the SQL HTTP +/// layer and friends) and an optional `Bearer` challenge with a +/// `resource_metadata` URL per RFC 9728. The latter is only set on routes +/// that opt in to OAuth discovery (today: `/api/mcp/*`); other routes +/// emit only `Basic` so their behavior is unchanged. +#[derive(Debug, Clone, Default)] +pub(crate) struct WwwAuthenticateChallenges { + /// Whether to emit `WWW-Authenticate: Basic realm=Materialize`. + pub include_basic: bool, + /// If `Some`, also emit `WWW-Authenticate: Bearer + /// resource_metadata=""`. The URL points at this server's RFC 9728 + /// Protected Resource Metadata document, which advertises the + /// authorization server the client should use. + pub bearer_resource_metadata: Option, + /// If `Some`, also emit `scope=""` inside the Bearer challenge. + /// Tells clients which OAuth scope to request a token with for this + /// resource. Only set in conjunction with `bearer_resource_metadata` + /// (a scope challenge with no resource hint would be confusing). + pub bearer_scope: Option<&'static str>, +} + #[derive(Debug, Error)] pub(crate) enum AuthError { #[error("role dissallowed")] @@ -889,7 +966,7 @@ pub(crate) enum AuthError { Frontegg(#[from] FronteggError), #[error("missing authorization header")] MissingHttpAuthentication { - include_www_authenticate_header: bool, + challenges: WwwAuthenticateChallenges, }, #[error("{0}")] MismatchedUser(String), @@ -913,13 +990,42 @@ impl IntoResponse for AuthError { // exception — its payload is a sanitized `OidcError::Display` that the // console embeds in the login-page error. let body = match &self { - AuthError::MissingHttpAuthentication { - include_www_authenticate_header, - } if *include_www_authenticate_header => { - headers.insert( - http::header::WWW_AUTHENTICATE, - HeaderValue::from_static("Basic realm=Materialize"), - ); + // Bearer goes first so OAuth-aware clients see it before the + // Basic fallback. RFC 7235 allows emitting multiple + // `WWW-Authenticate` headers; we use one per scheme so each + // challenge is unambiguously framed — some parsers struggle + // with multiple schemes on a single header value. + AuthError::MissingHttpAuthentication { challenges } => { + if let Some(resource_metadata) = &challenges.bearer_resource_metadata { + // `scope` is hard-coded to a vetted constant + // (`MCP_SCOPE`); only `resource_metadata` is derived + // from a header value, and `resolve_host` has already + // round-tripped it through the URI grammar. The quoted + // form follows RFC 6749 §3.3 / RFC 6750 §3. + let value = match &challenges.bearer_scope { + Some(scope) => format!( + "Bearer scope=\"{scope}\", resource_metadata=\"{resource_metadata}\"", + ), + None => format!("Bearer resource_metadata=\"{resource_metadata}\""), + }; + match HeaderValue::from_str(&value) { + Ok(v) => { + headers.append(http::header::WWW_AUTHENTICATE, v); + } + Err(e) => { + warn!( + "skipping Bearer WWW-Authenticate challenge: invalid header \ + value derived from resource_metadata={resource_metadata:?}: {e}", + ); + } + } + } + if challenges.include_basic { + headers.append( + http::header::WWW_AUTHENTICATE, + HeaderValue::from_static("Basic realm=Materialize"), + ); + } "unauthorized".to_string() } AuthError::OidcFailed(message) => message.clone(), @@ -998,6 +1104,7 @@ async fn http_auth( oidc_rx: Delayed, adapter_client_rx: Delayed, allowed_roles: AllowedRoles, + http_host_name: Option, ) -> Result { let creds = if let Some(basic) = req.headers().typed_get::>() { Some(Credentials::Password { @@ -1056,10 +1163,35 @@ async fn http_auth( } let path = req.uri().path(); - let include_www_authenticate_header = path == "/" + let include_basic = path == "/" || PROFILING_API_ENDPOINTS .iter() - .any(|prefix| path.starts_with(prefix)); + .any(|prefix| path.starts_with(prefix)) + || path.starts_with("/api/mcp/"); + // For the MCP routes we additionally advertise OAuth via RFC 9728 so + // clients like Claude Desktop's Custom Connectors and ChatGPT remote + // MCP can discover the authorization server. The `Basic` challenge + // stays on these routes so existing curl/Bearer-already users still + // see a usable challenge. See `crate::http::oauth_metadata` for the + // discovery document. + // OAuth discovery is only meaningful when the listener actually + // validates tokens. When the listener uses the `None` authenticator + // (anonymous_http_user), there is no OAuth flow to advertise. + let (bearer_resource_metadata, bearer_scope) = if path.starts_with("/api/mcp/") + && !matches!(authenticator_kind, listeners::AuthenticatorKind::None) + { + ( + oauth_metadata::metadata_url(&req, http_host_name.as_deref()), + Some(oauth_metadata::MCP_SCOPE), + ) + } else { + (None, None) + }; + let challenges = WwwAuthenticateChallenges { + include_basic, + bearer_resource_metadata, + bearer_scope, + }; let authenticator = get_authenticator( authenticator_kind, creds.as_ref(), @@ -1069,13 +1201,7 @@ async fn http_auth( ) .await; - let user = auth( - &authenticator, - creds, - allowed_roles, - include_www_authenticate_header, - ) - .await?; + let user = auth(&authenticator, creds, allowed_roles, &challenges).await?; // Add the authenticated user as an extension so downstream handlers can // inspect it if necessary. @@ -1164,7 +1290,11 @@ async fn init_ws( &adapter_client_rx, ) .await; - let user = auth(&authenticator, Some(creds), allowed_roles, false).await?; + // WebSocket init: no 401-with-challenge contract — the + // client is reading WS frames, not parsing HTTP headers — so + // we just suppress challenge emission entirely. + let no_challenges = WwwAuthenticateChallenges::default(); + let user = auth(&authenticator, Some(creds), allowed_roles, &no_challenges).await?; user } (None, None) => anyhow::bail!("expected auth information"), @@ -1271,7 +1401,7 @@ async fn auth( authenticator: &Authenticator, creds: Option, allowed_roles: AllowedRoles, - include_www_authenticate_header: bool, + challenges: &WwwAuthenticateChallenges, ) -> Result { let (name, external_metadata_rx, authenticated, groups) = match authenticator { Authenticator::Frontegg(frontegg) => match creds { @@ -1292,7 +1422,7 @@ async fn auth( } None => { return Err(AuthError::MissingHttpAuthentication { - include_www_authenticate_header, + challenges: challenges.clone(), }); } }, @@ -1306,7 +1436,7 @@ async fn auth( } _ => { return Err(AuthError::MissingHttpAuthentication { - include_www_authenticate_header, + challenges: challenges.clone(), }); } }, @@ -1315,7 +1445,7 @@ async fn auth( // If we do, it's a server misconfiguration. // Just in case, we return a 401 rather than panic. return Err(AuthError::MissingHttpAuthentication { - include_www_authenticate_header, + challenges: challenges.clone(), }); } Authenticator::Oidc(oidc) => match creds { @@ -1330,7 +1460,7 @@ async fn auth( } _ => { return Err(AuthError::MissingHttpAuthentication { - include_www_authenticate_header, + challenges: challenges.clone(), }); } }, diff --git a/src/environmentd/src/http/oauth_metadata.rs b/src/environmentd/src/http/oauth_metadata.rs new file mode 100644 index 0000000000000..62a80c2e1cba2 --- /dev/null +++ b/src/environmentd/src/http/oauth_metadata.rs @@ -0,0 +1,586 @@ +// Copyright Materialize, Inc. and contributors. All rights reserved. +// +// Use of this software is governed by the Business Source License +// included in the LICENSE file. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0. + +//! OAuth 2.0 Protected Resource Metadata for the MCP endpoints. +//! +//! Implements [RFC 9728] so MCP-aware clients (e.g. Claude Desktop's Custom +//! Connectors, ChatGPT remote MCP) can discover the authorization server +//! they need to authenticate against in order to call our MCP endpoints. +//! +//! The discovery document is published at +//! `/.well-known/oauth-protected-resource` and is **public** (no auth). +//! Clients reach it either by following the `resource_metadata` parameter +//! emitted in a `WWW-Authenticate` challenge from a 401 on `/api/mcp/*` +//! (see [`crate::http::AuthError`]), or by probing the well-known URI +//! directly. +//! +//! ## Host derivation +//! +//! The `resource` field and the `resource_metadata` URL embedded in the +//! 401 challenge are both absolute URLs and therefore depend on knowing +//! the externally-visible host of this environment. We resolve that in +//! this order: +//! +//! 1. `HttpConfig::http_host_name` (set by the operator). +//! 2. The request's `Host` header. +//! +//! We deliberately do **not** consult `X-Forwarded-Host` or +//! `X-Forwarded-Proto`: environmentd has no proxy-trust configuration +//! today, and trusting those headers blind would let any client reaching +//! the server directly poison the published metadata URLs (a host-header +//! injection on the OAuth flow). Deployments behind a load balancer that +//! rewrites `Host` are expected to set `http_host_name` explicitly. +//! +//! ## When the document is published +//! +//! The handler returns 404 when no OAuth flow is meaningful for the +//! listener — either because `oidc_issuer` is unset (no authorization +//! server to advertise) or because the listener's authenticator is +//! [`listeners::AuthenticatorKind::None`] (no token would ever be +//! validated). Returning an empty or fake document instead would mislead +//! clients into starting an OAuth dance they cannot complete. +//! +//! [RFC 9728]: https://datatracker.ietf.org/doc/html/rfc9728 + +use std::time::Duration; + +use axum::Extension; +use axum::extract::Request; +use axum::response::{IntoResponse, Response}; +use http::{HeaderValue, StatusCode}; +use mz_adapter::Client; +use mz_adapter_types::dyncfgs::OIDC_ISSUER; +use mz_ore::metric; +use mz_ore::metrics::MetricsRegistry; +use mz_server_core::listeners; +use prometheus::IntCounterVec; +use serde::Serialize; +use tracing::warn; +use url::Url; + +use crate::http::Delayed; + +/// Bounds how long the unauthenticated discovery handler waits for the +/// adapter client. Without this, a wedged adapter could pin connections +/// indefinitely from any caller on the network. +const ADAPTER_WAIT_TIMEOUT: Duration = Duration::from_secs(2); + +/// The well-known path served by this module. +/// +/// Per [RFC 9728 §3] this is the OAuth 2.0 Protected Resource Metadata +/// well-known URI; MCP clients probe it as a fallback when no +/// `resource_metadata` parameter is present in a 401's `WWW-Authenticate`. +/// +/// [RFC 9728 §3]: https://datatracker.ietf.org/doc/html/rfc9728#section-3 +pub(crate) const PROTECTED_RESOURCE_METADATA_PATH: &str = "/.well-known/oauth-protected-resource"; + +/// Path-suffixed aliases of [`PROTECTED_RESOURCE_METADATA_PATH`] per +/// RFC 9728 §3.1. Both MCP endpoints serve an identical metadata +/// document today, so the same handler is mounted at all three paths; +/// the aliases exist so strict clients that always probe with a path +/// suffix do not have to fall back to the bare URI. +pub(crate) const PROTECTED_RESOURCE_METADATA_PATH_AGENT: &str = + "/.well-known/oauth-protected-resource/api/mcp/agent"; +pub(crate) const PROTECTED_RESOURCE_METADATA_PATH_DEVELOPER: &str = + "/.well-known/oauth-protected-resource/api/mcp/developer"; + +/// OAuth scope advertised for the MCP endpoints. We do not enforce +/// scope server-side today (authorization happens at the SQL layer via +/// RBAC), so a single coarse scope is the honest advertisement: +/// clients know what to request and we don't overclaim per-route +/// authorization we are not performing. +pub(crate) const MCP_SCOPE: &str = "mcp.read"; + +/// `private` keeps shared caches (CDNs, forward proxies) from serving a +/// document built for one listener to clients of another; one hour +/// bounds how long an `oidc_issuer` change takes to be reflected +/// in the field. +const METADATA_CACHE_CONTROL: &str = "private, max-age=3600"; + +/// JSON shape returned by [`PROTECTED_RESOURCE_METADATA_PATH`]. A +/// strict subset of [RFC 9728 §2]; further fields can be added without +/// breaking clients per the RFC's extensibility guidance. +/// +/// [RFC 9728 §2]: https://datatracker.ietf.org/doc/html/rfc9728#section-2 +#[derive(Debug, Serialize, PartialEq, Eq)] +pub(crate) struct ProtectedResourceMetadata { + /// Canonical URL of this resource server. Clients use this as the + /// `resource` parameter in their token request (RFC 8707). + pub resource: String, + /// Authorization servers a client may use to obtain a token. Each + /// entry is an issuer URL whose metadata document is fetchable at + /// `/.well-known/oauth-authorization-server` (RFC 8414) + /// or `/.well-known/openid-configuration` (OIDC Discovery). + pub authorization_servers: Vec, + /// Mechanisms by which the client may present the bearer token. + /// We accept the `Authorization` header only. + pub bearer_methods_supported: Vec, + /// Scopes a client may request a token for; see [`MCP_SCOPE`]. + pub scopes_supported: Vec, +} + +/// Prometheus counter for the discovery endpoint, labeled by outcome. +/// `status` label values are stable strings (rather than HTTP status +/// numbers) so dashboards do not have to map integers to failure +/// paths: +/// +/// * `ok` — 200 with a metadata document. +/// * `no_auth_listener` — 404, listener does not validate tokens. +/// * `no_issuer` — 404, `oidc_issuer` is unset. +/// * `invalid_issuer` — 503, `oidc_issuer` is not a valid URL. +/// * `no_host` — 400, request had no usable host. +/// * `adapter_unavailable` — 503, adapter client not ready. +#[derive(Debug, Clone)] +pub struct OauthMetadataMetrics { + requests: IntCounterVec, +} + +impl OauthMetadataMetrics { + pub fn register_into(registry: &MetricsRegistry) -> Self { + Self { + requests: registry.register(metric!( + name: "mz_oauth_protected_resource_metadata_requests_total", + help: "Total number of requests to the OAuth Protected Resource Metadata endpoint.", + var_labels: ["status"], + )), + } + } + + fn inc(&self, status: &'static str) { + self.requests.with_label_values(&[status]).inc(); + } +} + +/// Per-listener config for the discovery handler. Carried as an axum +/// `Extension` because the same environmentd process can serve +/// listeners with different `authenticator_kind` and `http_host_name`. +#[derive(Debug, Clone)] +pub(crate) struct DiscoveryConfig { + /// Operator-configured external host (without scheme). Beats the + /// `Host` header when set. + pub http_host_name: Option, + /// `None` makes the handler refuse to publish: no OAuth flow to + /// advertise. + pub authenticator_kind: listeners::AuthenticatorKind, +} + +/// HTTP handler for [`PROTECTED_RESOURCE_METADATA_PATH`]. +/// +/// Always public — no authentication is performed, by design. RFC 9728 +/// places no auth requirements on this endpoint; clients must be able to +/// fetch it before they have a token. +/// +/// Returns 404 when this listener does not validate tokens (`None` +/// authenticator) or has no issuer configured, 503 if the adapter client +/// is not yet available (a brief window at startup), 400 if the request +/// has no host information to construct a URL with, and 200 with the +/// JSON document otherwise. +pub(crate) async fn handle_protected_resource_metadata( + Extension(adapter_client_rx): Extension>, + Extension(config): Extension, + Extension(metrics): Extension, + req: Request, +) -> Response { + if matches!( + config.authenticator_kind, + listeners::AuthenticatorKind::None + ) { + metrics.inc("no_auth_listener"); + return StatusCode::NOT_FOUND.into_response(); + } + + let Some(host) = resolve_host(&req, config.http_host_name.as_deref()) else { + warn!( + "oauth-protected-resource: no http_host_name configured and request has no Host header" + ); + metrics.inc("no_host"); + return (StatusCode::BAD_REQUEST, "no host available").into_response(); + }; + let resource = format!("{PUBLISHED_SCHEME}://{host}/api/mcp"); + + let adapter_client = + match tokio::time::timeout(ADAPTER_WAIT_TIMEOUT, adapter_client_rx.clone()).await { + Ok(Ok(client)) => client, + Ok(Err(_)) | Err(_) => { + metrics.inc("adapter_unavailable"); + return (StatusCode::SERVICE_UNAVAILABLE, "adapter not ready").into_response(); + } + }; + let system_vars = adapter_client.get_system_vars().await; + let Some(issuer) = OIDC_ISSUER.get(system_vars.dyncfgs()) else { + // No OAuth authorization server is configured. Per RFC 9728 the + // document MUST contain at least one entry in + // `authorization_servers`, so the honest response is 404 rather + // than an empty document that misleads the client. + metrics.inc("no_issuer"); + return StatusCode::NOT_FOUND.into_response(); + }; + let issuer = match validate_issuer_url(&issuer) { + Ok(validated) => validated.to_string(), + Err(err) => { + warn!(%issuer, error = %err, "oauth-protected-resource: refusing to publish invalid oidc_issuer"); + metrics.inc("invalid_issuer"); + return (StatusCode::SERVICE_UNAVAILABLE, err).into_response(); + } + }; + + let metadata = ProtectedResourceMetadata { + resource, + authorization_servers: vec![issuer], + bearer_methods_supported: vec!["header".to_string()], + scopes_supported: vec![MCP_SCOPE.to_string()], + }; + + let mut response = axum::Json(metadata).into_response(); + response.headers_mut().insert( + http::header::CACHE_CONTROL, + HeaderValue::from_static(METADATA_CACHE_CONTROL), + ); + metrics.inc("ok"); + response +} + +/// Validates `oidc_issuer` before it is published: parseable URL, +/// http/https scheme, no userinfo (we publish it on a public endpoint), +/// no query or fragment (RFC 8414 §2). Returns the **original** string +/// unchanged — `url.to_string()` would silently normalise (e.g. add a +/// trailing slash) and a mutated issuer would not match the `iss` +/// claim in tokens minted by the IdP. +fn validate_issuer_url(issuer: &str) -> Result<&str, &'static str> { + let url = Url::parse(issuer).map_err(|_| "oidc_issuer is not a parseable URL")?; + if !matches!(url.scheme(), "https" | "http") { + return Err("oidc_issuer must use the https or http scheme"); + } + if !url.username().is_empty() || url.password().is_some() { + return Err("oidc_issuer must not contain userinfo"); + } + if url.query().is_some() || url.fragment().is_some() { + return Err("oidc_issuer must not contain a query or fragment"); + } + Ok(issuer) +} + +/// Builds the absolute URL of the protected resource metadata document +/// for use as the `resource_metadata` parameter in a `WWW-Authenticate` +/// challenge. Returns `None` if the request lacks enough host information +/// to construct a URL; the caller is expected to skip the Bearer challenge +/// in that case rather than emit a malformed value. +pub(crate) fn metadata_url(req: &Request, http_host_name: Option<&str>) -> Option { + let host = resolve_host(req, http_host_name)?; + Some(format!( + "{PUBLISHED_SCHEME}://{host}{PROTECTED_RESOURCE_METADATA_PATH}" + )) +} + +/// Resolves the host string to embed in published absolute URLs. +/// +/// Prefers the operator-configured `http_host_name`; falls back to the +/// request's `Host` header. **Never consults `X-Forwarded-*`** — see the +/// module-level "Host derivation" notes. +/// +/// The returned value is parsed through [`http::uri::Authority`] before +/// it is returned, so it is guaranteed to be a syntactically valid +/// `host[:port]` per RFC 3986. This is the second layer of defense +/// against header-smuggling attacks: even if a future change accepts a +/// malicious value as input, the parser rejects anything containing +/// characters outside the URI host grammar (notably `"`, whitespace, +/// `;`, etc.) so the value cannot break out of the quoted +/// `resource_metadata="..."` parameter in a `WWW-Authenticate` challenge +/// or smuggle additional fields into the published `resource` URL. +fn resolve_host(req: &Request, http_host_name: Option<&str>) -> Option { + let candidate = http_host_name + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + .or_else(|| { + req.headers() + .get(http::header::HOST) + .and_then(|v| v.to_str().ok()) + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + })?; + // Round-trip through `http::uri::Authority` to confirm the value is + // a syntactically valid `host[:port]`. This rejects header-smuggling + // payloads (quotes, whitespace, control chars, parameter-delimiters) + // that `HeaderValue::from_str` lets through. + let authority = candidate.parse::().ok()?; + if authority.as_str() != candidate { + // The Authority parser is permissive about some forms (e.g. + // `userinfo@host`), so additionally require the parsed form to + // round-trip exactly. Anything that re-renders differently is + // refused. + return None; + } + Some(candidate) +} + +/// Scheme used in published URLs. OAuth 2.1 §3.1 requires `https` for +/// all OAuth endpoints, and environmentd is fronted by TLS in every +/// production deployment. Plain-HTTP local dev is not a supported +/// OAuth flow. +const PUBLISHED_SCHEME: &str = "https"; + +#[cfg(test)] +mod tests { + use super::*; + + use axum::body::Body; + use http::Request; + + fn req_with_host(host: &str) -> Request { + Request::builder() + .uri("https://ignored.example.com/") + .header(http::header::HOST, host) + .body(Body::empty()) + .unwrap() + } + + /// Pin the serialized field names — clients key off them. + #[mz_ore::test] + fn test_metadata_serialization_matches_rfc9728_field_names() { + let metadata = ProtectedResourceMetadata { + resource: "https://mcp.example.com/api/mcp".to_string(), + authorization_servers: vec!["https://auth.example.com".to_string()], + bearer_methods_supported: vec!["header".to_string()], + scopes_supported: vec!["mcp.read".to_string()], + }; + let json = serde_json::to_value(&metadata).unwrap(); + assert_eq!( + json, + serde_json::json!({ + "resource": "https://mcp.example.com/api/mcp", + "authorization_servers": ["https://auth.example.com"], + "bearer_methods_supported": ["header"], + "scopes_supported": ["mcp.read"], + }), + ); + } + + /// The well-known path is part of the public contract with clients. + #[mz_ore::test] + fn test_well_known_path_is_rfc9728_canonical() { + assert_eq!( + PROTECTED_RESOURCE_METADATA_PATH, + "/.well-known/oauth-protected-resource", + ); + } + + /// `http_host_name` beats the request's Host header. + #[mz_ore::test] + fn test_resolve_host_prefers_http_host_name() { + let req = req_with_host("internal.local:6876"); + assert_eq!( + resolve_host(&req, Some("public.example.com")).as_deref(), + Some("public.example.com"), + ); + } + + /// Empty or whitespace-only `http_host_name` falls back to `Host`. + #[mz_ore::test] + fn test_resolve_host_ignores_blank_config() { + let req = req_with_host("public.example.com"); + for blank in ["", " ", "\t"] { + assert_eq!( + resolve_host(&req, Some(blank)).as_deref(), + Some("public.example.com"), + "blank http_host_name = {blank:?} should fall back to Host", + ); + } + } + + /// Host header is the final fallback; port must be preserved. + #[mz_ore::test] + fn test_resolve_host_falls_back_to_host_header_with_port() { + let req = req_with_host("example.com:8080"); + assert_eq!( + resolve_host(&req, None).as_deref(), + Some("example.com:8080"), + ); + } + + /// Security regression guard: `X-Forwarded-Host` is never trusted + /// (see module-level "Host derivation" notes). + #[mz_ore::test] + fn test_resolve_host_ignores_x_forwarded_host() { + let req = Request::builder() + .uri("https://ignored.example.com/") + .header(http::header::HOST, "honest.example.com") + .header("x-forwarded-host", "evil.example.com") + .body(Body::empty()) + .unwrap(); + assert_eq!( + resolve_host(&req, None).as_deref(), + Some("honest.example.com"), + "X-Forwarded-Host must not influence the resolved host", + ); + } + + /// No host config and no `Host` header → `None`. + #[mz_ore::test] + fn test_resolve_host_returns_none_when_unavailable() { + let req = Request::builder() + .uri("https://ignored.example.com/") + .body(Body::empty()) + .unwrap(); + assert_eq!(resolve_host(&req, None), None); + } + + /// Defense in depth against header-smuggling: a Host value with + /// characters outside the URI host grammar must be rejected so it + /// cannot break out of the quoted `resource_metadata="..."` + /// parameter or smuggle a host into the published `resource` URL. + /// The payloads here are bytes that `HeaderValue::from_str` + /// already accepts (control bytes are blocked upstream). + #[mz_ore::test] + fn test_resolve_host_rejects_smuggled_characters() { + for malicious in [ + // The headline regression: closes the quoted + // resource_metadata parameter and injects a second one. + "attacker.example.net\" foo=bar", + // Whitespace splits the `host:port` token. + "host with space.example.com", + // Semicolons inside a quoted parameter still terminate + // `auth-param` values in some lenient parsers. + "host\";evil=1", + // Backslash is reserved in URI host grammar. + "host\\backslash", + // Quote alone is enough to terminate the parameter. + "host\"quote", + ] { + let req = req_with_host(malicious); + assert_eq!( + resolve_host(&req, None), + None, + "smuggling payload via Host header must be rejected: {malicious:?}", + ); + assert_eq!( + resolve_host(&req, Some(malicious)), + None, + "smuggling payload via http_host_name must be rejected: {malicious:?}", + ); + } + } + + /// Pin the assembled URL — accidental path drift (extra slashes, + /// dropped suffix) breaks every connected client. + #[mz_ore::test] + fn test_metadata_url_assembles_canonical_suffix() { + let req = req_with_host("example.com"); + assert_eq!( + metadata_url(&req, Some("public.example.com")).as_deref(), + Some("https://public.example.com/.well-known/oauth-protected-resource"), + ); + } + + /// Pin the path-suffixed aliases — strict RFC 9728 §3.1 clients + /// probe these before the bare URI. + #[mz_ore::test] + fn test_path_suffixed_alias_paths_are_correct() { + assert_eq!( + PROTECTED_RESOURCE_METADATA_PATH_AGENT, + "/.well-known/oauth-protected-resource/api/mcp/agent", + ); + assert_eq!( + PROTECTED_RESOURCE_METADATA_PATH_DEVELOPER, + "/.well-known/oauth-protected-resource/api/mcp/developer", + ); + } + + /// Pin the wire-visible scope string — clients ask the IdP for a + /// token with this exact value. + #[mz_ore::test] + fn test_mcp_scope_constant() { + assert_eq!(MCP_SCOPE, "mcp.read"); + } + + /// Counter families only appear in `gather()` after a label + /// combination is observed, so each status value is touched first. + #[mz_ore::test] + fn test_metric_registers() { + let registry = MetricsRegistry::new(); + let metrics = OauthMetadataMetrics::register_into(®istry); + for status in [ + "ok", + "no_auth_listener", + "no_issuer", + "invalid_issuer", + "no_host", + "adapter_unavailable", + ] { + metrics.inc(status); + } + let names: Vec = registry + .gather() + .iter() + .map(|m| m.name().to_string()) + .collect(); + assert!( + names + .iter() + .any(|n| n == "mz_oauth_protected_resource_metadata_requests_total"), + "metric should be registered, got: {names:?}", + ); + } + + /// IPv6 literals (`[::1]:8080`) are valid `Authority` syntax and + /// must round-trip — a regression silently breaks IPv6 deployments. + #[mz_ore::test] + fn test_resolve_host_accepts_ipv6_literal() { + for host in ["[::1]", "[::1]:8080", "[2001:db8::1]:443"] { + let req = req_with_host(host); + assert_eq!( + resolve_host(&req, None).as_deref(), + Some(host), + "IPv6 literal must round-trip through Authority: {host:?}", + ); + } + } + + /// Well-formed issuers pass through unchanged; normalisation + /// would break byte-for-byte match against the `iss` token claim. + #[mz_ore::test] + fn test_validate_issuer_url_accepts_well_formed() { + for issuer in [ + "https://issuer.example.com", + "https://issuer.example.com/realms/main", + "http://localhost:8080", + ] { + assert_eq!( + validate_issuer_url(issuer), + Ok(issuer), + "expected {issuer:?} to pass validation", + ); + } + } + + /// Rejects values that would confuse clients downstream or leak + /// embedded secrets in a public document. + #[mz_ore::test] + fn test_validate_issuer_url_rejects_invalid() { + // Format: (issuer, expected substring of the error reason). + let cases: &[(&str, &str)] = &[ + ("not a url", "parseable"), + ("issuer.example.com", "parseable"), + ("ftp://issuer.example.com", "scheme"), + ("https://user:pass@issuer.example.com", "userinfo"), + ("https://user@issuer.example.com", "userinfo"), + ("https://issuer.example.com?foo=bar", "query"), + ("https://issuer.example.com#frag", "query"), + ]; + for (issuer, expected_substr) in cases { + let err = validate_issuer_url(issuer) + .expect_err(&format!("expected {issuer:?} to be rejected as invalid",)); + assert!( + err.contains(expected_substr), + "for {issuer:?}, expected reason to contain {expected_substr:?}, got {err:?}", + ); + } + } +} diff --git a/src/environmentd/src/lib.rs b/src/environmentd/src/lib.rs index 8d60c15554070..f19432af18f0a 100644 --- a/src/environmentd/src/lib.rs +++ b/src/environmentd/src/lib.rs @@ -385,6 +385,8 @@ impl Listeners { let metrics_registry = config.metrics_registry.clone(); let metrics = http::Metrics::register_into(&metrics_registry, "mz_http"); let mcp_metrics = http::mcp_metrics::McpMetrics::register_into(&metrics_registry); + let oauth_metadata_metrics = + http::oauth_metadata::OauthMetadataMetrics::register_into(&metrics_registry); let mut http_listener_handles = BTreeMap::new(); for (name, listener) in self.http { let authenticator_kind = listener.config.authenticator_kind(); @@ -398,6 +400,7 @@ impl Listeners { adapter_client_rx: adapter_client_rx.clone(), active_connection_counter: active_connection_counter.clone(), helm_chart_version: config.helm_chart_version.clone(), + http_host_name: config.http_host_name.clone(), source, tls, authenticator_kind, @@ -409,6 +412,7 @@ impl Listeners { metrics: metrics.clone(), metrics_registry: metrics_registry.clone(), mcp_metrics: mcp_metrics.clone(), + oauth_metadata_metrics: oauth_metadata_metrics.clone(), allowed_roles: listener.config.allowed_roles(), internal_route_config: Arc::clone(&internal_route_config), routes_enabled: listener.config.routes.clone(), diff --git a/src/environmentd/tests/server.rs b/src/environmentd/tests/server.rs index 3ba3dbfa08c8c..84f4f24a64831 100644 --- a/src/environmentd/tests/server.rs +++ b/src/environmentd/tests/server.rs @@ -30,6 +30,7 @@ use futures::FutureExt; use http::Request; use itertools::Itertools; use jsonwebtoken::{DecodingKey, EncodingKey}; +use mz_auth::password::Password; use mz_environmentd::test_util::{self, Ca, KAFKA_ADDRS, PostgresErrorExt, make_pg_tls}; use mz_environmentd::{WebSocketAuth, WebSocketResponse}; use mz_frontegg_auth::{ @@ -5726,6 +5727,373 @@ fn test_mcp_agent_with_data_product() { ); } +/// Verifies that the `WWW-Authenticate` challenge on a 401 for an MCP +/// route includes both the Bearer (with `resource_metadata`) and Basic +/// challenges, so OAuth-aware clients (Claude Desktop, ChatGPT remote +/// MCP) can discover the protected resource metadata while existing +/// callers that send Basic still see a usable challenge. This is the +/// half of RFC 9728 that doesn't require an authorization server to be +/// configured — even with no `oidc_issuer` set, the Bearer challenge +/// itself fires (clients then probe the well-known URI and get a 404, +/// which is the documented fallback path). +#[mz_ore::test] +fn test_mcp_unauth_emits_bearer_and_basic_challenges() { + // Use the Frontegg-like Password authenticator setup so the MCP + // path actually hits the 401 (None authenticator would just default + // to anonymous_http_user). Frontegg is heavyweight to set up in a + // test, but Password gives us the same 401 surface for our purposes. + let server = test_util::TestHarness::default() + .with_mcp_routes(true, true) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .with_system_parameter_default("enable_mcp_developer".to_string(), "true".to_string()) + .with_password_auth(Password("mz_system_password".to_string())) + .start_blocking(); + + for endpoint in ["agent", "developer"] { + let url = format!("http://{}/api/mcp/{endpoint}", server.http_local_addr()); + let res = Client::new() + .post(&url) + .header("Content-Type", "application/json") + .body(r#"{"jsonrpc":"2.0","id":1,"method":"tools/list"}"#) + .send() + .unwrap(); + assert_eq!( + res.status(), + StatusCode::UNAUTHORIZED, + "MCP {endpoint} without auth should 401", + ); + let challenges: Vec = res + .headers() + .get_all(http::header::WWW_AUTHENTICATE) + .iter() + .map(|v| v.to_str().unwrap().to_string()) + .collect(); + assert!( + challenges.iter().any(|c| c.starts_with("Bearer ") + && c.contains("resource_metadata=") + && c.contains("/.well-known/oauth-protected-resource") + && c.contains("scope=\"mcp.read\"")), + "expected a Bearer challenge with resource_metadata AND scope on /api/mcp/{endpoint}, got: {challenges:?}", + ); + assert!( + challenges.iter().any(|c| c.starts_with("Basic ")), + "expected a Basic challenge alongside Bearer so curl/legacy callers \ + still see a usable challenge, got: {challenges:?}", + ); + } +} + +/// The SQL HTTP layer must NOT gain the Bearer challenge — only MCP +/// routes opt into OAuth discovery. Regression guard so we don't +/// accidentally widen the OAuth surface to the rest of the HTTP server. +#[mz_ore::test] +fn test_sql_http_unauth_keeps_only_basic_challenge() { + let server = test_util::TestHarness::default() + .with_password_auth(Password("mz_system_password".to_string())) + .start_blocking(); + + // `/` triggers the WWW-Authenticate path (see auth_middleware) so we + // get a 401 with the challenge header. + let res = Client::new() + .get(&format!("http://{}/", server.http_local_addr())) + .send() + .unwrap(); + assert_eq!(res.status(), StatusCode::UNAUTHORIZED); + let challenges: Vec = res + .headers() + .get_all(http::header::WWW_AUTHENTICATE) + .iter() + .map(|v| v.to_str().unwrap().to_string()) + .collect(); + assert!( + challenges.iter().any(|c| c.starts_with("Basic ")), + "SQL HTTP layer should still emit Basic challenge: {challenges:?}", + ); + assert!( + !challenges.iter().any(|c| c.starts_with("Bearer ")), + "SQL HTTP layer must NOT advertise OAuth: {challenges:?}", + ); +} + +/// The well-known protected resource metadata document is public (no +/// auth) and returns the configured authorization server when +/// `oidc_issuer` is set. Pinning the JSON shape protects MCP clients +/// from accidental contract drift. +/// +/// Uses `with_password_auth` because the discovery handler 404s on +/// `None`-authenticator listeners (it would be misleading to publish a +/// document when no token would ever be validated). Any authenticator +/// kind other than `None` would do; Password is the lightest to set up +/// in tests. +#[mz_ore::test] +fn test_oauth_protected_resource_metadata_advertises_issuer() { + let server = test_util::TestHarness::default() + .with_password_auth(Password("mz_system_password".to_string())) + .with_mcp_routes(true, false) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .with_system_parameter_default( + "oidc_issuer".to_string(), + "https://issuer.test.example.com".to_string(), + ) + .start_blocking(); + + let res = Client::new() + .get(&format!( + "http://{}/.well-known/oauth-protected-resource", + server.http_local_addr() + )) + .send() + .unwrap(); + assert_eq!(res.status(), StatusCode::OK); + let body: serde_json::Value = res.json().unwrap(); + + // `resource` is built from the request host and must point at the + // MCP path prefix — clients use this as the `aud` value in their + // RFC 8707 resource indicator. + let resource = body["resource"].as_str().expect("resource is a string"); + assert!( + resource.ends_with("/api/mcp"), + "resource should point at /api/mcp prefix, got: {resource}", + ); + + let authorization_servers: Vec = body["authorization_servers"] + .as_array() + .expect("authorization_servers is an array") + .iter() + .map(|v| v.as_str().unwrap().to_string()) + .collect(); + assert_eq!( + authorization_servers, + vec!["https://issuer.test.example.com".to_string()], + "should advertise the configured oidc_issuer verbatim", + ); + + let bearer_methods: Vec = body["bearer_methods_supported"] + .as_array() + .expect("bearer_methods_supported is an array") + .iter() + .map(|v| v.as_str().unwrap().to_string()) + .collect(); + assert_eq!( + bearer_methods, + vec!["header".to_string()], + "we only accept the Authorization header, not a query param", + ); + + let scopes_supported: Vec = body["scopes_supported"] + .as_array() + .expect("scopes_supported is an array") + .iter() + .map(|v| v.as_str().unwrap().to_string()) + .collect(); + assert_eq!( + scopes_supported, + vec!["mcp.read".to_string()], + "should advertise the mcp.read scope so clients know what to request", + ); +} + +/// Path-suffixed well-known URIs (RFC 9728 §3.1) MUST serve the same +/// document as the bare well-known URI. Strict clients look these up +/// first based on the resource path; if they 404 here, those clients +/// never make it to the working bare URI. +#[mz_ore::test] +fn test_oauth_protected_resource_metadata_path_suffixed_aliases() { + let server = test_util::TestHarness::default() + .with_password_auth(Password("mz_system_password".to_string())) + .with_mcp_routes(true, true) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .with_system_parameter_default("enable_mcp_developer".to_string(), "true".to_string()) + .with_system_parameter_default( + "oidc_issuer".to_string(), + "https://issuer.test.example.com".to_string(), + ) + .start_blocking(); + + for suffix in [ + "/.well-known/oauth-protected-resource", + "/.well-known/oauth-protected-resource/api/mcp/agent", + "/.well-known/oauth-protected-resource/api/mcp/developer", + ] { + let res = Client::new() + .get(&format!("http://{}{suffix}", server.http_local_addr())) + .send() + .unwrap(); + assert_eq!(res.status(), StatusCode::OK, "expected 200 at {suffix}"); + let body: serde_json::Value = res.json().unwrap(); + let authorization_servers: Vec = body["authorization_servers"] + .as_array() + .expect("authorization_servers is an array") + .iter() + .map(|v| v.as_str().unwrap().to_string()) + .collect(); + assert_eq!( + authorization_servers, + vec!["https://issuer.test.example.com".to_string()], + "all three URIs must serve the same metadata document", + ); + } +} + +/// An `oidc_issuer` value that does not parse as a URL must NOT be +/// published verbatim — RFC 9728 §3 requires entries in +/// `authorization_servers` to be valid URLs, and publishing garbage +/// would steer every client into an unrecoverable error on their next +/// discovery hop. The honest response is a 503 plus a server-side +/// warning so operators can see and fix the misconfiguration. +#[mz_ore::test] +fn test_oauth_protected_resource_metadata_rejects_invalid_issuer() { + let server = test_util::TestHarness::default() + .with_password_auth(Password("mz_system_password".to_string())) + .with_mcp_routes(true, false) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .with_system_parameter_default( + "oidc_issuer".to_string(), + // Whitespace + missing scheme: not a valid URI per `http::Uri`. + "not a url".to_string(), + ) + .start_blocking(); + + let res = Client::new() + .get(&format!( + "http://{}/.well-known/oauth-protected-resource", + server.http_local_addr() + )) + .send() + .unwrap(); + assert_eq!( + res.status(), + StatusCode::SERVICE_UNAVAILABLE, + "invalid oidc_issuer must surface as 503, not a published malformed doc", + ); +} + +/// When no `oidc_issuer` is configured on a listener that *does* +/// validate tokens, the endpoint MUST 404. RFC 9728 requires +/// `authorization_servers` to be non-empty, and returning an honest +/// 404 lets clients fall back to whatever else they support instead of +/// being misled by a half-baked document. +#[mz_ore::test] +fn test_oauth_protected_resource_metadata_404_when_no_issuer() { + let server = test_util::TestHarness::default() + .with_password_auth(Password("mz_system_password".to_string())) + .with_mcp_routes(true, false) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .start_blocking(); + + let res = Client::new() + .get(&format!( + "http://{}/.well-known/oauth-protected-resource", + server.http_local_addr() + )) + .send() + .unwrap(); + assert_eq!( + res.status(), + StatusCode::NOT_FOUND, + "no oidc_issuer => 404, not an empty metadata doc", + ); +} + +/// End-to-end discovery flow: an unauthenticated MCP request emits a +/// 401 with a `resource_metadata` URL on the same host as the request, +/// and the discovery document at that path resolves to a 200 with the +/// configured issuer. Pinning the sequence guards against any one +/// piece being broken in a way the per-piece tests would miss. +/// +/// The published `resource_metadata` URL uses the `https` scheme even +/// though the test listener is plain HTTP (we always advertise `https`; +/// see `oauth_metadata::PUBLISHED_SCHEME`), so we extract the path +/// suffix and re-issue the GET against the actual local HTTP listener. +#[mz_ore::test] +fn test_oauth_protected_resource_metadata_end_to_end_flow() { + let server = test_util::TestHarness::default() + .with_password_auth(Password("mz_system_password".to_string())) + .with_mcp_routes(true, false) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .with_system_parameter_default( + "oidc_issuer".to_string(), + "https://issuer.test.example.com".to_string(), + ) + .start_blocking(); + + let local_addr = server.http_local_addr(); + let mcp_res = Client::new() + .post(&format!("http://{local_addr}/api/mcp/agent")) + .header("Content-Type", "application/json") + .body(r#"{"jsonrpc":"2.0","id":1,"method":"tools/list"}"#) + .send() + .unwrap(); + assert_eq!(mcp_res.status(), StatusCode::UNAUTHORIZED); + + let bearer_challenge = mcp_res + .headers() + .get_all(http::header::WWW_AUTHENTICATE) + .iter() + .map(|v| v.to_str().unwrap().to_string()) + .find(|c| c.starts_with("Bearer ")) + .expect("a Bearer WWW-Authenticate challenge"); + let advertised_url = bearer_challenge + .split(',') + .find_map(|part| { + part.trim() + .strip_prefix("resource_metadata=\"") + .and_then(|s| s.strip_suffix('"')) + }) + .expect("resource_metadata parameter in Bearer challenge"); + assert!( + advertised_url.starts_with("https://"), + "discovery URL must always be https: {advertised_url}", + ); + + let path_suffix = advertised_url + .splitn(4, '/') + .nth(3) + .expect("path on the advertised URL"); + let discovery_res = Client::new() + .get(&format!("http://{local_addr}/{path_suffix}")) + .send() + .unwrap(); + assert_eq!(discovery_res.status(), StatusCode::OK); + let body: serde_json::Value = discovery_res.json().unwrap(); + assert_eq!( + body["authorization_servers"] + .as_array() + .and_then(|a| a.first()) + .and_then(|v| v.as_str()), + Some("https://issuer.test.example.com"), + ); +} + +/// A `None`-authenticator listener (anonymous_http_user) MUST 404 even +/// if an `oidc_issuer` is configured: publishing on a listener that +/// never validates tokens would mislead clients into a flow they cannot +/// complete. +#[mz_ore::test] +fn test_oauth_protected_resource_metadata_404_on_no_auth_listener() { + let server = test_util::TestHarness::default() + .with_mcp_routes(true, false) + .with_system_parameter_default("enable_mcp_agent".to_string(), "true".to_string()) + .with_system_parameter_default( + "oidc_issuer".to_string(), + "https://issuer.test.example.com".to_string(), + ) + .start_blocking(); + + let res = Client::new() + .get(&format!( + "http://{}/.well-known/oauth-protected-resource", + server.http_local_addr() + )) + .send() + .unwrap(); + assert_eq!( + res.status(), + StatusCode::NOT_FOUND, + "None-authenticator listener must not publish OAuth metadata even with oidc_issuer set", + ); +} + /// Verifies that MCP requests update the Prometheus counters and that /// tool calls record a histogram observation. #[mz_ore::test] diff --git a/test/mcp/listener_config_password.json b/test/mcp/listener_config_password.json new file mode 100644 index 0000000000000..c465374b12d3b --- /dev/null +++ b/test/mcp/listener_config_password.json @@ -0,0 +1,50 @@ +{ + "sql": { + "external": { + "addr": "0.0.0.0:6875", + "authenticator_kind": "Password", + "allowed_roles": "NormalAndInternal", + "enable_tls": false + }, + "internal": { + "addr": "0.0.0.0:6877", + "authenticator_kind": "None", + "allowed_roles": "Internal", + "enable_tls": false + } + }, + "http": { + "external": { + "addr": "0.0.0.0:6876", + "authenticator_kind": "Password", + "allowed_roles": "NormalAndInternal", + "enable_tls": false, + "routes": { + "base": true, + "webhook": false, + "internal": false, + "metrics": false, + "profiling": false, + "mcp_agent": true, + "mcp_developer": true, + "console_config": true + } + }, + "internal": { + "addr": "0.0.0.0:6878", + "authenticator_kind": "None", + "allowed_roles": "NormalAndInternal", + "enable_tls": false, + "routes": { + "base": true, + "webhook": false, + "internal": true, + "metrics": true, + "profiling": false, + "mcp_agent": true, + "mcp_developer": true, + "console_config": false + } + } + } +} diff --git a/test/mcp/mzcompose.py b/test/mcp/mzcompose.py index 7981b4e9c37d7..0b5544b91a13c 100644 --- a/test/mcp/mzcompose.py +++ b/test/mcp/mzcompose.py @@ -9,6 +9,8 @@ """End-to-end tests for the MCP (Model Context Protocol) HTTP endpoints.""" +import re + import requests from materialize import MZ_ROOT @@ -45,6 +47,14 @@ def post_mcp(c: Composition, endpoint: str, body: dict) -> requests.Response: def workflow_default(c: Composition) -> None: + for name in c.workflows: + if name == "default": + continue + with c.test_case(name): + c.workflow(name) + + +def workflow_endpoints(c: Composition) -> None: c.up("materialized") # MCP feature flags default to true; no explicit enable needed. @@ -352,6 +362,48 @@ def workflow_default(c: Composition) -> None: r = post_mcp(c, "developer", jsonrpc("tools/list")) assert r.status_code == 200 + # -- OAuth Protected Resource Metadata (RFC 9728) ------------------------- + # + # End-to-end coverage of the discovery endpoint that lets MCP-aware + # clients (Claude Desktop, ChatGPT remote MCP) negotiate OAuth. + # Three scenarios: + # + # 1. On the no-auth listener the endpoint MUST 404 — there is no + # OAuth flow to advertise when the listener doesn't validate + # tokens. This is the security canary: if the discovery endpoint + # ever starts publishing a document on a no-auth listener, + # something is wrong. + # 2. With `oidc_issuer` unset the endpoint MUST 404 even when the + # listener does validate tokens, because RFC 9728 requires at + # least one authorization server. + # 3. The 401 on `/api/mcp/*` does NOT emit a Bearer challenge on + # this no-auth listener — same reason: nothing to advertise. + + discovery_url = ( + f"http://localhost:{c.port('materialized', 6876)}" + "/.well-known/oauth-protected-resource" + ) + + with c.test_case("oauth_metadata_404_on_no_auth_listener"): + r = requests.get(discovery_url) + assert r.status_code == 404, ( + "discovery endpoint must 404 on a None-authenticator listener; " + f"got {r.status_code}: {r.text}" + ) + + with c.test_case("oauth_metadata_no_bearer_challenge_on_no_auth_listener"): + # MCP 401 path: with no auth configured the listener auto-provisions + # `anonymous_http_user` instead of returning 401, so we can't + # observe the challenge headers directly here. The unit/integration + # tests in src/environmentd/tests/server.rs cover the + # authenticated-listener case. This case asserts only that the + # MCP route still responds (so we know it is wired) and that the + # discovery endpoint stays a 404. + r = post_mcp(c, "agent", jsonrpc("tools/list")) + assert ( + r.status_code == 200 + ), f"MCP route should serve anon users on no-auth listener: {r.status_code}" + # -- agent: disable/enable via flag ---------------------------------------- with c.test_case("agent_disable_via_flag"): @@ -380,3 +432,117 @@ def workflow_default(c: Composition) -> None: r = post_mcp(c, "agent", jsonrpc("tools/list")) assert r.status_code == 200 + + +def workflow_oauth_metadata_host_injection(c: Composition) -> None: + with c.override( + Materialized( + listeners_config_path=f"{MZ_ROOT}/test/mcp/listener_config_password.json", + ) + ): + c.up("materialized") + base = f"http://localhost:{c.port('materialized', 6876)}" + c.sql( + "ALTER SYSTEM SET oidc_issuer = 'https://issuer.example.com'", + user="mz_system", + port=6877, + print_statement=False, + ) + + attacker_host = 'attacker.example.net" foo=bar' + attacker_prefix = "https://attacker.example.net" + + with c.test_case("vuln_www_authenticate_host_injection"): + r = requests.post( + f"{base}/api/mcp/agent", + json=jsonrpc("tools/list"), + headers={"X-Forwarded-Host": attacker_host}, + ) + assert r.status_code == 401, f"{r.status_code}: {r.text}" + challenges = r.headers.get("WWW-Authenticate", "") + # The Bearer challenge has both `scope` and `resource_metadata` + # parameters; their relative order is part of the wire format + # but we don't pin it here — what matters for this regression + # guard is that resource_metadata is not pointing at the + # attacker-controlled host. + m = re.search(r'resource_metadata="([^"]*)"', challenges) + assert m, challenges + assert not m.group(1).startswith(attacker_prefix), m.group(1) + + with c.test_case("vuln_metadata_resource_host_injection"): + r = requests.get( + f"{base}/.well-known/oauth-protected-resource", + headers={"X-Forwarded-Host": attacker_host}, + ) + assert r.status_code == 200, f"{r.status_code}: {r.text}" + resource = r.json().get("resource", "") + assert not resource.startswith(attacker_prefix), resource + + with c.test_case("vuln_metadata_cache_control_missing"): + r = requests.get(f"{base}/.well-known/oauth-protected-resource") + cache_control = r.headers.get("Cache-Control", "") + assert "no-store" in cache_control or "private" in cache_control, repr( + cache_control + ) + + +def workflow_oauth_metadata_extras(c: Composition) -> None: + """Smoke tests for the production-ready additions to the discovery + endpoint: `scopes_supported`, scope in WWW-Authenticate, path-suffixed + well-known aliases, and invalid-issuer rejection.""" + with c.override( + Materialized( + listeners_config_path=f"{MZ_ROOT}/test/mcp/listener_config_password.json", + ) + ): + c.up("materialized") + base = f"http://localhost:{c.port('materialized', 6876)}" + c.sql( + "ALTER SYSTEM SET oidc_issuer = 'https://issuer.example.com'", + user="mz_system", + port=6877, + print_statement=False, + ) + + with c.test_case("metadata_advertises_scope"): + r = requests.get(f"{base}/.well-known/oauth-protected-resource") + assert r.status_code == 200, f"{r.status_code}: {r.text}" + scopes = r.json().get("scopes_supported", []) + assert scopes == ["mcp.read"], scopes + + with c.test_case("path_suffixed_aliases_serve_same_doc"): + base_doc = requests.get( + f"{base}/.well-known/oauth-protected-resource" + ).json() + for suffix in ("api/mcp/agent", "api/mcp/developer"): + url = f"{base}/.well-known/oauth-protected-resource/{suffix}" + r = requests.get(url) + assert r.status_code == 200, f"{url}: {r.status_code}: {r.text}" + assert r.json() == base_doc, url + + with c.test_case("www_authenticate_emits_scope"): + r = requests.post( + f"{base}/api/mcp/agent", + json=jsonrpc("tools/list"), + ) + assert r.status_code == 401, f"{r.status_code}: {r.text}" + challenges = r.headers.get("WWW-Authenticate", "") + assert 'scope="mcp.read"' in challenges, challenges + assert "resource_metadata=" in challenges, challenges + + with c.test_case("invalid_issuer_returns_503"): + c.sql( + "ALTER SYSTEM SET oidc_issuer = 'not a url'", + user="mz_system", + port=6877, + print_statement=False, + ) + r = requests.get(f"{base}/.well-known/oauth-protected-resource") + assert r.status_code == 503, f"{r.status_code}: {r.text}" + # Restore so subsequent test cases (if any are added) start clean. + c.sql( + "ALTER SYSTEM SET oidc_issuer = 'https://issuer.example.com'", + user="mz_system", + port=6877, + print_statement=False, + )