diff --git a/hscontrol/policy/policyutil/reduce.go b/hscontrol/policy/policyutil/reduce.go index 5bcfbe41..a1fae190 100644 --- a/hscontrol/policy/policyutil/reduce.go +++ b/hscontrol/policy/policyutil/reduce.go @@ -6,7 +6,6 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" - "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" ) @@ -48,33 +47,18 @@ func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcf continue DEST_LOOP } - // If the node advertises routes, ensure filter rules - // targeting those routes are preserved. Exit routes - // (0.0.0.0/0, ::/0) are skipped because exit nodes - // handle traffic via AllowedIPs/routing, not packet - // filter rules. This matches Tailscale SaaS behavior. - // - // NOTE: This uses RoutableIPs (advertised routes) - // rather than SubnetRoutes (approved routes). The two - // sets are identical in all 98 golden file captures - // from Tailscale SaaS, so we cannot determine from - // captured data which set Tailscale actually checks. - // RoutableIPs is a superset of SubnetRoutes (which - // further filters by approval), so this is the more - // permissive choice. See MISSING_SAAS_DATA.md for - // the capture needed to resolve this ambiguity. - if node.Hostinfo().Valid() { - routableIPs := node.Hostinfo().RoutableIPs() - if routableIPs.Len() > 0 { - for _, routableIP := range routableIPs.All() { - if tsaddr.IsExitRoute(routableIP) { - continue - } - if expanded.OverlapsPrefix(routableIP) { - dests = append(dests, dest) - continue DEST_LOOP - } - } + // If the node has approved subnet routes, preserve + // filter rules targeting those routes. SubnetRoutes() + // returns only approved, non-exit routes — matching + // Tailscale SaaS behavior, which does not generate + // filter rules for advertised-but-unapproved routes. + // Exit routes (0.0.0.0/0, ::/0) are excluded by + // SubnetRoutes() and handled separately via + // AllowedIPs/routing. + for _, subnetRoute := range node.SubnetRoutes() { + if expanded.OverlapsPrefix(subnetRoute) { + dests = append(dests, dest) + continue DEST_LOOP } } } @@ -127,23 +111,16 @@ func reduceCapGrantRule( } } - // Also check routable IPs (subnet routes) — nodes that - // advertise routes should receive CapGrant rules for - // destinations that overlap their routes. - if node.Hostinfo().Valid() { - routableIPs := node.Hostinfo().RoutableIPs() - if routableIPs.Len() > 0 { - for _, dst := range cg.Dsts { - for _, routableIP := range routableIPs.All() { - if tsaddr.IsExitRoute(routableIP) { - continue - } - - if dst.Overlaps(routableIP) { - // For route overlaps, keep the original prefix. - matchingDsts = append(matchingDsts, dst) - } - } + // Also check approved subnet routes — nodes serving + // approved routes should receive CapGrant rules for + // destinations that overlap those routes. SubnetRoutes() + // excludes both unapproved and exit routes, matching + // Tailscale SaaS behavior. + for _, dst := range cg.Dsts { + for _, subnetRoute := range node.SubnetRoutes() { + if dst.Overlaps(subnetRoute) { + // For route overlaps, keep the original prefix. + matchingDsts = append(matchingDsts, dst) } } } diff --git a/hscontrol/policy/policyutil/reduce_test.go b/hscontrol/policy/policyutil/reduce_test.go index 91705550..ae5ab0bd 100644 --- a/hscontrol/policy/policyutil/reduce_test.go +++ b/hscontrol/policy/policyutil/reduce_test.go @@ -196,6 +196,9 @@ func TestReduceFilterRules(t *testing.T) { netip.MustParsePrefix("10.33.0.0/16"), }, }, + ApprovedRoutes: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, }, peers: types.Nodes{ &types.Node{ @@ -518,6 +521,7 @@ func TestReduceFilterRules(t *testing.T) { Hostinfo: &tailcfg.Hostinfo{ RoutableIPs: []netip.Prefix{netip.MustParsePrefix("8.0.0.0/16"), netip.MustParsePrefix("16.0.0.0/16")}, }, + ApprovedRoutes: []netip.Prefix{netip.MustParsePrefix("8.0.0.0/16"), netip.MustParsePrefix("16.0.0.0/16")}, }, peers: types.Nodes{ &types.Node{ @@ -601,6 +605,7 @@ func TestReduceFilterRules(t *testing.T) { Hostinfo: &tailcfg.Hostinfo{ RoutableIPs: []netip.Prefix{netip.MustParsePrefix("8.0.0.0/8"), netip.MustParsePrefix("16.0.0.0/8")}, }, + ApprovedRoutes: []netip.Prefix{netip.MustParsePrefix("8.0.0.0/8"), netip.MustParsePrefix("16.0.0.0/8")}, }, peers: types.Nodes{ &types.Node{ @@ -676,7 +681,8 @@ func TestReduceFilterRules(t *testing.T) { Hostinfo: &tailcfg.Hostinfo{ RoutableIPs: []netip.Prefix{netip.MustParsePrefix("172.16.0.0/24")}, }, - Tags: []string{"tag:access-servers"}, + ApprovedRoutes: []netip.Prefix{netip.MustParsePrefix("172.16.0.0/24")}, + Tags: []string{"tag:access-servers"}, }, peers: types.Nodes{ &types.Node{ @@ -784,16 +790,13 @@ func TestReduceFilterRules(t *testing.T) { } } -// TestReduceFilterRulesPartialApproval documents the behavior of -// ReduceFilterRules when RoutableIPs (advertised) and SubnetRoutes -// (approved) differ. In production, SubnetRoutes is always a subset -// of RoutableIPs, but the two code paths in ReduceFilterRules handle -// them independently: -// - Lines 56-68: Check RoutableIPs (advertised, may be unapproved) -// - Lines 73-79: Check SubnetRoutes (approved only) -// -// This test documents that ReduceFilterRules includes filter rules -// for advertised-but-unapproved routes via the RoutableIPs check. +// TestReduceFilterRulesPartialApproval verifies that ReduceFilterRules +// only preserves filter rules for routes that are both advertised +// (RoutableIPs) AND approved (ApprovedRoutes), matching Tailscale +// SaaS behavior. Advertised-but-unapproved routes do not cause rule +// preservation: SaaS never generates filter rules for unapproved +// routes, and headscale consults node.SubnetRoutes() (which filters +// by approval) rather than Hostinfo.RoutableIPs() (which does not). func TestReduceFilterRulesPartialApproval(t *testing.T) { tests := []struct { name string @@ -828,7 +831,7 @@ func TestReduceFilterRulesPartialApproval(t *testing.T) { wantRoutes: []string{"10.33.0.0/16"}, }, { - name: "unapproved-route-still-included-via-routableips", + name: "unapproved-route-excluded", node: &types.Node{ IPv4: ap("100.64.0.1"), IPv6: ap("fd7a:115c:a1e0::1"), @@ -853,11 +856,11 @@ func TestReduceFilterRulesPartialApproval(t *testing.T) { }, }, }, - // The RoutableIPs check (line 63) matches because - // 172.16.0.0/24 IS in RoutableIPs. The rule is - // kept even though the route is not approved. - wantCount: 1, - wantRoutes: []string{"172.16.0.0/24"}, + // SubnetRoutes() does NOT contain 172.16.0.0/24 + // (only approved routes), and the ACL dst does not + // overlap the node's own IPs, so the rule is + // dropped. This matches Tailscale SaaS behavior. + wantCount: 0, }, { name: "neither-advertised-nor-approved-excluded",