From 95c6129315623256a035aca02d057ec6bc03c867 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 22 Oct 2026 06:39:21 +0000 Subject: [PATCH] policy: offer exit routes only to viewers whose rules reach the internet Every destination overlaps /0, so any rule offered a peer's exit routes. They now need a rule to the internet, the test exit-node visibility uses; via steering defers them to a regular grant only on that test. --- hscontrol/policy/matcher/matcher.go | 15 +------ hscontrol/policy/v2/issue_3233_test.go | 10 +++++ hscontrol/policy/v2/policy.go | 37 +++++++++++---- .../v2/tailscale_routes_data_compat_test.go | 5 --- hscontrol/servertest/via_compat_test.go | 5 --- hscontrol/state/state.go | 5 +-- hscontrol/types/node.go | 34 +++++++------- hscontrol/types/node_test.go | 45 +++++++++++++++++++ hscontrol/util/net.go | 9 ++++ 9 files changed, 115 insertions(+), 50 deletions(-) diff --git a/hscontrol/policy/matcher/matcher.go b/hscontrol/policy/matcher/matcher.go index f93cfe54d..3ac5c6e62 100644 --- a/hscontrol/policy/matcher/matcher.go +++ b/hscontrol/policy/matcher/matcher.go @@ -7,7 +7,6 @@ import ( "github.com/juanfont/headscale/hscontrol/util" "go4.org/netipx" - "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" ) @@ -154,17 +153,7 @@ func overlaps(ranges []netipx.IPRange, r netipx.IPRange) bool { // DestsIsTheInternet reports whether the destination covers "the // internet" — the set represented by autogroup:internet, special-cased -// for exit nodes. Returns true if either family's /0 is contained -// (0.0.0.0/0 or ::/0), or if dests is a superset of [util.TheInternet]. A -// single-family /0 counts because operators may write it directly and -// it still denotes the whole internet for that family. +// for exit nodes. See [util.CoversTheInternet]. func (m *Match) DestsIsTheInternet() bool { - if m.dests.ContainsPrefix(tsaddr.AllIPv4()) || - m.dests.ContainsPrefix(tsaddr.AllIPv6()) { - return true - } - - // Superset-of-[util.TheInternet] check handles merged filter rules - // where the internet prefixes are combined with other dests. - return util.IPSetSubsetOf(util.TheInternet(), m.dests) + return util.CoversTheInternet(m.dests) } diff --git a/hscontrol/policy/v2/issue_3233_test.go b/hscontrol/policy/v2/issue_3233_test.go index 9a7924a5e..7de1c48d5 100644 --- a/hscontrol/policy/v2/issue_3233_test.go +++ b/hscontrol/policy/v2/issue_3233_test.go @@ -171,6 +171,16 @@ func TestViaInternetExitSteeringSurvivesUnrelatedRules(t *testing.T) { extra: `"acls": [{"action": "accept", "src": ["autogroup:member"], "dst": ["*:*"]}],`, wantExcluded: false, }, + { + name: "acl-ipv4-default", + extra: `"acls": [{"action": "accept", "src": ["autogroup:member"], "dst": ["0.0.0.0/0:*"]}],`, + wantExcluded: false, + }, + { + name: "acl-ipv6-default", + extra: `"acls": [{"action": "accept", "src": ["autogroup:member"], "dst": ["::/0:*"]}],`, + wantExcluded: false, + }, } for _, tt := range tests { diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 667d908ee..8b53c5c4e 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -16,6 +16,7 @@ import ( "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/policy/policyutil" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util" "github.com/puzpuzpuz/xsync/v4" "github.com/rs/zerolog/log" "go4.org/netipx" @@ -1734,14 +1735,19 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via continue } + reachesInternet := grantReachesInternet(grant, resolvedDstPrefixes[i]) + // 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) { + // Only a grant to the internet covers an exit route, + // as [types.Node.CanAccessRoutes] decides. + covers := dstPrefix.Overlaps(p) && + (reachesInternet || !tsaddr.IsExitRoute(p)) + if covers && !slices.Contains(result.UsePrimary, p) { result.UsePrimary = append(result.UsePrimary, p) } } @@ -1755,7 +1761,7 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via // A regular grant to the internet lets the viewer use any // exit node, not just the via-tagged ones. - if grantReachesInternet(grant) { + if reachesInternet { result.Exclude = slices.DeleteFunc(result.Exclude, tsaddr.IsExitRoute) } } @@ -1764,11 +1770,13 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via return result } -// grantReachesInternet reports whether a grant's destinations include -// the internet. Neither the wildcard nor autogroup:internet resolves -// to 0.0.0.0/0, so check the aliases themselves. -func grantReachesInternet(grant Grant) bool { - return slices.ContainsFunc(grant.Destinations, func(d Alias) bool { +// grantReachesInternet reports whether a grant's destinations, resolved +// to dsts, reach the internet by the test the grant's filter rule gets +// ([matcher.Match.DestsIsTheInternet]). Neither the wildcard nor +// autogroup:internet resolves to the internet here, so check those +// aliases themselves. +func grantReachesInternet(grant Grant, dsts []netip.Prefix) bool { + internetAlias := slices.ContainsFunc(grant.Destinations, func(d Alias) bool { if ag, ok := d.(*AutoGroup); ok { return ag.Is(AutoGroupInternet) } @@ -1777,6 +1785,19 @@ func grantReachesInternet(grant Grant) bool { return ok }) + if internetAlias { + return true + } + + var b netipx.IPSetBuilder + for _, p := range dsts { + b.AddPrefix(p) + } + + // Resolved prefixes are valid, so the builder cannot fail. + set, _ := b.IPSet() + + return util.CoversTheInternet(set) } func (pm *PolicyManager) Version() int { diff --git a/hscontrol/policy/v2/tailscale_routes_data_compat_test.go b/hscontrol/policy/v2/tailscale_routes_data_compat_test.go index a07591939..b6d9d9492 100644 --- a/hscontrol/policy/v2/tailscale_routes_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_routes_data_compat_test.go @@ -183,9 +183,6 @@ var routesSkipReasons = map[string]string{ "routes-m4-member-self/m-odin2/filter": filterAddressForm, "routes-m4-member-self/r-exit/filter": "FILTER_ADDRESS_FORM: same addresses, different wire form and rule order", - "routes-m1-wildcard-src-multi-range/srv/peer-allowed-ips": exitRoutesOnOverlap, - "routes-m6-subnet-to-subnet-multi-route/r-multi/peer-allowed-ips": exitRoutesOnOverlap, - "routes-m1-wildcard-src-multi-range/m-odin1/peers": sameUserPeers, "routes-m1-wildcard-src-multi-range/m-odin2/peers": sameUserPeers, "routes-m2-member-partial-subnet/m-odin1/peers": sameUserPeers, @@ -272,8 +269,6 @@ func notCompared(prefixes []string, key string) bool { const filterAddressForm = "FILTER_ADDRESS_FORM: same addresses, different wire form" -const exitRoutesOnOverlap = "EXIT_ROUTES_ON_OVERLAP: headscale offers a peer's exit routes to a viewer whose rules reach only nodes or subnets; SaaS leaves them out" - const sameUserPeers = "SAME_USER_PEERS: SaaS lists a user's own devices as each other's peers with no rule linking them; headscale peers only what the policy links" // routesSetByAPI reports whether the capture's routes were approved through diff --git a/hscontrol/servertest/via_compat_test.go b/hscontrol/servertest/via_compat_test.go index 13bdad274..6434ae8ab 100644 --- a/hscontrol/servertest/via_compat_test.go +++ b/hscontrol/servertest/via_compat_test.go @@ -107,9 +107,6 @@ var routesMapCompatTests = []struct { var viaCompatSkipReasons = map[string]string{ "tagged-self": "TAGGED_SELF_PROFILE: SaaS gives a tagged node a profile of its own, headscale the shared tagged-devices one", - "routes-m1-wildcard-src-multi-range/srv/peer-allowed-ips": exitRoutesOnOverlap, - "routes-m6-subnet-to-subnet-multi-route/r-multi/peer-allowed-ips": exitRoutesOnOverlap, - "routes-m1-wildcard-src-multi-range/m-odin1/peers": sameUserPeers, "routes-m1-wildcard-src-multi-range/m-odin2/peers": sameUserPeers, "routes-m2-member-partial-subnet/m-odin1/peers": sameUserPeers, @@ -120,8 +117,6 @@ var viaCompatSkipReasons = map[string]string{ "routes-m6-subnet-to-subnet-multi-route/m-odin2/peers": sameUserPeers, } -const exitRoutesOnOverlap = "EXIT_ROUTES_ON_OVERLAP: headscale offers a peer's exit routes to a viewer whose rules reach only nodes or subnets; SaaS leaves them out" - const sameUserPeers = "SAME_USER_PEERS: SaaS lists a user's own devices as each other's peers with no rule linking them; headscale peers only what the policy links" // TestRoutesMapCompat replays routesMapCompatTests through diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 42cf8688a..d9650f0f9 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1559,9 +1559,8 @@ func (s *State) GetNodePrimaryRoutes(nodeID types.NodeID) []netip.Prefix { // AllowedIPs, combining primary routes (from HA election), approved exit // routes, and via grant steering. // -// Approved exit routes (0.0.0.0/0, ::/0) are included alongside subnet -// routes — they appear in every peer's AllowedIPs, while unapproved -// ones do not. +// Approved exit routes (0.0.0.0/0, ::/0) are reduced alongside subnet +// routes: a viewer gets them when its rules reach the internet. func (s *State) RoutesForPeer( viewer, peer types.NodeView, matchers []matcher.Match, diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index f39989271..b890632cd 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -345,9 +345,8 @@ func (node *Node) Prefixes() []netip.Prefix { } // ExitRoutes returns the node's approved exit routes (0.0.0.0/0 -// and/or ::/0). Consumed unconditionally by RoutesForPeer when the -// viewer uses an exit node; excluded from [Node.CanAccessRoute] which only -// handles non-exit routing. +// and/or ::/0). [Node.CanAccessRoutes] offers them only to a viewer whose +// rules reach the internet. func (node *Node) ExitRoutes() []netip.Prefix { return node.approvedRoutes(true) } @@ -471,19 +470,10 @@ func (node *Node) CanAccessRoute(matchers []matcher.Match, route netip.Prefix) b // CanAccessRoutes returns the authorized routes in input order, evaluating the // node's matcher membership once for the whole list. // -// Unlike [Node.CanAccess], this function intentionally does NOT check -// [matcher.Match.DestsIsTheInternet]. Exit routes (0.0.0.0/0, ::/0) are handled by -// RoutesForPeer (state.go) which adds them unconditionally from -// [Node.ExitRoutes], not through ACL-based route filtering. The -// [matcher.Match.DestsIsTheInternet] check in [Node.CanAccess] exists solely for peer -// visibility determination (should two nodes see each other), which -// is a separate concern from route prefix authorization. -// -// Additionally, autogroup:internet is explicitly skipped during filter -// rule compilation (filter.go), so no matchers ever contain "the -// internet" from internet-targeted ACLs. Wildcard "*" dests produce -// matchers where [matcher.Match.DestsOverlapsPrefixes](0.0.0.0/0) already returns -// true, so the check would be redundant for that case. +// Exit routes (0.0.0.0/0, ::/0) are kept only for a rule from the node to the +// internet ([matcher.Match.DestsIsTheInternet]), the test [Node.CanAccess] +// applies to exit nodes: every destination overlaps /0, so overlap would offer +// them for any rule. func (node *Node) CanAccessRoutes( matchers []matcher.Match, routes []netip.Prefix, @@ -514,7 +504,19 @@ func (node *Node) CanAccessRoutes( var result []netip.Prefix for _, route := range routes { + exit := tsaddr.IsExitRoute(route) + for i, m := range matchers { + if exit { + if sides[i].isSrc && m.DestsIsTheInternet() { + result = append(result, route) + + break + } + + continue + } + if (sides[i].isSrc && m.DestsOverlapsPrefixes(route)) || (sides[i].isDst && m.SrcsOverlapsPrefixes(route)) { result = append(result, route) diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index c224c7bba..5ad1d634b 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -14,8 +14,10 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/util" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "pgregory.net/rapid" + "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" "tailscale.com/types/key" "tailscale.com/util/dnsname" @@ -1227,6 +1229,37 @@ func TestIsExitNodeSubnetRouterAllocs(t *testing.T) { require.Zero(t, allocs) } +// TestCanAccessRoutesExitNeedsInternet checks that a viewer is offered a +// peer's exit routes only when one of its rules reaches the internet, not for +// any rule whose destination overlaps /0. +func TestCanAccessRoutesExitNeedsInternet(t *testing.T) { + viewer := Node{IPv4: new(netip.MustParseAddr("100.64.0.1"))} + routes := []netip.Prefix{tsaddr.AllIPv4(), tsaddr.AllIPv6(), netip.MustParsePrefix("10.40.7.0/24")} + rule := func(src string, dsts ...string) []matcher.Match { + r := tailcfg.FilterRule{SrcIPs: []string{src}} + for _, dst := range dsts { + r.DstPorts = append(r.DstPorts, tailcfg.NetPortRange{IP: dst, Ports: tailcfg.PortRangeAny}) + } + + return matcher.MatchesFromFilterRules([]tailcfg.FilterRule{r}) + } + + internet := util.PrefixesToString(util.TheInternet().Prefixes()) + + for name, tt := range map[string]struct { + matchers []matcher.Match + want []netip.Prefix + }{ + "to a subnet": {rule("100.64.0.1", "10.40.7.0/24"), routes[2:]}, + "from anywhere": {rule("*", "100.64.0.1"), routes[2:]}, + "to everything": {rule("100.64.0.1", "*"), routes}, + "to the internet": {rule("100.64.0.1", internet...), routes[:2]}, + "to another viewer": {rule("100.64.0.1", "100.64.0.2"), nil}, + } { + assert.Equalf(t, tt.want, viewer.CanAccessRoutes(tt.matchers, routes), "%s", name) + } +} + func TestCanAccessRoutesMatchesReference(t *testing.T) { prefixStrings := []string{ "100.64.0.1/32", "100.64.0.2/32", "100.64.0.0/24", @@ -1316,6 +1349,18 @@ func canAccessRouteReference( src := node.IPs() subnetRoutes := node.SubnetRoutes() + if tsaddr.IsExitRoute(route) { + for _, matcher := range matchers { + fromNode := matcher.SrcsContainsIPs(src...) || + (len(subnetRoutes) > 0 && matcher.SrcsOverlapsPrefixes(subnetRoutes...)) + if fromNode && matcher.DestsIsTheInternet() { + return true + } + } + + return false + } + for _, matcher := range matchers { if matcher.SrcsContainsIPs(src...) && matcher.DestsOverlapsPrefixes(route) { return true diff --git a/hscontrol/util/net.go b/hscontrol/util/net.go index c3c15cefb..cdefd55a1 100644 --- a/hscontrol/util/net.go +++ b/hscontrol/util/net.go @@ -63,6 +63,15 @@ var TheInternet = sync.OnceValue(func() *netipx.IPSet { return theInternetSet }) +// CoversTheInternet reports whether set covers [TheInternet]. Either +// family's /0 counts alone, because operators write it directly and it +// still denotes the whole internet for that family. +func CoversTheInternet(set *netipx.IPSet) bool { + return set.ContainsPrefix(tsaddr.AllIPv4()) || + set.ContainsPrefix(tsaddr.AllIPv6()) || + IPSetSubsetOf(TheInternet(), set) +} + // IPSetSubsetOf reports whether every prefix of candidate is contained in // container. A nil candidate or container returns false. func IPSetSubsetOf(candidate, container *netipx.IPSet) bool {