From e5fcd01ee60a7896c34beb7551992ae96e25e04d Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 18 May 2026 09:42:28 +0000 Subject: [PATCH] policy/v2: match via-grant destinations by prefix overlap slices.Contains required exact equality between grant dst and the advertised subnet route. Any non-identical pair was rejected, so a via grant with broader (or narrower) dst emitted no filter rule and added no route to the viewer's AllowedIPs. Tailscale SaaS uses containment in either direction. Switch to slices.ContainsFunc(routes, dst.Overlaps) for filter rule emission (keep dst literal in DstPorts), and append overlapping advertised routes to ViaRoutesForPeer.Include / Exclude. Rewrite the multi-router HA election and regular-grant overlap detection to key off the matched routes rather than the dst. Resolve *Host aliases to *Prefix once in compileOneViaGrant and at the top of ViaRoutesForPeer so the switch arms reach them. Fixes #3267 --- hscontrol/policy/v2/compiled.go | 114 ++++++++++++++------ hscontrol/policy/v2/issue_3267_test.go | 120 +++++++++++++++++++++ hscontrol/policy/v2/policy.go | 123 +++++++++++---------- hscontrol/policy/v2/policy_test.go | 141 +++++++++++++++++++++++++ 4 files changed, 402 insertions(+), 96 deletions(-) create mode 100644 hscontrol/policy/v2/issue_3267_test.go diff --git a/hscontrol/policy/v2/compiled.go b/hscontrol/policy/v2/compiled.go index 04aca941..89824ee1 100644 --- a/hscontrol/policy/v2/compiled.go +++ b/hscontrol/policy/v2/compiled.go @@ -66,12 +66,52 @@ type selfGrantData struct { } // viaGrantData holds data needed for per-node via-grant compilation. -// Sources are already resolved into srcIPStrings. +// Sources are already resolved into srcIPStrings; destinations are +// pre-resolved into prefixes plus a flag for autogroup:internet, which +// the consumers handle separately because its gate is per-node +// (IsExitNode()) rather than per-prefix overlap. type viaGrantData struct { - viaTags []Tag - destinations Aliases - internetProtocols []ProtocolPort - srcIPStrings []string + viaTags []Tag + resolvedDsts []netip.Prefix + hasAutoGroupInternet bool + internetProtocols []ProtocolPort + srcIPStrings []string +} + +// resolveViaDestinations splits a via grant's destinations into the +// flat list of IP prefixes they resolve to plus a flag for +// autogroup:internet. Every alias kind goes through Alias.Resolve so +// adding a new alias type to the policy parser does not silently +// disappear from the via path. Non-IP alias kinds (tag, user, group, +// wildcard) resolve to /32 host IPs that never overlap with subnet +// route advertisements and therefore contribute nothing here. +func resolveViaDestinations( + pol *Policy, + users types.Users, + nodes views.Slice[types.NodeView], + dsts Aliases, +) ([]netip.Prefix, bool) { + var ( + prefixes []netip.Prefix + hasAutoGroupInternet bool + ) + + for _, d := range dsts { + if ag, ok := d.(*AutoGroup); ok && ag.Is(AutoGroupInternet) { + hasAutoGroupInternet = true + + continue + } + + ips, err := d.Resolve(pol, users, nodes) + if err != nil || ips == nil { + continue + } + + prefixes = append(prefixes, ips.Prefixes()...) + } + + return prefixes, hasAutoGroupInternet } // userNodeIndex maps user IDs to their untagged nodes. Built once per @@ -343,12 +383,17 @@ func (pol *Policy) compileOneViaGrant( hasWildcard := sourcesHaveWildcard(grant.Sources) hasDangerAll := sourcesHaveDangerAll(grant.Sources) + resolvedDsts, hasAutoGroupInternet := resolveViaDestinations( + pol, users, nodes, grant.Destinations, + ) + return &compiledGrant{ category: grantCategoryVia, via: &viaGrantData{ - viaTags: grant.Via, - destinations: grant.Destinations, - internetProtocols: grant.InternetProtocols, + viaTags: grant.Via, + resolvedDsts: resolvedDsts, + hasAutoGroupInternet: hasAutoGroupInternet, + internetProtocols: grant.InternetProtocols, srcIPStrings: srcIPsWithRoutes( srcResolved, hasWildcard, hasDangerAll, nodes, ), @@ -759,39 +804,40 @@ func compileViaForNode( return nil } - // Find matching destination prefixes. SubnetRoutes() excludes exit - // routes, so the *Prefix check below sees only subnet advertisements; - // the *AutoGroup AutoGroupInternet branch checks IsExitNode() instead. + // SubnetRoutes excludes exit routes, so the overlap gate below sees + // only subnet advertisements. autogroup:internet on a via-tagged + // exit advertiser is handled separately because its eligibility is + // per-node (IsExitNode) rather than per-prefix overlap. nodeSubnetRoutes := node.SubnetRoutes() var viaDstPrefixes []netip.Prefix - for _, dst := range cg.via.destinations { - switch d := dst.(type) { - case *Prefix: - dstPrefix := netip.Prefix(*d) - if slices.Contains(nodeSubnetRoutes, dstPrefix) { - viaDstPrefixes = append( - viaDstPrefixes, dstPrefix, - ) - } - case *AutoGroup: - // autogroup:internet on a via-tagged exit advertiser - // becomes a rule whose DstPorts enumerate - // util.TheInternet(). The matchers derived from this - // rule let Node.CanAccess surface the exit node to the - // grant source via DestsIsTheInternet. ReduceFilterRules - // strips the rule from the wire format on non-exit - // advertisers, preserving SaaS PacketFilter encoding. - if d.Is(AutoGroupInternet) && node.IsExitNode() { - viaDstPrefixes = append( - viaDstPrefixes, - util.TheInternet().Prefixes()..., - ) - } + for _, dstPrefix := range cg.via.resolvedDsts { + // Equality would reject any broader or narrower dst relative + // to the advertised route. Containment in either direction + // matches the operator's authorisation: a broader dst restricts + // traffic to the subset the router serves; a narrower dst rides + // on a router covering more than the operator asked for. The + // rule emits the literal dst either way because that is what + // the policy authorised. + if slices.ContainsFunc(nodeSubnetRoutes, dstPrefix.Overlaps) { + viaDstPrefixes = append(viaDstPrefixes, dstPrefix) } } + // autogroup:internet on a via-tagged exit advertiser becomes a rule + // whose DstPorts enumerate util.TheInternet(). The matchers derived + // from this rule let Node.CanAccess surface the exit node to the + // grant source via DestsIsTheInternet. ReduceFilterRules strips the + // rule from the wire format on non-exit advertisers, preserving + // SaaS PacketFilter encoding. + if cg.via.hasAutoGroupInternet && node.IsExitNode() { + viaDstPrefixes = append( + viaDstPrefixes, + util.TheInternet().Prefixes()..., + ) + } + if len(viaDstPrefixes) == 0 { return nil } diff --git a/hscontrol/policy/v2/issue_3267_test.go b/hscontrol/policy/v2/issue_3267_test.go new file mode 100644 index 00000000..bcfd05a5 --- /dev/null +++ b/hscontrol/policy/v2/issue_3267_test.go @@ -0,0 +1,120 @@ +package v2 + +import ( + "net/netip" + "slices" + "testing" + + "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + "tailscale.com/tailcfg" +) + +const issue3267AliceEmail = "alice@headscale.net" + +// TestIssue3267ViaGrantBroaderDestination locks the SaaS contract for +// a via grant whose destination is a host alias broader than (or +// narrower than) the router's advertised subnet route. Alice's only +// path into the subnet is via tag:subnet-router. The grant destination +// resolves to a /64 (IPv6) or /16 (IPv4), and the router advertises a +// contained /120 / /24. Pre-fix the policy compiler emitted no rule +// and ViaRoutesForPeer left Include empty because the prefix relation +// was checked by slices.Contains (exact equality). SaaS behaviour is +// the authority — see testdata/grant_results/via-grant-v47..v51 for +// the equivalent compatibility regression. +func TestIssue3267ViaGrantBroaderDestination(t *testing.T) { + t.Parallel() + + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "alice", Email: issue3267AliceEmail}, //nolint:goconst + } + + cases := []struct { + name string + hostAlias string + dst string // value the hosts alias resolves to + advertised string // narrower prefix the router actually serves + }{ + { + name: "ipv6_4via6_64_dst_with_120_advertised", + hostAlias: "example-4via6", + dst: "fd7a:115c:a1e0:b1a::/64", + advertised: "fd7a:115c:a1e0:b1a:0:13:ad2:7300/120", + }, + { + name: "ipv4_16_dst_with_24_advertised", + hostAlias: "subnet", + dst: "10.33.0.0/16", + advertised: "10.33.5.0/24", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + aliceLaptop := node("alice-laptop", "100.64.0.10", "fd7a:115c:a1e0::a", users[0]) + aliceLaptop.ID = 1 + + router := node("subnet-router", "100.64.0.11", "fd7a:115c:a1e0::b", users[0]) + router.ID = 2 + router.Tags = []string{"tag:subnet-router"} + route := netip.MustParsePrefix(tc.advertised) + router.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}} + router.ApprovedRoutes = []netip.Prefix{route} + + nodes := types.Nodes{aliceLaptop, router} + + policy := `{ + "tagOwners": { + "tag:subnet-router": ["` + issue3267AliceEmail + `"] + }, + "hosts": { + "` + tc.hostAlias + `": "` + tc.dst + `" + }, + "grants": [ + { + "src": ["` + issue3267AliceEmail + `"], + "dst": ["` + tc.hostAlias + `"], + "via": ["tag:subnet-router"], + "ip": ["icmp:*"] + } + ] + }` + + pm, err := NewPolicyManager([]byte(policy), users, nodes.ViewSlice()) + require.NoError(t, err) + + pol, err := unmarshalPolicy([]byte(policy)) + require.NoError(t, err) + require.NoError(t, pol.validate()) + + t.Run("compileFilterRulesForNode_emits_rule_with_grant_dst", func(t *testing.T) { + t.Parallel() + + rules, err := pol.compileFilterRulesForNode(users, router.View(), nodes.ViewSlice()) + require.NoError(t, err) + + found := slices.ContainsFunc(rules, func(r tailcfg.FilterRule) bool { + return slices.ContainsFunc(r.DstPorts, func(d tailcfg.NetPortRange) bool { + return d.IP == tc.dst + }) + }) + require.Truef(t, found, + "router %s must receive a via filter rule whose DstPorts.IP equals the grant dst %q; got rules=%+v", + router.Hostname, tc.dst, rules) + }) + + t.Run("ViaRoutesForPeer_includes_advertised_prefix", func(t *testing.T) { + t.Parallel() + + result := pm.ViaRoutesForPeer(aliceLaptop.View(), router.View()) + require.Contains(t, result.Include, route, + "alice viewing tag:subnet-router must Include advertised prefix %s — drives AllowedIPs in state.RoutesForPeer", route) + require.Empty(t, result.Exclude, + "alice viewing tag:subnet-router must not Exclude any prefix — there is no competing via tag") + }) + }) + } +} diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index a602655f..bf462d44 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1009,11 +1009,13 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via grants = append(grants, aclToGrants(acl)...) } - // Resolve each grant's sources against the viewer once. The three - // passes below reuse this result instead of calling src.Resolve - // per grant per pass. + // Resolve each grant's sources against the viewer once, and each + // grant's destinations into a flat prefix list. The three passes + // below reuse both results instead of re-resolving per pass. viewerIPs := viewer.IPs() viewerMatchesGrant := make([]bool, len(grants)) + resolvedDstPrefixes := make([][]netip.Prefix, len(grants)) + grantHasAutoGroupInternet := make([]bool, len(grants)) for i, grant := range grants { for _, src := range grant.Sources { @@ -1028,6 +1030,10 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via break } } + + resolvedDstPrefixes[i], grantHasAutoGroupInternet[i] = resolveViaDestinations( + pm.pol, pm.users, pm.nodes, grant.Destinations, + ) } for i, grant := range grants { @@ -1039,32 +1045,32 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via continue } - // Collect destination prefixes that the peer actually advertises. + // Filter rules and AllowedIPs are different layers. The filter + // rule carries the dst (the authorisation surface). AllowedIPs + // carries the advertised route (the routing fact the viewer + // needs to pick this peer). This loop builds the AllowedIPs + // side, so it emits routes — not dst prefixes. peerSubnetRoutes := peer.SubnetRoutes() var matchedPrefixes []netip.Prefix - for _, dst := range grant.Destinations { - switch d := dst.(type) { - case *Prefix: - dstPrefix := netip.Prefix(*d) - if slices.Contains(peerSubnetRoutes, dstPrefix) { - matchedPrefixes = append(matchedPrefixes, dstPrefix) - } - case *AutoGroup: - // Per-viewer steering for autogroup:internet: a peer - // advertising approved exit routes is the via-tagged - // node's analogue of "advertises the destination". - // The downstream Include/Exclude split below restricts - // alice to exit nodes carrying the via tag. - if d.Is(AutoGroupInternet) && peer.IsExitNode() { - matchedPrefixes = append( - matchedPrefixes, peer.ExitRoutes()..., - ) + for _, dstPrefix := range resolvedDstPrefixes[i] { + for _, route := range peerSubnetRoutes { + if dstPrefix.Overlaps(route) { + matchedPrefixes = append(matchedPrefixes, route) } } } + // Per-viewer steering for autogroup:internet: a peer advertising + // approved exit routes is the via-tagged node's analogue of + // "advertises the destination". The downstream Include/Exclude + // split below restricts the viewer to exit nodes carrying the + // via tag. + if grantHasAutoGroupInternet[i] && peer.IsExitNode() { + matchedPrefixes = append(matchedPrefixes, peer.ExitRoutes()...) + } + if len(matchedPrefixes) == 0 { continue } @@ -1121,40 +1127,35 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via continue } - for _, dst := range grant.Destinations { - d, ok := dst.(*Prefix) - if !ok { - continue - } + // Elect per matched route, not per dst — a peer can only + // be primary for a prefix it actually advertises, and one + // dst may cover multiple distinct routes. + for _, dstPrefix := range resolvedDstPrefixes[i] { + for _, included := range slices.Clone(result.Include) { + if !dstPrefix.Overlaps(included) { + continue + } - dstPrefix := netip.Prefix(*d) - if !slices.Contains(result.Include, dstPrefix) { - continue - } + var viaPrimaryID types.NodeID - // Find the lowest-ID peer with this via tag that - // advertises this prefix — the via-group primary. - var viaPrimaryID types.NodeID - - for _, viaTag := range grant.Via { - for _, node := range pm.nodes.All() { - if node.HasTag(string(viaTag)) && - slices.Contains(node.SubnetRoutes(), dstPrefix) { - if viaPrimaryID == 0 || node.ID() < viaPrimaryID { - viaPrimaryID = node.ID() + for _, viaTag := range grant.Via { + for _, node := range pm.nodes.All() { + if node.HasTag(string(viaTag)) && + slices.Contains(node.SubnetRoutes(), included) { + if viaPrimaryID == 0 || node.ID() < viaPrimaryID { + viaPrimaryID = node.ID() + } } } } - } - // If the current peer is not the via-group primary, - // demote the prefix from Include to Exclude. - if viaPrimaryID != 0 && peer.ID() != viaPrimaryID { - result.Include = slices.DeleteFunc(result.Include, func(p netip.Prefix) bool { - return p == dstPrefix - }) - if !slices.Contains(result.Exclude, dstPrefix) { - result.Exclude = append(result.Exclude, dstPrefix) + if viaPrimaryID != 0 && peer.ID() != viaPrimaryID { + result.Include = slices.DeleteFunc(result.Include, func(p netip.Prefix) bool { + return p == included + }) + if !slices.Contains(result.Exclude, included) { + result.Exclude = append(result.Exclude, included) + } } } } @@ -1175,21 +1176,19 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via continue } - for _, dst := range grant.Destinations { - if d, ok := dst.(*Prefix); ok { - dstPrefix := netip.Prefix(*d) - if slices.Contains(result.Include, dstPrefix) && - !slices.Contains(result.UsePrimary, dstPrefix) { - result.UsePrimary = append(result.UsePrimary, dstPrefix) + // A non-via grant covering routes that a via grant included + // defers to global HA primary election. Match by overlap so + // a broader or narrower regular dst still catches the + // routes the via grant added to Include. + for _, dstPrefix := range resolvedDstPrefixes[i] { + for _, p := range result.Include { + if dstPrefix.Overlaps(p) && + !slices.Contains(result.UsePrimary, p) { + result.UsePrimary = append(result.UsePrimary, p) } - - // A regular grant overrides a via exclusion: the - // peer doesn't need the via tag if the viewer has - // direct (non-via) access to the prefix. - result.Exclude = slices.DeleteFunc(result.Exclude, func(p netip.Prefix) bool { - return p == dstPrefix - }) } + + result.Exclude = slices.DeleteFunc(result.Exclude, dstPrefix.Overlaps) } } } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 20a60848..74a24f27 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -1813,6 +1813,147 @@ func TestViaRoutesForPeer(t *testing.T) { "client should NOT be able to access 10.0.0.0/24 via matchers alone; "+ "state.RoutesForPeer adds via routes after ReduceRoutes to fix this") }) + + t.Run("broader_dst_includes_narrower_advertised_route", func(t *testing.T) { + t.Parallel() + + nodes := types.Nodes{ + { + ID: 1, + Hostname: "viewer", + IPv4: ap("100.64.0.1"), + User: new(users[0]), + UserID: new(users[0].ID), + Hostinfo: &tailcfg.Hostinfo{}, + }, + { + ID: 2, + Hostname: "router", + IPv4: ap("100.64.0.2"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:router"}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{mp("10.33.5.0/24")}, + }, + ApprovedRoutes: []netip.Prefix{mp("10.33.5.0/24")}, + }, + } + + pol := `{ + "tagOwners": { + "tag:router": ["user1@"] + }, + "grants": [{ + "src": ["user1@"], + "dst": ["10.0.0.0/8"], + "ip": ["*"], + "via": ["tag:router"] + }] + }` + + pm, err := NewPolicyManager([]byte(pol), users, nodes.ViewSlice()) + require.NoError(t, err) + + result := pm.ViaRoutesForPeer(nodes[0].View(), nodes[1].View()) + require.Equal(t, []netip.Prefix{mp("10.33.5.0/24")}, result.Include, + "Include must hold the advertised route /24, not the broader grant dst /8") + require.Empty(t, result.Exclude) + }) + + t.Run("narrower_dst_includes_advertised_route", func(t *testing.T) { + t.Parallel() + + nodes := types.Nodes{ + { + ID: 1, + Hostname: "viewer", + IPv4: ap("100.64.0.1"), + User: new(users[0]), + UserID: new(users[0].ID), + Hostinfo: &tailcfg.Hostinfo{}, + }, + { + ID: 2, + Hostname: "router", + IPv4: ap("100.64.0.2"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:router"}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{mp("10.33.0.0/16")}, + }, + ApprovedRoutes: []netip.Prefix{mp("10.33.0.0/16")}, + }, + } + + pol := `{ + "tagOwners": { + "tag:router": ["user1@"] + }, + "grants": [{ + "src": ["user1@"], + "dst": ["10.33.5.0/24"], + "ip": ["*"], + "via": ["tag:router"] + }] + }` + + pm, err := NewPolicyManager([]byte(pol), users, nodes.ViewSlice()) + require.NoError(t, err) + + result := pm.ViaRoutesForPeer(nodes[0].View(), nodes[1].View()) + require.Equal(t, []netip.Prefix{mp("10.33.0.0/16")}, result.Include, + "Include must hold the advertised route /16 that covers the narrower grant dst /24") + require.Empty(t, result.Exclude) + }) + + t.Run("disjoint_dst_emits_nothing", func(t *testing.T) { + t.Parallel() + + nodes := types.Nodes{ + { + ID: 1, + Hostname: "viewer", + IPv4: ap("100.64.0.1"), + User: new(users[0]), + UserID: new(users[0].ID), + Hostinfo: &tailcfg.Hostinfo{}, + }, + { + ID: 2, + Hostname: "router", + IPv4: ap("100.64.0.2"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:router"}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{mp("10.33.0.0/16")}, + }, + ApprovedRoutes: []netip.Prefix{mp("10.33.0.0/16")}, + }, + } + + pol := `{ + "tagOwners": { + "tag:router": ["user1@"] + }, + "grants": [{ + "src": ["user1@"], + "dst": ["192.168.0.0/16"], + "ip": ["*"], + "via": ["tag:router"] + }] + }` + + pm, err := NewPolicyManager([]byte(pol), users, nodes.ViewSlice()) + require.NoError(t, err) + + result := pm.ViaRoutesForPeer(nodes[0].View(), nodes[1].View()) + require.Empty(t, result.Include, + "disjoint dst must produce nothing — the via gate requires advertised-route overlap") + require.Empty(t, result.Exclude) + }) } // TestBuildPeerMap_AutogroupInternetMakesExitNodeVisible reproduces