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

Updates #3157

hscontrol/policy/v2/tailscale_routes_data_compat_test.go

@@ -1,14 +1,26 @@
-// This file implements a data-driven test runner for routes compatibility tests.
+// This file implements data-driven test runners for routes compatibility tests.
-// It loads JSON golden files from testdata/routes_results/ROUTES-*.json and
+// 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,
+					)
+				}
+			}
+		})
+	}
+}