Skip to content

Commit f8bdec5

Browse files
ethanndicksonethanndickson
authored
fix: pin workspace agent API client to intended agent (#26600) (#26622)
Backport of #26600 to `release/2.29`. Original PR: #26600 - fix: pin workspace agent API client to intended agent Merge commit: eeb2624 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 This 2.29 ESR backport was resolved using the existing `release/2.32` backport (#26612) as a reference; that resolution applies cleanly here. #26600 was built on a separate request-context refactor of `apiClient` that is not present on this release branch, so the redirect block and agent-address pinning are applied to the existing `apiClient()`, and the request-context-bounded dial test (which depends on that refactor) is omitted. This branch had no `agentconn_test.go`, so the redirect regression tests are added as a new `agentconn_test.go`. The generated agent-conn mock additionally required its `net/http` import for the new `AppHTTPClient` method.
1 parent b76aed9 commit f8bdec5

7 files changed

Lines changed: 269 additions & 40 deletions

File tree

‎coderd/aitasks.go‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package coderd
33
import (
44
"context"
55
"fmt"
6-
"net"
76
"net/http"
87
"net/url"
98
"slices"
@@ -945,12 +944,6 @@ func (api *API) authAndDoWithTaskAppClient(
945944
}
946945
defer release()
947946

948-
client := &http.Client{
949-
Transport: &http.Transport{
950-
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
951-
return agentConn.DialContext(ctx, network, addr)
952-
},
953-
},
954-
}
947+
client := agentConn.AppHTTPClient()
955948
return do(ctx, client, parsedURL)
956949
}

‎coderd/tailnet.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,7 @@ func (s *ServerTailnet) AgentConn(ctx context.Context, agentID uuid.UUID) (works
293293
conn = workspacesdk.NewAgentConn(s.conn, workspacesdk.AgentConnOptions{
294294
AgentID: agentID,
295295
CloseFunc: func() error { return workspacesdk.ErrSkipClose },
296+
Logger: s.logger,
296297
})
297298

298299
// Since we now have an open conn, be careful to close it if we error

‎codersdk/workspacesdk/agentconn.go‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ type AgentConn interface {
5353
DebugMagicsock(ctx context.Context) ([]byte, error)
5454
DebugManifest(ctx context.Context) ([]byte, error)
5555
DialContext(ctx context.Context, network string, addr string) (net.Conn, error)
56+
AppHTTPClient() *http.Client
5657
GetPeerDiagnostics() tailnet.PeerDiagnostics
5758
ListContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error)
5859
ListeningPorts(ctx context.Context) (codersdk.WorkspaceAgentListeningPortsResponse, error)
@@ -88,6 +89,7 @@ func (c *agentConn) TailnetConn() *tailnet.Conn {
8889
type AgentConnOptions struct {
8990
AgentID uuid.UUID
9091
CloseFunc func() error
92+
Logger slog.Logger
9193
}
9294

9395
func (c *agentConn) agentAddress() netip.Addr {
@@ -300,6 +302,24 @@ func (c *agentConn) DialContext(ctx context.Context, network string, addr string
300302
}
301303
}
302304

305+
// AppHTTPClient returns an HTTP client for reaching HTTP apps served by this
306+
// workspace agent. Redirects are blocked to prevent misuse.
307+
func (c *agentConn) AppHTTPClient() *http.Client {
308+
return &http.Client{
309+
CheckRedirect: func(*http.Request, []*http.Request) error {
310+
return http.ErrUseLastResponse
311+
},
312+
Transport: &http.Transport{
313+
// Disable keep-alives so these short-lived clients don't leave
314+
// idle connections (and their goroutines) lingering after they're
315+
// discarded.
316+
DisableKeepAlives: true,
317+
// Host locked to agent, port from URL.
318+
DialContext: c.DialContext,
319+
},
320+
}
321+
}
322+
303323
// ListeningPorts lists the ports that are currently in use by the workspace.
304324
func (c *agentConn) ListeningPorts(ctx context.Context) (codersdk.WorkspaceAgentListeningPortsResponse, error) {
305325
ctx, span := tracing.StartSpan(ctx)
@@ -654,7 +674,12 @@ func (c *agentConn) apiRequest(ctx context.Context, method, path string, body in
654674
// apiClient returns an HTTP client that can be used to make
655675
// requests to the workspace agent's HTTP API server.
656676
func (c *agentConn) apiClient() *http.Client {
677+
agentAddr := netip.AddrPortFrom(c.agentAddress(), AgentHTTPAPIServerPort)
657678
return &http.Client{
679+
// Redirects are blocked to prevent misuse.
680+
CheckRedirect: func(*http.Request, []*http.Request) error {
681+
return http.ErrUseLastResponse
682+
},
658683
Transport: &http.Transport{
659684
// Disable keep alives as we're usually only making a single
660685
// request, and this triggers goleak in tests
@@ -674,16 +699,21 @@ func (c *agentConn) apiClient() *http.Client {
674699
return nil, xerrors.Errorf("request %q does not appear to be for http api", addr)
675700
}
676701

677-
if !c.AwaitReachable(ctx) {
678-
return nil, xerrors.Errorf("workspace agent not reachable in time: %v", ctx.Err())
702+
if reqAddr, err := netip.ParseAddr(host); err != nil || reqAddr != agentAddr.Addr() {
703+
c.opts.Logger.Warn(ctx, "blocked workspace agent API request to unintended host",
704+
slog.F("agent_id", c.opts.AgentID),
705+
slog.F("request_host", host),
706+
slog.F("intended_agent_addr", agentAddr.Addr()),
707+
)
708+
return nil, xerrors.Errorf("request host %q does not match intended agent %q", host, agentAddr.Addr())
679709
}
680710

681-
ipAddr, err := netip.ParseAddr(host)
682-
if err != nil {
683-
return nil, xerrors.Errorf("parse host addr: %w", err)
711+
if !c.AwaitReachable(ctx) {
712+
return nil, xerrors.Errorf("workspace agent not reachable in time: %v", ctx.Err())
684713
}
685714

686-
conn, err := c.Conn.DialContextTCP(ctx, netip.AddrPortFrom(ipAddr, AgentHTTPAPIServerPort))
715+
// Always dial the pinned agent address, never the request host.
716+
conn, err := c.Conn.DialContextTCP(ctx, agentAddr)
687717
if err != nil {
688718
return nil, xerrors.Errorf("dial http api: %w", err)
689719
}

‎codersdk/workspacesdk/agentconn_test.go‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
package workspacesdk_test
2+
3+
import (
4+
"context"
5+
"errors"
6+
"fmt"
7+
"net"
8+
"net/http"
9+
"net/netip"
10+
"strings"
11+
"sync/atomic"
12+
"testing"
13+
14+
"github.com/google/uuid"
15+
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
17+
"tailscale.com/tailcfg"
18+
19+
"github.com/coder/coder/v2/codersdk/workspacesdk"
20+
"github.com/coder/coder/v2/tailnet"
21+
"github.com/coder/coder/v2/tailnet/proto"
22+
"github.com/coder/coder/v2/tailnet/tailnettest"
23+
"github.com/coder/coder/v2/testutil"
24+
)
25+
26+
func TestAgentConnRejectsCrossAgentRedirects(t *testing.T) {
27+
t.Parallel()
28+
29+
derpMap, _ := tailnettest.RunDERPAndSTUN(t)
30+
cases := []struct {
31+
name string
32+
status int
33+
invoke func(context.Context, workspacesdk.AgentConn) error
34+
}{
35+
{
36+
name: "get 302",
37+
status: http.StatusFound,
38+
invoke: func(ctx context.Context, conn workspacesdk.AgentConn) error {
39+
_, err := conn.ListeningPorts(ctx)
40+
return err
41+
},
42+
},
43+
{
44+
name: "post 307",
45+
status: http.StatusTemporaryRedirect,
46+
invoke: func(ctx context.Context, conn workspacesdk.AgentConn) error {
47+
return conn.WriteFile(ctx, "/tmp/attacker", strings.NewReader("redirect-body"))
48+
},
49+
},
50+
{
51+
name: "post 308",
52+
status: http.StatusPermanentRedirect,
53+
invoke: func(ctx context.Context, conn workspacesdk.AgentConn) error {
54+
return conn.WriteFile(ctx, "/tmp/attacker", strings.NewReader("redirect-body"))
55+
},
56+
},
57+
}
58+
59+
for _, tc := range cases {
60+
t.Run(tc.name, func(t *testing.T) {
61+
t.Parallel()
62+
63+
ctx := testutil.Context(t, testutil.WaitMedium)
64+
65+
clientID := uuid.New()
66+
attackerID := uuid.New()
67+
victimID := uuid.New()
68+
clientConn, _ := newTailnetConn(t, derpMap, clientID, "client")
69+
attackerConn, attackerIP := newTailnetConn(t, derpMap, attackerID, "attacker")
70+
victimConn, victimIP := newTailnetConn(t, derpMap, victimID, "victim")
71+
stitchTailnet(t, map[uuid.UUID]*tailnet.Conn{
72+
clientID: clientConn,
73+
attackerID: attackerConn,
74+
victimID: victimConn,
75+
})
76+
77+
var victimHit atomic.Bool
78+
victimRouter := http.NewServeMux()
79+
victimRouter.HandleFunc("/api/v0/listening-ports", func(rw http.ResponseWriter, _ *http.Request) {
80+
victimHit.Store(true)
81+
rw.Header().Set("Content-Type", "application/json")
82+
_, _ = rw.Write([]byte(`{"ports":[]}`))
83+
})
84+
victimRouter.HandleFunc("/api/v0/write-file", func(rw http.ResponseWriter, _ *http.Request) {
85+
victimHit.Store(true)
86+
rw.WriteHeader(http.StatusOK)
87+
})
88+
serveTailnetHTTP(t, victimConn, victimRouter)
89+
90+
victimBaseURL := fmt.Sprintf("http://[%s]:%d", victimIP, workspacesdk.AgentHTTPAPIServerPort)
91+
attackerRouter := http.NewServeMux()
92+
attackerRouter.HandleFunc("/", func(rw http.ResponseWriter, r *http.Request) {
93+
http.Redirect(rw, r, victimBaseURL+r.URL.RequestURI(), tc.status)
94+
})
95+
serveTailnetHTTP(t, attackerConn, attackerRouter)
96+
97+
require.True(t, clientConn.AwaitReachable(ctx, attackerIP))
98+
require.True(t, clientConn.AwaitReachable(ctx, victimIP))
99+
100+
conn := workspacesdk.NewAgentConn(clientConn, workspacesdk.AgentConnOptions{
101+
AgentID: attackerID,
102+
})
103+
104+
err := tc.invoke(ctx, conn)
105+
require.Error(t, err)
106+
require.False(t, victimHit.Load())
107+
})
108+
}
109+
}
110+
111+
// TestAgentConnAppHTTPClientRefusesRedirects verifies the app HTTP client does
112+
// not follow redirects.
113+
func TestAgentConnAppHTTPClientRefusesRedirects(t *testing.T) {
114+
t.Parallel()
115+
116+
tailnetConn, err := tailnet.NewConn(&tailnet.Options{
117+
Addresses: []netip.Prefix{tailnet.TailscaleServicePrefix.RandomPrefix()},
118+
Logger: testutil.Logger(t),
119+
})
120+
require.NoError(t, err)
121+
t.Cleanup(func() {
122+
_ = tailnetConn.Close()
123+
})
124+
125+
conn := workspacesdk.NewAgentConn(tailnetConn, workspacesdk.AgentConnOptions{
126+
AgentID: uuid.New(),
127+
})
128+
129+
client := conn.AppHTTPClient()
130+
require.NotNil(t, client.CheckRedirect)
131+
req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://example.invalid/", nil)
132+
require.NoError(t, err)
133+
require.ErrorIs(t, client.CheckRedirect(req, nil), http.ErrUseLastResponse)
134+
}
135+
136+
func newTailnetConn(t *testing.T, derpMap *tailcfg.DERPMap, id uuid.UUID, name string) (*tailnet.Conn, netip.Addr) {
137+
t.Helper()
138+
139+
addr := tailnet.TailscaleServicePrefix.AddrFromUUID(id)
140+
conn, err := tailnet.NewConn(&tailnet.Options{
141+
ID: id,
142+
Addresses: []netip.Prefix{netip.PrefixFrom(addr, 128)},
143+
Logger: testutil.Logger(t).Named(name),
144+
DERPMap: derpMap,
145+
})
146+
require.NoError(t, err)
147+
t.Cleanup(func() {
148+
assert.NoError(t, conn.Close())
149+
})
150+
151+
return conn, addr
152+
}
153+
154+
func serveTailnetHTTP(t *testing.T, conn *tailnet.Conn, handler http.Handler) {
155+
t.Helper()
156+
157+
ln, err := conn.Listen("tcp", fmt.Sprintf(":%d", workspacesdk.AgentHTTPAPIServerPort))
158+
require.NoError(t, err)
159+
160+
server := &http.Server{Handler: handler, ReadHeaderTimeout: testutil.WaitShort}
161+
t.Cleanup(func() {
162+
assert.NoError(t, server.Close())
163+
assert.NoError(t, ln.Close())
164+
})
165+
166+
go func() {
167+
err := server.Serve(ln)
168+
if err != nil && !errors.Is(err, net.ErrClosed) && !errors.Is(err, http.ErrServerClosed) {
169+
assert.NoError(t, err)
170+
}
171+
}()
172+
}
173+
174+
// stitchTailnet cross-programs every conn's node into every other conn, the
175+
// N-peer analog of tailnet's stitch test helper, so the peers can reach each
176+
// other without a coordinator.
177+
func stitchTailnet(t *testing.T, conns map[uuid.UUID]*tailnet.Conn) {
178+
t.Helper()
179+
180+
sendNode := func(srcID uuid.UUID, node *tailnet.Node) {
181+
protoNode, err := tailnet.NodeToProto(node)
182+
if !assert.NoError(t, err) {
183+
return
184+
}
185+
for dstID, dst := range conns {
186+
if dstID == srcID {
187+
continue
188+
}
189+
err = dst.UpdatePeers([]*proto.CoordinateResponse_PeerUpdate{{
190+
Id: srcID[:],
191+
Node: protoNode,
192+
Kind: proto.CoordinateResponse_PeerUpdate_NODE,
193+
}})
194+
assert.NoError(t, err)
195+
}
196+
}
197+
198+
for srcID, src := range conns {
199+
src.SetNodeCallback(func(node *tailnet.Node) {
200+
sendNode(srcID, node)
201+
})
202+
if node := src.Node(); node != nil {
203+
sendNode(srcID, node)
204+
}
205+
}
206+
207+
t.Cleanup(func() {
208+
for _, conn := range conns {
209+
conn.SetNodeCallback(nil)
210+
}
211+
})
212+
}

‎codersdk/workspacesdk/agentconnmock/agentconnmock.go‎

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎codersdk/workspacesdk/workspacesdk.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,7 @@ func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *
299299
<-controller.Closed()
300300
return conn.Close()
301301
},
302+
Logger: options.Logger,
302303
})
303304

304305
if !agentConn.AwaitReachable(dialCtx) {

0 commit comments

Comments
 (0)