diff --git a/hscontrol/policy/policyutil/doc.go b/hscontrol/policy/policyutil/doc.go new file mode 100644 index 00000000..902a43e4 --- /dev/null +++ b/hscontrol/policy/policyutil/doc.go @@ -0,0 +1,9 @@ +// Package policyutil contains pure functions that transform compiled +// policy rules for a specific node. The headline function is +// ReduceFilterRules, which filters global rules down to those relevant +// to one node. +// +// A node's SubnetRoutes (approved, non-exit) participate in rule +// matching so subnet routers receive filter rules for destinations +// their subnets cover — the fix for issue #3169. +package policyutil diff --git a/hscontrol/policy/policyutil/reduce.go b/hscontrol/policy/policyutil/reduce.go index c62691e0..34cce2e8 100644 --- a/hscontrol/policy/policyutil/reduce.go +++ b/hscontrol/policy/policyutil/reduce.go @@ -2,10 +2,10 @@ package policyutil import ( "net/netip" + "slices" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" - "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" ) @@ -17,6 +17,7 @@ import ( // to this function. Use PolicyManager.FilterForNode() instead, which handles both cases. func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcfg.FilterRule { ret := []tailcfg.FilterRule{} + subnetRoutes := node.SubnetRoutes() for _, rule := range rules { // Handle CapGrant rules separately — they use CapGrant[].Dsts @@ -33,60 +34,37 @@ func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcf // record if the rule is actually relevant for the given node. var dests []tailcfg.NetPortRange - DEST_LOOP: for _, dest := range rule.DstPorts { expanded, err := util.ParseIPSet(dest.IP, nil) - // Fail closed, if we can't parse it, then we should not allow - // access. + // Fail closed: unparseable dests are dropped. if err != nil { - continue DEST_LOOP + continue } if node.InIPSet(expanded) { dests = append(dests, dest) - continue DEST_LOOP + continue } - // 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 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 - } - } - } - } - - // 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 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. + if slices.ContainsFunc(subnetRoutes, expanded.OverlapsPrefix) { + dests = append(dests, dest) } } if len(dests) > 0 { - ret = append(ret, tailcfg.FilterRule{ - SrcIPs: rule.SrcIPs, - DstPorts: dests, - IPProto: rule.IPProto, - }) + // Struct-copy preserves any unknown future FilterRule + // fields. + out := rule + out.DstPorts = dests + ret = append(ret, out) } } @@ -105,6 +83,7 @@ func reduceCapGrantRule( var capGrants []tailcfg.CapGrant nodeIPs := node.IPs() + subnetRoutes := node.SubnetRoutes() for _, cg := range rule.CapGrant { // Collect the node's IPs that fall within any of this @@ -114,8 +93,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 { @@ -128,28 +108,30 @@ 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) - } - } + // Asymmetric on purpose: the IP-match loop above narrows broad + // prefixes to node-specific /32 or /128 so peers receive only + // the minimum routing surface. The route-match loop below + // preserves the original prefix so the subnet-serving node + // receives the full CapGrant scope. SubnetRoutes() excludes + // both unapproved and exit routes, matching Tailscale SaaS + // behavior. + for _, dst := range cg.Dsts { + for _, subnetRoute := range subnetRoutes { + if dst.Overlaps(subnetRoute) { + // For route overlaps, keep the original prefix. + matchingDsts = append(matchingDsts, dst) } } } if len(matchingDsts) > 0 { + // A Dst can be appended twice when a broad prefix both + // contains a node IP and overlaps one of its approved + // subnet routes. Sort + Compact dedups; netip.Prefix is + // comparable so Compact works with ==. + slices.SortFunc(matchingDsts, netip.Prefix.Compare) + matchingDsts = slices.Compact(matchingDsts) + capGrants = append(capGrants, tailcfg.CapGrant{ Dsts: matchingDsts, CapMap: cg.CapMap, diff --git a/hscontrol/policy/policyutil/reduce_test.go b/hscontrol/policy/policyutil/reduce_test.go index 893f5b2f..4c722f91 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{ @@ -759,7 +765,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 +789,494 @@ func TestReduceFilterRules(t *testing.T) { } } } + +// 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 + 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-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"), + // 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}, + }, + }, + }, + // 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", + 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{}, + }, + { + // A broad prefix that both contains the node IP (/32 narrow) + // and overlaps one of its approved subnet routes (route + // preserved) would otherwise be emitted twice. + name: "capgrant-ip-and-route-overlap-dedup", + node: &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/24"), + }, + }, + ApprovedRoutes: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/24"), + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/24"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/relay-target": nil, + }, + }, + }, + }, + }, + want: []tailcfg.FilterRule{ + { + SrcIPs: []string{"*"}, + CapGrant: []tailcfg.CapGrant{ + { + Dsts: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + netip.MustParsePrefix("100.64.0.0/24"), + }, + CapMap: tailcfg.PeerCapMap{ + "tailscale.com/cap/relay-target": nil, + }, + }, + }, + }, + }, + }, + } + + 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, + ) + } + } + }) + } +}