mirror of
https://github.com/juanfont/headscale.git
synced 2026-07-08 00:50:20 +09:00
policyutil: fix reduceCapGrantRule and add reduction tests
Fix fragile IP indexing in reduceCapGrantRule: replace hard-coded nodeIPs[0]/nodeIPs[1] access with a range loop, matching the pattern used by the adjacent broad-prefix branch. The old code would panic if a node had zero IPs. Remove redundant SubnetRoutes check in ReduceFilterRules: the check was strictly redundant with the RoutableIPs check since SubnetRoutes is always a subset of RoutableIPs. Document that golden data always has routable_ips == approved_routes so we cannot determine which set Tailscale actually checks. Add tests: - TestReduceFilterRulesCapGrant: 7 cases covering IP narrowing, non-matching filtered out, subnet route overlap, exit route skipping, mixed DstPorts+CapGrant, IPv4-only, and zero-IP nodes - TestReduceFilterRulesPartialApproval: 3 cases documenting behavior for approved, unapproved-but-advertised, and non-advertised routes Updates #3157
This commit is contained in:
Kristoffer Dalby
@@ -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 {
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user