feat: support disabling reverse/local port forwarding in agent SSH server#24026
feat: support disabling reverse/local port forwarding in agent SSH server#24026
Conversation
…erver Add --block-reverse-port-forwarding and --block-local-port-forwarding flags to the agent CLI, allowing template admins to disable ssh -R and ssh -L through environment variables set on the coder_agent Terraform resource. This addresses a sandbox escape vector where reverse tunnels can be used to bypass workspace network isolation in AI agent containment scenarios.
f0ssel
changed the title
Documentation CheckUpdates Needed
New Documentation Needed
Automated review via Coder Tasks |
I think this need to be set on the resource the |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Clean design, correct plumbing from CLI flags through agent Options to SSH Config. The BlockFileTransfer pattern is the right model to follow, and the TCP forwarding callbacks work as intended.
The core problem: the SSH server has four forwarding paths (TCP local, TCP reverse, Unix local, Unix reverse), but the new flags only gate the two TCP paths. The Unix domain socket paths in forward.go have no config access and accept all requests unconditionally. For a feature motivated by AI agent sandbox escape prevention, this is a complete bypass.
Severity breakdown: 2x P1, 1x P2, 2x P3.
As Kurapika put it: "The PR description says 'a reverse tunnel lets anything inside the workspace reach the user's local machine.' Unix socket reverse forwarding does exactly the same thing."
PS. The test names (TestAgent_TCPLocalForwardingBlocked) are more honest than the flag names about what's actually blocked. Worth preserving that honesty when adding Unix socket blocking tests.
PS. Agent-side env var enforcement means a workspace root user can restart the agent without these flags. This is the same threat model as BlockFileTransfer and is a known limitation, not a regression from this PR.
🤖 This review was automatically generated with Coder Agents.
| return true | ||
| }, | ||
| ReversePortForwardingCallback: func(ctx ssh.Context, bindHost string, bindPort uint32) bool { | ||
| if s.config.BlockReversePortForwarding { |
There was a problem hiding this comment.
P1 Unix reverse socket forwarding (streamlocal-forward@openssh.com) bypasses BlockReversePortForwarding. (Kurapika P1, Meruem P0, Bisky P1, Mafuuu P1, Kite P1, Knov P1)
forwardedUnixHandler.HandleSSHRequest (forward.go:54) handles streamlocal-forward@openssh.com and listens on a Unix socket unconditionally. It has no reference to the server config. When BlockReversePortForwarding is true, sshClient.ListenTCP(addr) is rejected by this callback, but sshClient.ListenUnix(path) hits forwardedUnixHandler unchecked.
This is the exact escape vector the PR description warns about: a reverse tunnel lets anything inside the workspace reach the user's local machine. Unix sockets are just a different transport.
Fix: either pass the config to newForwardedUnixHandler or wrap the request handler registration in a closure that checks s.config.BlockReversePortForwarding and returns false, nil when blocked.
🤖
| require.NoError(t, err) | ||
| defer sshClient.Close() | ||
|
|
||
| _, err = sshClient.Dial("tcp", fmt.Sprintf("127.0.0.1:%d", remotePort)) |
There was a problem hiding this comment.
P3 Blocked-forwarding tests assert only require.Error without checking the rejection reason. (Bisky P3)
Both blocked tests do require.Error(t, err) and nothing else. The dial could fail for any reason (timeout, port collision, network flake) and the test would still pass. For a security-critical behavior, asserting the rejection came from the blocking callback is worth the effort.
Consider require.ErrorContains(t, err, "administratively prohibited") or similar to confirm the SSH Prohibited rejection.
🤖
There was a problem hiding this comment.
Fixed in the latest commit. All four blocked-forwarding tests now assert the specific SSH rejection string instead of bare require.Error.
🤖 Generated by Coder Agents
| Description: fmt.Sprintf("Block file transfer using known applications: %s.", strings.Join(agentssh.BlockedFileTransferCommands, ",")), | ||
| Value: serpent.BoolOf(&blockFileTransfer), | ||
| }, | ||
| { |
There was a problem hiding this comment.
P3 CLI description says (ssh -L) but ssh -L also supports Unix socket targets, which are not blocked. (Gon P2, Bisky Note, Mafuuu Note, Knov Note)
Same applies to (ssh -R) on line 509. A template admin reads "Block local port forwarding through the SSH server (ssh -L)" and believes all -L usage is denied. It is not: Unix socket forwarding goes through separate handlers.
Once the Unix socket bypass is fixed (see findings on agentssh.go), this becomes accurate. If Unix sockets remain unblocked, the descriptions should say (direct-tcpip only) or similar.
🤖
Extend port forwarding blocking to cover all four SSH forwarding paths: TCP local, TCP reverse, Unix local, and Unix reverse. Without this, the Unix domain socket paths (direct-streamlocal, streamlocal-forward) bypass the blocking flags entirely.
Use require.ErrorContains to verify the error comes from the SSH blocking callback rather than a network flake. Local forwarding tests check for "administratively prohibited" (gossh.Prohibited), reverse forwarding tests check for "request denied by peer".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
All five Round 1 findings resolved. All four SSH forwarding paths (TCP local, TCP reverse, Unix local, Unix reverse) are now blocked. Tests cover each path with specific rejection string assertions. Clean implementation.
One P3, two notes.
The P3: no test verifies the two flags are independent. Blocking local doesn't affect reverse (and vice versa). The current tests catch field-name typos (each sets only one flag), but not higher-level coupling from a future refactor that merges the flags or middleware that reads both. As Bisky put it: "The independence contract is implicit. One test per direction would make it explicit."
Note: sub-agent SSH servers in devcontainers don't inherit the blocking env vars. CODER_AGENT_BLOCK_REVERSE_PORT_FORWARDING and CODER_AGENT_BLOCK_LOCAL_PORT_FORWARDING are not propagated to runSubAgentInContainer (agentcontainers/api.go). Same gap as BlockFileTransfer, pre-existing pattern, not introduced by this PR. Worth tracking if AI agent containment in devcontainers becomes a use case.
Note: the direct-tcpip handler wraps the channel with NewJetbrainsChannelWatcher (reads /proc) before the library's DirectTCPIPHandler rejects via LocalPortForwardingCallback. The direct-streamlocal@openssh.com handler avoids this by checking the flag first. No correctness or security impact, just a wasted syscall per blocked TCP local forward attempt.
🤖 This review was automatically generated with Coder Agents.
| requireEcho(t, conn) | ||
| } | ||
|
|
||
| func TestAgent_TCPLocalForwardingBlocked(t *testing.T) { |
There was a problem hiding this comment.
P3 No test verifies the two flags are independent. (Bisky P3)
Each blocked test sets only its own flag and verifies the corresponding direction is rejected. No test sets BlockLocalPortForwarding=true and confirms reverse forwarding still works, or vice versa.
For the AI containment use case, a template admin blocking reverse tunnels must not break local forwarding needed by IDE tooling. One test per direction would make this explicit:
func TestAgent_LocalBlockedDoesNotAffectReverse(t *testing.T) {
// Set BlockLocalPortForwarding=true only.
// Assert ListenTCP still succeeds.
}🤖
There was a problem hiding this comment.
Re-raising. 6 of 7 reviewers independently flagged this again. No response, no ticket, no code change.
The strongest argument (Meruem): the flags are plumbed through three struct hops (cli -> Options -> agent struct -> Config). A transposition at any hop (e.g. BlockReversePortForwarding: a.blockLocalPortForwarding) would pass all four existing blocked tests because each only exercises its own direction. The result: blocking local silently blocks reverse instead, and the intended direction stays open.
Two tests, one per direction. Each sets one flag, attempts both directions, asserts the unblocked direction succeeds.
🤖
There was a problem hiding this comment.
Added in the latest commit: TestAgent_LocalBlockedDoesNotAffectReverse and TestAgent_ReverseBlockedDoesNotAffectLocal. Each sets one flag and verifies the opposite direction still works end-to-end.
🤖 Generated by Coder Agents
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 3 (first proper round with full reviewer prompts). Seven reviewers, all prior P1/P2 findings verified resolved. All four SSH forwarding paths are blocked. Tests cover each path with specific SSH rejection string assertions. Claims in CLI descriptions match the implementation. Clean.
One open P3 from round 2 (#comment-3047871971), re-raised by 6 of 7 reviewers: no test verifies the two flags are independent. As Meruem put it: "A transposition at any hop would pass all four existing blocked tests because each only exercises its own direction." Bisky's sketch from round 2 still applies.
Two notes worth knowing:
(1) Unix socket blocked log entries omit the socket path. TCP blocking logs include destination_host/destination_port, but the Unix local handler at agentssh.go:238 logs only "unix local port forward blocked" with no structured context about what path was denied. The reverse handler at forward.go:68 checks before payload parsing so this is unavoidable, but the local handler could parse first.
(2) Sub-agents spawned for devcontainers do not inherit CODER_AGENT_BLOCK_*_PORT_FORWARDING env vars. Same gap as BlockFileTransfer. Pre-existing pattern, not introduced by this PR. Worth tracking if AI agent containment in devcontainers is a use case.
🤖 This review was automatically generated with Coder Agents.
Add tests that set one blocking flag and confirm the opposite direction still works. This catches field-name transpositions at any of the three struct hops (cli -> Options -> agent -> Config) that would silently swap which direction gets blocked.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
All six findings from rounds 1-3 are resolved. The flag independence tests (#comment-3047871971) land in commit 4918e85 with a comment explaining the transposition risk they guard against, showing the author absorbed the reviewer reasoning rather than mechanically complying. Clean.
No new findings. Zero P0-P3 from 6 reviewers.
As Bisky put it: "Remove the blocking code and every one of them fails. These are real gems."
Two process observations from Mafu-san worth noting: (1) the initial commit described blocking "both TCP and Unix" but only implemented TCP, producing the round-1 P1 findings; grepping for all forwarding handler registrations in NewServer before writing the description would have caught this; (2) finding F6 (flag independence tests) went two rounds without author response before being addressed. Silence is not a disposition.
PS. Sub-agent devcontainer env var propagation gap remains a known limitation (same as BlockFileTransfer). Not introduced by this PR.
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Looks solid, human approved! 🏂🏻
3 participants
The agent SSH server unconditionally allows all four SSH forwarding paths (TCP local, TCP reverse, Unix local, Unix reverse). This is a sandbox escape vector when workspaces are used for AI agent containment — a reverse tunnel lets anything inside the workspace reach the user's local machine, bypassing network isolation.
This adds two new agent CLI flags / environment variables:
--block-reverse-port-forwarding/CODER_AGENT_BLOCK_REVERSE_PORT_FORWARDING— blocks both TCP (ssh -R) and Unix socket reverse forwarding--block-local-port-forwarding/CODER_AGENT_BLOCK_LOCAL_PORT_FORWARDING— blocks both TCP (ssh -L) and Unix socket local forwardingTemplate admins can set these via the
envblock on the container/VM resource that runs the agent (e.g.docker_container,kubernetes_pod), or viacoder_envresources tied to the agent.Fixes #22275
Implementation notes
Follows the existing
BlockFileTransferpattern:agent/agentssh/agentssh.go— NewBlockReversePortForwardingandBlockLocalPortForwardingfields onConfig. TCP callbacks check these before allowing forwarding. Thedirect-streamlocal@openssh.comchannel handler is wrapped to reject Unix local forwards.agent/agentssh/forward.go—forwardedUnixHandlergains ablockReversePortForwardingfield to rejectstreamlocal-forward@openssh.comrequests.agent/agent.go— New fields onOptionsandagentstruct, plumbed to SSH config.cli/agent.go— New serpent flags with env vars.