From c36cedc32f7666a0ef5d62fa89987179bcf73147 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 28 Mar 2026 12:48:19 +0000 Subject: [PATCH] policy/v2: fix via grants in BuildPeerMap, MatchersForNode, and ViaRoutesForPeer Use per-node compilation path for via grants in BuildPeerMap and MatchersForNode to ensure via-granted nodes appear in peer maps. Fix ViaRoutesForPeer golden test route inference to correctly resolve via grant effects. Updates #2180 --- hscontrol/policy/v2/policy.go | 43 ++-- hscontrol/servertest/via_compat_test.go | 290 +++++++++++++++++------- hscontrol/state/state.go | 6 +- 3 files changed, 244 insertions(+), 95 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index e4b469b5..b4689fb2 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -379,8 +379,10 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ pm.mu.Lock() defer pm.mu.Unlock() - // If we have a global filter, use it for all nodes (normal case) - if !pm.usesAutogroupSelf { + // If we have a global filter, use it for all nodes (normal case). + // Via grants require the per-node path because the global filter + // skips via grants (compileFilterRules: if len(grant.Via) > 0 { continue }). + if !pm.needsPerNodeFilter { ret := make(map[types.NodeID][]types.NodeView, nodes.Len()) // Build the map of all peers according to the matchers. @@ -403,7 +405,7 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ return ret } - // For autogroup:self (empty global filter), build per-node peer relationships + // For autogroup:self or via grants, build per-node peer relationships ret := make(map[types.NodeID][]types.NodeView, nodes.Len()) // Pre-compute per-node matchers using unreduced compiled rules @@ -438,15 +440,21 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ nodeJ := nodes.At(j) matchersJ, hasFilterJ := nodeMatchers[nodeJ.ID()] - // If either node can access the other, both should see each other as peers. - // This symmetric visibility is required for proper network operation: - // - Admin with *:* rule should see tagged servers (even if servers - // can't access admin) - // - Servers should see admin so they can respond to admin's connections + // Check all access directions for symmetric peer visibility. + // For via grants, filter rules exist on the via-designated node + // (e.g., router-a) with sources being the client (group-a). + // We need to check BOTH: + // 1. nodeI.CanAccess(matchersI, nodeJ) — can nodeI reach nodeJ? + // 2. nodeJ.CanAccess(matchersI, nodeI) — can nodeJ reach nodeI + // using nodeI's matchers? (reverse direction: the matchers + // on the via node accept traffic FROM the source) + // Same for matchersJ in both directions. canIAccessJ := hasFilterI && nodeI.CanAccess(matchersI, nodeJ) canJAccessI := hasFilterJ && nodeJ.CanAccess(matchersJ, nodeI) + canJReachI := hasFilterI && nodeJ.CanAccess(matchersI, nodeI) + canIReachJ := hasFilterJ && nodeI.CanAccess(matchersJ, nodeJ) - if canIAccessJ || canJAccessI { + if canIAccessJ || canJAccessI || canJReachI || canIReachJ { ret[nodeI.ID()] = append(ret[nodeI.ID()], nodeJ) ret[nodeJ.ID()] = append(ret[nodeJ.ID()], nodeI) } @@ -556,12 +564,14 @@ func (pm *PolicyManager) MatchersForNode(node types.NodeView) ([]matcher.Match, pm.mu.Lock() defer pm.mu.Unlock() - // For global policies, return the shared global matchers - if !pm.usesAutogroupSelf { + // For global policies, return the shared global matchers. + // Via grants require per-node matchers because the global matchers + // are empty for via-grant-only policies. + if !pm.needsPerNodeFilter { return pm.matchers, nil } - // For autogroup:self, get unreduced compiled rules and create matchers + // For autogroup:self or via grants, get unreduced compiled rules and create matchers compiledRules, err := pm.compileFilterRulesForNodeLocked(node) if err != nil { return nil, err @@ -889,7 +899,6 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via // Collect destination prefixes that the peer actually advertises. peerSubnetRoutes := peer.SubnetRoutes() - peerExitRoutes := peer.ExitRoutes() var matchedPrefixes []netip.Prefix @@ -901,9 +910,11 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via matchedPrefixes = append(matchedPrefixes, dstPrefix) } case *AutoGroup: - if d.Is(AutoGroupInternet) && len(peerExitRoutes) > 0 { - matchedPrefixes = append(matchedPrefixes, peerExitRoutes...) - } + // autogroup:internet via grants do NOT affect AllowedIPs or + // route steering for exit nodes. Tailscale SaaS handles exit + // traffic forwarding through the client's exit node selection + // mechanism, not through AllowedIPs. Verified by golden + // captures GRANT-V14 through GRANT-V36. } } diff --git a/hscontrol/servertest/via_compat_test.go b/hscontrol/servertest/via_compat_test.go index 0bc624f6..fba2de7b 100644 --- a/hscontrol/servertest/via_compat_test.go +++ b/hscontrol/servertest/via_compat_test.go @@ -48,25 +48,15 @@ type goldenCapture struct { } type goldenNetmap struct { - SelfNode json.RawMessage `json:"SelfNode"` Peers []goldenPeer `json:"Peers"` - PacketFilter json.RawMessage `json:"PacketFilter"` PacketFilterRules json.RawMessage `json:"PacketFilterRules"` - DNS json.RawMessage `json:"DNS"` - SSHPolicy json.RawMessage `json:"SSHPolicy"` - Domain string `json:"Domain"` - UserProfiles json.RawMessage `json:"UserProfiles"` } type goldenPeer struct { - Name string `json:"Name"` - Addresses []string `json:"Addresses"` - AllowedIPs []string `json:"AllowedIPs"` - PrimaryRoutes []string `json:"PrimaryRoutes"` - Tags []string `json:"Tags"` - ExitNodeOption *bool `json:"ExitNodeOption"` - Online *bool `json:"Online"` - Cap int `json:"Cap"` + Name string `json:"Name"` + AllowedIPs []string `json:"AllowedIPs"` + PrimaryRoutes []string `json:"PrimaryRoutes"` + Tags []string `json:"Tags"` } type goldenWhois struct { @@ -86,13 +76,11 @@ var viaCompatTests = []struct { } // TestViaGrantMapCompat loads golden captures from Tailscale SaaS and -// compares headscale's full MapResponse against the captured netmap. +// compares headscale's MapResponse structure against the captured netmap. // -// For each viewing node, it compares: -// - Peer set (which peers are visible) -// - Per-peer AllowedIPs (via steering changes which routes appear on which peer) -// - Per-peer PrimaryRoutes (which node is primary for a subnet) -// - PacketFilter rule count +// The comparison is IP-independent: it validates peer visibility, route +// prefixes in AllowedIPs, and PrimaryRoutes — not literal Tailscale IP +// addresses which differ between Tailscale SaaS and headscale allocation. func TestViaGrantMapCompat(t *testing.T) { t.Parallel() @@ -119,9 +107,7 @@ func TestViaGrantMapCompat(t *testing.T) { } } -// taggedNodes are the nodes we create in the servertest. User-owned nodes -// are excluded because the servertest uses a single user for all tagged -// nodes, which doesn't map to the multi-user Tailscale topology. +// taggedNodes are the nodes we create in the servertest. var taggedNodes = []string{ "exit-a", "exit-b", "exit-node", "group-a-client", "group-b-client", @@ -169,46 +155,20 @@ func runViaMapCompat(t *testing.T, gf goldenFile) { require.NotEmpty(t, clients, "no relevant nodes created") - // Compute expected peer counts from golden netmap. - expectedPeerCounts := map[string]int{} + // Determine which routes each node should advertise. If the golden + // topology has explicit advertised_routes, use those. Otherwise infer + // from the policy's autoApprovers.routes: if a node's tags match an + // approver tag for a route prefix, the node should advertise it. + nodeRoutes := inferNodeRoutes(gf) - for viewerName := range clients { - capture := gf.Captures[viewerName] - if capture.Netmap != nil { - // Count peers from golden netmap that are in our client set. - count := 0 - - for _, peer := range capture.Netmap.Peers { - peerName := extractHostname(peer.Name) - if _, isOurs := clients[peerName]; isOurs { - count++ - } - } - - expectedPeerCounts[viewerName] = count - } - } - - // Wait for expected peers. + // Advertise and approve routes FIRST. Via grants depend on routes + // being advertised for compileViaGrant to produce filter rules. for name, c := range clients { - expected := expectedPeerCounts[name] - if expected > 0 { - c.WaitForPeers(t, expected, 30*time.Second) - } - } - - // Advertise and approve routes. - for name, c := range clients { - topoNode := gf.Topology.Nodes[name] - if len(topoNode.AdvertisedRoutes) == 0 { + routes := nodeRoutes[name] + if len(routes) == 0 { continue } - var routes []netip.Prefix - for _, r := range topoNode.AdvertisedRoutes { - routes = append(routes, netip.MustParsePrefix(r)) - } - c.Direct().SetHostinfo(&tailcfg.Hostinfo{ BackendLogID: "servertest-" + name, Hostname: name, @@ -226,9 +186,31 @@ func runViaMapCompat(t *testing.T, gf goldenFile) { srv.App.Change(routeChange) } - // Wait for route propagation. - for _, c := range clients { - c.WaitForCondition(t, "routes settled", 15*time.Second, + // Wait for peers based on golden netmap expected counts. + for viewerName, c := range clients { + capture := gf.Captures[viewerName] + if capture.Netmap == nil { + continue + } + + expected := 0 + + for _, peer := range capture.Netmap.Peers { + peerName := extractHostname(peer.Name) + if _, isOurs := clients[peerName]; isOurs { + expected++ + } + } + + if expected > 0 { + c.WaitForPeers(t, expected, 30*time.Second) + } + } + + // Ensure all nodes have received at least one MapResponse, + // including nodes with 0 expected peers that skipped WaitForPeers. + for name, c := range clients { + c.WaitForCondition(t, name+" initial netmap", 15*time.Second, func(nm *netmap.NetworkMap) bool { return nm != nil }) @@ -245,14 +227,20 @@ func runViaMapCompat(t *testing.T, gf goldenFile) { nm := c.Netmap() require.NotNil(t, nm, "netmap is nil") - compareNetmap(t, viewerName, nm, capture.Netmap, clients) + compareNetmap(t, nm, capture.Netmap, clients) }) } } +// compareNetmap compares the headscale MapResponse against the golden +// netmap data in an IP-independent way. It validates: +// - Peer visibility (which peers are present, by hostname) +// - Route prefixes in AllowedIPs (non-Tailscale-IP entries like 10.44.0.0/16) +// - Number of Tailscale IPs per peer (should be 2: one v4 + one v6) +// - PrimaryRoutes per peer +// - PacketFilter rule count func compareNetmap( t *testing.T, - _ string, // viewerName unused but kept for signature clarity got *netmap.NetworkMap, want *goldenNetmap, clients map[string]*servertest.TestClient, @@ -293,12 +281,22 @@ func compareNetmap( continue } - var aips []string + // Separate AllowedIPs into Tailscale IPs (node addresses) + // and route prefixes (subnets, exit routes). + var tsIPs []netip.Prefix + + var routePrefixes []string + for i := range peer.AllowedIPs().Len() { - aips = append(aips, peer.AllowedIPs().At(i).String()) + prefix := peer.AllowedIPs().At(i) + if isTailscaleIP(prefix) { + tsIPs = append(tsIPs, prefix) + } else { + routePrefixes = append(routePrefixes, prefix.String()) + } } - slices.Sort(aips) + slices.Sort(routePrefixes) var proutes []string for i := range peer.PrimaryRoutes().Len() { @@ -308,28 +306,55 @@ func compareNetmap( slices.Sort(proutes) gotPeers[name] = peerSummary{ - AllowedIPs: aips, + TailscaleIPs: tsIPs, + RoutePrefixes: routePrefixes, PrimaryRoutes: proutes, } } - // Compare peer visibility. + // Compare peer visibility: golden peers must be present. for name, wantPeer := range wantPeers { gotPeer, visible := gotPeers[name] if !visible { - t.Errorf("peer %s: visible in Tailscale SaaS (AllowedIPs=%v), missing in headscale", - name, wantPeer.AllowedIPs) + wantRoutes := extractRoutePrefixes(wantPeer.AllowedIPs) + t.Errorf("peer %s: visible in Tailscale SaaS (routes=%v), missing in headscale", + name, wantRoutes) continue } - // Compare AllowedIPs. - wantAIPs := make([]string, len(wantPeer.AllowedIPs)) - copy(wantAIPs, wantPeer.AllowedIPs) - slices.Sort(wantAIPs) + // Compare route prefixes in AllowedIPs (IP-independent). + wantRoutes := extractRoutePrefixes(wantPeer.AllowedIPs) + slices.Sort(wantRoutes) - assert.Equalf(t, wantAIPs, gotPeer.AllowedIPs, - "peer %s: AllowedIPs mismatch", name) + assert.Equalf(t, wantRoutes, gotPeer.RoutePrefixes, + "peer %s: route prefixes in AllowedIPs mismatch", name) + + // Tailscale IPs: count should match, and they must belong to + // this peer (not some other node's IPs). + wantTSIPCount := countTailscaleIPs(wantPeer.AllowedIPs) + + assert.Lenf(t, gotPeer.TailscaleIPs, wantTSIPCount, + "peer %s: Tailscale IP count mismatch", name) + + // Verify the Tailscale IPs are actually this peer's addresses. + if peerClient, ok := clients[name]; ok { + peerNM := peerClient.Netmap() + if peerNM != nil && peerNM.SelfNode.Valid() { + peerAddrs := map[netip.Prefix]bool{} + + addrs := peerNM.SelfNode.Addresses() + for i := range addrs.Len() { + peerAddrs[addrs.At(i)] = true + } + + for _, tsIP := range gotPeer.TailscaleIPs { + assert.Truef(t, peerAddrs[tsIP], + "peer %s: AllowedIPs contains Tailscale IP %s which is NOT this peer's address (peer has %v)", + name, tsIP, peerAddrs) + } + } + } // Compare PrimaryRoutes. assert.ElementsMatchf(t, wantPeer.PrimaryRoutes, gotPeer.PrimaryRoutes, @@ -355,8 +380,117 @@ func compareNetmap( } type peerSummary struct { - AllowedIPs []string - PrimaryRoutes []string + TailscaleIPs []netip.Prefix // Tailscale address entries from AllowedIPs + RoutePrefixes []string // non-Tailscale-IP AllowedIPs (sorted) + PrimaryRoutes []string // sorted +} + +// isTailscaleIP returns true if the prefix is a single-host Tailscale +// address (/32 for IPv4 in CGNAT range, /128 for IPv6 in Tailscale ULA). +func isTailscaleIP(prefix netip.Prefix) bool { + addr := prefix.Addr() + + if addr.Is4() && prefix.Bits() == 32 { + // CGNAT range 100.64.0.0/10 + return addr.As4()[0] == 100 && (addr.As4()[1]&0xC0) == 64 + } + + if addr.Is6() && prefix.Bits() == 128 { + // Tailscale ULA fd7a:115c:a1e0::/48 + b := addr.As16() + + return b[0] == 0xfd && b[1] == 0x7a && b[2] == 0x11 && b[3] == 0x5c //nolint:gosec // As16 returns [16]byte, indexing [0..3] is safe + } + + return false +} + +// extractRoutePrefixes returns the non-Tailscale-IP entries from an +// AllowedIPs list (subnet routes, exit routes, etc.). +func extractRoutePrefixes(allowedIPs []string) []string { + var routes []string + + for _, aip := range allowedIPs { + prefix, err := netip.ParsePrefix(aip) + if err != nil { + continue + } + + if !isTailscaleIP(prefix) { + routes = append(routes, aip) + } + } + + return routes +} + +// countTailscaleIPs returns the number of Tailscale IP entries in an +// AllowedIPs list. +func countTailscaleIPs(allowedIPs []string) int { + count := 0 + + for _, aip := range allowedIPs { + prefix, err := netip.ParsePrefix(aip) + if err != nil { + continue + } + + if isTailscaleIP(prefix) { + count++ + } + } + + return count +} + +// inferNodeRoutes determines which routes each node should advertise. +// If the golden topology has explicit advertised_routes, those are used. +// Otherwise, routes are inferred from the golden netmap data: if a node +// appears as a peer with route prefixes in AllowedIPs, it should +// advertise those routes. +func inferNodeRoutes(gf goldenFile) map[string][]netip.Prefix { + result := map[string][]netip.Prefix{} + + // First use explicit advertised_routes from topology. + for name, node := range gf.Topology.Nodes { + for _, r := range node.AdvertisedRoutes { + result[name] = append(result[name], netip.MustParsePrefix(r)) + } + } + + // If any node already has routes, the topology is populated — use as-is. + for _, routes := range result { + if len(routes) > 0 { + return result + } + } + + // Infer from the golden netmap: scan all captures for peers with + // route prefixes in AllowedIPs. If node X appears as a peer with + // route prefix 10.44.0.0/16, then X should advertise that route. + for _, capture := range gf.Captures { + if capture.Netmap == nil { + continue + } + + for _, peer := range capture.Netmap.Peers { + peerName := extractHostname(peer.Name) + routes := extractRoutePrefixes(peer.AllowedIPs) + + for _, r := range routes { + prefix, err := netip.ParsePrefix(r) + if err != nil { + continue + } + + if !slices.Contains(result[peerName], prefix) { + result[peerName] = append(result[peerName], prefix) + } + } + } + } + + return result } // extractHostname extracts the hostname from a Tailscale FQDN like diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 3e3f32ef..ee868538 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -693,7 +693,11 @@ func (s *State) ListPeers(nodeID types.NodeID, peerIDs ...types.NodeID) views.Sl return s.nodeStore.ListPeers(nodeID) } - // For specific peerIDs, filter from all nodes + // For specific peerIDs, filter from all nodes. + // This path is used for incremental updates (NodeAdded, NodeChanged) + // where the caller already knows which peer IDs are involved. + // The peer visibility filtering happens in the mapper's buildTailPeers + // via MatchersForNode/ReduceNodes. allNodes := s.nodeStore.ListNodes() nodeIDSet := make(map[types.NodeID]struct{}, len(peerIDs))