policy/v2: add peer visibility compat tests for subnet-to-subnet ACLs

Add TestRoutesCompatPeerVisibility and TestRoutesCompatNoFalsePositivePeers which use the Tailscale SaaS golden file data to validate the CanAccess / ReduceNodes / ReduceRoutes code paths for subnet-to-subnet ACL scenarios. These tests derive expected peer relationships from the golden file captures: if Tailscale SaaS delivers filter rules to a node with SrcIPs overlapping another node's subnet routes, those nodes must be peers. This exercises the CanAccess fix where subnet routes are treated as source identity, ensuring the fix is validated against real SaaS behavior. Also fix buildRoutesUsersAndNodes to auto-assign unique node IDs when the golden files don't provide them, which is required for ReduceNodes to correctly skip self-comparisons. Updates #3157 Updates #3169
2026-07-07 16:40:21 +09:00 · 2026-04-02 16:18:37 +00:00
parent 798c8a9edb
commit 8addacb06a
1 changed files with 448 additions and 6 deletions
@@ -1,14 +1,26 @@
-// This file implements a data-driven test runner for routes compatibility tests.
-// It loads JSON golden files from testdata/routes_results/ROUTES-*.json and
+// This file implements data-driven test runners for routes compatibility tests.
+// It loads HuJSON golden files from testdata/routes_results/ROUTES-*.hujson and
 // compares headscale's route-aware ACL engine output against the expected
 // packet filter rules.
 //
-// Each JSON file contains:
+// Each HuJSON file contains:
 //   - A full policy (groups, tagOwners, hosts, acls)
 //   - A topology section with nodes, including routable_ips and approved_routes
 //   - Expected packet_filter_rules per node
 //
-// Test data source: testdata/routes_results/ROUTES-*.json
+// Two test runners use this data:
+//
+//   - TestRoutesCompat: validates filter rule compilation (compileFilterRulesForNode
+//     + ReduceFilterRules) against golden file captures.
+//
+//   - TestRoutesCompatPeerVisibility: validates peer visibility (CanAccess /
+//     ReduceNodes) for the subnet-to-subnet scenarios (f10–f15). These tests
+//     derive expected peer relationships from the golden file captures: if
+//     Tailscale SaaS delivers filter rules to a node, then the subnet routers
+//     referenced in those rules must be visible as peers. This exercises the
+//     CanAccess fix from issue #3157.
+//
+// Test data source: testdata/routes_results/ROUTES-*.hujson
 // Original source: Tailscale SaaS captures + headscale-generated expansions

 package v2
@@ -18,14 +30,18 @@ import (
 	"net/netip"
 	"os"
 	"path/filepath"
+	"slices"
+	"sort"
 	"testing"

 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/juanfont/headscale/hscontrol/policy/policyutil"
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tailscale/hujson"
+	"go4.org/netipx"
 	"gorm.io/gorm"
 	"tailscale.com/tailcfg"
 )
@@ -113,12 +129,22 @@ func buildRoutesUsersAndNodes(
 		}
 	}

-	// Build nodes
+	// Build nodes.
+	// Auto-assign unique IDs when the JSON topology does not provide
+	// them (id defaults to 0). Unique IDs are required by ReduceNodes /
+	// BuildPeerMap which skip peers by comparing node.ID.
 	nodes := make(types.Nodes, 0, len(topo.Nodes))
+	autoID := 1

 	for _, nodeDef := range topo.Nodes {
+		nodeID := nodeDef.ID
+		if nodeID == 0 {
+			nodeID = autoID
+			autoID++
+		}
+
 		node := &types.Node{
-			ID:        types.NodeID(nodeDef.ID), //nolint:gosec
+			ID:        types.NodeID(nodeID), //nolint:gosec
 			GivenName: nodeDef.Hostname,
 			IPv4:      ptrAddr(nodeDef.IPv4),
 			IPv6:      ptrAddr(nodeDef.IPv6),
@@ -186,6 +212,18 @@ func buildRoutesUsersAndNodes(
 // routesSkipReasons documents WHY tests are expected to fail.
 var routesSkipReasons = map[string]string{}

+// subnetToSubnetFiles lists the golden files that test subnet-to-subnet
+// ACL scenarios. These are the scenarios where the fix for issue #3157
+// (CanAccess considering subnet routes as source identity) is critical.
+var subnetToSubnetFiles = []string{
+	"ROUTES-f10_subnet_to_subnet_issue3157",
+	"ROUTES-f11_subnet_to_subnet_bidirectional",
+	"ROUTES-f12_subnet_to_subnet_host_aliases",
+	"ROUTES-f13_subnet_to_subnet_disjoint",
+	"ROUTES-f14_subnet_to_subnet_overlapping_one_router",
+	"ROUTES-f15_subnet_to_subnet_cross_routers",
+}
+
 // TestRoutesCompat is a data-driven test that loads all ROUTES-*.json test
 // files and compares headscale's route-aware ACL engine output against the
 // expected behavior.
@@ -311,3 +349,407 @@ func TestRoutesCompat(t *testing.T) {
 		})
 	}
 }
+
+// derivePeerPairsFromCaptures builds the set of expected peer pairs from
+// golden file captures. For each node that receives filter rules from
+// Tailscale SaaS, the SrcIPs identify subnets whose traffic will arrive
+// at this node. Any other node whose approved subnet routes overlap
+// those SrcIPs must be peered with this node — otherwise the traffic
+// cannot flow.
+//
+// Returns a set of unordered node-name pairs that must be peers.
+func derivePeerPairsFromCaptures(
+	t *testing.T,
+	tf routesTestFile,
+	nodes types.Nodes,
+) map[[2]string]bool {
+	t.Helper()
+
+	pairs := make(map[[2]string]bool)
+
+	for dstNodeName, capture := range tf.Captures {
+		captureIsNull := len(capture.PacketFilterRules) == 0 ||
+			string(capture.PacketFilterRules) == "null"
+		if captureIsNull {
+			continue
+		}
+
+		var rules []tailcfg.FilterRule
+
+		err := json.Unmarshal(capture.PacketFilterRules, &rules)
+		require.NoError(t, err,
+			"%s/%s: failed to unmarshal capture rules",
+			tf.TestID, dstNodeName,
+		)
+
+		// Build an IPSet of all SrcIPs from the capture's filter rules.
+		var srcBuilder netipx.IPSetBuilder
+
+		for _, rule := range rules {
+			for _, srcIP := range rule.SrcIPs {
+				prefix, err := netip.ParsePrefix(srcIP)
+				if err != nil {
+					// Single IP like "100.x.y.z" — try as host address.
+					addr, err2 := netip.ParseAddr(srcIP)
+					require.NoError(t, err2,
+						"%s/%s: cannot parse SrcIP %q",
+						tf.TestID, dstNodeName, srcIP,
+					)
+
+					srcBuilder.Add(addr)
+
+					continue
+				}
+
+				srcBuilder.AddPrefix(prefix)
+			}
+		}
+
+		srcSet, err := srcBuilder.IPSet()
+		require.NoError(t, err)
+
+		// Find all nodes whose SubnetRoutes overlap srcSet.
+		for _, node := range nodes {
+			if node.GivenName == dstNodeName {
+				continue
+			}
+
+			if slices.ContainsFunc(node.SubnetRoutes(), srcSet.OverlapsPrefix) {
+				pair := orderedPair(dstNodeName, node.GivenName)
+				pairs[pair] = true
+			}
+		}
+	}
+
+	return pairs
+}
+
+// orderedPair returns a canonical [2]string with the names sorted
+// so that (A,B) and (B,A) map to the same key.
+func orderedPair(a, b string) [2]string {
+	if a > b {
+		return [2]string{b, a}
+	}
+
+	return [2]string{a, b}
+}
+
+// TestRoutesCompatPeerVisibility is a data-driven test that validates peer
+// visibility (CanAccess) for subnet-to-subnet ACL scenarios using the same
+// golden file data captured from Tailscale SaaS.
+//
+// Unlike TestRoutesCompat which tests filter rule compilation
+// (compileFilterRulesForNode + ReduceFilterRules), this test exercises the
+// CanAccess code path that determines whether two nodes should see each
+// other as peers. This is the code path fixed in issue #3157: before the
+// fix, CanAccess only checked node IPs against matcher sources, missing
+// the case where a node's approved subnet routes overlap the source set.
+//
+// The test derives expected peer pairs from the golden file captures:
+// if Tailscale SaaS delivers filter rules to node X with SrcIPs
+// overlapping node Y's subnet routes, then Y must be able to CanAccess X
+// (Y acts as source identity for its advertised subnets).
+func TestRoutesCompatPeerVisibility(t *testing.T) {
+	t.Parallel()
+
+	for _, testID := range subnetToSubnetFiles {
+		file := filepath.Join(
+			"testdata", "routes_results", testID+".hujson",
+		)
+		tf := loadRoutesTestFile(t, file)
+
+		t.Run(tf.TestID, func(t *testing.T) {
+			t.Parallel()
+
+			// Build topology from JSON.
+			users, nodes := buildRoutesUsersAndNodes(t, tf.Topology)
+
+			// Convert Tailscale SaaS user emails to headscale format.
+			policyJSON := convertPolicyUserEmails(tf.Input.FullPolicy)
+
+			// Create a PolicyManager — this compiles the global filter
+			// rules and produces matchers used for peer visibility.
+			pm, err := NewPolicyManager(
+				policyJSON, users, nodes.ViewSlice(),
+			)
+			require.NoError(t, err,
+				"%s: failed to create policy manager", tf.TestID,
+			)
+
+			// Derive expected peer pairs from golden file captures.
+			wantPairs := derivePeerPairsFromCaptures(t, tf, nodes)
+			require.NotEmpty(t, wantPairs,
+				"%s: no peer pairs derived — golden file has no "+
+					"subnet-to-subnet relationships to test",
+				tf.TestID,
+			)
+
+			t.Run("CanAccess", func(t *testing.T) {
+				// For each expected pair, verify that at least one
+				// direction of CanAccess returns true.
+				for pair := range wantPairs {
+					nodeA := findNodeByGivenName(nodes, pair[0])
+					nodeB := findNodeByGivenName(nodes, pair[1])
+					require.NotNilf(t, nodeA,
+						"node %s not found", pair[0],
+					)
+					require.NotNilf(t, nodeB,
+						"node %s not found", pair[1],
+					)
+
+					// Get matchers — these are the unreduced global
+					// matchers used for peer relationship determination.
+					matchers, err := pm.MatchersForNode(nodeA.View())
+					require.NoError(t, err)
+
+					canAccess := nodeA.View().CanAccess(
+						matchers, nodeB.View(),
+					) || nodeB.View().CanAccess(
+						matchers, nodeA.View(),
+					)
+					assert.Truef(t, canAccess,
+						"%s: %s and %s should be peers "+
+							"(subnet routers must see each other "+
+							"when ACL references their subnets)",
+						tf.TestID, pair[0], pair[1],
+					)
+				}
+			})
+
+			t.Run("ReduceNodes", func(t *testing.T) {
+				// Build the complete peer map using CanAccess and
+				// verify it contains all expected pairs.
+				// This is equivalent to policy.ReduceNodes but
+				// inlined to avoid an import cycle with the policy
+				// package.
+				for _, node := range nodes {
+					matchers, err := pm.MatchersForNode(
+						node.View(),
+					)
+					require.NoError(t, err)
+
+					var peerNames []string
+
+					for _, peer := range nodes {
+						if peer.ID == node.ID {
+							continue
+						}
+
+						if node.View().CanAccess(
+							matchers, peer.View(),
+						) || peer.View().CanAccess(
+							matchers, node.View(),
+						) {
+							peerNames = append(
+								peerNames, peer.GivenName,
+							)
+						}
+					}
+
+					// Collect expected peers for this node.
+					var wantPeers []string
+
+					for pair := range wantPairs {
+						if pair[0] == node.GivenName {
+							wantPeers = append(
+								wantPeers, pair[1],
+							)
+						} else if pair[1] == node.GivenName {
+							wantPeers = append(
+								wantPeers, pair[0],
+							)
+						}
+					}
+
+					if len(wantPeers) == 0 {
+						continue
+					}
+
+					sort.Strings(peerNames)
+					sort.Strings(wantPeers)
+
+					for _, wantPeer := range wantPeers {
+						assert.Containsf(t, peerNames, wantPeer,
+							"%s: node %s should have peer %s "+
+								"in ReduceNodes result",
+							tf.TestID, node.GivenName, wantPeer,
+						)
+					}
+				}
+			})
+
+			t.Run("ReduceRoutes", func(t *testing.T) {
+				// For each node that has captures with filter rules,
+				// verify that CanAccessRoute returns true for the
+				// destination routes referenced in those rules, when
+				// called from a node whose subnet routes overlap the
+				// source CIDRs.
+				for dstNodeName, capture := range tf.Captures {
+					captureIsNull := len(
+						capture.PacketFilterRules,
+					) == 0 ||
+						string(
+							capture.PacketFilterRules,
+						) == "null"
+					if captureIsNull {
+						continue
+					}
+
+					var rules []tailcfg.FilterRule
+
+					err := json.Unmarshal(
+						capture.PacketFilterRules, &rules,
+					)
+					require.NoError(t, err)
+
+					// Extract destination prefixes from the rules.
+					var dstPrefixes []netip.Prefix
+
+					for _, rule := range rules {
+						for _, dp := range rule.DstPorts {
+							prefix, err := netip.ParsePrefix(
+								dp.IP,
+							)
+							if err != nil {
+								continue
+							}
+
+							dstPrefixes = append(
+								dstPrefixes, prefix,
+							)
+						}
+					}
+
+					// For each source node (whose subnets overlap
+					// the SrcIPs), verify it can access the dst
+					// routes.
+					for pair := range wantPairs {
+						var srcNodeName string
+
+						switch {
+						case pair[0] == dstNodeName:
+							srcNodeName = pair[1]
+						case pair[1] == dstNodeName:
+							srcNodeName = pair[0]
+						default:
+							continue
+						}
+
+						srcNode := findNodeByGivenName(
+							nodes, srcNodeName,
+						)
+						require.NotNil(t, srcNode)
+
+						matchers, err := pm.MatchersForNode(
+							srcNode.View(),
+						)
+						require.NoError(t, err)
+
+						for _, route := range dstPrefixes {
+							canAccess := srcNode.View().CanAccessRoute(
+								matchers, route,
+							)
+							assert.Truef(t, canAccess,
+								"%s: node %s (routing %v) "+
+									"should be able to access "+
+									"route %s on node %s",
+								tf.TestID, srcNodeName,
+								srcNode.SubnetRoutes(),
+								route, dstNodeName,
+							)
+						}
+					}
+				}
+			})
+		})
+	}
+}
+
+// TestRoutesCompatNoFalsePositivePeers verifies that nodes which do NOT
+// have subnet routes overlapping an ACL's source or destination CIDRs
+// are NOT incorrectly peered with subnet routers.
+//
+// This is the negative counterpart to TestRoutesCompatPeerVisibility:
+// while that test verifies subnet routers CAN see each other, this test
+// verifies that unrelated nodes (tagged-server, user1, etc.) are NOT
+// made peers of subnet routers solely because of subnet-to-subnet ACLs.
+func TestRoutesCompatNoFalsePositivePeers(t *testing.T) {
+	t.Parallel()
+
+	// nodesWithoutRoutes lists nodes that have no subnet routes and whose
+	// IPs don't appear in any subnet-to-subnet ACL. They should never be
+	// peers of subnet routers through these ACLs alone.
+	nodesWithoutRoutes := []string{
+		"tagged-server",
+		"tagged-prod",
+		"tagged-client",
+		"user1",
+		"user-kris",
+		"user-mon",
+	}
+
+	for _, testID := range subnetToSubnetFiles {
+		file := filepath.Join(
+			"testdata", "routes_results", testID+".hujson",
+		)
+		tf := loadRoutesTestFile(t, file)
+
+		t.Run(tf.TestID, func(t *testing.T) {
+			t.Parallel()
+
+			users, nodes := buildRoutesUsersAndNodes(t, tf.Topology)
+			policyJSON := convertPolicyUserEmails(tf.Input.FullPolicy)
+
+			pm, err := NewPolicyManager(
+				policyJSON, users, nodes.ViewSlice(),
+			)
+			require.NoError(t, err)
+
+			// Collect the set of nodes that participate in the ACL
+			// (have non-null captures).
+			routerNodes := make(map[string]bool)
+
+			for nodeName, capture := range tf.Captures {
+				captureIsNull := len(capture.PacketFilterRules) == 0 ||
+					string(capture.PacketFilterRules) == "null"
+				if !captureIsNull {
+					routerNodes[nodeName] = true
+				}
+			}
+
+			for _, nonRouterName := range nodesWithoutRoutes {
+				nonRouter := findNodeByGivenName(
+					nodes, nonRouterName,
+				)
+				if nonRouter == nil {
+					continue
+				}
+
+				matchers, err := pm.MatchersForNode(
+					nonRouter.View(),
+				)
+				require.NoError(t, err)
+
+				for routerName := range routerNodes {
+					router := findNodeByGivenName(
+						nodes, routerName,
+					)
+					require.NotNil(t, router)
+
+					canAccess := nonRouter.View().CanAccess(
+						matchers, router.View(),
+					) || router.View().CanAccess(
+						matchers, nonRouter.View(),
+					)
+
+					assert.Falsef(t, canAccess,
+						"%s: non-router node %s should NOT "+
+							"be a peer of subnet router %s "+
+							"via subnet-to-subnet ACL alone",
+						tf.TestID, nonRouterName, routerName,
+					)
+				}
+			}
+		})
+	}
+}