diff --git a/internal/mcp/oauth.go b/internal/mcp/oauth.go index 678d58045..05a3b296e 100644 --- a/internal/mcp/oauth.go +++ b/internal/mcp/oauth.go @@ -63,8 +63,9 @@ func tokenToStored(t oauth.Token) StoredToken { } // OAuthConfig describes how to authenticate to a remote MCP server using OAuth. -// Endpoints may be discovered from the server's metadata document; explicit -// values here override or fill in anything discovery cannot provide. +// Endpoints may be discovered through RFC 9728 protected-resource metadata and +// RFC 8414 authorization-server metadata; explicit values override or fill in +// anything discovery cannot provide. type OAuthConfig struct { ClientID string ClientSecret string @@ -85,6 +86,11 @@ type authServerMetadata struct { TokenEndpoint string `json:"token_endpoint"` RegistrationEndpoint string `json:"registration_endpoint"` ScopesSupported []string `json:"scopes_supported"` + + protectedResourceURL string + protectAuthorizationEndpoint bool + protectTokenEndpoint bool + protectRegistrationEndpoint bool } // pkceParams holds a PKCE verifier/challenge pair. @@ -120,7 +126,7 @@ type authorizationFlow struct { // discoverAuthorizationServer fetches the RFC 8414 authorization server metadata // at the well-known path under baseURL, via the shared engine. func discoverAuthorizationServer(ctx context.Context, client *http.Client, baseURL string) (authServerMetadata, error) { - meta, err := oauth.DiscoverAuthorizationServer(ctx, client, baseURL) + meta, err := oauth.DiscoverAuthorizationServer(ctx, withoutDiscoveryRedirects(client), baseURL) if err != nil { return authServerMetadata{}, err } @@ -133,10 +139,9 @@ func discoverAuthorizationServer(ctx context.Context, client *http.Client, baseU }, nil } -// validateResolvedEndpoints applies the shared https/loopback endpoint rule to -// every non-empty endpoint in the resolved metadata — configured or discovered -// alike — so discovery metadata can never downgrade an MCP login to an insecure -// or attacker-controlled authorization, token, or registration endpoint. +// validateResolvedEndpoints applies the shared https/loopback scheme rule to +// every non-empty endpoint in the resolved metadata. Protected-resource +// discovery adds its stricter provenance-aware network rule below. func validateResolvedEndpoints(metadata authServerMetadata) error { for _, ep := range []struct{ kind, url string }{ {"authorization", metadata.AuthorizationEndpoint}, @@ -153,9 +158,43 @@ func validateResolvedEndpoints(metadata authServerMetadata) error { return nil } -// resolveAuthorizationServer discovers metadata and applies explicit config -// overrides. Configured endpoints take precedence over discovered ones, and act -// as a fallback when discovery fails or omits a value. +func validateProtectedDiscoveredEndpoints(ctx context.Context, metadata authServerMetadata) error { + if metadata.protectedResourceURL == "" { + return nil + } + policy, err := newOAuthDiscoveryNetworkPolicy(metadata.protectedResourceURL) + if err != nil { + return err + } + for _, endpoint := range []struct { + kind string + url string + protected bool + }{ + {kind: "authorization", url: metadata.AuthorizationEndpoint, protected: metadata.protectAuthorizationEndpoint}, + {kind: "token", url: metadata.TokenEndpoint, protected: metadata.protectTokenEndpoint}, + {kind: "registration", url: metadata.RegistrationEndpoint, protected: metadata.protectRegistrationEndpoint}, + } { + if !endpoint.protected || strings.TrimSpace(endpoint.url) == "" { + continue + } + parsed, parseErr := url.Parse(endpoint.url) + if parseErr != nil || parsed.Hostname() == "" { + return fmt.Errorf("mcp oauth: %s endpoint: %w", endpoint.kind, errUnsafeOAuthDiscoveryTarget) + } + if err := policy.validateLiteralHost(parsed.Hostname()); err != nil { + return fmt.Errorf("mcp oauth: %s endpoint: %w", endpoint.kind, err) + } + if _, err := policy.resolve(ctx, parsed.Hostname()); err != nil { + return fmt.Errorf("mcp oauth: %s endpoint: %w", endpoint.kind, err) + } + } + return nil +} + +// resolveAuthorizationServer follows protected-resource metadata when the MCP +// endpoint advertises it, falls back to direct authorization-server discovery +// for legacy providers, then applies explicit config overrides. func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseURL string, cfg OAuthConfig) (authServerMetadata, error) { // When the config supplies both the authorization and token endpoints // directly, skip network discovery entirely: there is nothing to discover, @@ -174,17 +213,40 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR return metadata, nil } + discoveryClient := withoutDiscoveryRedirects(client) discoveryBase := strings.TrimSpace(cfg.IssuerURL) + protectedResourceDiscovery := false if discoveryBase == "" { - discoveryBase = baseURL + issuer, found, protectedErr := discoverProtectedResourceAuthorizationServer(ctx, discoveryClient, baseURL) + if protectedErr != nil { + return authServerMetadata{}, protectedErr + } + if found { + discoveryBase = issuer + protectedResourceDiscovery = true + discoveryClient, protectedErr = newAdvertisedOAuthDiscoveryClient(discoveryClient, baseURL) + if protectedErr != nil { + return authServerMetadata{}, protectedErr + } + } else { + discoveryBase = baseURL + } } - metadata, err := discoverAuthorizationServer(ctx, client, discoveryBase) + metadata, err := discoverAuthorizationServer(ctx, discoveryClient, discoveryBase) if err != nil { // Discovery failures are non-fatal when the config supplies the endpoints // directly; otherwise surface the discovery error. metadata = authServerMetadata{} } + if protectedResourceDiscovery { + if strings.TrimSpace(metadata.Issuer) == "" { + return authServerMetadata{}, errors.New("mcp oauth: authorization server metadata is missing its required issuer") + } + if metadata.Issuer != discoveryBase { + return authServerMetadata{}, errors.New("mcp oauth: authorization server metadata issuer does not match protected resource metadata") + } + } if endpoint := strings.TrimSpace(cfg.AuthorizationEndpoint); endpoint != "" { metadata.AuthorizationEndpoint = endpoint @@ -195,6 +257,12 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR if endpoint := strings.TrimSpace(cfg.RegistrationEndpoint); endpoint != "" { metadata.RegistrationEndpoint = endpoint } + if protectedResourceDiscovery { + metadata.protectedResourceURL = strings.TrimSpace(baseURL) + metadata.protectAuthorizationEndpoint = strings.TrimSpace(cfg.AuthorizationEndpoint) == "" + metadata.protectTokenEndpoint = strings.TrimSpace(cfg.TokenEndpoint) == "" + metadata.protectRegistrationEndpoint = strings.TrimSpace(cfg.RegistrationEndpoint) == "" + } if strings.TrimSpace(metadata.AuthorizationEndpoint) == "" { return authServerMetadata{}, errors.New("no authorization endpoint discovered or configured") @@ -205,6 +273,9 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR if err := validateResolvedEndpoints(metadata); err != nil { return authServerMetadata{}, err } + if err := validateProtectedDiscoveredEndpoints(ctx, metadata); err != nil { + return authServerMetadata{}, err + } return metadata, nil } @@ -363,10 +434,25 @@ func Login(ctx context.Context, options LoginOptions) (StoredToken, error) { } defer listener.Close() redirectURI := fmt.Sprintf("http://%s/callback", listener.Addr().String()) + protectedEndpointClient := httpClient + if metadata.protectRegistrationEndpoint || metadata.protectTokenEndpoint { + protectedEndpointClient, err = newAdvertisedOAuthDiscoveryClient(httpClient, metadata.protectedResourceURL) + if err != nil { + return StoredToken{}, err + } + } + tokenClient := httpClient + if metadata.protectTokenEndpoint { + tokenClient = protectedEndpointClient + } if strings.TrimSpace(cfg.ClientID) == "" { if registration := strings.TrimSpace(metadata.RegistrationEndpoint); registration != "" { - clientID, clientSecret, regErr := registerClient(loginCtx, httpClient, registration, redirectURI, cfg.Scopes) + registrationClient := httpClient + if metadata.protectRegistrationEndpoint { + registrationClient = protectedEndpointClient + } + clientID, clientSecret, regErr := registerClient(loginCtx, registrationClient, registration, redirectURI, cfg.Scopes) if regErr != nil { return StoredToken{}, regErr } @@ -390,7 +476,7 @@ func Login(ctx context.Context, options LoginOptions) (StoredToken, error) { } flow := &authorizationFlow{ - httpClient: httpClient, + httpClient: tokenClient, metadata: metadata, config: cfg, pkce: pkce, diff --git a/internal/mcp/oauth_discovery_network.go b/internal/mcp/oauth_discovery_network.go new file mode 100644 index 000000000..05f280e20 --- /dev/null +++ b/internal/mcp/oauth_discovery_network.go @@ -0,0 +1,290 @@ +package mcp + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "net" + "net/http" + "net/netip" + "net/url" + "strings" +) + +var errUnsafeOAuthDiscoveryTarget = errors.New("mcp oauth: refused non-public server-advertised discovery target") + +var nonPublicOAuthDiscoveryPrefixes = []netip.Prefix{ + netip.MustParsePrefix("0.0.0.0/8"), + netip.MustParsePrefix("10.0.0.0/8"), + netip.MustParsePrefix("100.64.0.0/10"), + netip.MustParsePrefix("127.0.0.0/8"), + netip.MustParsePrefix("169.254.0.0/16"), + netip.MustParsePrefix("172.16.0.0/12"), + netip.MustParsePrefix("192.0.0.0/24"), + netip.MustParsePrefix("192.0.2.0/24"), + netip.MustParsePrefix("192.88.99.0/24"), + netip.MustParsePrefix("192.168.0.0/16"), + netip.MustParsePrefix("198.18.0.0/15"), + netip.MustParsePrefix("198.51.100.0/24"), + netip.MustParsePrefix("203.0.113.0/24"), + netip.MustParsePrefix("224.0.0.0/4"), + netip.MustParsePrefix("240.0.0.0/4"), + netip.MustParsePrefix("::/128"), + netip.MustParsePrefix("::1/128"), + netip.MustParsePrefix("100::/64"), + netip.MustParsePrefix("2001:2::/48"), + netip.MustParsePrefix("2001:db8::/32"), + netip.MustParsePrefix("fc00::/7"), + netip.MustParsePrefix("fe80::/10"), + netip.MustParsePrefix("ff00::/8"), +} + +type oauthDiscoveryNetworkPolicy struct { + allowLoopback bool + lookupNetIP func(context.Context, string, string) ([]netip.Addr, error) +} + +type advertisedOAuthDiscoveryTransport struct { + base http.RoundTripper + policy oauthDiscoveryNetworkPolicy +} + +// newAdvertisedOAuthDiscoveryClient binds server-advertised discovery to the +// public network. Loopback targets remain available only when the operator +// explicitly configured a loopback MCP resource. +func newAdvertisedOAuthDiscoveryClient(client *http.Client, resourceURL string) (*http.Client, error) { + policy, err := newOAuthDiscoveryNetworkPolicy(resourceURL) + if err != nil { + return nil, err + } + copied := withoutDiscoveryRedirects(client) + transport := copied.Transport + if transport == nil { + transport = http.DefaultTransport + } + copied.Transport = advertisedOAuthDiscoveryTransport{base: transport, policy: policy} + return copied, nil +} + +func newOAuthDiscoveryNetworkPolicy(resourceURL string) (oauthDiscoveryNetworkPolicy, error) { + parsed, err := url.Parse(strings.TrimSpace(resourceURL)) + if err != nil || parsed.Hostname() == "" { + return oauthDiscoveryNetworkPolicy{}, errors.New("mcp oauth: invalid MCP resource URL for discovery policy") + } + return oauthDiscoveryNetworkPolicy{allowLoopback: isExplicitOAuthLoopbackHost(parsed.Hostname())}, nil +} + +func validateAdvertisedOAuthDiscoveryURL(resourceURL string, targetURL string) error { + policy, err := newOAuthDiscoveryNetworkPolicy(resourceURL) + if err != nil { + return err + } + parsed, err := url.Parse(targetURL) + if err != nil || parsed.Hostname() == "" { + return errUnsafeOAuthDiscoveryTarget + } + return policy.validateLiteralHost(parsed.Hostname()) +} + +func (transport advertisedOAuthDiscoveryTransport) RoundTrip(request *http.Request) (*http.Response, error) { + if request == nil || request.URL == nil { + return nil, errors.New("mcp oauth: discovery request has no URL") + } + if err := transport.policy.validateLiteralHost(request.URL.Hostname()); err != nil { + return nil, err + } + + baseTransport, ok := transport.base.(*http.Transport) + if !ok { + return nil, errors.New("mcp oauth: cannot enforce discovery network policy with a custom HTTP transport") + } + addresses, err := transport.policy.resolve(request.Context(), request.URL.Hostname()) + if err != nil { + return nil, err + } + var lastErr error + for index, address := range addresses { + attempt := request + if index > 0 && request.Body != nil { + if request.GetBody == nil { + break + } + body, bodyErr := request.GetBody() + if bodyErr != nil { + return nil, bodyErr + } + attempt = request.Clone(request.Context()) + attempt.Body = body + } + response, roundTripErr := roundTripPinnedOAuthDiscovery(attempt, baseTransport, address) + if roundTripErr == nil { + return response, nil + } + lastErr = roundTripErr + } + if lastErr == nil { + lastErr = errUnsafeOAuthDiscoveryTarget + } + return nil, lastErr +} + +func roundTripPinnedOAuthDiscovery(request *http.Request, base *http.Transport, address netip.Addr) (*http.Response, error) { + pinnedRequest := request.Clone(request.Context()) + pinnedURL := *request.URL + originalHost := request.URL.Hostname() + port := request.URL.Port() + if port == "" { + switch strings.ToLower(request.URL.Scheme) { + case "http": + port = "80" + case "https": + port = "443" + default: + return nil, errUnsafeOAuthDiscoveryTarget + } + } + pinnedURL.Host = net.JoinHostPort(address.String(), port) + pinnedRequest.URL = &pinnedURL + if pinnedRequest.Host == "" { + pinnedRequest.Host = request.URL.Host + } + + pinnedTransport := base.Clone() + pinnedTransport.DisableKeepAlives = true + var proxyURL *url.URL + if base.Proxy != nil { + var err error + proxyURL, err = base.Proxy(request) + if err != nil { + return nil, fmt.Errorf("mcp oauth: resolve discovery proxy: %w", err) + } + if proxyURL == nil { + pinnedTransport.Proxy = nil + } else { + pinnedTransport.Proxy = http.ProxyURL(proxyURL) + } + } + if pinnedTransport.TLSClientConfig == nil { + pinnedTransport.TLSClientConfig = &tls.Config{ServerName: originalHost} //nolint:gosec // standard verification remains enabled + } else { + pinnedTransport.TLSClientConfig = pinnedTransport.TLSClientConfig.Clone() + pinnedTransport.TLSClientConfig.ServerName = originalHost + } + // A custom TLS dialer must not resolve the advertised hostname again after + // the policy has pinned it. For an HTTPS proxy, however, that proxy remains + // the TLS first hop and the transport sends CONNECT to the pinned target. + dialContext := pinnedTransport.DialContext + if dialContext == nil { + dialContext = (&net.Dialer{}).DialContext + } + pinnedAddress := net.JoinHostPort(address.String(), port) + targetTLSConfig := pinnedTransport.TLSClientConfig + pinnedTransport.DialTLSContext = func(ctx context.Context, network string, address string) (net.Conn, error) { + if proxyURL != nil && strings.EqualFold(proxyURL.Scheme, "https") { + proxyTLSConfig := targetTLSConfig.Clone() + proxyTLSConfig.ServerName = proxyURL.Hostname() + return dialOAuthDiscoveryTLS(ctx, dialContext, network, address, proxyTLSConfig) + } + return dialOAuthDiscoveryTLS(ctx, dialContext, network, pinnedAddress, targetTLSConfig) + } + return pinnedTransport.RoundTrip(pinnedRequest) +} + +func dialOAuthDiscoveryTLS(ctx context.Context, dialContext func(context.Context, string, string) (net.Conn, error), network string, address string, config *tls.Config) (net.Conn, error) { + connection, err := dialContext(ctx, network, address) + if err != nil { + return nil, err + } + tlsConnection := tls.Client(connection, config) + if err := tlsConnection.HandshakeContext(ctx); err != nil { + connection.Close() + return nil, err + } + return tlsConnection, nil +} + +func (policy oauthDiscoveryNetworkPolicy) resolve(ctx context.Context, host string) ([]netip.Addr, error) { + if address, err := netip.ParseAddr(strings.Trim(host, "[]")); err == nil { + address = address.Unmap() + if err := policy.validateAddress(address); err != nil { + return nil, err + } + return []netip.Addr{address}, nil + } + + lookupNetIP := policy.lookupNetIP + if lookupNetIP == nil { + lookupNetIP = net.DefaultResolver.LookupNetIP + } + addresses, err := lookupNetIP(ctx, "ip", host) + if err != nil { + return nil, errors.New("mcp oauth: resolve server-advertised discovery target failed") + } + if len(addresses) == 0 { + return nil, errors.New("mcp oauth: server-advertised discovery target resolved to no addresses") + } + explicitLoopback := isExplicitOAuthLoopbackHost(host) + resolved := make([]netip.Addr, 0, len(addresses)) + for _, address := range addresses { + address = address.Unmap() + if explicitLoopback && !address.IsLoopback() { + return nil, errUnsafeOAuthDiscoveryTarget + } + if err := policy.validateAddress(address); err != nil { + return nil, err + } + resolved = append(resolved, address) + } + return resolved, nil +} + +func (policy oauthDiscoveryNetworkPolicy) validateLiteralHost(host string) error { + normalized := strings.ToLower(strings.TrimSuffix(strings.Trim(host, "[]"), ".")) + if normalized == "" { + return errUnsafeOAuthDiscoveryTarget + } + if isCloudMetadataHostname(normalized) { + return errUnsafeOAuthDiscoveryTarget + } + if isExplicitOAuthLoopbackHost(normalized) && !policy.allowLoopback { + return errUnsafeOAuthDiscoveryTarget + } + if address, err := netip.ParseAddr(normalized); err == nil { + return policy.validateAddress(address.Unmap()) + } + return nil +} + +func (policy oauthDiscoveryNetworkPolicy) validateAddress(address netip.Addr) error { + if policy.allowLoopback && address.IsLoopback() { + return nil + } + if !address.IsValid() || !address.IsGlobalUnicast() || address.IsPrivate() { + return errUnsafeOAuthDiscoveryTarget + } + for _, prefix := range nonPublicOAuthDiscoveryPrefixes { + if prefix.Contains(address) { + return errUnsafeOAuthDiscoveryTarget + } + } + return nil +} + +func isExplicitOAuthLoopbackHost(host string) bool { + normalized := strings.ToLower(strings.TrimSuffix(strings.Trim(host, "[]"), ".")) + if normalized == "localhost" || strings.HasSuffix(normalized, ".localhost") { + return true + } + address, err := netip.ParseAddr(normalized) + return err == nil && address.Unmap().IsLoopback() +} + +func isCloudMetadataHostname(host string) bool { + switch strings.ToLower(strings.TrimSuffix(host, ".")) { + case "metadata", "metadata.google.internal", "metadata.azure.internal": + return true + default: + return false + } +} diff --git a/internal/mcp/oauth_discovery_network_test.go b/internal/mcp/oauth_discovery_network_test.go new file mode 100644 index 000000000..06bf50fab --- /dev/null +++ b/internal/mcp/oauth_discovery_network_test.go @@ -0,0 +1,180 @@ +package mcp + +import ( + "bufio" + "context" + "errors" + "io" + "net" + "net/http" + "net/netip" + "net/url" + "strings" + "sync/atomic" + "testing" +) + +func TestOAuthDiscoveryNetworkPolicyRejectsNonPublicAddresses(t *testing.T) { + policy := oauthDiscoveryNetworkPolicy{} + for _, value := range []string{ + "127.0.0.1", + "10.0.0.1", + "169.254.169.254", + "192.0.2.10", + "224.0.0.1", + "::1", + "fc00::1", + "fe80::1", + "2001:db8::1", + } { + t.Run(value, func(t *testing.T) { + if err := policy.validateAddress(netip.MustParseAddr(value)); err == nil { + t.Fatalf("address %s should be rejected", value) + } + }) + } + for _, value := range []string{"8.8.8.8", "2606:4700:4700::1111"} { + if err := policy.validateAddress(netip.MustParseAddr(value)); err != nil { + t.Fatalf("public address %s rejected: %v", value, err) + } + } + loopbackPolicy := oauthDiscoveryNetworkPolicy{allowLoopback: true} + if err := loopbackPolicy.validateAddress(netip.MustParseAddr("127.0.0.1")); err != nil { + t.Fatalf("explicit loopback resource should allow loopback discovery: %v", err) + } + if err := loopbackPolicy.validateAddress(netip.MustParseAddr("10.0.0.1")); err == nil { + t.Fatal("loopback development must not permit arbitrary private targets") + } +} + +func TestProtectedDiscoveryRejectsNonPublicOAuthEndpoints(t *testing.T) { + for _, endpoint := range []struct { + name string + metadata authServerMetadata + }{ + { + name: "authorization", + metadata: authServerMetadata{ + AuthorizationEndpoint: "http://127.0.0.1/authorize", + protectAuthorizationEndpoint: true, + }, + }, + { + name: "token", + metadata: authServerMetadata{ + TokenEndpoint: "https://10.0.0.1/token", + protectTokenEndpoint: true, + }, + }, + { + name: "registration", + metadata: authServerMetadata{ + RegistrationEndpoint: "https://169.254.169.254/register", + protectRegistrationEndpoint: true, + }, + }, + } { + t.Run(endpoint.name, func(t *testing.T) { + endpoint.metadata.protectedResourceURL = "https://mcp.example/mcp" + if err := validateProtectedDiscoveredEndpoints(context.Background(), endpoint.metadata); err == nil { + t.Fatalf("protected %s endpoint should be rejected", endpoint.name) + } + }) + } +} + +func TestAdvertisedOAuthDiscoveryRejectsHostnameResolvingPrivateAtUseTime(t *testing.T) { + var dialed atomic.Bool + policy := oauthDiscoveryNetworkPolicy{ + lookupNetIP: func(context.Context, string, string) ([]netip.Addr, error) { + return []netip.Addr{netip.MustParseAddr("10.0.0.12")}, nil + }, + } + client := &http.Client{Transport: advertisedOAuthDiscoveryTransport{ + base: &http.Transport{DialContext: func(context.Context, string, string) (net.Conn, error) { + dialed.Store(true) + return nil, io.EOF + }}, + policy: policy, + }} + + _, err := client.Get("https://auth.example/.well-known/oauth-authorization-server") + if err == nil || !strings.Contains(err.Error(), errUnsafeOAuthDiscoveryTarget.Error()) { + t.Fatalf("error = %v, want private-resolution rejection", err) + } + if dialed.Load() { + t.Fatal("private resolved address reached the dialer") + } +} + +func TestAdvertisedOAuthDiscoveryPinsResolvedAddress(t *testing.T) { + const publicAddress = "93.184.216.34" + var dialedAddress string + policy := oauthDiscoveryNetworkPolicy{ + lookupNetIP: func(context.Context, string, string) ([]netip.Addr, error) { + return []netip.Addr{netip.MustParseAddr(publicAddress)}, nil + }, + } + baseTransport := &http.Transport{DialContext: func(_ context.Context, _, address string) (net.Conn, error) { + dialedAddress = address + clientConn, serverConn := net.Pipe() + go func() { + defer serverConn.Close() + reader := bufio.NewReader(serverConn) + for { + line, err := reader.ReadString('\n') + if err != nil || line == "\r\n" { + break + } + } + _, _ = io.WriteString(serverConn, "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\n{}") + }() + return clientConn, nil + }} + client := &http.Client{Transport: advertisedOAuthDiscoveryTransport{base: baseTransport, policy: policy}} + + response, err := client.Get("http://auth.example/metadata") + if err != nil { + t.Fatalf("GET pinned discovery target: %v", err) + } + response.Body.Close() + if dialedAddress != net.JoinHostPort(publicAddress, "80") { + t.Fatalf("dialed %q, want resolved address", dialedAddress) + } +} + +func TestPinnedOAuthDiscoveryPreservesHTTPSProxyFirstHop(t *testing.T) { + for _, test := range []struct { + name string + proxy string + firstHop string + }{ + {name: "HTTP", proxy: "http://proxy.example:8080", firstHop: "proxy.example:8080"}, + {name: "HTTPS", proxy: "https://proxy.example:8443", firstHop: "proxy.example:8443"}, + {name: "SOCKS", proxy: "socks5://proxy.example:1080", firstHop: "proxy.example:1080"}, + } { + t.Run(test.name, func(t *testing.T) { + proxyURL, err := url.Parse(test.proxy) + if err != nil { + t.Fatalf("parse proxy URL: %v", err) + } + var dialedAddress string + baseTransport := &http.Transport{ + Proxy: http.ProxyURL(proxyURL), + DialContext: func(_ context.Context, _, address string) (net.Conn, error) { + dialedAddress = address + return nil, errors.New("stop after observing first hop") + }, + } + request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://auth.example/metadata", nil) + if err != nil { + t.Fatalf("create request: %v", err) + } + + _, _ = roundTripPinnedOAuthDiscovery(request, baseTransport, netip.MustParseAddr("93.184.216.34")) + if dialedAddress != test.firstHop { + t.Fatalf("dialed %q, want %s proxy first hop", dialedAddress, test.name) + } + }) + } +} diff --git a/internal/mcp/oauth_protected_resource.go b/internal/mcp/oauth_protected_resource.go new file mode 100644 index 000000000..105ef33e0 --- /dev/null +++ b/internal/mcp/oauth_protected_resource.go @@ -0,0 +1,233 @@ +package mcp + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" + + "github.com/Gitlawb/zero/internal/oauth" +) + +const protectedResourceMetadataLimit = 1 << 20 + +type protectedResourceMetadata struct { + Resource string `json:"resource"` + AuthorizationServers []string `json:"authorization_servers"` +} + +// withoutDiscoveryRedirects returns a shallow client copy that exposes 3xx +// responses to the caller. Every discovered URL is validated before use, so a +// redirect must never replace that validated target with an unchecked one. +func withoutDiscoveryRedirects(client *http.Client) *http.Client { + if client == nil { + client = http.DefaultClient + } + copied := *client + // Discovery requests are deliberately unauthenticated. A caller-supplied + // jar must not turn a plain metadata GET into a credential-bearing request. + copied.Jar = nil + copied.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + return &copied +} + +// discoverProtectedResourceAuthorizationServer follows the RFC 9728 URL +// advertised by a protected MCP resource. found is false when the endpoint does +// not advertise protected-resource metadata, allowing legacy direct discovery. +func discoverProtectedResourceAuthorizationServer(ctx context.Context, client *http.Client, resourceURL string) (issuer string, found bool, err error) { + resourceURL = strings.TrimSpace(resourceURL) + client = withoutDiscoveryRedirects(client) + metadataURL, found, err := probeProtectedResourceMetadataURL(ctx, client, resourceURL) + if err != nil || !found { + return "", found, err + } + if err := validateResourceMetadataOrigin(resourceURL, metadataURL); err != nil { + return "", true, err + } + client, err = newAdvertisedOAuthDiscoveryClient(client, resourceURL) + if err != nil { + return "", true, err + } + metadata, err := fetchProtectedResourceMetadata(ctx, client, metadataURL) + if err != nil { + return "", true, err + } + if metadata.Resource != resourceURL { + return "", true, errors.New("mcp oauth: protected resource metadata resource does not match the MCP server URL") + } + if len(metadata.AuthorizationServers) == 0 { + return "", true, errors.New("mcp oauth: protected resource metadata has no authorization_servers") + } + issuer = metadata.AuthorizationServers[0] + if strings.TrimSpace(issuer) == "" { + return "", true, errors.New("mcp oauth: protected resource metadata has an empty authorization server") + } + if issuer != strings.TrimSpace(issuer) { + return "", true, errors.New("mcp oauth: protected resource authorization server must not contain surrounding whitespace") + } + if err := oauth.ValidateEndpointURL(issuer); err != nil { + return "", true, fmt.Errorf("mcp oauth: protected resource authorization server: %w", err) + } + if err := validateAdvertisedOAuthDiscoveryURL(resourceURL, issuer); err != nil { + return "", true, err + } + return issuer, true, nil +} + +func validateResourceMetadataOrigin(resourceURL string, metadataURL string) error { + resource, resourceErr := url.Parse(resourceURL) + metadata, metadataErr := url.Parse(metadataURL) + if resourceErr != nil || metadataErr != nil || !sameMCPOrigin(resource, metadata) { + return errors.New("mcp oauth: protected resource metadata URL must be same-origin with the MCP server URL") + } + return nil +} + +func probeProtectedResourceMetadataURL(ctx context.Context, client *http.Client, resourceURL string) (string, bool, error) { + if client == nil { + client = http.DefaultClient + } + request, err := http.NewRequestWithContext(ctx, http.MethodGet, strings.TrimSpace(resourceURL), nil) + if err != nil { + return "", false, errors.New("mcp oauth: invalid MCP resource URL") + } + request.Header.Set("Accept", "application/json, text/event-stream") + request.Header.Set("MCP-Protocol-Version", defaultProtocolVersion) + response, err := client.Do(request) + if err != nil { + // A resource that cannot be probed may still support the legacy direct + // authorization-server discovery flow. + return "", false, nil + } + defer response.Body.Close() + if response.StatusCode != http.StatusUnauthorized { + return "", false, nil + } + metadataURL, found, err := parseResourceMetadataURL(response.Header.Values("WWW-Authenticate")) + if err != nil { + return "", false, fmt.Errorf("mcp oauth: invalid WWW-Authenticate resource_metadata parameter: %w", err) + } + if !found { + return "", false, nil + } + if err := oauth.ValidateEndpointURL(metadataURL); err != nil { + return "", false, fmt.Errorf("mcp oauth: protected resource metadata URL: %w", err) + } + return metadataURL, true, nil +} + +func fetchProtectedResourceMetadata(ctx context.Context, client *http.Client, metadataURL string) (protectedResourceMetadata, error) { + if client == nil { + client = http.DefaultClient + } + request, err := http.NewRequestWithContext(ctx, http.MethodGet, metadataURL, nil) + if err != nil { + return protectedResourceMetadata{}, errors.New("mcp oauth: invalid protected resource metadata URL") + } + request.Header.Set("Accept", "application/json") + response, err := client.Do(request) + if err != nil { + return protectedResourceMetadata{}, errors.New("mcp oauth: fetch protected resource metadata failed") + } + defer response.Body.Close() + if response.StatusCode < http.StatusOK || response.StatusCode >= http.StatusMultipleChoices { + return protectedResourceMetadata{}, fmt.Errorf("mcp oauth: protected resource metadata returned HTTP %d", response.StatusCode) + } + var metadata protectedResourceMetadata + if err := json.NewDecoder(io.LimitReader(response.Body, protectedResourceMetadataLimit)).Decode(&metadata); err != nil { + return protectedResourceMetadata{}, errors.New("mcp oauth: decode protected resource metadata failed") + } + return metadata, nil +} + +func parseResourceMetadataURL(headerValues []string) (string, bool, error) { + var selected string + for _, headerValue := range headerValues { + for index := 0; index < len(headerValue); { + if headerValue[index] == '"' { + _, next, err := readAuthQuotedString(headerValue, index) + if err != nil { + // Malformed parameters for another authentication scheme do + // not suppress a valid challenge in a separate header value. + break + } + index = next + continue + } + if !isAuthTokenByte(headerValue[index]) { + index++ + continue + } + start := index + for index < len(headerValue) && isAuthTokenByte(headerValue[index]) { + index++ + } + name := headerValue[start:index] + for index < len(headerValue) && (headerValue[index] == ' ' || headerValue[index] == '\t') { + index++ + } + if index >= len(headerValue) || headerValue[index] != '=' { + continue + } + index++ + for index < len(headerValue) && (headerValue[index] == ' ' || headerValue[index] == '\t') { + index++ + } + if !strings.EqualFold(name, "resource_metadata") { + continue + } + if index >= len(headerValue) || headerValue[index] != '"' { + return "", false, errors.New("resource_metadata must be a quoted URL") + } + value, next, err := readAuthQuotedString(headerValue, index) + if err != nil { + return "", false, err + } + value = strings.TrimSpace(value) + if value == "" { + return "", false, errors.New("resource_metadata URL is empty") + } + if selected != "" && selected != value { + return "", false, errors.New("conflicting resource_metadata URLs") + } + selected = value + index = next + } + } + return selected, selected != "", nil +} + +func readAuthQuotedString(value string, quote int) (string, int, error) { + var decoded strings.Builder + for index := quote + 1; index < len(value); index++ { + switch value[index] { + case '"': + return decoded.String(), index + 1, nil + case '\\': + index++ + if index >= len(value) { + return "", 0, errors.New("unterminated quoted resource_metadata URL") + } + decoded.WriteByte(value[index]) + default: + if value[index] < 0x20 || value[index] == 0x7f { + return "", 0, errors.New("resource_metadata URL contains a control character") + } + decoded.WriteByte(value[index]) + } + } + return "", 0, errors.New("unterminated quoted resource_metadata URL") +} + +func isAuthTokenByte(value byte) bool { + if value >= 'a' && value <= 'z' || value >= 'A' && value <= 'Z' || value >= '0' && value <= '9' { + return true + } + return strings.ContainsRune("!#$%&'*+-.^_`|~", rune(value)) +} diff --git a/internal/mcp/oauth_protected_resource_test.go b/internal/mcp/oauth_protected_resource_test.go new file mode 100644 index 000000000..0c0b5fa9c --- /dev/null +++ b/internal/mcp/oauth_protected_resource_test.go @@ -0,0 +1,742 @@ +package mcp + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "testing" + "time" +) + +func TestParseResourceMetadataURL(t *testing.T) { + got, found, err := parseResourceMetadataURL([]string{ + `Basic realm="resource_metadata=not-a-parameter"`, + `Bearer realm="OAuth", scope="read", Resource_Metadata="https://mcp.example/resource-metadata"`, + }) + if err != nil || !found { + t.Fatalf("parseResourceMetadataURL() found=%v err=%v", found, err) + } + if got != "https://mcp.example/resource-metadata" { + t.Fatalf("resource metadata URL = %q", got) + } + + got, found, err = parseResourceMetadataURL([]string{ + `Basic realm="unterminated`, + `Bearer resource_metadata="https://mcp.example/fallback-metadata"`, + }) + if err != nil || !found || got != "https://mcp.example/fallback-metadata" { + t.Fatalf("unrelated malformed challenge blocked discovery: got=%q found=%v err=%v", got, found, err) + } + + for name, headers := range map[string][]string{ + "unquoted": {`Bearer resource_metadata=https://mcp.example/metadata`}, + "unterminated": {`Bearer resource_metadata="https://mcp.example/metadata`}, + "conflicting": { + `Bearer resource_metadata="https://mcp.example/one"`, + `DPoP resource_metadata="https://mcp.example/two"`, + }, + } { + t.Run(name, func(t *testing.T) { + if _, _, err := parseResourceMetadataURL(headers); err == nil { + t.Fatal("expected malformed resource_metadata rejection") + } + }) + } +} + +func TestProtectedResourceDiscoveryRejectsInvalidMetadata(t *testing.T) { + for _, test := range []struct { + name string + challenge func(base string) string + metadata func(base string) string + wantError string + }{ + { + name: "malformed challenge", + challenge: func(string) string { return `Bearer resource_metadata="unterminated` }, + wantError: "WWW-Authenticate", + }, + { + name: "insecure metadata URL", + challenge: func(string) string { return `Bearer resource_metadata="http://metadata.example/resource"` }, + wantError: "metadata URL", + }, + { + name: "invalid JSON", + challenge: func(base string) string { return `Bearer resource_metadata="` + base + `/metadata"` }, + metadata: func(string) string { return `{` }, + wantError: "decode protected resource metadata", + }, + { + name: "resource mismatch", + challenge: func(base string) string { return `Bearer resource_metadata="` + base + `/metadata"` }, + metadata: func(base string) string { + return `{"resource":"` + base + `/other","authorization_servers":["https://auth.example"]}` + }, + wantError: "does not match", + }, + { + name: "resource leading whitespace mismatch", + challenge: func(base string) string { return `Bearer resource_metadata="` + base + `/metadata"` }, + metadata: func(base string) string { + return `{"resource":" ` + base + `/mcp","authorization_servers":["https://auth.example"]}` + }, + wantError: "does not match", + }, + { + name: "resource trailing whitespace mismatch", + challenge: func(base string) string { return `Bearer resource_metadata="` + base + `/metadata"` }, + metadata: func(base string) string { + return `{"resource":"` + base + `/mcp ","authorization_servers":["https://auth.example"]}` + }, + wantError: "does not match", + }, + { + name: "authorization servers missing", + challenge: func(base string) string { return `Bearer resource_metadata="` + base + `/metadata"` }, + metadata: func(base string) string { + return `{"resource":"` + base + `/mcp"}` + }, + wantError: "authorization_servers", + }, + { + name: "insecure authorization server", + challenge: func(base string) string { return `Bearer resource_metadata="` + base + `/metadata"` }, + metadata: func(base string) string { + return `{"resource":"` + base + `/mcp","authorization_servers":["http://auth.example"]}` + }, + wantError: "authorization server", + }, + } { + t.Run(test.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + w.Header().Set("WWW-Authenticate", test.challenge(base)) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + if test.metadata == nil { + http.NotFound(w, r) + return + } + _, _ = w.Write([]byte(test.metadata(base))) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + _, _, err := discoverProtectedResourceAuthorizationServer(context.Background(), server.Client(), server.URL+"/mcp") + if err == nil || !strings.Contains(err.Error(), test.wantError) { + t.Fatalf("error = %v, want %q", err, test.wantError) + } + }) + } +} + +func TestProtectedResourceProbeSendsNoCredentials(t *testing.T) { + var authorization, cookie string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authorization = r.Header.Get("Authorization") + cookie = r.Header.Get("Cookie") + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("create cookie jar: %v", err) + } + serverURL, err := url.Parse(server.URL) + if err != nil { + t.Fatalf("parse server URL: %v", err) + } + jar.SetCookies(serverURL, []*http.Cookie{{Name: "session", Value: "secret"}}) + client := server.Client() + client.Jar = jar + + _, found, err := discoverProtectedResourceAuthorizationServer(context.Background(), client, server.URL+"/mcp") + if err != nil || found { + t.Fatalf("discovery found=%v err=%v", found, err) + } + if authorization != "" || cookie != "" { + t.Fatalf("probe sent credentials: authorization=%q cookie=%q", authorization, cookie) + } +} + +func TestDirectAuthorizationServerDiscoverySendsNoCookies(t *testing.T) { + var cookie string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cookie = r.Header.Get("Cookie") + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base, + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + })) + defer server.Close() + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("create cookie jar: %v", err) + } + serverURL, err := url.Parse(server.URL) + if err != nil { + t.Fatalf("parse server URL: %v", err) + } + jar.SetCookies(serverURL, []*http.Cookie{{Name: "session", Value: "secret"}}) + client := server.Client() + client.Jar = jar + + if _, err := resolveAuthorizationServer(context.Background(), client, server.URL, OAuthConfig{}); err != nil { + t.Fatalf("resolveAuthorizationServer() error = %v", err) + } + if cookie != "" { + t.Fatalf("direct discovery sent cookie %q", cookie) + } +} + +func TestProtectedResourceDiscoveryRejectsAdvertisedLoopbackMetadata(t *testing.T) { + var metadataHits atomic.Int64 + client := &http.Client{Transport: roundTripperFunc(func(request *http.Request) (*http.Response, error) { + response := &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + Request: request, + } + switch request.URL.String() { + case "https://mcp.example/mcp": + response.StatusCode = http.StatusUnauthorized + response.Header.Set("WWW-Authenticate", `Bearer resource_metadata="http://127.0.0.1/metadata"`) + case "http://127.0.0.1/metadata": + metadataHits.Add(1) + response.Body = io.NopCloser(strings.NewReader(`{"resource":"https://mcp.example/mcp","authorization_servers":["https://auth.example"]}`)) + default: + response.StatusCode = http.StatusNotFound + } + return response, nil + })} + + _, _, err := discoverProtectedResourceAuthorizationServer(context.Background(), client, "https://mcp.example/mcp") + if err == nil { + t.Fatal("advertised loopback metadata URL should be rejected") + } + if metadataHits.Load() != 0 { + t.Fatalf("loopback metadata target was fetched %d time(s)", metadataHits.Load()) + } +} + +func TestProtectedResourceDiscoveryRejectsAdvertisedLoopbackIssuer(t *testing.T) { + var issuerHits atomic.Int64 + client := &http.Client{Transport: roundTripperFunc(func(request *http.Request) (*http.Response, error) { + response := &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + Request: request, + } + switch request.URL.String() { + case "https://mcp.example/mcp": + response.StatusCode = http.StatusUnauthorized + response.Header.Set("WWW-Authenticate", `Bearer resource_metadata="https://mcp.example/metadata"`) + case "https://mcp.example/metadata": + response.Body = io.NopCloser(strings.NewReader(`{"resource":"https://mcp.example/mcp","authorization_servers":["http://127.0.0.1"]}`)) + case "http://127.0.0.1/.well-known/oauth-authorization-server": + issuerHits.Add(1) + response.Body = io.NopCloser(strings.NewReader(`{"issuer":"http://127.0.0.1","authorization_endpoint":"http://127.0.0.1/authorize","token_endpoint":"http://127.0.0.1/token"}`)) + default: + response.StatusCode = http.StatusNotFound + } + return response, nil + })} + + _, err := resolveAuthorizationServer(context.Background(), client, "https://mcp.example/mcp", OAuthConfig{}) + if err == nil { + t.Fatal("advertised loopback issuer should be rejected") + } + if issuerHits.Load() != 0 { + t.Fatalf("loopback issuer target was fetched %d time(s)", issuerHits.Load()) + } +} + +func TestProtectedResourceDiscoveryRejectsAdvertisedPrivateTargets(t *testing.T) { + for _, target := range []string{ + "http://10.0.0.8/metadata", + "https://169.254.169.254/metadata", + "https://metadata.google.internal/metadata", + } { + t.Run(target, func(t *testing.T) { + if err := validateAdvertisedOAuthDiscoveryURL("https://mcp.example/mcp", target); err == nil { + t.Fatalf("advertised target %q should be rejected", target) + } + }) + } +} + +func TestProtectedDiscoveryPathsSendNoCookies(t *testing.T) { + var probeCookie, resourceCookie, issuerCookie string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + probeCookie = r.Header.Get("Cookie") + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + resourceCookie = r.Header.Get("Cookie") + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": base + "/mcp", + "authorization_servers": []string{base}, + }) + case "/.well-known/oauth-authorization-server": + issuerCookie = r.Header.Get("Cookie") + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base, + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("create cookie jar: %v", err) + } + serverURL, err := url.Parse(server.URL) + if err != nil { + t.Fatalf("parse server URL: %v", err) + } + jar.SetCookies(serverURL, []*http.Cookie{{Name: "session", Value: "secret"}}) + client := server.Client() + client.Jar = jar + + if _, err := resolveAuthorizationServer(context.Background(), client, server.URL+"/mcp", OAuthConfig{}); err != nil { + t.Fatalf("resolveAuthorizationServer() error = %v", err) + } + if probeCookie != "" || resourceCookie != "" || issuerCookie != "" { + t.Fatalf("discovery sent cookies: probe=%q resource=%q issuer=%q", probeCookie, resourceCookie, issuerCookie) + } +} + +func TestProtectedDiscoveryPreservesExplicitEndpointOverride(t *testing.T) { + fixture := newPublicProtectedOAuthFixture(t, "", "https://93.184.216.35/token", "", "") + explicitToken := "http://127.0.0.1:19432/token" + metadata, err := resolveAuthorizationServer(context.Background(), fixture.client, fixture.resourceURL, OAuthConfig{TokenEndpoint: explicitToken}) + if err != nil { + t.Fatalf("resolve with explicit endpoint override: %v", err) + } + if metadata.TokenEndpoint != explicitToken || metadata.protectTokenEndpoint { + t.Fatalf("explicit token override lost provenance: endpoint=%q protected=%v", metadata.TokenEndpoint, metadata.protectTokenEndpoint) + } +} + +func TestProtectedDiscoveryRejectsLoopbackRegistrationEndpointBeforeUse(t *testing.T) { + var registrationHits atomic.Int64 + registration := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + registrationHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{"client_id": "registered-client"}) + })) + defer registration.Close() + fixture := newPublicProtectedOAuthFixture(t, registration.URL+"/register", "https://93.184.216.35/token", "", "") + + _, err := Login(context.Background(), LoginOptions{ + ServerName: "registration-ssrf", + ServerURL: fixture.resourceURL, + HTTPClient: fixture.client, + OpenBrowser: func(string) error { + return errors.New("authorization browser should not open") + }, + Timeout: 5 * time.Second, + }) + if err == nil { + t.Fatal("protected discovery should reject a loopback registration endpoint") + } + if registrationHits.Load() != 0 { + t.Fatalf("loopback registration endpoint received %d request(s)", registrationHits.Load()) + } +} + +func TestProtectedDiscoveryRejectsLoopbackTokenEndpointBeforeUse(t *testing.T) { + var tokenHits atomic.Int64 + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + tokenHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "unexpected", + "token_type": "Bearer", + }) + })) + defer tokenServer.Close() + fixture := newPublicProtectedOAuthFixture(t, "", tokenServer.URL+"/token", "", "") + + _, err := Login(context.Background(), LoginOptions{ + ServerName: "token-ssrf", + ServerURL: fixture.resourceURL, + Config: OAuthConfig{ClientID: "configured-client"}, + HTTPClient: fixture.client, + OpenBrowser: func(authURL string) error { + parsed, parseErr := url.Parse(authURL) + if parseErr != nil { + return parseErr + } + callbackURL := parsed.Query().Get("redirect_uri") + "?code=code&state=" + url.QueryEscape(parsed.Query().Get("state")) + go func() { + for attempt := 0; attempt < 20; attempt++ { + response, requestErr := http.Get(callbackURL) + if requestErr == nil { + response.Body.Close() + return + } + time.Sleep(25 * time.Millisecond) + } + }() + return nil + }, + Timeout: 5 * time.Second, + }) + if err == nil { + t.Fatal("protected discovery should reject a loopback token endpoint") + } + if tokenHits.Load() != 0 { + t.Fatalf("loopback token endpoint received %d request(s)", tokenHits.Load()) + } +} + +func TestProtectedDiscoveryRegistrationRefusesPublicToPrivateRedirect(t *testing.T) { + var victimHits atomic.Int64 + victim := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + victimHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{"client_id": "redirected-client"}) + })) + defer victim.Close() + fixture := newPublicProtectedOAuthFixture(t, "", "https://93.184.216.35/token", victim.URL+"/register", "") + + _, err := Login(context.Background(), LoginOptions{ + ServerName: "registration-redirect", + ServerURL: fixture.resourceURL, + HTTPClient: fixture.client, + OpenBrowser: func(string) error { + return errors.New("authorization browser should not open") + }, + Timeout: 5 * time.Second, + }) + if err == nil { + t.Fatal("protected registration redirect should be refused") + } + if victimHits.Load() != 0 { + t.Fatalf("registration redirect reached private victim %d time(s)", victimHits.Load()) + } +} + +func TestProtectedDiscoveryTokenRefusesPublicToPrivateRedirect(t *testing.T) { + var victimHits atomic.Int64 + victim := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + victimHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "redirected-token", "token_type": "Bearer"}) + })) + defer victim.Close() + fixture := newPublicProtectedOAuthFixture(t, "", "", "", victim.URL+"/token") + + _, err := Login(context.Background(), LoginOptions{ + ServerName: "token-redirect", + ServerURL: fixture.resourceURL, + Config: OAuthConfig{ClientID: "configured-client"}, + HTTPClient: fixture.client, + OpenBrowser: func(authURL string) error { + parsed, parseErr := url.Parse(authURL) + if parseErr != nil { + return parseErr + } + callbackURL := parsed.Query().Get("redirect_uri") + "?code=code&state=" + url.QueryEscape(parsed.Query().Get("state")) + go func() { + for attempt := 0; attempt < 20; attempt++ { + response, requestErr := http.Get(callbackURL) + if requestErr == nil { + response.Body.Close() + return + } + time.Sleep(25 * time.Millisecond) + } + }() + return nil + }, + Timeout: 5 * time.Second, + }) + if err == nil { + t.Fatal("protected token redirect should be refused") + } + if victimHits.Load() != 0 { + t.Fatalf("token redirect reached private victim %d time(s)", victimHits.Load()) + } +} + +type publicProtectedOAuthFixture struct { + client *http.Client + resourceURL string +} + +func newPublicProtectedOAuthFixture(t *testing.T, registrationEndpoint string, tokenEndpoint string, registrationRedirect string, tokenRedirect string) publicProtectedOAuthFixture { + t.Helper() + var resourceOrigin, issuerOrigin string + gateway := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + resourceHost := strings.TrimPrefix(resourceOrigin, "https://") + issuerHost := strings.TrimPrefix(issuerOrigin, "https://") + switch { + case r.Host == resourceHost && r.URL.Path == "/mcp": + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+resourceOrigin+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case r.Host == resourceHost && r.URL.Path == "/metadata": + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": resourceOrigin + "/mcp", + "authorization_servers": []string{issuerOrigin}, + }) + case r.Host == issuerHost && r.URL.Path == "/.well-known/oauth-authorization-server": + metadataRegistration := registrationEndpoint + metadataToken := tokenEndpoint + if registrationRedirect != "" { + metadataRegistration = issuerOrigin + "/register" + } + if tokenRedirect != "" { + metadataToken = issuerOrigin + "/token" + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": issuerOrigin, + "authorization_endpoint": issuerOrigin + "/authorize", + "token_endpoint": metadataToken, + "registration_endpoint": metadataRegistration, + }) + case r.Host == issuerHost && r.URL.Path == "/register" && registrationRedirect != "": + http.Redirect(w, r, registrationRedirect, http.StatusTemporaryRedirect) + case r.Host == issuerHost && r.URL.Path == "/token" && tokenRedirect != "": + http.Redirect(w, r, tokenRedirect, http.StatusTemporaryRedirect) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(gateway.Close) + port := gateway.Listener.Addr().(*net.TCPAddr).Port + resourceOrigin = fmt.Sprintf("https://93.184.216.34:%d", port) + issuerOrigin = fmt.Sprintf("https://93.184.216.35:%d", port) + + transport := gateway.Client().Transport.(*http.Transport).Clone() + transport.TLSClientConfig = transport.TLSClientConfig.Clone() + transport.TLSClientConfig.InsecureSkipVerify = true //nolint:gosec // hermetic public-address routing fixture + dialer := &net.Dialer{} + gatewayAddress := gateway.Listener.Addr().String() + transport.DialContext = func(ctx context.Context, network string, address string) (net.Conn, error) { + host, _, err := net.SplitHostPort(address) + if err == nil && (host == "93.184.216.34" || host == "93.184.216.35") { + return dialer.DialContext(ctx, network, gatewayAddress) + } + return dialer.DialContext(ctx, network, address) + } + return publicProtectedOAuthFixture{ + client: &http.Client{Transport: transport}, + resourceURL: resourceOrigin + "/mcp", + } +} + +func TestProtectedResourceProbeDoesNotFollowRedirects(t *testing.T) { + var redirectedHits atomic.Int64 + redirected := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + redirectedHits.Add(1) + w.WriteHeader(http.StatusUnauthorized) + })) + defer redirected.Close() + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, redirected.URL+"/mcp", http.StatusFound) + })) + defer source.Close() + + _, found, err := discoverProtectedResourceAuthorizationServer(context.Background(), source.Client(), source.URL+"/mcp") + if err != nil || found { + t.Fatalf("redirected probe found=%v err=%v", found, err) + } + if redirectedHits.Load() != 0 { + t.Fatalf("probe followed redirect %d time(s)", redirectedHits.Load()) + } +} + +func TestProtectedResourceMetadataDoesNotFollowRedirects(t *testing.T) { + var redirectedHits atomic.Int64 + redirected := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + redirectedHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": "https://attacker.example/mcp", + "authorization_servers": []string{"https://attacker.example"}, + }) + })) + defer redirected.Close() + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + http.Redirect(w, r, redirected.URL+"/metadata", http.StatusFound) + } + })) + defer source.Close() + + _, _, err := discoverProtectedResourceAuthorizationServer(context.Background(), source.Client(), source.URL+"/mcp") + if err == nil || !strings.Contains(err.Error(), "HTTP 302") { + t.Fatalf("error = %v, want redirect refusal", err) + } + if redirectedHits.Load() != 0 { + t.Fatalf("metadata fetch followed redirect %d time(s)", redirectedHits.Load()) + } +} + +func TestResolveAuthorizationServerRejectsProtectedIssuerMismatch(t *testing.T) { + authorizationServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": "https://unexpected.example", + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + })) + defer authorizationServer.Close() + resourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": base + "/mcp", + "authorization_servers": []string{authorizationServer.URL}, + }) + } + })) + defer resourceServer.Close() + + _, err := resolveAuthorizationServer(context.Background(), resourceServer.Client(), resourceServer.URL+"/mcp", OAuthConfig{}) + if err == nil || !strings.Contains(err.Error(), "issuer does not match") { + t.Fatalf("error = %v, want issuer mismatch", err) + } +} + +func TestResolveAuthorizationServerRejectsMissingProtectedIssuer(t *testing.T) { + authorizationServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + })) + defer authorizationServer.Close() + resourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": base + "/mcp", + "authorization_servers": []string{authorizationServer.URL}, + }) + } + })) + defer resourceServer.Close() + + _, err := resolveAuthorizationServer(context.Background(), resourceServer.Client(), resourceServer.URL+"/mcp", OAuthConfig{}) + if err == nil || !strings.Contains(err.Error(), "issuer") { + t.Fatalf("error = %v, want missing issuer rejection", err) + } +} + +func TestResolveAuthorizationServerRejectsWhitespaceAlteredProtectedIssuer(t *testing.T) { + for _, alter := range []struct { + name string + apply func(string) string + }{ + {name: "leading", apply: func(value string) string { return " " + value }}, + {name: "trailing", apply: func(value string) string { return value + " " }}, + } { + t.Run(alter.name, func(t *testing.T) { + authorizationServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": alter.apply(base), + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + })) + defer authorizationServer.Close() + resourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": base + "/mcp", + "authorization_servers": []string{authorizationServer.URL}, + }) + } + })) + defer resourceServer.Close() + + _, err := resolveAuthorizationServer(context.Background(), resourceServer.Client(), resourceServer.URL+"/mcp", OAuthConfig{}) + if err == nil || !strings.Contains(err.Error(), "issuer does not match") { + t.Fatalf("error = %v, want whitespace issuer mismatch", err) + } + }) + } +} + +func TestAuthorizationServerMetadataDoesNotFollowRedirects(t *testing.T) { + var redirectedHits atomic.Int64 + redirected := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + redirectedHits.Add(1) + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base, + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + })) + defer redirected.Close() + authorizationServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, redirected.URL+"/.well-known/oauth-authorization-server", http.StatusFound) + })) + defer authorizationServer.Close() + resourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/metadata": + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": base + "/mcp", + "authorization_servers": []string{authorizationServer.URL}, + }) + } + })) + defer resourceServer.Close() + + _, err := resolveAuthorizationServer(context.Background(), resourceServer.Client(), resourceServer.URL+"/mcp", OAuthConfig{}) + if err == nil { + t.Fatal("authorization metadata redirect should fail") + } + if redirectedHits.Load() != 0 { + t.Fatalf("authorization metadata fetch followed redirect %d time(s)", redirectedHits.Load()) + } +} diff --git a/internal/mcp/oauth_test.go b/internal/mcp/oauth_test.go index bbccf5cde..4047c2554 100644 --- a/internal/mcp/oauth_test.go +++ b/internal/mcp/oauth_test.go @@ -10,6 +10,7 @@ import ( "net/http/httptest" "net/url" "strings" + "sync/atomic" "testing" "time" @@ -145,6 +146,130 @@ func TestResolveEndpointsConfigOverridesDiscovery(t *testing.T) { } } +func TestResolveAuthorizationServerUsesProtectedResourceMetadata(t *testing.T) { + var authorizationMetadataHits atomic.Int64 + authorizationServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/.well-known/oauth-authorization-server" { + http.NotFound(w, r) + return + } + authorizationMetadataHits.Add(1) + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base, + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + "registration_endpoint": base + "/register", + }) + })) + defer authorizationServer.Close() + + var resourceProbeHits, resourceMetadataHits atomic.Int64 + resourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/mcp": + resourceProbeHits.Add(1) + metadataURL := "http://" + r.Host + "/resource-metadata" + w.Header().Set("WWW-Authenticate", `Bearer realm="OAuth", resource_metadata="`+metadataURL+`"`) + w.WriteHeader(http.StatusUnauthorized) + case "/resource-metadata": + resourceMetadataHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": "http://" + r.Host + "/mcp", + "authorization_servers": []string{authorizationServer.URL}, + }) + default: + http.NotFound(w, r) + } + })) + defer resourceServer.Close() + + metadata, err := resolveAuthorizationServer(context.Background(), resourceServer.Client(), resourceServer.URL+"/mcp", OAuthConfig{}) + if err != nil { + t.Fatalf("resolveAuthorizationServer() error = %v", err) + } + if metadata.Issuer != authorizationServer.URL { + t.Fatalf("issuer = %q, want %q", metadata.Issuer, authorizationServer.URL) + } + if metadata.AuthorizationEndpoint != authorizationServer.URL+"/authorize" || metadata.TokenEndpoint != authorizationServer.URL+"/token" { + t.Fatalf("resolved metadata = %#v", metadata) + } + if resourceProbeHits.Load() != 1 || resourceMetadataHits.Load() != 1 || authorizationMetadataHits.Load() != 1 { + t.Fatalf("discovery hits = probe:%d resource:%d authorization:%d", resourceProbeHits.Load(), resourceMetadataHits.Load(), authorizationMetadataHits.Load()) + } +} + +func TestResolveAuthorizationServerFallsBackToDirectDiscovery(t *testing.T) { + var resourceProbeHits, directDiscoveryHits atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/mcp": + resourceProbeHits.Add(1) + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + <-r.Context().Done() + case "/.well-known/oauth-authorization-server/mcp": + directDiscoveryHits.Add(1) + base := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base + "/mcp", + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + metadata, err := resolveAuthorizationServer(context.Background(), server.Client(), server.URL+"/mcp", OAuthConfig{}) + if err != nil { + t.Fatalf("resolveAuthorizationServer() error = %v", err) + } + if metadata.AuthorizationEndpoint != server.URL+"/authorize" || metadata.TokenEndpoint != server.URL+"/token" { + t.Fatalf("resolved metadata = %#v", metadata) + } + if resourceProbeHits.Load() != 1 || directDiscoveryHits.Load() != 1 { + t.Fatalf("discovery hits = probe:%d direct:%d", resourceProbeHits.Load(), directDiscoveryHits.Load()) + } +} + +func TestResolveAuthorizationServerIssuerOverrideSkipsResourceProbe(t *testing.T) { + var resourceProbeHits, issuerDiscoveryHits atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + resourceProbeHits.Add(1) + w.WriteHeader(http.StatusUnauthorized) + case "/.well-known/oauth-authorization-server/issuer": + issuerDiscoveryHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base + "/issuer", + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + metadata, err := resolveAuthorizationServer(context.Background(), server.Client(), server.URL+"/mcp", OAuthConfig{IssuerURL: server.URL + "/issuer"}) + if err != nil { + t.Fatalf("resolveAuthorizationServer() error = %v", err) + } + if metadata.AuthorizationEndpoint != server.URL+"/authorize" || metadata.TokenEndpoint != server.URL+"/token" { + t.Fatalf("resolved metadata = %#v", metadata) + } + if resourceProbeHits.Load() != 0 || issuerDiscoveryHits.Load() != 1 { + t.Fatalf("discovery hits = resource:%d issuer:%d", resourceProbeHits.Load(), issuerDiscoveryHits.Load()) + } +} + func TestPKCEChallengeIsS256OfVerifier(t *testing.T) { pkce, err := newPKCE() if err != nil { @@ -448,3 +573,98 @@ func TestFullFlowStoresTokens(t *testing.T) { t.Fatalf("login token = %#v", result) } } + +func TestFullFlowUsesProtectedResourceDiscovery(t *testing.T) { + var resourceProbeHits, resourceMetadataHits, authorizationMetadataHits, tokenHits atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + switch r.URL.Path { + case "/mcp": + resourceProbeHits.Add(1) + if r.Header.Get("Authorization") != "" || r.Header.Get("Cookie") != "" { + t.Error("resource probe sent credentials") + } + w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="`+base+`/resource-metadata"`) + w.WriteHeader(http.StatusUnauthorized) + case "/resource-metadata": + resourceMetadataHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "resource": base + "/mcp", + "authorization_servers": []string{base}, + }) + case "/.well-known/oauth-authorization-server": + authorizationMetadataHits.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": base, + "authorization_endpoint": base + "/authorize", + "token_endpoint": base + "/token", + }) + case "/token": + tokenHits.Add(1) + if err := r.ParseForm(); err != nil { + t.Errorf("parse token form: %v", err) + } + if r.Form.Get("code") != "authorization-code-secret" || r.Form.Get("code_verifier") == "" { + t.Error("token exchange omitted the authorization code or PKCE verifier") + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "access-token-secret", + "refresh_token": "refresh-token-secret", + "token_type": "Bearer", + "expires_in": 3600, + }) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + opener := func(authURL string) error { + parsed, err := url.Parse(authURL) + if err != nil { + return err + } + if parsed.Path != "/authorize" || parsed.Query().Get("code_challenge_method") != "S256" || parsed.Query().Get("code_challenge") == "" { + return errors.New("authorization request omitted the discovered endpoint or PKCE challenge") + } + if parsed.Query().Get("code") != "" || parsed.Query().Get("code_verifier") != "" { + return errors.New("authorization request exposed an authorization code or PKCE verifier") + } + state := parsed.Query().Get("state") + redirect := parsed.Query().Get("redirect_uri") + go func() { + callbackURL := redirect + "?code=authorization-code-secret&state=" + url.QueryEscape(state) + for attempt := 0; attempt < 20; attempt++ { + response, requestErr := http.Get(callbackURL) + if requestErr == nil { + response.Body.Close() + return + } + time.Sleep(50 * time.Millisecond) + } + }() + return nil + } + + token, err := Login(context.Background(), LoginOptions{ + ServerName: "notion-style", + ServerURL: server.URL + "/mcp", + Config: OAuthConfig{ + ClientID: "client-123", + Scopes: []string{"read"}, + }, + HTTPClient: server.Client(), + OpenBrowser: opener, + Timeout: 5 * time.Second, + Now: time.Now, + }) + if err != nil { + t.Fatalf("Login() error = %v", err) + } + if token.AccessToken != "access-token-secret" || token.RefreshToken != "refresh-token-secret" { + t.Fatal("login did not return the issued tokens") + } + if resourceProbeHits.Load() != 1 || resourceMetadataHits.Load() != 1 || authorizationMetadataHits.Load() != 1 || tokenHits.Load() != 1 { + t.Fatalf("flow hits = probe:%d resource:%d authorization:%d token:%d", resourceProbeHits.Load(), resourceMetadataHits.Load(), authorizationMetadataHits.Load(), tokenHits.Load()) + } +}