From eeb2624549ddb85538e493af3e678fdb185a809f Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Tue, 23 Jun 2026 23:06:44 +1000 Subject: [PATCH] fix: pin workspace agent API client to intended agent (#26600) ## Summary The control-plane HTTP client used to talk to workspace agents followed HTTP redirects and trusted the redirected host, letting a malicious workspace agent bounce a coderd request onto a different agent on the shared tailnet. Because the agent HTTP API on port 4 is unauthenticated (it relies on tailnet reachability plus control-plane authorization), this allowed cross-tenant file read/write and remote code execution. This PR refuses redirects and pins every dial to the intended agent. Closes CODAGT-668. ## Problem `agentConn.apiClient` in `codersdk/workspacesdk/agentconn.go` constructed an `http.Client` with no `CheckRedirect`, so Go's default policy followed up to 10 redirects. Its custom `Transport.DialContext` parsed the host from the (post-redirect) request URL and dialed that IP over the shared tailnet, validating only that the port was `AgentHTTPAPIServerPort` (4). It never pinned the connection to the intended `AgentID` / `agentAddress()`. A workspace owner (any regular org member, not just admins) controls their own agent and can make its port-4 handler return a `3xx` `Location` pointing at a victim agent's tailnet IP. When a control-plane action (for example a chat tool or the HTTP MCP server) sends an agent API request to the attacker's agent, coderd acts as a confused deputy and replays the request against the victim: - `301/302/303` rewrite POST to GET, but `307/308` preserve method and body when the body is replayable. The real callers pass replayable bodies, so a redirected `POST /api/v0/write-file` writes attacker-controlled content into the victim workspace and a redirected `POST /api/v0/processes/start` executes it, giving RCE on the victim agent. The dangerous callers run server-side on coderd's single deployment-wide `ServerTailnet`, which is authorized to tunnel to any agent, so the blast radius is cross-tenant / cross-organization (limited in practice to victim agents coderd currently has a live tunnel to). ## Fix In `agentConn.apiClient`: - Set `CheckRedirect: http.ErrUseLastResponse` so the client never follows a redirect. A `3xx` is surfaced to the caller as the response (which the existing `ReadBodyAsError` path turns into an error) instead of being replayed against another host. - Capture the intended agent address once from `AgentID` (`agentAddr := netip.AddrPortFrom(c.agentAddress(), AgentHTTPAPIServerPort)`), reject any dial whose host or port does not match it, and always dial that pinned address rather than the URL-derived host. In `coderd/aitasks.go`, the task app proxy client (`taskAppHTTPClient`) also now sets `CheckRedirect: http.ErrUseLastResponse`. This client dials through `agentConn.DialContext`, which already pins the host to the originating workspace's agent (it takes only the port from the dial address), so it was never cross-agent. The change is hardening for parity so a malicious app cannot bounce the request to a different port on the same agent. ## Hardening and defense in depth The two layers are independent. `CheckRedirect` removes the redirect-following behavior entirely, and the dial pinning guarantees that even a request constructed with a foreign host can only ever reach the intended agent. Removing either one in the future cannot, on its own, reintroduce the cross-agent vector. ## Tests - `codersdk/workspacesdk/agentconn_redirect_test.go` builds a three-peer tailnet (client, attacker, victim). The attacker agent redirects to the victim's port-4 URL, and the test asserts that `GET` `302`, `POST` `307`, and `POST` `308` all return an error and that the victim is never contacted. - `coderd/aitasks_internal_test.go` adds `TestTaskAppHTTPClient_RejectsRedirect`, which verifies the task app client surfaces a `307` instead of following it to a stand-in victim. ## Why this closes the whole vulnerability class `apiClient` is the only HTTP chokepoint to the agent port-4 API, so fixing it covers every server-side caller: - Every agent HTTP API method in `agentConn` funnels through `apiClient`, either via `apiRequest`, a direct `apiClient(ctx).Do(...)` (`ExecuteDesktopAction`), or as the websocket `HTTPClient` (`WatchContainers`, `WatchGit`, `ConnectDesktopVNC`). The websocket handshake matters here: `coder/websocket` follows `3xx` during the handshake by default and only requires `101` on the final hop, but it honors the underlying client's `CheckRedirect`, so reusing `apiClient` closes the websocket paths too. - The HTTP MCP server coderd hosts at `/api/experimental/mcp/http` registers tools (`coder_workspace_bash`, `_write_file`, `_read_file`, `_edit_files`, etc.) that reach the agent through `workspacesdk.AgentConn` methods, so they go through `apiClient` and are covered. The same is true for agent-hosted MCP, which coderd reaches only via `agentConn.CallMCPTool` / `ListMCPTools`. coderd never opens an MCP client connection directly to an agent over the tailnet. - Raw-TCP agent services (reconnecting PTY, SSH, speedtest, generic `DialContext`) speak non-HTTP protocols and have no redirect surface. The workspace apps reverse proxy targets user app ports, not port 4, forwards `3xx` to the browser rather than following them, and pins its transport to the request's agent. - `provisionerd` does not talk to the agent HTTP API at all. No other server-side client follows redirects to an agent-controllable tailnet host, so no further redirect changes are required for this class. --- coderd/aitasks.go | 9 +- coderd/tailnet.go | 1 + codersdk/workspacesdk/agentconn.go | 43 +++- codersdk/workspacesdk/agentconn_test.go | 198 ++++++++++++++++++ .../agentconnmock/agentconnmock.go | 14 ++ codersdk/workspacesdk/workspacesdk.go | 1 + scaletest/agentconn/run.go | 29 +-- 7 files changed, 253 insertions(+), 42 deletions(-) diff --git a/coderd/aitasks.go b/coderd/aitasks.go index 7518a98d33..3f88e8ede2 100644 --- a/coderd/aitasks.go +++ b/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) } diff --git a/coderd/tailnet.go b/coderd/tailnet.go index 4d73c89fd1..ec061f015b 100644 --- a/coderd/tailnet.go +++ b/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 diff --git a/codersdk/workspacesdk/agentconn.go b/codersdk/workspacesdk/agentconn.go index 934ae051c8..6869f42903 100644 --- a/codersdk/workspacesdk/agentconn.go +++ b/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) ListProcesses(ctx context.Context) (ListProcessesResponse, error) @@ -157,6 +158,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 { @@ -369,6 +371,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) @@ -1362,7 +1382,12 @@ func (c *agentConn) apiRequest(ctx context.Context, method, path string, body in // scoped to a single request: its transport cancels in-flight dials // once reqCtx ends. func (c *agentConn) apiClient(reqCtx context.Context) *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 @@ -1376,11 +1401,17 @@ func (c *agentConn) apiClient(reqCtx context.Context) *http.Client { if err != nil { return nil, xerrors.Errorf("split host port %q: %w", addr, err) } - - // Verify that the port is TailnetStatisticsPort. if port != strconv.Itoa(AgentHTTPAPIServerPort) { 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()) + } // http.Transport detaches ctx from the request context so // a pending dial can outlive its request and serve future @@ -1398,12 +1429,8 @@ func (c *agentConn) apiClient(reqCtx context.Context) *http.Client { 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) } diff --git a/codersdk/workspacesdk/agentconn_test.go b/codersdk/workspacesdk/agentconn_test.go index 9a5e3a93bb..c77cbae3ce 100644 --- a/codersdk/workspacesdk/agentconn_test.go +++ b/codersdk/workspacesdk/agentconn_test.go @@ -2,15 +2,25 @@ 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" "go.uber.org/goleak" + "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" ) @@ -64,3 +74,191 @@ func TestAgentConn_DialBoundedByRequestContext(t *testing.T) { goleak.VerifyNone(t, ignoreCurrent) } + +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) + } + }) +} diff --git a/codersdk/workspacesdk/agentconnmock/agentconnmock.go b/codersdk/workspacesdk/agentconnmock/agentconnmock.go index bfde170d01..524fc6a38a 100644 --- a/codersdk/workspacesdk/agentconnmock/agentconnmock.go +++ b/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() diff --git a/codersdk/workspacesdk/workspacesdk.go b/codersdk/workspacesdk/workspacesdk.go index 67eab8b4bc..f0311c7973 100644 --- a/codersdk/workspacesdk/workspacesdk.go +++ b/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) { diff --git a/scaletest/agentconn/run.go b/scaletest/agentconn/run.go index 4a4587e478..f26db4355a 100644 --- a/scaletest/agentconn/run.go +++ b/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)) - }, - }, - } -}