mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd): block SSRF in MCP OAuth2 discovery and client registration (#27989)
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
+6
-4
@@ -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.
|
||||
|
||||
@@ -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
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user