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