diff --git a/crates/tinymcp-bus/src/errors/mod.rs b/crates/tinymcp-bus/src/errors/mod.rs index 5ab3781d..e9341e93 100644 --- a/crates/tinymcp-bus/src/errors/mod.rs +++ b/crates/tinymcp-bus/src/errors/mod.rs @@ -67,6 +67,8 @@ pub const CLIENT_BUILD: &str = "ai.tinyhumans.tinymcp.Error.ClientBuild"; pub const SERIALIZATION: &str = "ai.tinyhumans.tinymcp.Error.Serialization"; /// The installed-server store could not do what was asked of it. pub const STORE: &str = "ai.tinyhumans.tinymcp.Error.Store"; +/// A host-supplied credential store could not do what was asked of it. +pub const CREDENTIAL_STORE: &str = "ai.tinyhumans.tinymcp.Error.CredentialStore"; /// The store's directory or file could not be reached. pub const STORE_IO: &str = "ai.tinyhumans.tinymcp.Error.StoreIo"; /// The module could not do something it needed of the bus, such as serving a @@ -103,6 +105,7 @@ pub const ALL: &[&str] = &[ CLIENT_BUILD, SERIALIZATION, STORE, + CREDENTIAL_STORE, STORE_IO, BUS, INVALID_ARGUMENT, diff --git a/crates/tinymcp/src/error/mod.rs b/crates/tinymcp/src/error/mod.rs index 11e47810..24acbb16 100644 --- a/crates/tinymcp/src/error/mod.rs +++ b/crates/tinymcp/src/error/mod.rs @@ -254,6 +254,20 @@ pub enum Error { source: Box, }, + /// A host-supplied credential store could not do what was asked of it. + /// + /// Raised by a host's [`OAuthCredentialStore`] implementation, whose own + /// error type this crate cannot name; carried as prose, like [`Self::Bus`]. + /// + /// [`OAuthCredentialStore`]: crate::registry::OAuthCredentialStore + #[error("mcp credential store failure while {action}: {detail}")] + CredentialStore { + /// What was being attempted, in the present participle. + action: String, + /// What the host's store reported. Must not contain a secret. + detail: String, + }, + /// The store's directory or file could not be reached. #[error("mcp store is unreachable at `{}`: {source}", path.display())] StoreIo { @@ -392,6 +406,7 @@ impl Error { Self::ClientBuild { .. } => errors::CLIENT_BUILD, Self::Serialization { .. } => errors::SERIALIZATION, Self::Store { .. } => errors::STORE, + Self::CredentialStore { .. } => errors::CREDENTIAL_STORE, Self::StoreIo { .. } => errors::STORE_IO, Self::Bus { .. } => errors::BUS, Self::InvalidArgument { .. } => errors::INVALID_ARGUMENT, diff --git a/crates/tinymcp/src/error/mod_tests.rs b/crates/tinymcp/src/error/mod_tests.rs index cb3a3a04..962820b3 100644 --- a/crates/tinymcp/src/error/mod_tests.rs +++ b/crates/tinymcp/src/error/mod_tests.rs @@ -38,6 +38,10 @@ fn assorted_other_errors() -> Vec { Error::UnsupportedProtocolVersion { version: "1999-01-01".into(), }, + Error::CredentialStore { + action: "writing credentials".into(), + detail: "the vault is sealed".into(), + }, Error::MalformedResponse { detail: "no result member".into(), }, diff --git a/crates/tinymcp/src/registry/mod.rs b/crates/tinymcp/src/registry/mod.rs index d1a30502..8aad1335 100644 --- a/crates/tinymcp/src/registry/mod.rs +++ b/crates/tinymcp/src/registry/mod.rs @@ -23,7 +23,7 @@ pub mod supervisor; pub use boot::{BootOutcome, connect_installed_servers}; pub use config_doc::{AppliedServer, ConfigApplyReport}; pub use connections::{Connections, ProbeOutcome, REMOTE_REQUEST_TIMEOUT}; -pub use oauth::{AuthDetection, AuthKind, OAuthFlow}; +pub use oauth::{AuthDetection, AuthKind, OAuthCredentialStore, OAuthFlow}; pub use ops::McpRegistry; pub use setup::{SecretRef, SecretVault}; pub use sources::{Registries, RegistrySource}; diff --git a/crates/tinymcp/src/registry/oauth/credentials.rs b/crates/tinymcp/src/registry/oauth/credentials.rs new file mode 100644 index 00000000..cb31c653 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/credentials.rs @@ -0,0 +1,91 @@ +//! Where the flow keeps what it mints. +//! +//! The flow needs three things from storage: the endpoint of the server being +//! signed in to, and read/write access to that server's credential map — the +//! `Authorization` value and the refresh bundle under [`OAUTH_BUNDLE_KEY`]. +//! [`OAuthCredentialStore`] is exactly that, so a host can back sign-in and +//! refresh with its own secret store (a per-tenant vault, say) instead of the +//! `SQLite` [`Store`]. [`Store`] implements it, so existing callers are +//! unchanged. +//! +//! [`OAUTH_BUNDLE_KEY`]: super::OAUTH_BUNDLE_KEY + +use std::collections::BTreeMap; +use std::future::Future; + +use crate::error::Result; +use crate::registry::Store; +use tinymcp_bus::Transport; + +/// The storage the OAuth flow and [`refresh_if_expired`] read and write. +/// +/// `server_id` is the host's key for the server. It is opaque to the flow, so a +/// multi-tenant host can qualify it (`"/"`) and route each call +/// to the right tenant's secrets; [`OAuthFlow::pending_server`] hands it back +/// from a redirect's `state` before completing. +/// +/// Credentials are a whole map, replaced on write. The flow always reads, +/// merges and writes back, so a value the user set beside their sign-in +/// survives it. +/// +/// The futures are `Send` so the flow can run on a multi-threaded runtime. +/// +/// [`refresh_if_expired`]: super::refresh_if_expired +/// [`OAuthFlow::pending_server`]: super::OAuthFlow::pending_server +pub trait OAuthCredentialStore: Send + Sync { + /// The endpoint of an HTTP-remote server, or `None` when the server is not + /// one (a subprocess has no HTTP authorization). + /// + /// # Errors + /// + /// Whatever the store reports when the server cannot be looked up. + fn remote_url(&self, server_id: &str) -> impl Future>> + Send; + + /// Every credential stored for `server_id`; empty when there are none. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be read. + fn load_credentials( + &self, + server_id: &str, + ) -> impl Future>> + Send; + + /// Replaces every credential stored for `server_id` with `credentials`. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be written. + fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> impl Future> + Send; +} + +#[allow(clippy::unused_async_trait_impl)] +impl OAuthCredentialStore for Store { + async fn remote_url(&self, server_id: &str) -> Result> { + let server = self.get_server(server_id)?; + Ok(match server.transport { + Transport::HttpRemote { url } if !url.is_empty() => Some(url), + _ => None, + }) + } + + async fn load_credentials(&self, server_id: &str) -> Result> { + self.load_env_values(server_id) + } + + async fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> Result<()> { + self.set_env_values(server_id, credentials)?; + // The listing of credential *names* lives on the server row and is + // what a caller shows the user, so it has to learn about new keys. + let names: Vec = credentials.keys().cloned().collect(); + self.update_env_keys(server_id, &names) + } +} diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs new file mode 100644 index 00000000..2880611c --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs @@ -0,0 +1,128 @@ +//! Refusing internal targets among the endpoints a server advertises. +//! +//! The authorization, registration and token endpoints come from the server +//! being signed in to, and the flow then POSTs to them **from the host**. An +//! unchecked endpoint is a server-side request-forgery primitive: a hostile +//! server can aim the flow at an internal service or the cloud metadata +//! address. A host serving untrusted servers turns this guard on with +//! [`OAuthFlow::require_public_endpoints`](super::OAuthFlow::require_public_endpoints). +//! It is off by default because a desktop host legitimately signs in to +//! loopback development servers. + +use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; +use std::time::Duration; + +use reqwest::Url; + +use crate::error::{Error, Result}; + +/// Refuses `raw` unless it is `https` and every address its host resolves to +/// is public. `what` names the endpoint in the error. +/// +/// A single blocked record is enough to refuse. The returned addresses can be +/// pinned to the HTTP client so the eventual connection uses the vetted result +/// instead of resolving the name again. Resolution runs on the blocking pool +/// so a slow resolver cannot stall the executor. +/// +/// # Errors +/// +/// [`Error::MalformedResponse`] naming the endpoint and why it was refused. +pub(super) async fn guard_endpoint(raw: &str, what: &str) -> Result> { + let refuse = |why: String| Error::malformed(format!("{what} endpoint refused: {why}")); + let url = Url::parse(raw).map_err(|error| refuse(format!("not a valid url: {error}")))?; + if url.scheme() != "https" { + return Err(refuse(format!("must use https, not `{}`", url.scheme()))); + } + let host = url + .host_str() + .ok_or_else(|| refuse("it has no host".to_string()))? + .trim_start_matches('[') + .trim_end_matches(']') + .to_string(); + let port = url.port_or_known_default().unwrap_or(443); + + let addresses: Vec = if let Ok(ip) = host.parse::() { + vec![SocketAddr::new(ip, port)] + } else { + tokio::task::spawn_blocking(move || { + (host.as_str(), port) + .to_socket_addrs() + .map(Iterator::collect::>) + }) + .await + .map_err(|error| refuse(format!("resolution did not finish: {error}")))? + .map_err(|error| refuse(format!("its host does not resolve: {error}")))? + }; + if addresses.is_empty() { + return Err(refuse("its host does not resolve".to_string())); + } + if addresses.iter().any(|addr| is_blocked_ip(&addr.ip())) { + return Err(refuse("it resolves to a disallowed address".to_string())); + } + Ok(addresses) +} + +/// Builds an HTTP client pinned to the public addresses checked for `raw`. +/// Redirects are disabled so a validated endpoint cannot replay credentials to +/// a different, unchecked destination. +pub(super) async fn guarded_client(raw: &str, what: &str) -> Result { + let addresses = guard_endpoint(raw, what).await?; + let url = Url::parse(raw) + .map_err(|error| Error::malformed(format!("invalid {what} url: {error}")))?; + let host = url + .host_str() + .ok_or_else(|| Error::malformed(format!("{what} endpoint has no host")))? + .trim_start_matches('[') + .trim_end_matches(']'); + client_pinned_to(host, &addresses) +} + +fn client_pinned_to(host: &str, addresses: &[SocketAddr]) -> Result { + let mut builder = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .connect_timeout(Duration::from_secs(10)) + .redirect(reqwest::redirect::Policy::none()); + if host.parse::().is_err() { + builder = builder.resolve_to_addrs(host, addresses); + } + builder.build().map_err(|source| Error::ClientBuild { + source: Box::new(source.without_url()), + }) +} + +/// Whether an address is one the flow must never POST OAuth material to: +/// loopback, private or unique-local, link-local (the metadata address among +/// them), unspecified, broadcast, documentation, multicast, or `0.0.0.0/8`. +pub(super) fn is_blocked_ip(ip: &IpAddr) -> bool { + match ip { + IpAddr::V4(v4) => { + v4.is_loopback() + || v4.is_private() + || v4.is_link_local() + || v4.is_unspecified() + || v4.is_broadcast() + || v4.is_documentation() + || v4.is_multicast() + || v4.octets()[0] == 0 + || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xc0) == 64) + || (v4.octets()[0] == 198 && (v4.octets()[1] & 0xfe) == 18) + || v4.octets()[0] >= 240 + } + IpAddr::V6(v6) => { + if let Some(v4) = v6.to_ipv4() { + return is_blocked_ip(&IpAddr::V4(v4)); + } + v6.is_loopback() + || v6.is_unspecified() + || v6.is_multicast() + || (v6.segments()[0] & 0xfe00) == 0xfc00 + || (v6.segments()[0] & 0xffc0) == 0xfe80 + || (v6.segments()[0] == 0x0064 && v6.segments()[1] == 0xff9b) + || (v6.segments()[0] & 0xffc0) == 0xfec0 + } + } +} + +#[cfg(test)] +#[path = "endpoint_guard_tests.rs"] +mod tests; diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs new file mode 100644 index 00000000..c8646c6e --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs @@ -0,0 +1,111 @@ +//! Unit tests for the discovery-endpoint guard. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::net::IpAddr; + +use super::{client_pinned_to, guard_endpoint, guarded_client, is_blocked_ip}; + +fn ip(raw: &str) -> IpAddr { + raw.parse().unwrap() +} + +#[test] +fn internal_and_special_addresses_are_blocked() { + for raw in [ + "127.0.0.1", + "10.1.2.3", + "172.16.0.1", + "192.168.1.1", + "169.254.169.254", + "0.0.0.0", + "0.1.2.3", + "100.64.0.1", + "100.127.255.254", + "198.18.0.1", + "198.19.255.254", + "240.0.0.1", + "255.255.255.255", + "192.0.2.1", + "224.0.0.1", + "::1", + "::", + "fd00::1", + "fe80::1", + "ff02::1", + "64:ff9b::7f00:1", + "fec0::1", + "::127.0.0.1", + "::ffff:127.0.0.1", + "::ffff:169.254.169.254", + ] { + assert!(is_blocked_ip(&ip(raw)), "{raw} should be blocked"); + } +} + +#[test] +fn public_addresses_are_allowed() { + for raw in [ + "8.8.8.8", + "1.1.1.1", + "2606:4700:4700::1111", + "::ffff:8.8.8.8", + "2001:4860:4860::8888", + ] { + assert!(!is_blocked_ip(&ip(raw)), "{raw} should be allowed"); + } +} + +#[tokio::test] +async fn a_public_https_literal_passes() { + let addresses = guard_endpoint("https://8.8.8.8/token", "token") + .await + .expect("public https"); + assert_eq!(addresses[0].ip(), ip("8.8.8.8")); +} + +#[tokio::test] +async fn a_public_https_literal_builds_a_pinned_client() { + guarded_client("https://8.8.8.8/token", "token") + .await + .expect("pinned public https client"); +} + +#[test] +fn a_hostname_client_pins_the_addresses_that_were_checked() { + let addresses = ["8.8.8.8:443".parse().expect("socket address")]; + client_pinned_to("public.example", &addresses).expect("pinned hostname client"); +} + +#[tokio::test] +async fn plain_http_is_refused_even_to_a_public_host() { + let error = guard_endpoint("http://8.8.8.8/token", "token") + .await + .unwrap_err() + .to_string(); + assert!(error.contains("https"), "{error}"); +} + +#[tokio::test] +async fn the_cloud_metadata_address_is_refused() { + let error = guard_endpoint("https://169.254.169.254/latest", "registration") + .await + .unwrap_err() + .to_string(); + assert!(error.contains("registration"), "{error}"); + assert!(error.contains("disallowed"), "{error}"); +} + +#[tokio::test] +async fn a_hostname_resolving_to_loopback_is_refused() { + assert!( + guard_endpoint("https://localhost/token", "token") + .await + .is_err() + ); +} + +#[tokio::test] +async fn an_unparseable_endpoint_is_refused() { + assert!(guard_endpoint("not a url", "token").await.is_err()); +} diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index a9fcacbc..f33d43dd 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -9,12 +9,13 @@ use reqwest::Url; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; -use super::tokens::{persist, post_form}; +use super::credentials::OAuthCredentialStore; +use super::endpoint_guard::{guard_endpoint, guarded_client}; +use super::tokens::{oauth_failure_reason, persist, post_form}; use super::types::{AuthDetection, PendingAuthorization, TokenResponse}; use crate::error::{Error, Result}; -use crate::registry::Store; use crate::transport::http::McpHttpClient; -use tinymcp_bus::{McpProxyConfig, Transport}; +use tinymcp_bus::McpProxyConfig; /// URL-safe base64 without padding, which is what OAuth uses throughout. const B64: base64::engine::general_purpose::GeneralPurpose = @@ -48,8 +49,10 @@ const CLIENT_NAME: &str = "TinyMCP"; #[derive(Debug)] pub struct OAuthFlow { http: reqwest::Client, + no_redirect_http: reqwest::Client, proxy: Option, pending: Mutex>, + public_endpoints_only: bool, } impl OAuthFlow { @@ -65,26 +68,57 @@ impl OAuthFlow { .map_err(|source| Error::ClientBuild { source: Box::new(source.without_url()), })?; + let no_redirect_http = reqwest::Client::builder() + .timeout(Duration::from_secs(AUTH_SERVER_TIMEOUT_SECS)) + .redirect(reqwest::redirect::Policy::none()) + .build() + .map_err(|source| Error::ClientBuild { + source: Box::new(source.without_url()), + })?; Ok(Self { http, + no_redirect_http, proxy, pending: Mutex::new(HashMap::new()), + public_endpoints_only: false, }) } + /// Refuses authorization, registration and token endpoints that are not + /// `https` or that resolve to an internal address. + /// + /// Those endpoints are advertised by the server being signed in to and + /// `POST`ed to from the host, so a host connecting to servers it does not + /// trust turns this on. Off by default: a desktop host signs in to loopback + /// development servers. The token endpoint is re-checked when a code is + /// exchanged, so a host re-pointed between `begin` and `complete` is + /// refused too. [`refresh_if_expired`](super::refresh_if_expired) does not + /// re-check: it only ever posts to an endpoint this guard already passed. + #[must_use] + pub fn require_public_endpoints(mut self) -> Self { + self.public_endpoints_only = true; + self + } + /// The HTTP client this flow uses, for the refresh path. #[must_use] pub fn http(&self) -> &reqwest::Client { - &self.http + if self.public_endpoints_only { + &self.no_redirect_http + } else { + &self.http + } } /// Classifies what a server wants before it will talk. /// /// Decided by probing, not by reading registry metadata, which is often /// wrong about this. A server that answers without a challenge is open; one - /// that challenges with a usable authorization endpoint wants a browser - /// sign-in; anything else wants a static token. + /// that challenges with an authorization server [`Self::begin`] can drive + /// (authorize, token and dynamic-registration endpoints, and the + /// authorization-code grant) wants a browser sign-in; anything else wants a + /// static token. /// /// A discovery failure reports a static token rather than an error. The /// user can paste one and find out, which beats being blocked by a probe @@ -96,8 +130,11 @@ impl OAuthFlow { /// [`Error::Store`] when it cannot be read. A store failure is *not* /// collapsed into "open": reporting a server as needing nothing when the /// lookup failed would show the user a state that was never checked. - pub async fn detect(&self, store: &Store, server_id: &str) -> Result { - let Some(url) = Self::remote_url(store, server_id)? else { + pub async fn detect(&self, store: &S, server_id: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { + let Some(url) = store.remote_url(server_id).await? else { // A subprocess install has no HTTP authorization to discover. return Ok(AuthDetection::open()); }; @@ -112,10 +149,7 @@ impl OAuthFlow { Ok(Some(context)) => Ok(context .authorization_server_metadata .iter() - .find(|metadata| { - metadata.authorization_endpoint.is_some() - && supports_authorization_code(metadata) - }) + .find(|metadata| can_drive_sign_in(metadata)) .and_then(|metadata| { metadata.authorization_endpoint.clone().map(|endpoint| { AuthDetection::oauth(endpoint, metadata.grant_types_supported.clone()) @@ -144,13 +178,13 @@ impl OAuthFlow { /// offers everything the flow needs, [`Error::MalformedResponse`] when /// registration answers with something unusable, plus whatever the /// transport returns. - pub async fn begin( - &self, - store: &Store, - server_id: &str, - redirect_uri: &str, - ) -> Result { - let url = Self::remote_url(store, server_id)? + pub async fn begin(&self, store: &S, server_id: &str, redirect_uri: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { + let url = store + .remote_url(server_id) + .await? .ok_or_else(|| Error::malformed("oauth applies only to http-remote servers"))?; let client = McpHttpClient::builder(url.clone()) @@ -168,12 +202,7 @@ impl OAuthFlow { let metadata = context .authorization_server_metadata .iter() - .find(|metadata| { - metadata.authorization_endpoint.is_some() - && metadata.token_endpoint.is_some() - && metadata.registration_endpoint.is_some() - && supports_authorization_code(metadata) - }) + .find(|metadata| can_drive_sign_in(metadata)) .ok_or_else(|| Error::AuthDiscovery { detail: "no advertised authorization server offers an authorize endpoint, a token \ endpoint, and dynamic client registration together" @@ -192,8 +221,19 @@ impl OAuthFlow { )); }; + let registration_http = self + .registration_client( + &authorization_endpoint, + &token_endpoint, + ®istration_endpoint, + ) + .await?; let (client_id, client_secret) = self - .register_client(®istration_endpoint, redirect_uri) + .register_client( + registration_http.as_ref().unwrap_or(&self.http), + ®istration_endpoint, + redirect_uri, + ) .await?; let (code_verifier, code_challenge) = generate_pkce(); @@ -248,7 +288,10 @@ impl OAuthFlow { /// /// Returns [`Error::MalformedResponse`] when the state is unknown or /// expired, plus whatever the token endpoint returns. - pub async fn complete(&self, store: &Store, state: &str, code: &str) -> Result { + pub async fn complete(&self, store: &S, state: &str, code: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { let pending = { let mut pending = self.pending.lock(); prune_expired(&mut pending); @@ -267,9 +310,10 @@ impl OAuthFlow { form.push(("client_secret", secret)); } - let body = post_form(&self.http, &pending.token_endpoint, &form).await?; + let guarded_http = self.token_client(&pending.token_endpoint).await?; + let http = guarded_http.as_ref().unwrap_or(&self.http); + let body = post_form(http, &pending.token_endpoint, &form).await?; let tokens = TokenResponse::parse(&body)?; - persist( store, &pending.server_id, @@ -277,7 +321,8 @@ impl OAuthFlow { pending.client_secret.as_deref(), &pending.token_endpoint, &tokens, - )?; + ) + .await?; tracing::info!( server_id = %pending.server_id, @@ -286,6 +331,18 @@ impl OAuthFlow { Ok(pending.server_id) } + /// The server a parked authorization is for, without consuming it. + /// + /// A browser redirect carries no session, so a host keeping credentials + /// per tenant learns which store to [`complete`](Self::complete) into from + /// the returned `state` alone. `None` when the state is unknown or expired. + #[must_use] + pub fn pending_server(&self, state: &str) -> Option { + let mut pending = self.pending.lock(); + prune_expired(&mut pending); + pending.get(state).map(|parked| parked.server_id.clone()) + } + /// How many authorizations are parked. For tests and diagnostics. #[must_use] pub fn pending_count(&self) -> usize { @@ -294,6 +351,34 @@ impl OAuthFlow { pending.len() } + pub(super) async fn registration_client( + &self, + authorization_endpoint: &str, + token_endpoint: &str, + registration_endpoint: &str, + ) -> Result> { + if self.public_endpoints_only { + guard_endpoint(authorization_endpoint, "authorization").await?; + guard_endpoint(token_endpoint, "token").await?; + Ok(Some( + guarded_client(registration_endpoint, "registration").await?, + )) + } else { + Ok(None) + } + } + + pub(super) async fn token_client( + &self, + token_endpoint: &str, + ) -> Result> { + if self.public_endpoints_only { + Ok(Some(guarded_client(token_endpoint, "token").await?)) + } else { + Ok(None) + } + } + /// Registers a client dynamically, per RFC 7591. /// /// A confidential client is requested, and the secret is kept when one is @@ -301,6 +386,7 @@ impl OAuthFlow { /// token exchange then fails without it. async fn register_client( &self, + http: &reqwest::Client, registration_endpoint: &str, redirect_uri: &str, ) -> Result<(String, Option)> { @@ -312,8 +398,7 @@ impl OAuthFlow { "token_endpoint_auth_method": "client_secret_post", }); - let response = self - .http + let response = http .post(registration_endpoint) .json(&body) .send() @@ -330,7 +415,7 @@ impl OAuthFlow { return Err(Error::Http { endpoint: crate::redact_endpoint(registration_endpoint), status: status.as_u16(), - body: text, + body: oauth_failure_reason(&text), }); } @@ -353,15 +438,21 @@ impl OAuthFlow { Ok((client_id, client_secret)) } +} - /// The endpoint of an HTTP-remote install, or `None` for a subprocess one. - fn remote_url(store: &Store, server_id: &str) -> Result> { - let server = store.get_server(server_id)?; - Ok(match server.transport { - Transport::HttpRemote { ref url } if !url.is_empty() => Some(url.clone()), - _ => None, - }) - } +/// Whether this flow can run a sign-in against an authorization server: it +/// advertises an authorize endpoint, a token endpoint and dynamic client +/// registration, and accepts an authorization code. +/// +/// [`OAuthFlow::detect`] and [`OAuthFlow::begin`] both ask this one question, +/// so detection never offers a sign-in that `begin` would then refuse — a +/// server with no registration endpoint (Slack's, for one) reads as wanting a +/// static token, not as a Sign in button that leads nowhere. +fn can_drive_sign_in(metadata: &tinymcp_bus::AuthorizationServerMetadata) -> bool { + metadata.authorization_endpoint.is_some() + && metadata.token_endpoint.is_some() + && metadata.registration_endpoint.is_some() + && supports_authorization_code(metadata) } /// Whether an authorization server will accept an authorization code. diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index ce6d56a8..49649612 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -33,6 +33,10 @@ //! //! # What is stored, and where //! +//! Storage is an [`OAuthCredentialStore`]: the `SQLite` +//! [`crate::registry::Store`] by default, +//! or a host's own secret store. +//! //! The access token is stored as the server's `Authorization` header value, so //! the ordinary connect path picks it up with no special case. The bookkeeping //! needed to mint a new one — refresh token, client credentials, token endpoint, @@ -40,10 +44,13 @@ //! two underscores, which is the marker meaning "never send this as a request //! header and never show it in a credential list". +pub(crate) mod credentials; +pub(crate) mod endpoint_guard; pub(crate) mod flow; pub(crate) mod tokens; pub(crate) mod types; +pub use credentials::OAuthCredentialStore; pub use flow::OAuthFlow; pub use tokens::{OAUTH_BUNDLE_KEY, refresh_if_expired}; pub use types::{AuthDetection, AuthKind}; diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs new file mode 100644 index 00000000..fcdcac45 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -0,0 +1,350 @@ +//! The flow backed by a host's own secret store rather than the `SQLite` +//! [`Store`]: what a multi-tenant host needs to keep PKCE, registration and +//! refresh state in its own per-tenant secrets. + +use super::*; +use crate::registry::OAuthCredentialStore; +use std::io::{Read, Write}; +use std::net::TcpListener; + +/// A host's secret store: credentials in a map, keyed by whatever server id +/// the host chooses — here a tenant-qualified one. +#[derive(Debug, Default)] +struct HostSecrets { + url: Option, + values: parking_lot::Mutex>>, + refuse_writes: std::sync::atomic::AtomicBool, +} + +impl HostSecrets { + fn remote(url: &str) -> Self { + Self { + url: Some(url.to_string()), + ..Self::default() + } + } + + fn get(&self, server_id: &str, key: &str) -> Option { + self.values.lock().get(server_id)?.get(key).cloned() + } +} + +#[allow(clippy::unused_async_trait_impl)] +impl OAuthCredentialStore for HostSecrets { + async fn remote_url(&self, _server_id: &str) -> crate::Result> { + Ok(self.url.clone()) + } + + async fn load_credentials(&self, server_id: &str) -> crate::Result> { + Ok(self + .values + .lock() + .get(server_id) + .cloned() + .unwrap_or_default()) + } + + async fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> crate::Result<()> { + if self.refuse_writes.load(Ordering::SeqCst) { + return Err(Error::CredentialStore { + action: format!("writing credentials for {server_id}"), + detail: "the vault is sealed".into(), + }); + } + self.values + .lock() + .insert(server_id.to_string(), credentials.clone()); + Ok(()) + } +} + +const TENANT_SERVER: &str = "acme/linear"; + +#[tokio::test] +async fn a_host_store_backs_a_whole_sign_in() { + let (endpoint, _state) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let flow = flow(); + + let url = flow + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .expect("begin"); + let state = authorize_param(&url, "state").unwrap(); + let server_id = flow + .complete(&host, &state, "the-code") + .await + .expect("complete"); + + assert_eq!(server_id, TENANT_SERVER); + assert_eq!( + host.get(TENANT_SERVER, "Authorization").as_deref(), + Some("Bearer at-1") + ); + let bundle: Value = + serde_json::from_str(&host.get(TENANT_SERVER, OAUTH_BUNDLE_KEY).unwrap()).unwrap(); + assert_eq!(bundle["refresh_token"], json!("rt-1")); + assert_eq!(bundle["client_id"], json!("client-1")); +} + +/// The redirect carries no session, so a multi-tenant host has to learn which +/// tenant's store to complete into from the `state` alone — before consuming +/// it. +#[tokio::test] +async fn the_server_a_pending_state_belongs_to_can_be_read_before_completing() { + let (endpoint, _state) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let flow = flow(); + let url = flow + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .unwrap(); + let state = authorize_param(&url, "state").unwrap(); + + assert_eq!(flow.pending_server(&state).as_deref(), Some(TENANT_SERVER)); + assert_eq!(flow.pending_count(), 1, "peeking does not consume"); + assert_eq!(flow.pending_server("never-issued"), None); +} + +#[tokio::test] +async fn a_host_store_backs_refresh() { + let requests = TokenRequests::default(); + let base = serve(token_endpoint( + requests.clone(), + json!({ "access_token": "fresh", "expires_in": 3600 }), + )) + .await; + let host = HostSecrets::default(); + let bundle = json!({ + "refresh_token": "r1", + "client_id": "cli-1", + "client_secret": null, + "token_endpoint": format!("{base}/token"), + "expires_at": 1, + }); + host.values.lock().insert( + TENANT_SERVER.to_string(), + BTreeMap::from([(OAUTH_BUNDLE_KEY.to_string(), bundle.to_string())]), + ); + + let refreshed = refresh_if_expired(&host, &reqwest::Client::new(), TENANT_SERVER) + .await + .expect("refresh"); + + assert!(refreshed); + assert_eq!( + host.get(TENANT_SERVER, "Authorization").as_deref(), + Some("Bearer fresh") + ); + assert_eq!(requests.count.load(Ordering::SeqCst), 1); +} + +#[tokio::test] +async fn a_host_store_failure_is_reported_as_a_store_error() { + let (endpoint, _state) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + host.refuse_writes.store(true, Ordering::SeqCst); + let flow = flow(); + let url = flow + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .unwrap(); + let state = authorize_param(&url, "state").unwrap(); + + let error = flow + .complete(&host, &state, "the-code") + .await + .expect_err("a sealed vault"); + + assert!(matches!(error, Error::CredentialStore { .. }), "{error:?}"); + assert_eq!(error.wire_name(), tinymcp_bus::errors::CREDENTIAL_STORE); + assert!(error.to_string().contains("sealed"), "{error}"); +} + +#[tokio::test] +async fn a_host_store_with_no_remote_endpoint_cannot_be_signed_in_to() { + let error = flow() + .begin( + &HostSecrets::default(), + TENANT_SERVER, + "https://host.test/cb", + ) + .await + .expect_err("no endpoint"); + assert!( + matches!(error, Error::MalformedResponse { .. }), + "{error:?}" + ); +} + +// --------------------------------------------------------------------------- +// Detection agrees with what `begin` can drive +// --------------------------------------------------------------------------- + +/// Slack's MCP endpoint challenges properly and advertises an authorize +/// endpoint, but no registration endpoint — so there is no client to mint and +/// `begin` refuses. Offering a Sign in button for it is offering a dead end; +/// detection has to answer "paste a token" instead. +#[tokio::test] +async fn detection_offers_a_token_when_there_is_no_dynamic_registration() { + let (endpoint, state) = authority_with_challenge().await; + state.without_registration.store(true, Ordering::SeqCst); + let store = store_with_remote(&endpoint); + let flow = flow(); + + let detection = flow.detect(&store, "srv-1").await.expect("detect"); + + assert_eq!(detection.kind, AuthKind::Token, "{detection:?}"); + assert!( + flow.begin(&store, "srv-1", "http://127.0.0.1:1/cb") + .await + .is_err(), + "begin refuses the same server, so detection must not offer it" + ); +} + +// --------------------------------------------------------------------------- +// A failure body never carries what was submitted +// --------------------------------------------------------------------------- + +/// A refresh whose token endpoint fails with `reply`, echoing the form back. +async fn rejected_refresh(reply: String) -> Error { + let app = Router::new().route( + "/token", + post(move |body: String| { + let reply = reply.replace("{form}", &body); + async move { (AxumStatus::BAD_REQUEST, reply) } + }), + ); + let base = serve(app).await; + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, &format!("{base}/token"), Some("r1-secret-refresh")); + refresh_if_expired(&store, &reqwest::Client::new(), "srv-1") + .await + .expect_err("a rejected refresh") +} + +/// Some token endpoints echo the submitted form in their error body — the +/// refresh token, the client secret. That body reaches logs and user +/// interfaces through the error, so only the standard OAuth `error` and +/// `error_description` survive: they are the *why*, and carry no secret. +#[tokio::test] +async fn a_token_endpoint_failure_keeps_the_reason_but_not_an_echo() { + let error = rejected_refresh( + json!({ + "error": "invalid_grant", + "error_description": "refresh token revoked", + "request": "{form}", + }) + .to_string(), + ) + .await; + + let Error::Http { body, .. } = &error else { + panic!("expected an http error, got {error:?}"); + }; + assert!(body.contains("invalid_grant"), "{body}"); + assert!(body.contains("refresh token revoked"), "{body}"); + assert!(!body.contains("r1-secret-refresh"), "{body}"); + assert!(!body.contains("sec-1"), "{body}"); +} + +#[tokio::test] +async fn a_non_json_token_endpoint_failure_is_not_echoed() { + let error = rejected_refresh("you sent: {form}".to_string()).await; + let rendered = error.to_string(); + assert!(!rendered.contains("r1-secret-refresh"), "{rendered}"); + assert!(!rendered.contains("sec-1"), "{rendered}"); +} + +// --------------------------------------------------------------------------- +// Refusing internal endpoints a hostile server advertises +// --------------------------------------------------------------------------- + +/// The authorization server's endpoints come from the server being signed in +/// to, and the flow POSTs to them from the host. A host serving untrusted +/// servers opts into refusing internal targets; the loopback authority here is +/// exactly such a target, so nothing may be registered with it. +#[tokio::test] +async fn a_guarded_flow_refuses_an_internal_authorization_server() { + let (endpoint, state) = authority_with_challenge().await; + let store = store_with_remote(&endpoint); + let flow = flow().require_public_endpoints(); + + let error = flow + .begin(&store, "srv-1", "https://host.test/cb") + .await + .expect_err("a loopback authorization server"); + + assert!(error.to_string().contains("https"), "{error}"); + assert_eq!(state.registrations.load(Ordering::SeqCst), 0); + assert_eq!(flow.pending_count(), 0); +} + +#[tokio::test] +async fn a_guarded_flow_refresh_client_does_not_follow_redirects() { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind listener"); + let endpoint = format!("http://{}/refresh", listener.local_addr().expect("address")); + let server = std::thread::spawn(move || { + let (mut socket, _) = listener.accept().expect("accept request"); + let mut request = [0; 1024]; + let _ = socket.read(&mut request).expect("read request"); + socket + .write_all( + b"HTTP/1.1 302 Found\r\nLocation: /redirected\r\nContent-Length: 0\r\nConnection: close\r\n\r\n", + ) + .expect("write redirect"); + }); + + let response = flow() + .require_public_endpoints() + .http() + .get(endpoint) + .send() + .await + .expect("receive redirect response"); + + assert_eq!(response.status(), reqwest::StatusCode::FOUND); + server.join().expect("server thread"); +} + +#[tokio::test] +async fn public_endpoint_clients_validate_and_pin_registration_and_token_hosts() { + let flow = flow().require_public_endpoints(); + let registration = flow + .registration_client( + "https://8.8.8.8/authorize", + "https://1.1.1.1/token", + "https://9.9.9.9/register", + ) + .await + .expect("validate public endpoints"); + assert!(registration.is_some()); + assert!( + flow.token_client("https://8.8.8.8/token") + .await + .expect("build token client") + .is_some() + ); +} + +#[tokio::test] +async fn desktop_flow_keeps_the_default_clients() { + let flow = flow(); + assert!( + flow.registration_client("not a url", "not a url", "not a url") + .await + .expect("desktop flow skips public endpoint checks") + .is_none() + ); + assert!( + flow.token_client("not a url") + .await + .expect("desktop flow skips public endpoint checks") + .is_none() + ); +} diff --git a/crates/tinymcp/src/registry/oauth/mod_tests.rs b/crates/tinymcp/src/registry/oauth/mod_tests.rs index f9278bba..abba73c7 100644 --- a/crates/tinymcp/src/registry/oauth/mod_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_tests.rs @@ -1222,3 +1222,10 @@ fn a_kind_serializes_as_the_same_spelling_it_reports() { ); } } + +// --------------------------------------------------------------------------- +// A host-owned credential store +// --------------------------------------------------------------------------- + +#[path = "mod_host_store_tests.rs"] +mod host_store; diff --git a/crates/tinymcp/src/registry/oauth/tokens.rs b/crates/tinymcp/src/registry/oauth/tokens.rs index 2aa51103..954c7100 100644 --- a/crates/tinymcp/src/registry/oauth/tokens.rs +++ b/crates/tinymcp/src/registry/oauth/tokens.rs @@ -4,9 +4,9 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde_json::Value; +use super::credentials::OAuthCredentialStore; use super::types::{OAuthBundle, TokenResponse}; use crate::error::{Error, Result}; -use crate::registry::Store; /// The reserved credential key holding the refresh bundle. /// @@ -46,12 +46,15 @@ const DEFAULT_TOKEN_LIFETIME_SECS: u64 = 3600; /// or the token endpoint answers with something unusable, [`Error::Http`] when /// it answers with a failure status, and [`Error::Transport`] when it cannot be /// reached. -pub async fn refresh_if_expired( - store: &Store, +pub async fn refresh_if_expired( + store: &S, http: &reqwest::Client, server_id: &str, -) -> Result { - let env = store.load_env_values(server_id)?; +) -> Result +where + S: OAuthCredentialStore + ?Sized, +{ + let env = store.load_credentials(server_id).await?; let Some(raw_bundle) = env.get(OAUTH_BUNDLE_KEY) else { return Ok(false); }; @@ -96,7 +99,8 @@ pub async fn refresh_if_expired( bundle.client_secret.as_deref(), &bundle.token_endpoint, &tokens, - )?; + ) + .await?; tracing::info!(server_id, "refreshed an expired access token"); Ok(true) @@ -111,15 +115,18 @@ pub async fn refresh_if_expired( /// # Errors /// /// Returns [`Error::Serialization`] when the bundle cannot be encoded, and -/// [`Error::Store`] when it cannot be written. -pub(super) fn persist( - store: &Store, +/// whatever the store reports when it cannot be read or written. +pub(super) async fn persist( + store: &S, server_id: &str, client_id: &str, client_secret: Option<&str>, token_endpoint: &str, tokens: &TokenResponse, -) -> Result<()> { +) -> Result<()> +where + S: OAuthCredentialStore + ?Sized, +{ let bundle = OAuthBundle { refresh_token: tokens.refresh_token.clone(), client_id: client_id.to_string(), @@ -128,7 +135,7 @@ pub(super) fn persist( expires_at: now_unix() + tokens.expires_in.unwrap_or(DEFAULT_TOKEN_LIFETIME_SECS), }; - let mut env = store.load_env_values(server_id)?; + let mut env = store.load_credentials(server_id).await?; env.insert( ACCESS_TOKEN_KEY.to_string(), format!("Bearer {}", tokens.access_token), @@ -138,14 +145,7 @@ pub(super) fn persist( serde_json::to_string(&bundle)?, ); - store.set_env_values(server_id, &env)?; - - // The listing of credential *names* lives on the server row and is what a - // caller shows the user, so it has to learn about the two new keys. - let names: Vec = env.keys().cloned().collect(); - store.update_env_keys(server_id, &names)?; - - Ok(()) + store.store_credentials(server_id, &env).await } /// Posts a form to a token endpoint and returns its JSON body. @@ -175,12 +175,14 @@ pub(super) async fn post_form( // The body is read before the status is judged, because a token endpoint's // failure body is where it says *why* — `invalid_grant`, `invalid_client`. - // Discarding it would leave a caller with a bare status code. + // Discarding it would leave a caller with a bare status code; keeping it + // whole would keep whatever the server echoed back — see + // `oauth_failure_reason`. if !status.is_success() { return Err(Error::Http { endpoint: crate::redact_endpoint(endpoint), status: status.as_u16(), - body: text, + body: oauth_failure_reason(&text), }); } @@ -188,6 +190,32 @@ pub(super) async fn post_form( .map_err(|error| Error::malformed(format!("token endpoint replied with non-json: {error}"))) } +/// The part of an authorization server's failure body that is safe to keep: +/// the standard OAuth `error` code and `error_description` (RFC 6749 §5.2). +/// +/// Some servers echo the submitted form — a refresh token, a client secret, an +/// authorization code — into their error body, and the body reaches logs and +/// user interfaces through [`Error::Http`]. The two standard members are the +/// *why* and carry no secret, so they are all that is kept. A body that is not +/// a JSON object carries no standard reason and is dropped entirely. +pub(super) fn oauth_failure_reason(body: &str) -> String { + let Ok(Value::Object(fields)) = serde_json::from_str::(body) else { + return String::new(); + }; + let reason: serde_json::Map = ["error", "error_description"] + .into_iter() + .filter_map(|key| { + let value = fields.get(key)?.as_str()?; + Some((key.to_string(), Value::String(value.to_string()))) + }) + .collect(); + if reason.is_empty() { + String::new() + } else { + Value::Object(reason).to_string() + } +} + /// The current time in Unix seconds. /// /// A clock before the epoch reads as zero, which makes every token look expired