From 37b3f1124312b27f4ff8c698f58dbfa392b6f6b5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 11 Aug 2026 12:02:34 +0200 Subject: [PATCH] fix(coderd): block SSRF in MCP OAuth2 discovery and client registration (#27989) --- coderd/coderd.go | 8 + coderd/coderdtest/coderdtest.go | 89 ++++++---- coderd/mcp.go | 10 +- coderd/mcp_ssrf.go | 188 +++++++++++++++++++++ coderd/mcp_ssrf_internal_test.go | 274 +++++++++++++++++++++++++++++++ coderd/mcp_test.go | 73 ++++++++ 6 files changed, 602 insertions(+), 40 deletions(-) create mode 100644 coderd/mcp_ssrf.go create mode 100644 coderd/mcp_ssrf_internal_test.go diff --git a/coderd/coderd.go b/coderd/coderd.go index 75279d1c52..fad338d39d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -13,6 +13,7 @@ import ( "math" "net/http" httppprof "net/http/pprof" + "net/netip" "net/url" "path/filepath" "regexp" @@ -261,6 +262,13 @@ type Options struct { SSHConfig codersdk.SSHConfigResponse HTTPClient *http.Client + // MCPOAuth2DiscoveryAllowedIPRanges exempts IP ranges from the + // SSRF guard applied to MCP OAuth2 metadata discovery and dynamic + // client registration, which refuse private/internal destinations + // by default. This is a seam for tests, which serve their mock MCP + // servers on loopback; there is intentionally no user-facing + // configuration for it. + MCPOAuth2DiscoveryAllowedIPRanges []netip.Prefix // ChatStreamPartsDialer dials remote chat stream parts. // Set by enterprise for HA deployments. Nil uses chatd's local // in-process channel dialer. diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 7ab7ca8fcc..5377658a35 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -22,6 +22,7 @@ import ( "net" "net/http" "net/http/httptest" + "net/netip" "net/url" "regexp" "strconv" @@ -118,27 +119,31 @@ type Options struct { // AccessURL denotes a custom access URL. By default we use the httptest // server's URL. Setting this may result in unexpected behavior (especially // with running agents). - AccessURL *url.URL - AppHostname string - AWSCertificates awsidentity.Certificates - Authorizer rbac.Authorizer - AzureCertificates azureidentity.Options - GithubOAuth2Config *coderd.GithubOAuth2Config - RealIPConfig *httpmw.RealIPConfig - OIDCConfig *coderd.OIDCConfig - GoogleTokenValidator *idtoken.Validator - SSHKeygenAlgorithm gitsshkey.Algorithm - AutobuildTicker <-chan time.Time - AutobuildStats chan<- autobuild.Stats - Auditor audit.Auditor - TLSCertificates []tls.Certificate - ExternalAuthConfigs []*externalauth.Config - TrialGenerator func(ctx context.Context, body codersdk.LicensorTrialRequest) error - RefreshEntitlements func(ctx context.Context) error - TemplateScheduleStore schedule.TemplateScheduleStore - Coordinator tailnet.Coordinator - CoordinatorResumeTokenProvider tailnet.ResumeTokenProvider - ConnectionLogger connectionlog.ConnectionLogger + AccessURL *url.URL + AppHostname string + AWSCertificates awsidentity.Certificates + Authorizer rbac.Authorizer + AzureCertificates azureidentity.Options + GithubOAuth2Config *coderd.GithubOAuth2Config + RealIPConfig *httpmw.RealIPConfig + OIDCConfig *coderd.OIDCConfig + GoogleTokenValidator *idtoken.Validator + SSHKeygenAlgorithm gitsshkey.Algorithm + AutobuildTicker <-chan time.Time + AutobuildStats chan<- autobuild.Stats + Auditor audit.Auditor + TLSCertificates []tls.Certificate + ExternalAuthConfigs []*externalauth.Config + TrialGenerator func(ctx context.Context, body codersdk.LicensorTrialRequest) error + // MCPOAuth2DiscoveryAllowedIPRanges exempts IP ranges from the MCP + // OAuth2 discovery SSRF guard. Defaults to loopback so tests can + // serve mock MCP servers via httptest. + MCPOAuth2DiscoveryAllowedIPRanges []netip.Prefix + RefreshEntitlements func(ctx context.Context) error + TemplateScheduleStore schedule.TemplateScheduleStore + Coordinator tailnet.Coordinator + CoordinatorResumeTokenProvider tailnet.ResumeTokenProvider + ConnectionLogger connectionlog.ConnectionLogger HealthcheckFunc func(ctx context.Context, apiKey string, progress *healthcheck.Progress) *healthsdk.HealthcheckReport HealthcheckTimeout time.Duration @@ -318,6 +323,17 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can options.CoordinatorResumeTokenProvider = tailnet.NewInsecureTestResumeTokenProvider() } + if options.MCPOAuth2DiscoveryAllowedIPRanges == nil { + // Tests serve their mock MCP and authorization servers on + // loopback, which the MCP OAuth2 discovery SSRF guard blocks + // by default. Tests exercising the guard itself pass a + // narrower (possibly empty, non-nil) allowlist. + options.MCPOAuth2DiscoveryAllowedIPRanges = []netip.Prefix{ + netip.MustParsePrefix("127.0.0.0/8"), + netip.MustParsePrefix("::1/128"), + } + } + if options.NotificationsEnqueuer == nil { options.NotificationsEnqueuer = ¬ificationstest.FakeEnqueuer{} } @@ -612,21 +628,22 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can AgentConnectionUpdateFrequency: 150 * time.Millisecond, // Force a long disconnection timeout to ensure // agents are not marked as disconnected during slow tests. - AgentInactiveDisconnectTimeout: testutil.WaitShort, - ChatdInstructionLookupTimeout: options.ChatdInstructionLookupTimeout, - ChatProviderAPIKeys: options.ChatProviderAPIKeys, - ChatWorkerDisabled: options.ChatWorkerDisabled, - AccessURL: accessURL, - AppHostname: options.AppHostname, - AppHostnameRegex: appHostnameRegex, - Logger: *options.Logger, - CacheDir: cacheDir, - RuntimeConfig: runtimeManager, - Database: options.Database, - Pubsub: options.Pubsub, - ReplicaSyncPubsub: options.ReplicaSyncPubsub, - ExternalAuthConfigs: options.ExternalAuthConfigs, - UsageInserter: usageInserter, + AgentInactiveDisconnectTimeout: testutil.WaitShort, + ChatdInstructionLookupTimeout: options.ChatdInstructionLookupTimeout, + MCPOAuth2DiscoveryAllowedIPRanges: options.MCPOAuth2DiscoveryAllowedIPRanges, + ChatProviderAPIKeys: options.ChatProviderAPIKeys, + ChatWorkerDisabled: options.ChatWorkerDisabled, + AccessURL: accessURL, + AppHostname: options.AppHostname, + AppHostnameRegex: appHostnameRegex, + Logger: *options.Logger, + CacheDir: cacheDir, + RuntimeConfig: runtimeManager, + Database: options.Database, + Pubsub: options.Pubsub, + ReplicaSyncPubsub: options.ReplicaSyncPubsub, + ExternalAuthConfigs: options.ExternalAuthConfigs, + UsageInserter: usageInserter, Auditor: options.Auditor, ConnectionLogger: options.ConnectionLogger, diff --git a/coderd/mcp.go b/coderd/mcp.go index 9cf5795e12..4c7ebb918c 100644 --- a/coderd/mcp.go +++ b/coderd/mcp.go @@ -321,10 +321,12 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) { // Now build the callback URL with the actual ID. callbackURL := fmt.Sprintf("%s/api/experimental/mcp/servers/%s/oauth2/callback", api.AccessURL.String(), inserted.ID) - httpClient := api.HTTPClient - if httpClient == nil { - httpClient = &http.Client{Timeout: 30 * time.Second} - } + // Discovery targets are attacker-influenced (the MCP + // server URL and any endpoints or redirects it + // advertises), so all discovery traffic goes through an + // SSRF-guarded client that refuses private/internal + // destinations (CDM-02-002). + httpClient := newMCPDiscoveryHTTPClient(api.HTTPClient, api.MCPOAuth2DiscoveryAllowedIPRanges) result, err := discoverAndRegisterMCPOAuth2(ctx, httpClient, strings.TrimSpace(req.URL), callbackURL) if err != nil { // Clean up: delete the partially created config. diff --git a/coderd/mcp_ssrf.go b/coderd/mcp_ssrf.go new file mode 100644 index 0000000000..cd78264d3e --- /dev/null +++ b/coderd/mcp_ssrf.go @@ -0,0 +1,188 @@ +package coderd + +import ( + "context" + "net" + "net/http" + "net/netip" + "time" + + "golang.org/x/xerrors" +) + +// mcpDiscoveryExtraBlockedPrefixes lists special-use CIDR ranges that +// the stdlib classification methods (IsLoopback, IsPrivate, etc.) do +// not cover. Blocking these prevents SSRF against carrier-grade NAT, +// benchmarking, documentation, discard-only, and the all-zeros "this +// network" ranges. +// +// IPv6 ranges already handled by stdlib: +// - ::1/128 (IsLoopback) +// - fc00::/7 (IsPrivate, ULA) +// - fe80::/10 (IsLinkLocalUnicast) +// - ff00::/8 (IsMulticast) +// - ::/128 (IsUnspecified) +var mcpDiscoveryExtraBlockedPrefixes = []netip.Prefix{ + // IPv4 special-use ranges. + netip.MustParsePrefix("0.0.0.0/8"), // RFC 1122 "this network". + netip.MustParsePrefix("100.64.0.0/10"), // RFC 6598 carrier-grade NAT. + netip.MustParsePrefix("198.18.0.0/15"), // RFC 2544 benchmarking. + + // IPv6 special-use ranges not covered by stdlib. + netip.MustParsePrefix("64:ff9b:1::/48"), // RFC 8215 IPv4/IPv6 translation. + netip.MustParsePrefix("100::/64"), // RFC 6666 discard-only. + netip.MustParsePrefix("2001:2::/48"), // RFC 5180 benchmarking. + netip.MustParsePrefix("2001:db8::/32"), // RFC 3849 documentation. +} + +// isBlockedMCPDiscoveryAddr reports whether addr must not be reached +// during MCP OAuth2 discovery because it is in a private, loopback, +// link-local, multicast, unspecified, or other special-use range. +// IPv4-mapped IPv6 addresses are unmapped first so a literal like +// ::ffff:169.254.169.254 cannot bypass the IPv4 ranges. Prefixes in +// allowed exempt their range from blocking. +func isBlockedMCPDiscoveryAddr(addr netip.Addr, allowed []netip.Prefix) bool { + addr = addr.Unmap() + for _, prefix := range allowed { + if prefix.Contains(addr) { + return false + } + } + if addr.IsLoopback() || + addr.IsPrivate() || + addr.IsLinkLocalUnicast() || + addr.IsLinkLocalMulticast() || + addr.IsMulticast() || + addr.IsUnspecified() || + addr.IsInterfaceLocalMulticast() { + return true + } + for _, prefix := range mcpDiscoveryExtraBlockedPrefixes { + if prefix.Contains(addr) { + return true + } + } + return false +} + +// newMCPDiscoveryHTTPClient returns an HTTP client for MCP OAuth2 +// metadata discovery and Dynamic Client Registration that refuses to +// connect to private/internal addresses. Every URL fetched during +// discovery is attacker-influenced (the MCP server URL and any +// endpoints or redirects it advertises), so without this guard a +// hostile server can pivot coderd into internal infrastructure such +// as cloud metadata services (CDM-02-002). +// +// The guard validates the resolved IPs at dial time and dials a +// validated IP directly, so DNS rebinding cannot swap in a private +// address between validation and connect, and 3xx redirects to +// internal targets are blocked when the redirected connection is +// dialed. Requests never use a proxy: through a proxy the destination +// IP is invisible to the dialer and the guard would be ineffective. +// +// base contributes its timeout and (when its transport is an +// *http.Transport) TLS configuration; its dialing behavior is always +// replaced with the guarded dialer. +func newMCPDiscoveryHTTPClient(base *http.Client, allowed []netip.Prefix) *http.Client { + timeout := 30 * time.Second + var transport *http.Transport + if base != nil { + if base.Timeout > 0 { + timeout = base.Timeout + } + if t, ok := base.Transport.(*http.Transport); ok && t != nil { + transport = t.Clone() + } + } + if transport == nil { + if t, ok := http.DefaultTransport.(*http.Transport); ok { + transport = t.Clone() + } else { + transport = &http.Transport{} + } + } + + // Force every connection through the guarded dialer: no proxies + // and no alternate dial paths that would bypass it. + transport.Proxy = nil + //nolint:staticcheck // Deprecated fields are cleared so the guarded DialContext is authoritative. + transport.Dial = nil + //nolint:staticcheck // Deprecated fields are cleared so the guarded DialContext is authoritative. + transport.DialTLS = nil + transport.DialTLSContext = nil + transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + lookupNetwork := "ip" + switch network { + case "tcp": + case "tcp4": + lookupNetwork = "ip4" + case "tcp6": + lookupNetwork = "ip6" + default: + return nil, xerrors.Errorf("network %q not permitted for MCP OAuth2 discovery", network) + } + host, port, err := net.SplitHostPort(addr) + if err != nil { + return nil, xerrors.Errorf("split host/port %q: %w", addr, err) + } + ips, err := net.DefaultResolver.LookupNetIP(ctx, lookupNetwork, host) + if err != nil { + return nil, xerrors.Errorf("resolve %q: %w", host, err) + } + if len(ips) == 0 { + return nil, xerrors.Errorf("no addresses for %q", host) + } + // Reject when ANY resolved address is blocked so a single + // tainted DNS answer short-circuits the dial rather than + // racing it. + for _, ip := range ips { + if isBlockedMCPDiscoveryAddr(ip, allowed) { + return nil, xerrors.Errorf( + "connection to %q blocked: %s is in a private/reserved IP range not permitted for MCP OAuth2 discovery", + host, ip.Unmap(), + ) + } + } + // Dial a validated IP directly. Dialing by hostname would + // re-resolve, letting a hostile resolver swap in a private + // IP after validation (DNS rebinding). TLS verification + // still uses the URL hostname via the transport's TLS + // config. + var dialer net.Dialer + var firstErr error + for _, ip := range ips { + conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ip.Unmap().String(), port)) + if dialErr == nil { + return conn, nil + } + if firstErr == nil { + firstErr = dialErr + } + } + return nil, firstErr + } + + return &http.Client{ + Timeout: timeout, + Transport: transport, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + // Mirror the default client's redirect cap. + if len(via) >= 10 { + return xerrors.New("stopped after 10 redirects") + } + if req.URL.Scheme != "http" && req.URL.Scheme != "https" { + return xerrors.Errorf("redirect to non-HTTP scheme %q blocked", req.URL.Scheme) + } + // Defense in depth: reject redirects to blocked IP + // literals before the request is attempted. Hostnames + // are validated post-resolution by the guarded dialer. + if ip, err := netip.ParseAddr(req.URL.Hostname()); err == nil && isBlockedMCPDiscoveryAddr(ip, allowed) { + return xerrors.Errorf( + "redirect to %q blocked: destination is in a private/reserved IP range not permitted for MCP OAuth2 discovery", + req.URL.Host, + ) + } + return nil + }, + } +} diff --git a/coderd/mcp_ssrf_internal_test.go b/coderd/mcp_ssrf_internal_test.go new file mode 100644 index 0000000000..3d1d6a0d6f --- /dev/null +++ b/coderd/mcp_ssrf_internal_test.go @@ -0,0 +1,274 @@ +package coderd + +import ( + "net" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/testutil" +) + +func TestIsBlockedMCPDiscoveryAddr(t *testing.T) { + t.Parallel() + + cases := []struct { + addr string + allowed []netip.Prefix + blocked bool + }{ + // Loopback. + {addr: "127.0.0.1", blocked: true}, + {addr: "127.0.0.2", blocked: true}, + {addr: "::1", blocked: true}, + // Private (RFC 1918 / ULA). + {addr: "10.0.0.1", blocked: true}, + {addr: "172.16.5.4", blocked: true}, + {addr: "192.168.1.1", blocked: true}, + {addr: "fd12:3456::1", blocked: true}, + // Link-local, incl. cloud metadata. + {addr: "169.254.169.254", blocked: true}, + {addr: "fe80::1", blocked: true}, + // Unspecified and multicast. + {addr: "0.0.0.0", blocked: true}, + {addr: "::", blocked: true}, + {addr: "224.0.0.1", blocked: true}, + {addr: "ff02::1", blocked: true}, + // Special-use ranges not covered by stdlib checks. + {addr: "100.64.0.1", blocked: true}, // Carrier-grade NAT. + {addr: "198.18.0.1", blocked: true}, // Benchmarking. + {addr: "0.1.2.3", blocked: true}, // "This network". + {addr: "100::1", blocked: true}, // Discard-only. + {addr: "2001:db8::1", blocked: true}, // Documentation. + {addr: "64:ff9b:1::1", blocked: true}, // NAT64 translation. + // IPv4-mapped IPv6 must not bypass IPv4 ranges. + {addr: "::ffff:169.254.169.254", blocked: true}, + {addr: "::ffff:127.0.0.1", blocked: true}, + // Public addresses are not blocked. + {addr: "8.8.8.8", blocked: false}, + {addr: "1.1.1.1", blocked: false}, + {addr: "2606:4700:4700::1111", blocked: false}, + // Allowlisted prefixes exempt their range only. + { + addr: "127.0.0.1", + allowed: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}, + blocked: false, + }, + { + addr: "127.0.0.2", + allowed: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}, + blocked: true, + }, + { + addr: "169.254.169.254", + allowed: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}, + blocked: true, + }, + } + for _, tc := range cases { + t.Run(tc.addr, func(t *testing.T) { + t.Parallel() + got := isBlockedMCPDiscoveryAddr(netip.MustParseAddr(tc.addr), tc.allowed) + require.Equal(t, tc.blocked, got) + }) + } +} + +// startCanaryServer binds an HTTP server to 127.0.0.2, standing in +// for an internal-only service that MCP OAuth2 discovery must never +// reach. Returns the server and a hit counter. +func startCanaryServer(t *testing.T) (*httptest.Server, *atomic.Int64) { + t.Helper() + + ln, err := net.Listen("tcp", "127.0.0.2:0") + if err != nil { + t.Skipf("cannot bind 127.0.0.2 (loopback aliasing unsupported?): %v", err) + } + var hits atomic.Int64 + canary := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"internal":"secret"}`)) + })) + _ = canary.Listener.Close() + canary.Listener = ln + canary.Start() + t.Cleanup(canary.Close) + return canary, &hits +} + +// allowOnly127001 allows exactly 127.0.0.1 so tests can reach their +// attacker-controlled httptest server while all other loopback and +// internal addresses stay blocked. +var allowOnly127001 = []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")} + +func TestMCPDiscoveryHTTPClientSSRF(t *testing.T) { + t.Parallel() + + // Regression for CDM-02-002: discovery must not reach an MCP + // server URL that points at a private/internal address. + t.Run("BlocksLoopbackDiscovery", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + var hits atomic.Int64 + mcpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(mcpServer.Close) + + client := newMCPDiscoveryHTTPClient(nil, nil) + _, err := discoverAndRegisterMCPOAuth2(ctx, client, mcpServer.URL+"/v1/mcp", "https://coder.example.com/callback") + require.Error(t, err) + require.Contains(t, err.Error(), "not permitted for MCP OAuth2 discovery") + require.EqualValues(t, 0, hits.Load(), "loopback MCP server must never be contacted") + }) + + // Regression for CDM-02-002: a hostname that resolves to an + // internal address must be blocked at dial time (DNS rebinding + // cannot bypass URL-level checks). + t.Run("BlocksHostnameResolvingToLoopback", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + var hits atomic.Int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + _, port, err := net.SplitHostPort(strings.TrimPrefix(srv.URL, "http://")) + require.NoError(t, err) + + client := newMCPDiscoveryHTTPClient(nil, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://localhost:"+port+"/", nil) + require.NoError(t, err) + resp, err := client.Do(req) + if err == nil { + _ = resp.Body.Close() + } + require.Error(t, err) + require.Contains(t, err.Error(), "not permitted for MCP OAuth2 discovery") + require.EqualValues(t, 0, hits.Load(), "server behind loopback-resolving hostname must never be contacted") + }) + + // Regression for CDM-02-002: an allowed MCP server that redirects + // discovery fetches to an internal loopback address must not + // cause that address to be contacted. Before the fix, the canary + // received the redirected requests. + t.Run("BlocksDiscoveryRedirectToLoopbackCanary", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + canary, canaryHits := startCanaryServer(t) + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, canary.URL+r.URL.Path, http.StatusFound) + })) + t.Cleanup(attacker.Close) + + client := newMCPDiscoveryHTTPClient(nil, allowOnly127001) + _, err := discoverAndRegisterMCPOAuth2(ctx, client, attacker.URL+"/v1/mcp", "https://coder.example.com/callback") + require.Error(t, err) + require.Contains(t, err.Error(), "blocked") + require.EqualValues(t, 0, canaryHits.Load(), "internal canary must never be contacted via redirect") + }) + + // Regression for CDM-02-002: same as above for the Dynamic Client + // Registration POST (RFC 7591), which the finding called out + // explicitly. + t.Run("BlocksRegistrationRedirectToLoopbackCanary", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + canary, canaryHits := startCanaryServer(t) + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := "http://" + r.Host + switch r.URL.Path { + case "/.well-known/oauth-protected-resource": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"resource": "` + origin + `", "authorization_servers": ["` + origin + `"]}`)) + case "/.well-known/oauth-authorization-server": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "issuer": "` + origin + `", + "authorization_endpoint": "` + origin + `/authorize", + "token_endpoint": "` + origin + `/token", + "registration_endpoint": "` + origin + `/register" + }`)) + case "/register": + // Redirect the DCR POST to the internal canary. + http.Redirect(w, r, canary.URL+"/register", http.StatusTemporaryRedirect) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(attacker.Close) + + client := newMCPDiscoveryHTTPClient(nil, allowOnly127001) + _, err := discoverAndRegisterMCPOAuth2(ctx, client, attacker.URL, "https://coder.example.com/callback") + require.Error(t, err) + require.Contains(t, err.Error(), "blocked") + require.EqualValues(t, 0, canaryHits.Load(), "internal canary must never receive the registration POST") + }) + + // Regression for CDM-02-002: redirects to the cloud metadata IP + // are rejected before any connection is attempted. + t.Run("BlocksRedirectToMetadataIP", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound) + })) + t.Cleanup(attacker.Close) + + client := newMCPDiscoveryHTTPClient(nil, allowOnly127001) + _, err := discoverAndRegisterMCPOAuth2(ctx, client, attacker.URL+"/v1/mcp", "https://coder.example.com/callback") + require.Error(t, err) + require.Contains(t, err.Error(), "blocked") + }) + + // Allowlisted ranges (used by tests and coderdtest) still permit + // the full discovery + registration flow. + t.Run("AllowlistedDiscoverySucceeds", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := "http://" + r.Host + switch r.URL.Path { + case "/.well-known/oauth-protected-resource": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"resource": "` + origin + `", "authorization_servers": ["` + origin + `"]}`)) + case "/.well-known/oauth-authorization-server": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "issuer": "` + origin + `", + "authorization_endpoint": "` + origin + `/authorize", + "token_endpoint": "` + origin + `/token", + "registration_endpoint": "` + origin + `/register" + }`)) + case "/register": + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"client_id": "test-client", "client_secret": "test-secret"}`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + + client := newMCPDiscoveryHTTPClient(nil, allowOnly127001) + result, err := discoverAndRegisterMCPOAuth2(ctx, client, server.URL, "https://coder.example.com/callback") + require.NoError(t, err) + require.Equal(t, "test-client", result.clientID) + require.Equal(t, server.URL+"/authorize", result.authURL) + require.Equal(t, server.URL+"/token", result.tokenURL) + }) +} diff --git a/coderd/mcp_test.go b/coderd/mcp_test.go index 7445ce4e3d..064dcdba83 100644 --- a/coderd/mcp_test.go +++ b/coderd/mcp_test.go @@ -5,8 +5,10 @@ import ( "database/sql" "encoding/base64" "encoding/json" + "net" "net/http" "net/http/httptest" + "net/netip" "net/url" "strings" "sync" @@ -1510,6 +1512,77 @@ func TestMCPServerConfigsOAuth2AutoDiscovery(t *testing.T) { }) } +// TestMCPServerConfigsOAuth2AutoDiscoverySSRF is a regression test for +// CDM-02-002: OAuth2 auto-discovery followed attacker-controlled +// redirects to internal addresses. The canary on 127.0.0.2 stands in +// for an internal-only service (e.g. cloud metadata) and must never +// be reached, while the attacker's MCP server on 127.0.0.1 is +// reachable via the test allowlist. +func TestMCPServerConfigsOAuth2AutoDiscoverySSRF(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + canaryLn, err := net.Listen("tcp", "127.0.0.2:0") + if err != nil { + t.Skipf("cannot bind 127.0.0.2 (loopback aliasing unsupported?): %v", err) + } + var canaryHits atomic.Int64 + canary := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + canaryHits.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"internal":"secret"}`)) + })) + _ = canary.Listener.Close() + canary.Listener = canaryLn + canary.Start() + t.Cleanup(canary.Close) + + // Attacker-controlled MCP server: redirects every discovery + // fetch to the internal canary. + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, canary.URL+r.URL.Path, http.StatusFound) + })) + t.Cleanup(attacker.Close) + + providerKeys := coderdtest.FakeOpenAICompatProviderAPIKeys(t) + client := coderdtest.New(t, &coderdtest.Options{ + DeploymentValues: mcpDeploymentValues(t), + ChatProviderAPIKeys: &providerKeys, + // Allow only the attacker's address so the initial fetch + // succeeds; the canary's address stays blocked. + MCPOAuth2DiscoveryAllowedIPRanges: []netip.Prefix{ + netip.MustParsePrefix("127.0.0.1/32"), + }, + }) + _ = coderdtest.CreateFirstUser(t, client) + + _, err = client.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "SSRF Attacker", + Slug: "ssrf-attacker", + Transport: "streamable_http", + URL: attacker.URL + "/v1/mcp", + AuthType: "oauth2", + Availability: "default_on", + Enabled: true, + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }) + require.Error(t, err) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + require.Contains(t, sdkErr.Message, "auto-discovery failed") + require.EqualValues(t, 0, canaryHits.Load(), "internal canary must never be contacted via attacker redirect") + + // The partially created config must have been cleaned up. + configs, err := client.MCPServerConfigs(ctx) + require.NoError(t, err) + for _, config := range configs { + require.NotEqual(t, "ssrf-attacker", config.Slug) + } +} + // nolint:bodyclose func TestMCPServerOAuth2PKCE(t *testing.T) { t.Parallel()