mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: validate agent-supplied AllowedIPs in coordinator (#26144)
`AgentCoordinateeAuth.Authorize` validated every prefix in `upd.Node.Addresses` (each must be a `/128` derived from the authenticating agent's own UUID) but applied no equivalent check to `upd.Node.AllowedIps`. Because `AllowedIPs` are installed verbatim into the WireGuard peer config (`tailnet/configmaps.go`) and WireGuard routing is driven by `AllowedIPs`, a malicious agent could advertise a victim agent's `/128` and become an eligible route for that IP. With `ServerTailnet` tunneling to many agents and routing by destination IP, this could let an attacker intercept sessions intended for the victim workspace. This applies the same UUID-derivation validation to `AllowedIps` that already guards `Addresses`, extracted into a shared `authorizeNodePrefixes` helper. The check is the single chokepoint used by both the in-memory coordinator (`tailnet/coordinator.go`) and the Postgres coordinator (`enterprise/tailnet/connio.go`), so one fix covers both. Legitimate agents are unaffected: an agent's `AllowedIPs` is a clone of its `Addresses` (`tailnet/node.go`), which are already UUID-derived `/128`s. Fixes PLAT-264 (SEC-89): https://linear.app/codercom/issue/PLAT-264 <details> <summary>Implementation notes and decision log</summary> ### Root cause Asymmetric validation in `tailnet/tunnel.go`: `Addresses` were bound to the agent's UUID, but `AllowedIps` were trusted as-is and propagated into the WireGuard peer config, which drives routing. ### Why the fix is safe for legitimate agents - `tailnet/node.go` builds the node with `AllowedIPs: slices.Clone(u.addresses)`, identical to `Addresses`. - `agent/agent.go` sets those addresses to `TailscaleServicePrefix.PrefixFromUUID(agentID)` and `CoderServicePrefix.PrefixFromUUID(agentID)` (both `/128`, UUID-derived). - The existing `Addresses` check already accepts exactly those prefixes plus the legacy workspace agent IP, so identical validation of `AllowedIPs` passes for real traffic and only rejects forged prefixes. ### Coverage: one method, both coordinators `AgentCoordinateeAuth.Authorize` is the shared auth path. A failed `Authorize` is wrapped as `AuthorizationError{Wrapped: err}` and closes the agent's response stream. ### Tests - `tailnet/tunnel_internal_test.go`: fast unit tests on `Authorize` (valid AllowedIPs accepted; foreign `/128` rejected with `InvalidNodeAddressError`; wrong-bits rejected with `InvalidAddressBitsError`). - `tailnet/coordinator_test.go`: in-memory coordinator closes the agent stream on a forged `AllowedIp`. - `enterprise/tailnet/pgcoord_test.go`: same regression for the Postgres coordinator. Verified the regression tests fail when the new `AllowedIps` check is disabled, then pass with it enabled. Local validation: targeted tests (in-memory, internal, and Postgres-backed enterprise), plus `make pre-commit` (gen/fmt/lint/build) passing. </details> > Generated by Coder Agents on behalf of @f0ssel.
This commit is contained in:
@@ -7,6 +7,7 @@ import (
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"cdr.dev/slog/v3/sloggers/slogtest"
|
||||
@@ -102,7 +103,32 @@ func TestCoordinator(t *testing.T) {
|
||||
PreferredDerp: 10,
|
||||
})
|
||||
agent.AssertEventuallyResponsesClosed(
|
||||
tailnet.AuthorizationError{Wrapped: tailnet.InvalidNodeAddressError{Addr: prefix.Addr().String()}}.Error())
|
||||
tailnet.AuthorizationError{Wrapped: xerrors.Errorf("Addresses: %w", tailnet.InvalidNodeAddressError{Addr: prefix.Addr().String()})}.Error())
|
||||
})
|
||||
|
||||
t.Run("AgentWithoutClients_InvalidAllowedIP", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
coordinator := tailnet.NewCoordinator(logger)
|
||||
defer func() {
|
||||
err := coordinator.Close()
|
||||
require.NoError(t, err)
|
||||
}()
|
||||
agent := test.NewAgent(ctx, t, coordinator, "agent")
|
||||
defer agent.Close(ctx)
|
||||
// A valid self-address paired with an AllowedIP belonging to a different
|
||||
// (victim) agent must be rejected.
|
||||
victim := tailnet.TailscaleServicePrefix.PrefixFromUUID(uuid.New())
|
||||
agent.UpdateNode(&proto.Node{
|
||||
Addresses: []string{
|
||||
tailnet.TailscaleServicePrefix.PrefixFromUUID(agent.ID).String(),
|
||||
},
|
||||
AllowedIps: []string{victim.String()},
|
||||
PreferredDerp: 10,
|
||||
})
|
||||
agent.AssertEventuallyResponsesClosed(
|
||||
tailnet.AuthorizationError{Wrapped: xerrors.Errorf("AllowedIps: %w", tailnet.InvalidNodeAddressError{Addr: victim.Addr().String()})}.Error())
|
||||
})
|
||||
|
||||
t.Run("AgentWithoutClients_InvalidBits", func(t *testing.T) {
|
||||
@@ -124,7 +150,7 @@ func TestCoordinator(t *testing.T) {
|
||||
PreferredDerp: 10,
|
||||
})
|
||||
agent.AssertEventuallyResponsesClosed(
|
||||
tailnet.AuthorizationError{Wrapped: tailnet.InvalidAddressBitsError{Bits: 64}}.Error())
|
||||
tailnet.AuthorizationError{Wrapped: xerrors.Errorf("Addresses: %w", tailnet.InvalidAddressBitsError{Bits: 64})}.Error())
|
||||
})
|
||||
|
||||
t.Run("AgentWithClient", func(t *testing.T) {
|
||||
|
||||
+30
-13
@@ -71,21 +71,38 @@ func (a AgentCoordinateeAuth) Authorize(_ context.Context, req *proto.Coordinate
|
||||
}
|
||||
|
||||
if upd := req.GetUpdateSelf(); upd != nil {
|
||||
for _, addrStr := range upd.Node.Addresses {
|
||||
pre, err := netip.ParsePrefix(addrStr)
|
||||
if err != nil {
|
||||
return xerrors.Errorf("parse node address: %w", err)
|
||||
}
|
||||
// Both Addresses and AllowedIPs are installed into the WireGuard peer
|
||||
// config and drive routing, so an agent may only advertise prefixes
|
||||
// derived from its own UUID. Without this an agent could claim a victim
|
||||
// agent's IP and have traffic routed to it.
|
||||
if err := a.authorizeNodePrefixes(upd.Node.Addresses); err != nil {
|
||||
return xerrors.Errorf("Addresses: %w", err)
|
||||
}
|
||||
if err := a.authorizeNodePrefixes(upd.Node.AllowedIps); err != nil {
|
||||
return xerrors.Errorf("AllowedIps: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
if pre.Bits() != 128 {
|
||||
return InvalidAddressBitsError{pre.Bits()}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
if TailscaleServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
|
||||
CoderServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
|
||||
legacyWorkspaceAgentIP.Compare(pre.Addr()) != 0 {
|
||||
return InvalidNodeAddressError{pre.Addr().String()}
|
||||
}
|
||||
// authorizeNodePrefixes verifies that every prefix is a /128 address derived
|
||||
// from the agent's own UUID (or the legacy workspace agent IP).
|
||||
func (a AgentCoordinateeAuth) authorizeNodePrefixes(prefixes []string) error {
|
||||
for _, prefixStr := range prefixes {
|
||||
pre, err := netip.ParsePrefix(prefixStr)
|
||||
if err != nil {
|
||||
return xerrors.Errorf("parse node address: %w", err)
|
||||
}
|
||||
|
||||
if pre.Bits() != 128 {
|
||||
return InvalidAddressBitsError{pre.Bits()}
|
||||
}
|
||||
|
||||
if TailscaleServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
|
||||
CoderServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
|
||||
legacyWorkspaceAgentIP.Compare(pre.Addr()) != 0 {
|
||||
return InvalidNodeAddressError{pre.Addr().String()}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user