fix: pin workspace agent API client to intended agent (#26600) (#26611)

Backport of #26600 to `release/2.34`.

Original PR: #26600 - fix: pin workspace agent API client to intended
agent
Merge commit: eeb2624549
Requested by: @ethanndickson

## What this fixes

The workspace agent API client followed HTTP redirects and trusted the
redirected host, letting a malicious agent bounce a coderd request onto
a different agent's unauthenticated port-4 API (cross-tenant file
read/write and RCE, Cure53 CODAGT-668). `apiClient` now refuses
redirects and pins every dial to the intended agent address, and the
task-app / scaletest clients share `AppHTTPClient`, which blocks
redirects too.

## Conflict resolution

The automatic cherry-pick conflicted because #26600 was built on a
separate request-context refactor of `apiClient` that is not present on
this release branch. The redirect block and agent-address pinning are
applied to the existing `apiClient()` here, and the
request-context-bounded dial test (which depends on that refactor) is
omitted. This branch had no `agentconn_test.go`, so the redirect tests
are added as a new `agentconn_test.go`. The two redirect regression
tests added by #26600 are included.

Co-authored-by: Ethan Dickson <ethan@coder.com>

coderd/aitasks.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"net"
 	"net/http"
 	"net/url"
 	"slices"
@@ -1086,13 +1085,7 @@ func (api *API) authAndDoWithTaskAppClient(
 	}
 	defer release()
 
-	client := &http.Client{
-		Transport: &http.Transport{
-			DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
-				return agentConn.DialContext(ctx, network, addr)
-			},
-		},
-	}
+	client := agentConn.AppHTTPClient()
 	return do(ctx, client, parsedURL)
 }
 

coderd/tailnet.go

@@ -298,6 +298,7 @@ func (s *ServerTailnet) AgentConn(ctx context.Context, agentID uuid.UUID) (works
 	conn = workspacesdk.NewAgentConn(s.conn, workspacesdk.AgentConnOptions{
 		AgentID:   agentID,
 		CloseFunc: func() error { return workspacesdk.ErrSkipClose },
+		Logger:    s.logger,
 	})
 
 	// Since we now have an open conn, be careful to close it if we error

codersdk/workspacesdk/agentconn.go

@@ -102,6 +102,7 @@ type AgentConn interface {
 	DebugMagicsock(ctx context.Context) ([]byte, error)
 	DebugManifest(ctx context.Context) ([]byte, error)
 	DialContext(ctx context.Context, network string, addr string) (net.Conn, error)
+	AppHTTPClient() *http.Client
 	GetPeerDiagnostics() tailnet.PeerDiagnostics
 	ListContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error)
 	ListMCPTools(ctx context.Context) (ListMCPToolsResponse, error)
@@ -158,6 +159,7 @@ func (c *agentConn) SetExtraHeaders(h http.Header) {
 type AgentConnOptions struct {
 	AgentID   uuid.UUID
 	CloseFunc func() error
+	Logger    slog.Logger
 }
 
 func (c *agentConn) agentAddress() netip.Addr {
@@ -370,6 +372,24 @@ func (c *agentConn) DialContext(ctx context.Context, network string, addr string
 	}
 }
 
+// AppHTTPClient returns an HTTP client for reaching HTTP apps served by this
+// workspace agent. Redirects are blocked to prevent misuse.
+func (c *agentConn) AppHTTPClient() *http.Client {
+	return &http.Client{
+		CheckRedirect: func(*http.Request, []*http.Request) error {
+			return http.ErrUseLastResponse
+		},
+		Transport: &http.Transport{
+			// Disable keep-alives so these short-lived clients don't leave
+			// idle connections (and their goroutines) lingering after they're
+			// discarded.
+			DisableKeepAlives: true,
+			// Host locked to agent, port from URL.
+			DialContext: c.DialContext,
+		},
+	}
+}
+
 // ListeningPorts lists the ports that are currently in use by the workspace.
 func (c *agentConn) ListeningPorts(ctx context.Context) (codersdk.WorkspaceAgentListeningPortsResponse, error) {
 	ctx, span := tracing.StartSpan(ctx)
@@ -1358,7 +1378,12 @@ func (c *agentConn) apiRequest(ctx context.Context, method, path string, body in
 // apiClient returns an HTTP client that can be used to make
 // requests to the workspace agent's HTTP API server.
 func (c *agentConn) apiClient() *http.Client {
+	agentAddr := netip.AddrPortFrom(c.agentAddress(), AgentHTTPAPIServerPort)
 	return &http.Client{
+		// Redirects are blocked to prevent misuse.
+		CheckRedirect: func(*http.Request, []*http.Request) error {
+			return http.ErrUseLastResponse
+		},
 		Transport: &http.Transport{
 			// Disable keep alives as we're usually only making a single
 			// request, and this triggers goleak in tests
@@ -1378,16 +1403,21 @@ func (c *agentConn) apiClient() *http.Client {
 					return nil, xerrors.Errorf("request %q does not appear to be for http api", addr)
 				}
 
+				if reqAddr, err := netip.ParseAddr(host); err != nil || reqAddr != agentAddr.Addr() {
+					c.opts.Logger.Warn(ctx, "blocked workspace agent API request to unintended host",
+						slog.F("agent_id", c.opts.AgentID),
+						slog.F("request_host", host),
+						slog.F("intended_agent_addr", agentAddr.Addr()),
+					)
+					return nil, xerrors.Errorf("request host %q does not match intended agent %q", host, agentAddr.Addr())
+				}
+
 				if !c.AwaitReachable(ctx) {
 					return nil, xerrors.Errorf("workspace agent not reachable in time: %v", ctx.Err())
 				}
 
-				ipAddr, err := netip.ParseAddr(host)
-				if err != nil {
-					return nil, xerrors.Errorf("parse host addr: %w", err)
-				}
-
-				conn, err := c.Conn.DialContextTCP(ctx, netip.AddrPortFrom(ipAddr, AgentHTTPAPIServerPort))
+				// Always dial the pinned agent address, never the request host.
+				conn, err := c.Conn.DialContextTCP(ctx, agentAddr)
 				if err != nil {
 					return nil, xerrors.Errorf("dial http api: %w", err)
 				}

codersdk/workspacesdk/agentconn_test.go

@@ -0,0 +1,212 @@
+package workspacesdk_test
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net"
+	"net/http"
+	"net/netip"
+	"strings"
+	"sync/atomic"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
+
+	"github.com/coder/coder/v2/codersdk/workspacesdk"
+	"github.com/coder/coder/v2/tailnet"
+	"github.com/coder/coder/v2/tailnet/proto"
+	"github.com/coder/coder/v2/tailnet/tailnettest"
+	"github.com/coder/coder/v2/testutil"
+)
+
+func TestAgentConnRejectsCrossAgentRedirects(t *testing.T) {
+	t.Parallel()
+
+	derpMap, _ := tailnettest.RunDERPAndSTUN(t)
+	cases := []struct {
+		name   string
+		status int
+		invoke func(context.Context, workspacesdk.AgentConn) error
+	}{
+		{
+			name:   "get 302",
+			status: http.StatusFound,
+			invoke: func(ctx context.Context, conn workspacesdk.AgentConn) error {
+				_, err := conn.ListeningPorts(ctx)
+				return err
+			},
+		},
+		{
+			name:   "post 307",
+			status: http.StatusTemporaryRedirect,
+			invoke: func(ctx context.Context, conn workspacesdk.AgentConn) error {
+				return conn.WriteFile(ctx, "/tmp/attacker", strings.NewReader("redirect-body"))
+			},
+		},
+		{
+			name:   "post 308",
+			status: http.StatusPermanentRedirect,
+			invoke: func(ctx context.Context, conn workspacesdk.AgentConn) error {
+				return conn.WriteFile(ctx, "/tmp/attacker", strings.NewReader("redirect-body"))
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx := testutil.Context(t, testutil.WaitMedium)
+
+			clientID := uuid.New()
+			attackerID := uuid.New()
+			victimID := uuid.New()
+			clientConn, _ := newTailnetConn(t, derpMap, clientID, "client")
+			attackerConn, attackerIP := newTailnetConn(t, derpMap, attackerID, "attacker")
+			victimConn, victimIP := newTailnetConn(t, derpMap, victimID, "victim")
+			stitchTailnet(t, map[uuid.UUID]*tailnet.Conn{
+				clientID:   clientConn,
+				attackerID: attackerConn,
+				victimID:   victimConn,
+			})
+
+			var victimHit atomic.Bool
+			victimRouter := http.NewServeMux()
+			victimRouter.HandleFunc("/api/v0/listening-ports", func(rw http.ResponseWriter, _ *http.Request) {
+				victimHit.Store(true)
+				rw.Header().Set("Content-Type", "application/json")
+				_, _ = rw.Write([]byte(`{"ports":[]}`))
+			})
+			victimRouter.HandleFunc("/api/v0/write-file", func(rw http.ResponseWriter, _ *http.Request) {
+				victimHit.Store(true)
+				rw.WriteHeader(http.StatusOK)
+			})
+			serveTailnetHTTP(t, victimConn, victimRouter)
+
+			victimBaseURL := fmt.Sprintf("http://[%s]:%d", victimIP, workspacesdk.AgentHTTPAPIServerPort)
+			attackerRouter := http.NewServeMux()
+			attackerRouter.HandleFunc("/", func(rw http.ResponseWriter, r *http.Request) {
+				http.Redirect(rw, r, victimBaseURL+r.URL.RequestURI(), tc.status)
+			})
+			serveTailnetHTTP(t, attackerConn, attackerRouter)
+
+			require.True(t, clientConn.AwaitReachable(ctx, attackerIP))
+			require.True(t, clientConn.AwaitReachable(ctx, victimIP))
+
+			conn := workspacesdk.NewAgentConn(clientConn, workspacesdk.AgentConnOptions{
+				AgentID: attackerID,
+			})
+
+			err := tc.invoke(ctx, conn)
+			require.Error(t, err)
+			require.False(t, victimHit.Load())
+		})
+	}
+}
+
+// TestAgentConnAppHTTPClientRefusesRedirects verifies the app HTTP client does
+// not follow redirects.
+func TestAgentConnAppHTTPClientRefusesRedirects(t *testing.T) {
+	t.Parallel()
+
+	tailnetConn, err := tailnet.NewConn(&tailnet.Options{
+		Addresses: []netip.Prefix{tailnet.TailscaleServicePrefix.RandomPrefix()},
+		Logger:    testutil.Logger(t),
+	})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = tailnetConn.Close()
+	})
+
+	conn := workspacesdk.NewAgentConn(tailnetConn, workspacesdk.AgentConnOptions{
+		AgentID: uuid.New(),
+	})
+
+	client := conn.AppHTTPClient()
+	require.NotNil(t, client.CheckRedirect)
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://example.invalid/", nil)
+	require.NoError(t, err)
+	require.ErrorIs(t, client.CheckRedirect(req, nil), http.ErrUseLastResponse)
+}
+
+func newTailnetConn(t *testing.T, derpMap *tailcfg.DERPMap, id uuid.UUID, name string) (*tailnet.Conn, netip.Addr) {
+	t.Helper()
+
+	addr := tailnet.TailscaleServicePrefix.AddrFromUUID(id)
+	conn, err := tailnet.NewConn(&tailnet.Options{
+		ID:        id,
+		Addresses: []netip.Prefix{netip.PrefixFrom(addr, 128)},
+		Logger:    testutil.Logger(t).Named(name),
+		DERPMap:   derpMap,
+	})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		assert.NoError(t, conn.Close())
+	})
+
+	return conn, addr
+}
+
+func serveTailnetHTTP(t *testing.T, conn *tailnet.Conn, handler http.Handler) {
+	t.Helper()
+
+	ln, err := conn.Listen("tcp", fmt.Sprintf(":%d", workspacesdk.AgentHTTPAPIServerPort))
+	require.NoError(t, err)
+
+	server := &http.Server{Handler: handler, ReadHeaderTimeout: testutil.WaitShort}
+	t.Cleanup(func() {
+		assert.NoError(t, server.Close())
+		assert.NoError(t, ln.Close())
+	})
+
+	go func() {
+		err := server.Serve(ln)
+		if err != nil && !errors.Is(err, net.ErrClosed) && !errors.Is(err, http.ErrServerClosed) {
+			assert.NoError(t, err)
+		}
+	}()
+}
+
+// stitchTailnet cross-programs every conn's node into every other conn, the
+// N-peer analog of tailnet's stitch test helper, so the peers can reach each
+// other without a coordinator.
+func stitchTailnet(t *testing.T, conns map[uuid.UUID]*tailnet.Conn) {
+	t.Helper()
+
+	sendNode := func(srcID uuid.UUID, node *tailnet.Node) {
+		protoNode, err := tailnet.NodeToProto(node)
+		if !assert.NoError(t, err) {
+			return
+		}
+		for dstID, dst := range conns {
+			if dstID == srcID {
+				continue
+			}
+			err = dst.UpdatePeers([]*proto.CoordinateResponse_PeerUpdate{{
+				Id:   srcID[:],
+				Node: protoNode,
+				Kind: proto.CoordinateResponse_PeerUpdate_NODE,
+			}})
+			assert.NoError(t, err)
+		}
+	}
+
+	for srcID, src := range conns {
+		src.SetNodeCallback(func(node *tailnet.Node) {
+			sendNode(srcID, node)
+		})
+		if node := src.Node(); node != nil {
+			sendNode(srcID, node)
+		}
+	}
+
+	t.Cleanup(func() {
+		for _, conn := range conns {
+			conn.SetNodeCallback(nil)
+		}
+	})
+}

codersdk/workspacesdk/agentconnmock/agentconnmock.go

@@ -56,6 +56,20 @@ func (m *MockAgentConn) EXPECT() *MockAgentConnMockRecorder {
 	return m.recorder
 }
 
+// AppHTTPClient mocks base method.
+func (m *MockAgentConn) AppHTTPClient() *http.Client {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "AppHTTPClient")
+	ret0, _ := ret[0].(*http.Client)
+	return ret0
+}
+
+// AppHTTPClient indicates an expected call of AppHTTPClient.
+func (mr *MockAgentConnMockRecorder) AppHTTPClient() *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AppHTTPClient", reflect.TypeOf((*MockAgentConn)(nil).AppHTTPClient))
+}
+
 // AwaitReachable mocks base method.
 func (m *MockAgentConn) AwaitReachable(ctx context.Context) bool {
 	m.ctrl.T.Helper()

codersdk/workspacesdk/workspacesdk.go

@@ -300,6 +300,7 @@ func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *
 			<-controller.Closed()
 			return conn.Close()
 		},
+		Logger: options.Logger,
 	})
 
 	if !agentConn.AwaitReachable(dialCtx) {

scaletest/agentconn/run.go

@@ -214,7 +214,7 @@ func verifyConnection(ctx context.Context, logs io.Writer, conn workspacesdk.Age
 	ctx, span := tracing.StartSpan(ctx)
 	defer span.End()
 
-	client := agentHTTPClient(conn)
+	client := conn.AppHTTPClient()
 	for i := 0; i < verifyConnectionAttempts; i++ {
 		_, _ = fmt.Fprintf(logs, "\tVerify connection attempt %d/%d...\n", i+1, verifyConnectionAttempts)
 		verifyCtx, cancel := context.WithTimeout(ctx, defaultRequestTimeout)
@@ -258,7 +258,7 @@ func performInitialConnections(ctx context.Context, logs io.Writer, conn workspa
 	defer span.End()
 
 	_, _ = fmt.Fprintln(logs, "Performing initial service connections...")
-	client := agentHTTPClient(conn)
+	client := conn.AppHTTPClient()
 	for i, connSpec := range specs {
 		_, _ = fmt.Fprintf(logs, "\t%d. %s\n", i, connSpec.URL)
 
@@ -292,7 +292,7 @@ func holdConnection(ctx context.Context, logs io.Writer, conn workspacesdk.Agent
 	defer span.End()
 
 	eg, egCtx := errgroup.WithContext(ctx)
-	client := agentHTTPClient(conn)
+	client := conn.AppHTTPClient()
 	if len(specs) > 0 {
 		_, _ = fmt.Fprintln(logs, "\nStarting connection loops...")
 	}
@@ -362,26 +362,3 @@ func holdConnection(ctx context.Context, logs io.Writer, conn workspacesdk.Agent
 
 	return nil
 }
-
-func agentHTTPClient(conn workspacesdk.AgentConn) *http.Client {
-	return &http.Client{
-		Transport: &http.Transport{
-			DisableKeepAlives: true,
-			DialContext: func(ctx context.Context, _ string, addr string) (net.Conn, error) {
-				_, port, err := net.SplitHostPort(addr)
-				if err != nil {
-					return nil, xerrors.Errorf("split host port %q: %w", addr, err)
-				}
-
-				portUint, err := strconv.ParseUint(port, 10, 16)
-				if err != nil {
-					return nil, xerrors.Errorf("parse port %q: %w", port, err)
-				}
-
-				// Addr doesn't matter here, besides the port. DialContext will
-				// automatically choose the right IP to dial.
-				return conn.DialContext(ctx, "tcp", fmt.Sprintf("127.0.0.1:%d", portUint))
-			},
-		},
-	}
-}