Skip to content

Commit 9181b84

Browse files
f0sself0ssel
authored
fix: validate agent-supplied AllowedIPs in coordinator (backport #26144) (#26295)
Backport of #26144 (commit 9b351c3) to `release/2.29`. Validates agent-supplied `AllowedIPs` in the coordinator the same way node addresses are validated, preventing an agent from advertising routes for IPs it does not own. Cherry-pick was clean; `tailnet` coordinator tests pass on this branch. --- *This PR was generated by a Coder Agent on behalf of @f0ssel.*
1 parent 6f5ff1b commit 9181b84

3 files changed

Lines changed: 90 additions & 17 deletions

File tree

‎enterprise/tailnet/pgcoord_test.go‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ func TestPGCoordinatorSingle_AgentInvalidIP(t *testing.T) {
120120

121121
// The agent connection should be closed immediately after sending an invalid addr
122122
agent.AssertEventuallyResponsesClosed(
123-
agpl.AuthorizationError{Wrapped: agpl.InvalidNodeAddressError{Addr: prefix.Addr().String()}}.Error())
123+
agpl.AuthorizationError{Wrapped: xerrors.Errorf("Addresses: %w", agpl.InvalidNodeAddressError{Addr: prefix.Addr().String()})}.Error())
124124
assertEventuallyLost(ctx, t, store, agent.ID)
125125
}
126126

@@ -146,7 +146,37 @@ func TestPGCoordinatorSingle_AgentInvalidIPBits(t *testing.T) {
146146

147147
// The agent connection should be closed immediately after sending an invalid addr
148148
agent.AssertEventuallyResponsesClosed(
149-
agpl.AuthorizationError{Wrapped: agpl.InvalidAddressBitsError{Bits: 64}}.Error())
149+
agpl.AuthorizationError{Wrapped: xerrors.Errorf("Addresses: %w", agpl.InvalidAddressBitsError{Bits: 64})}.Error())
150+
assertEventuallyLost(ctx, t, store, agent.ID)
151+
}
152+
153+
func TestPGCoordinatorSingle_AgentInvalidAllowedIP(t *testing.T) {
154+
t.Parallel()
155+
156+
store, ps := dbtestutil.NewDB(t)
157+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitSuperLong)
158+
defer cancel()
159+
logger := testutil.Logger(t)
160+
coordinator, err := tailnet.NewPGCoord(ctx, logger, ps, store)
161+
require.NoError(t, err)
162+
defer coordinator.Close()
163+
164+
agent := agpltest.NewAgent(ctx, t, coordinator, "agent")
165+
defer agent.Close(ctx)
166+
// A valid self-address paired with an AllowedIP belonging to a different
167+
// (victim) agent must be rejected.
168+
victim := agpl.TailscaleServicePrefix.PrefixFromUUID(uuid.New())
169+
agent.UpdateNode(&proto.Node{
170+
Addresses: []string{
171+
agpl.TailscaleServicePrefix.PrefixFromUUID(agent.ID).String(),
172+
},
173+
AllowedIps: []string{victim.String()},
174+
PreferredDerp: 10,
175+
})
176+
177+
// The agent connection should be closed after sending an invalid AllowedIP.
178+
agent.AssertEventuallyResponsesClosed(
179+
agpl.AuthorizationError{Wrapped: xerrors.Errorf("AllowedIps: %w", agpl.InvalidNodeAddressError{Addr: victim.Addr().String()})}.Error())
150180
assertEventuallyLost(ctx, t, store, agent.ID)
151181
}
152182

‎tailnet/coordinator_test.go‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
"github.com/google/uuid"
99
"github.com/stretchr/testify/require"
10+
"golang.org/x/xerrors"
1011

1112
"cdr.dev/slog"
1213
"cdr.dev/slog/sloggers/slogtest"
@@ -102,7 +103,32 @@ func TestCoordinator(t *testing.T) {
102103
PreferredDerp: 10,
103104
})
104105
agent.AssertEventuallyResponsesClosed(
105-
tailnet.AuthorizationError{Wrapped: tailnet.InvalidNodeAddressError{Addr: prefix.Addr().String()}}.Error())
106+
tailnet.AuthorizationError{Wrapped: xerrors.Errorf("Addresses: %w", tailnet.InvalidNodeAddressError{Addr: prefix.Addr().String()})}.Error())
107+
})
108+
109+
t.Run("AgentWithoutClients_InvalidAllowedIP", func(t *testing.T) {
110+
t.Parallel()
111+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
112+
ctx := testutil.Context(t, testutil.WaitShort)
113+
coordinator := tailnet.NewCoordinator(logger)
114+
defer func() {
115+
err := coordinator.Close()
116+
require.NoError(t, err)
117+
}()
118+
agent := test.NewAgent(ctx, t, coordinator, "agent")
119+
defer agent.Close(ctx)
120+
// A valid self-address paired with an AllowedIP belonging to a different
121+
// (victim) agent must be rejected.
122+
victim := tailnet.TailscaleServicePrefix.PrefixFromUUID(uuid.New())
123+
agent.UpdateNode(&proto.Node{
124+
Addresses: []string{
125+
tailnet.TailscaleServicePrefix.PrefixFromUUID(agent.ID).String(),
126+
},
127+
AllowedIps: []string{victim.String()},
128+
PreferredDerp: 10,
129+
})
130+
agent.AssertEventuallyResponsesClosed(
131+
tailnet.AuthorizationError{Wrapped: xerrors.Errorf("AllowedIps: %w", tailnet.InvalidNodeAddressError{Addr: victim.Addr().String()})}.Error())
106132
})
107133

108134
t.Run("AgentWithoutClients_InvalidBits", func(t *testing.T) {
@@ -124,7 +150,7 @@ func TestCoordinator(t *testing.T) {
124150
PreferredDerp: 10,
125151
})
126152
agent.AssertEventuallyResponsesClosed(
127-
tailnet.AuthorizationError{Wrapped: tailnet.InvalidAddressBitsError{Bits: 64}}.Error())
153+
tailnet.AuthorizationError{Wrapped: xerrors.Errorf("Addresses: %w", tailnet.InvalidAddressBitsError{Bits: 64})}.Error())
128154
})
129155

130156
t.Run("AgentWithClient", func(t *testing.T) {

‎tailnet/tunnel.go‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -71,21 +71,38 @@ func (a AgentCoordinateeAuth) Authorize(_ context.Context, req *proto.Coordinate
7171
}
7272

7373
if upd := req.GetUpdateSelf(); upd != nil {
74-
for _, addrStr := range upd.Node.Addresses {
75-
pre, err := netip.ParsePrefix(addrStr)
76-
if err != nil {
77-
return xerrors.Errorf("parse node address: %w", err)
78-
}
74+
// Both Addresses and AllowedIPs are installed into the WireGuard peer
75+
// config and drive routing, so an agent may only advertise prefixes
76+
// derived from its own UUID. Without this an agent could claim a victim
77+
// agent's IP and have traffic routed to it.
78+
if err := a.authorizeNodePrefixes(upd.Node.Addresses); err != nil {
79+
return xerrors.Errorf("Addresses: %w", err)
80+
}
81+
if err := a.authorizeNodePrefixes(upd.Node.AllowedIps); err != nil {
82+
return xerrors.Errorf("AllowedIps: %w", err)
83+
}
84+
}
7985

80-
if pre.Bits() != 128 {
81-
return InvalidAddressBitsError{pre.Bits()}
82-
}
86+
return nil
87+
}
8388

84-
if TailscaleServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
85-
CoderServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
86-
legacyWorkspaceAgentIP.Compare(pre.Addr()) != 0 {
87-
return InvalidNodeAddressError{pre.Addr().String()}
88-
}
89+
// authorizeNodePrefixes verifies that every prefix is a /128 address derived
90+
// from the agent's own UUID (or the legacy workspace agent IP).
91+
func (a AgentCoordinateeAuth) authorizeNodePrefixes(prefixes []string) error {
92+
for _, prefixStr := range prefixes {
93+
pre, err := netip.ParsePrefix(prefixStr)
94+
if err != nil {
95+
return xerrors.Errorf("parse node address: %w", err)
96+
}
97+
98+
if pre.Bits() != 128 {
99+
return InvalidAddressBitsError{pre.Bits()}
100+
}
101+
102+
if TailscaleServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
103+
CoderServicePrefix.AddrFromUUID(a.ID).Compare(pre.Addr()) != 0 &&
104+
legacyWorkspaceAgentIP.Compare(pre.Addr()) != 0 {
105+
return InvalidNodeAddressError{pre.Addr().String()}
89106
}
90107
}
91108

0 commit comments

Comments
 (0)