## 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.
Fixes all our Go file imports to match the preferred spec that we've _mostly_ been using. For example:
```
import (
"context"
"time"
"github.com/prometheus/client_golang/prometheus"
"golang.org/x/xerrors"
"gopkg.in/natefinch/lumberjack.v2"
"cdr.dev/slog/v3"
"github.com/coder/coder/v2/codersdk/agentsdk"
"github.com/coder/serpent"
)
```
3 groups: standard library, 3rd partly libs, Coder libs.
This PR makes the change across the codebase. The PR in the stack above modifies our formatting to maintain this state of affairs, and is a separate PR so it's possible to review that one in detail.
Upgrades to slog v3 which includes a small, but backward incompatible API change to the acceptible call arguments when logging. This change allows us to verify via compile time type checking that arguments are correct and won't cause a panic, as was possible in slog v1, which this replaces (v2 was tagged but never used in coder/coder).
It also updates dependencies that also use slog and were updated.
I've left the `aibridge` dependency as a commit SHA, under the assumption that the team there (cc @pawbana @dannykopping ) will tag and update the dependency soon and on their own schedule.
Other dependencies, I pushed new tags.
Closes#19984
As part of this, I refactored the error template to take in a slice of
actions rather than using individual booleans and strings to control the
behavior.
We decided a link resolves the issue for now so that is what I added,
although we may want to consider a way to start the workspace and follow
the logs dynamically on that page and then show the app when finished
(similar to the tasks page), or at least make the link automatically
start the workspace instead of only taking you to the dashboard where
you have to then start the workspace.
Fixes https://github.com/coder/internal/issues/907
We convert `workspacesdk.AgentConn` to an interface and generate a mock
for it. This allows writing `coderd` tests that rely on the agent's HTTP
api to not have to set up an entire tailnet networking stack.
The agentsdk currently does a remap of the DERP map to change the
EmbeddedRelay node's URL to match the agent's access URL.
This PR makes changes to the `workspacesdk` (used by clients like the
CLI) and `vpn` (used by Coder Desktop) to match this behavior.
This enables us the ability to try Coder clients in dogfood over a VPN
without changing the global access URL.
chore of #14729
Refactors the `ServerTailnet` to use `tailnet.Controller` so that we reuse logic around reconnection and handling control messages, instead of reimplementing. This unifies our "client" use of the tailscale API across CLI, coderd, and wsproxy.
fixes https://github.com/coder/internal/issues/114
We need to wait for ServerTailnet goroutines to finish when closing down, otherwise we can race with the shutdown of coderd & the coordinator, which causes errors.
re: #14715
This PR introduces the Coder service prefix: `fd60:627a:a42b::/48` and refactors our existing code as calling the Tailscale service prefix explicitly (rather than implicitly).
Removes the unused `Addresses` agent option. All clients today assume they can compute the Agent's IP address based on its UUID, so an agent started with a custom address would break things.
Currently, importing `codersdk` just to interact with the API requires
importing tailscale, which causes builds to fail unless manually using
our fork.
I noticed a possible race where tailnet.Conn can try to dial the embedded region before we've set our custom dialer that send the DERP in-memory. This closes that race and adds a test case for servertailnet with no STUN and an embedded relay
I think this will resolve#12136 but lets get a proper test at the system level before closing.
Before this change, we only register the node callback at start of day for the server tailnet. If the coordinator changes, like we know happens when we are licensed for the PGCoordinator, we close the connection to the old coord, and open a new one to the new coord.
The callback is designed to direct the updates to the new coordinator, but there is nothing that specifically triggers it to fire after we connect to the new coordinator.
If we have STUN, then period re-STUNs will generally get it to fire eventually, but without STUN it we could go indefinitely without a callback.
This PR changes the servertailnet to re-register the callback each time we reconnect to the coordinator. Registering a callback (even if it's the same callback) triggers an immediate call with our node information, so the new coordinator will have it.
Fixes#8218
Removes `wsconncache` and related "is legacy?" functions and API calls that were used by it.
The only leftover is that Agents still use the legacy IP, so that back level clients or workspace proxies can dial them correctly.
We should eventually remove this: #11819
Adds support to `ServerTailnet` to set all peers lost before attempting to reconnect to the coordinator. In practice, this only really affects `wsproxy` since coderd has a local connection to the coordinator that only goes down if we're shutting down or change licenses.
This one is huge, and I'm sorry.
The problem is that once I change `tailnet.Conn` to start doing v2 behavior, I kind of have to change it everywhere, including in CoderSDK (CLI), the agent, wsproxy, and ServerTailnet.
There is still a bit more cleanup to do, and I need to add code so that when we lose connection to the Coordinator, we mark all peers as LOST, but that will be in a separate PR since this is big enough!
The `SingleTailnet` behavior only checked to see if the `MultiAgent` was
closed, but the websocket error was not being propogated into the
`MultiAgent`, causing it to never be swapped for a new working one.
Fixes https://github.com/coder/coder/issues/11401
Before:
```
Coder Workspace Proxy v0.0.0-devel+85ff030 - Your Self-Hosted Remote Development Platform
Started HTTP listener at http://0.0.0.0:3001
View the Web UI: http://127.0.0.1:3001
==> Logs will stream in below (press ctrl+c to gracefully exit):
2024-01-04 20:11:56.376 [warn] net.workspace-proxy.servertailnet: broadcast server node to agents ...
error= write message:
github.com/coder/coder/v2/enterprise/wsproxy/wsproxysdk.(*remoteMultiAgentHandler).writeJSON
/home/coder/coder/enterprise/wsproxy/wsproxysdk/wsproxysdk.go:524
- failed to write msg: WebSocket closed: failed to read frame header: EOF
```
After:
```
Coder Workspace Proxy v0.0.0-devel+12f1878 - Your Self-Hosted Remote Development Platform
Started HTTP listener at http://0.0.0.0:3001
View the Web UI: http://127.0.0.1:3001
==> Logs will stream in below (press ctrl+c to gracefully exit):
2024-01-04 20:26:38.545 [warn] net.workspace-proxy.servertailnet: multiagent closed, reinitializing
2024-01-04 20:26:38.546 [erro] net.workspace-proxy.servertailnet: reinit multi agent ...
error= dial coordinate websocket:
github.com/coder/coder/v2/enterprise/wsproxy/wsproxysdk.(*Client).DialCoordinator
/home/coder/coder/enterprise/wsproxy/wsproxysdk/wsproxysdk.go:454
- failed to WebSocket dial: failed to send handshake request: Get "http://127.0.0.1:3000/api/v2/workspaceproxies/me/coordinate": dial tcp 127.0.0.1:3000: connect: connection refused
2024-01-04 20:26:38.587 [erro] net.workspace-proxy.servertailnet: reinit multi agent ...
error= dial coordinate websocket:
github.com/coder/coder/v2/enterprise/wsproxy/wsproxysdk.(*Client).DialCoordinator
/home/coder/coder/enterprise/wsproxy/wsproxysdk/wsproxysdk.go:454
- failed to WebSocket dial: failed to send handshake request: Get "http://127.0.0.1:3000/api/v2/workspaceproxies/me/coordinate": dial tcp 127.0.0.1:3000: connect: connection refusedhandshake request: Get "http://127.0.0.1:3000/api/v2/workspaceproxies/me/coordinate": dial tcp 127.0.0.1:3000: connect: connection refused
2024-01-04 20:26:40.446 [info] net.workspace-proxy.servertailnet: successfully reinitialized multiagent agents=0 took=1.900892615s
```
* chore: add /v2 to import module path
go mod requires semantic versioning with versions greater than 1.x
This was a mechanical update by running:
```
go install github.com/marwan-at-work/mod/cmd/mod@latest
mod upgrade
```
Migrate generated files to import /v2
* Fix gen
Ethan