mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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.
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user