diff --git a/hscontrol/policy/policyutil/reduce.go b/hscontrol/policy/policyutil/reduce.go index c8b6a437..5bcfbe41 100644 --- a/hscontrol/policy/policyutil/reduce.go +++ b/hscontrol/policy/policyutil/reduce.go @@ -2,6 +2,7 @@ package policyutil import ( "net/netip" + "slices" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" @@ -47,12 +48,21 @@ func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcf continue DEST_LOOP } - // If the node exposes routes, ensure they are not removed - // when the filters are reduced. Exit routes (0.0.0.0/0, ::/0) - // are skipped here because exit nodes handle traffic via - // AllowedIPs/routing, not packet filter rules. This matches - // Tailscale SaaS behavior where exit nodes do not receive - // filter rules for destinations that only overlap via exit routes. + // 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 { @@ -67,17 +77,6 @@ func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcf } } } - - // Also check approved subnet routes - nodes should have access - // to subnets they're approved to route traffic for. - subnetRoutes := node.SubnetRoutes() - - for _, subnetRoute := range subnetRoutes { - if expanded.OverlapsPrefix(subnetRoute) { - dests = append(dests, dest) - continue DEST_LOOP - } - } } if len(dests) > 0 { @@ -113,8 +112,9 @@ func reduceCapGrantRule( for _, dst := range cg.Dsts { if dst.IsSingleIP() { - // Already a specific IP — keep it if it matches. - if dst.Addr() == nodeIPs[0] || (len(nodeIPs) > 1 && dst.Addr() == nodeIPs[1]) { + // Already a specific IP — keep it if it matches + // any of the node's IPs. + if slices.Contains(nodeIPs, dst.Addr()) { matchingDsts = append(matchingDsts, dst) } } else { diff --git a/hscontrol/policy/policyutil/reduce_test.go b/hscontrol/policy/policyutil/reduce_test.go index 893f5b2f..91705550 100644 --- a/hscontrol/policy/policyutil/reduce_test.go +++ b/hscontrol/policy/policyutil/reduce_test.go @@ -759,7 +759,9 @@ func TestReduceFilterRules(t *testing.T) { } for _, tt := range tests { - for idx, pmf := range policy.PolicyManagerFuncsForTest([]byte(tt.pol)) { + for idx, pmf := range policy.PolicyManagerFuncsForTest( + []byte(tt.pol), + ) { t.Run(fmt.Sprintf("%s-index%d", tt.name, idx), func(t *testing.T) { var ( pm policy.PolicyManager @@ -781,3 +783,448 @@ 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. +func TestReduceFilterRulesPartialApproval(t *testing.T) { + tests := []struct { + name string + node *types.Node + rules []tailcfg.FilterRule + wantCount int + wantRoutes []string + }{ + { + name: "approved-route-included", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + ApprovedRoutes: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + DstPorts: []tailcfg.NetPortRange{ + {IP: "10.33.0.0/16", Ports: tailcfg.PortRangeAny}, + }, + }, + }, + wantCount: 1, + wantRoutes: []string{"10.33.0.0/16"}, + }, + { + name: "unapproved-route-still-included-via-routableips", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + // Advertised but NOT approved: + netip.MustParsePrefix("172.16.0.0/24"), + }, + }, + ApprovedRoutes: []netip.Prefix{ + // Only 10.33.0.0/16 approved + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + DstPorts: []tailcfg.NetPortRange{ + // Targets the unapproved route + {IP: "172.16.0.0/24", Ports: tailcfg.PortRangeAny}, + }, + }, + }, + // 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"}, + }, + { + name: "neither-advertised-nor-approved-excluded", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + ApprovedRoutes: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + DstPorts: []tailcfg.NetPortRange{ + // Not advertised, not approved + {IP: "192.168.0.0/16", Ports: tailcfg.PortRangeAny}, + }, + }, + }, + wantCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := policyutil.ReduceFilterRules( + tt.node.View(), tt.rules, + ) + require.Len(t, got, tt.wantCount, + "rule count mismatch") + + if tt.wantCount > 0 { + var gotRoutes []string + for _, dp := range got[0].DstPorts { + gotRoutes = append(gotRoutes, dp.IP) + } + + require.Equal(t, tt.wantRoutes, gotRoutes) + } + }) + } +} + +// TestReduceFilterRulesCapGrant tests the CapGrant branch of +// ReduceFilterRules, which was previously untested. All existing +// test cases use ACL-only policies with DstPorts rules. +func TestReduceFilterRulesCapGrant(t *testing.T) { + tests := []struct { + name string + node *types.Node + rules []tailcfg.FilterRule + want []tailcfg.FilterRule + }{ + { + name: "capgrant-matches-node-ip-narrowed", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // Broad IPv4 prefix containing node IP + netip.MustParsePrefix("100.64.0.0/10"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // Only IPv4 narrowed (IPv6 not in /10) + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + }, + { + name: "capgrant-no-match-filtered-out", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // Different IP — doesn't match this node + netip.MustParsePrefix("100.64.0.99/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{}, + }, + { + name: "capgrant-with-subnet-route-overlap", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + ApprovedRoutes: []netip.Prefix{ + netip.MustParsePrefix("10.33.0.0/16"), + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // Subnet route overlap + netip.MustParsePrefix("10.0.0.0/8"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/relay-target": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // 10.0.0.0/8 doesn't contain node's + // CGNAT IP (100.64.0.1), so no IP + // narrowing. Only route overlap kept + // as original prefix. + netip.MustParsePrefix("10.0.0.0/8"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/relay-target": nil, + }, + }, + }, + }, + }, + }, + { + name: "capgrant-exit-route-skipped", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: tsaddr.ExitRoutes(), + }, + ApprovedRoutes: tsaddr.ExitRoutes(), + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // 0.0.0.0/0 overlaps the exit route + // but exit routes should be skipped + netip.MustParsePrefix("0.0.0.0/0"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + // Node IP narrowed (0.0.0.0/0 + // contains the node's IP) + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + }, + { + name: "mixed-dstports-and-capgrant-rules", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + }, + rules: []tailcfg.FilterRule{ + { + // DstPorts rule that matches + SrcIPs: []string{"10.0.0.0/8"}, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.1", Ports: tailcfg.PortRangeAny}, + }, + }, + { + // CapGrant rule that matches + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + { + // CapGrant rule that doesn't match + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.99/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.1", Ports: tailcfg.PortRangeAny}, + }, + }, + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + // Third rule filtered out — doesn't match node + }, + }, + { + name: "capgrant-ipv4-only-node", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + // IPv6 is nil, single-IP node + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + }, + { + name: "capgrant-zero-ip-node-no-panic", + node: &types.Node{ + // Both IPv4 and IPv6 are nil, zero IPs. + // Must not panic. + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"10.0.0.0/8"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/drive-sharer": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := policyutil.ReduceFilterRules(tt.node.View(), tt.rules) + + require.Len(t, got, len(tt.want), + "rule count mismatch") + + for i := range tt.want { + require.Equal(t, tt.want[i].SrcIPs, got[i].SrcIPs, + "rule[%d] SrcIPs", i) + require.Len(t, got[i].DstPorts, len(tt.want[i].DstPorts), + "rule[%d] DstPorts count", i) + require.Len(t, got[i].CapGrant, len(tt.want[i].CapGrant), + "rule[%d] CapGrant count", i) + + for j := range tt.want[i].CapGrant { + require.ElementsMatch(t, + tt.want[i].CapGrant[j].Dsts, + got[i].CapGrant[j].Dsts, + "rule[%d].CapGrant[%d] Dsts", i, j, + ) + } + } + }) + } +}