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

Fixes #2568

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

hscontrol/policy/v2/policy.go

@@ -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

hscontrol/policy/v2/types.go

@@ -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 {

hscontrol/policy/v2/types_test.go

@@ -1024,10 +1024,11 @@ func TestResolveAutoApprovers(t *testing.T) {
 	}
 
 	tests := []struct {
-		name    string
+		name            string
-		policy  *Policy
+		policy          *Policy
-		want    map[netip.Prefix]*netipx.IPSet
+		want            map[netip.Prefix]*netipx.IPSet
-		wantErr bool
+		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{
+			want:            map[netip.Prefix]*netipx.IPSet{},
-				tsaddr.AllIPv4(): mustIPSet("100.64.0.1/32"),
+			wantAllIPRoutes: mustIPSet("100.64.0.1/32"),
-				tsaddr.AllIPv6(): mustIPSet("100.64.0.1/32"),
+			wantErr:         false,
-			},
-			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")
+			}
 		})
 	}
 }