policy/v2: separate exit node and 0.0.0.0/0 routes (#2578)

* policy: add tests for route auto approval

Reproduce #2568

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy/v2: separate exit node and 0.0.0.0/0 routes

Fixes #2568

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
This commit is contained in:
Kristoffer Dalby 2025-05-10 00:20:04 +03:00 committed by GitHub
parent 377b854dd8
commit 37dc0dad35
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 873 additions and 40 deletions

View file

@ -31,6 +31,8 @@ type PolicyManager struct {
tagOwnerMapHash deephash.Sum
tagOwnerMap map[Tag]*netipx.IPSet
exitSetHash deephash.Sum
exitSet *netipx.IPSet
autoApproveMapHash deephash.Sum
autoApproveMap map[netip.Prefix]*netipx.IPSet
@ -97,7 +99,7 @@ func (pm *PolicyManager) updateLocked() (bool, error) {
pm.tagOwnerMap = tagMap
pm.tagOwnerMapHash = tagOwnerMapHash
autoMap, err := resolveAutoApprovers(pm.pol, pm.users, pm.nodes)
autoMap, exitSet, err := resolveAutoApprovers(pm.pol, pm.users, pm.nodes)
if err != nil {
return false, fmt.Errorf("resolving auto approvers map: %w", err)
}
@ -107,8 +109,13 @@ func (pm *PolicyManager) updateLocked() (bool, error) {
pm.autoApproveMap = autoMap
pm.autoApproveMapHash = autoApproveMapHash
exitSetHash := deephash.Hash(&autoMap)
exitSetChanged := exitSetHash != pm.exitSetHash
pm.exitSet = exitSet
pm.exitSetHash = exitSetHash
// If neither of the calculated values changed, no need to update nodes
if !filterChanged && !tagOwnerChanged && !autoApproveChanged {
if !filterChanged && !tagOwnerChanged && !autoApproveChanged && !exitSetChanged {
return false, nil
}
@ -207,6 +214,23 @@ func (pm *PolicyManager) NodeCanApproveRoute(node *types.Node, route netip.Prefi
return false
}
// If the route to-be-approved is an exit route, then we need to check
// if the node is in allowed to approve it. This is treated differently
// than the auto-approvers, as the auto-approvers are not allowed to
// approve the whole /0 range.
// However, an auto approver might be /0, meaning that they can approve
// all routes available, just not exit nodes.
if tsaddr.IsExitRoute(route) {
if pm.exitSet == nil {
return false
}
if slices.ContainsFunc(node.IPs(), pm.exitSet.Contains) {
return true
}
return false
}
pm.mu.Lock()
defer pm.mu.Unlock()
@ -224,14 +248,6 @@ func (pm *PolicyManager) NodeCanApproveRoute(node *types.Node, route netip.Prefi
// cannot just lookup in the prefix map and have to check
// if there is a "parent" prefix available.
for prefix, approveAddrs := range pm.autoApproveMap {
// We do not want the exit node entry to approve all
// sorts of routes. The logic here is that it would be
// unexpected behaviour to have specific routes approved
// just because the node is allowed to designate itself as
// an exit.
if tsaddr.IsExitRoute(prefix) {
continue
}
// Check if prefix is larger (so containing) and then overlaps
// the route to see if the node can approve a subset of an autoapprover

View file

@ -862,10 +862,11 @@ type AutoApproverPolicy struct {
// resolveAutoApprovers resolves the AutoApprovers to a map of netip.Prefix to netipx.IPSet.
// The resulting map can be used to quickly look up if a node can self-approve a route.
// It is intended for internal use in a PolicyManager.
func resolveAutoApprovers(p *Policy, users types.Users, nodes types.Nodes) (map[netip.Prefix]*netipx.IPSet, error) {
func resolveAutoApprovers(p *Policy, users types.Users, nodes types.Nodes) (map[netip.Prefix]*netipx.IPSet, *netipx.IPSet, error) {
if p == nil {
return nil, nil
return nil, nil, nil
}
var err error
routes := make(map[netip.Prefix]*netipx.IPSetBuilder)
@ -877,7 +878,7 @@ func resolveAutoApprovers(p *Policy, users types.Users, nodes types.Nodes) (map[
aa, ok := autoApprover.(Alias)
if !ok {
// Should never happen
return nil, fmt.Errorf("autoApprover %v is not an Alias", autoApprover)
return nil, nil, fmt.Errorf("autoApprover %v is not an Alias", autoApprover)
}
// If it does not resolve, that means the autoApprover is not associated with any IP addresses.
ips, _ := aa.Resolve(p, users, nodes)
@ -891,7 +892,7 @@ func resolveAutoApprovers(p *Policy, users types.Users, nodes types.Nodes) (map[
aa, ok := autoApprover.(Alias)
if !ok {
// Should never happen
return nil, fmt.Errorf("autoApprover %v is not an Alias", autoApprover)
return nil, nil, fmt.Errorf("autoApprover %v is not an Alias", autoApprover)
}
// If it does not resolve, that means the autoApprover is not associated with any IP addresses.
ips, _ := aa.Resolve(p, users, nodes)
@ -903,22 +904,20 @@ func resolveAutoApprovers(p *Policy, users types.Users, nodes types.Nodes) (map[
for prefix, builder := range routes {
ipSet, err := builder.IPSet()
if err != nil {
return nil, err
return nil, nil, err
}
ret[prefix] = ipSet
}
var exitNodeSet *netipx.IPSet
if len(p.AutoApprovers.ExitNode) > 0 {
exitNodeSet, err := exitNodeSetBuilder.IPSet()
exitNodeSet, err = exitNodeSetBuilder.IPSet()
if err != nil {
return nil, err
return nil, nil, err
}
ret[tsaddr.AllIPv4()] = exitNodeSet
ret[tsaddr.AllIPv6()] = exitNodeSet
}
return ret, nil
return ret, exitNodeSet, nil
}
type ACL struct {

View file

@ -1024,10 +1024,11 @@ func TestResolveAutoApprovers(t *testing.T) {
}
tests := []struct {
name string
policy *Policy
want map[netip.Prefix]*netipx.IPSet
wantErr bool
name string
policy *Policy
want map[netip.Prefix]*netipx.IPSet
wantAllIPRoutes *netipx.IPSet
wantErr bool
}{
{
name: "single-route",
@ -1041,7 +1042,8 @@ func TestResolveAutoApprovers(t *testing.T) {
want: map[netip.Prefix]*netipx.IPSet{
mp("10.0.0.0/24"): mustIPSet("100.64.0.1/32"),
},
wantErr: false,
wantAllIPRoutes: nil,
wantErr: false,
},
{
name: "multiple-routes",
@ -1057,7 +1059,8 @@ func TestResolveAutoApprovers(t *testing.T) {
mp("10.0.0.0/24"): mustIPSet("100.64.0.1/32"),
mp("10.0.1.0/24"): mustIPSet("100.64.0.2/32"),
},
wantErr: false,
wantAllIPRoutes: nil,
wantErr: false,
},
{
name: "exit-node",
@ -1066,11 +1069,9 @@ func TestResolveAutoApprovers(t *testing.T) {
ExitNode: AutoApprovers{ptr.To(Username("user1@"))},
},
},
want: map[netip.Prefix]*netipx.IPSet{
tsaddr.AllIPv4(): mustIPSet("100.64.0.1/32"),
tsaddr.AllIPv6(): mustIPSet("100.64.0.1/32"),
},
wantErr: false,
want: map[netip.Prefix]*netipx.IPSet{},
wantAllIPRoutes: mustIPSet("100.64.0.1/32"),
wantErr: false,
},
{
name: "group-route",
@ -1087,7 +1088,8 @@ func TestResolveAutoApprovers(t *testing.T) {
want: map[netip.Prefix]*netipx.IPSet{
mp("10.0.0.0/24"): mustIPSet("100.64.0.1/32", "100.64.0.2/32"),
},
wantErr: false,
wantAllIPRoutes: nil,
wantErr: false,
},
{
name: "tag-route-and-exit",
@ -1113,10 +1115,9 @@ func TestResolveAutoApprovers(t *testing.T) {
},
want: map[netip.Prefix]*netipx.IPSet{
mp("10.0.1.0/24"): mustIPSet("100.64.0.4/32"),
tsaddr.AllIPv4(): mustIPSet("100.64.0.5/32"),
tsaddr.AllIPv6(): mustIPSet("100.64.0.5/32"),
},
wantErr: false,
wantAllIPRoutes: mustIPSet("100.64.0.5/32"),
wantErr: false,
},
{
name: "mixed-routes-and-exit-nodes",
@ -1135,10 +1136,9 @@ func TestResolveAutoApprovers(t *testing.T) {
want: map[netip.Prefix]*netipx.IPSet{
mp("10.0.0.0/24"): mustIPSet("100.64.0.1/32", "100.64.0.2/32"),
mp("10.0.1.0/24"): mustIPSet("100.64.0.3/32"),
tsaddr.AllIPv4(): mustIPSet("100.64.0.1/32"),
tsaddr.AllIPv6(): mustIPSet("100.64.0.1/32"),
},
wantErr: false,
wantAllIPRoutes: mustIPSet("100.64.0.1/32"),
wantErr: false,
},
}
@ -1146,7 +1146,7 @@ func TestResolveAutoApprovers(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := resolveAutoApprovers(tt.policy, users, nodes)
got, gotAllIPRoutes, err := resolveAutoApprovers(tt.policy, users, nodes)
if (err != nil) != tt.wantErr {
t.Errorf("resolveAutoApprovers() error = %v, wantErr %v", err, tt.wantErr)
return
@ -1154,6 +1154,15 @@ func TestResolveAutoApprovers(t *testing.T) {
if diff := cmp.Diff(tt.want, got, cmps...); diff != "" {
t.Errorf("resolveAutoApprovers() mismatch (-want +got):\n%s", diff)
}
if tt.wantAllIPRoutes != nil {
if gotAllIPRoutes == nil {
t.Error("resolveAutoApprovers() expected non-nil allIPRoutes, got nil")
} else if diff := cmp.Diff(tt.wantAllIPRoutes, gotAllIPRoutes, cmps...); diff != "" {
t.Errorf("resolveAutoApprovers() allIPRoutes mismatch (-want +got):\n%s", diff)
}
} else if gotAllIPRoutes != nil {
t.Error("resolveAutoApprovers() expected nil allIPRoutes, got non-nil")
}
})
}
}