From 571e4476ca91c654a0cb003eecd0f92d15111a46 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 13:48:37 +0000 Subject: [PATCH] policy/v2: resolve an ACL's sources once for all its destinations An ACL compiles to one grant per destination, and each resolved and rendered the same sources. ViaRoutesForPeer only needs one grant per ACL. --- hscontrol/policy/v2/compiled.go | 120 ++++++++++++++++++++--------- hscontrol/policy/v2/filter_test.go | 23 ++++++ hscontrol/policy/v2/policy.go | 10 ++- hscontrol/policy/v2/policy_test.go | 66 ++++++++++++++++ 4 files changed, 183 insertions(+), 36 deletions(-) diff --git a/hscontrol/policy/v2/compiled.go b/hscontrol/policy/v2/compiled.go index 32623c30e..3bc0f7907 100644 --- a/hscontrol/policy/v2/compiled.go +++ b/hscontrol/policy/v2/compiled.go @@ -238,14 +238,9 @@ func (pol *Policy) compileGrants( return nil } - grants := pol.Grants - for _, acl := range pol.ACLs { - grants = append(grants, aclToGrants(acl)...) - } + compiled := make([]compiledGrant, 0, len(pol.Grants)+len(pol.ACLs)) - compiled := make([]compiledGrant, 0, len(grants)) - - for _, grant := range grants { + for _, grant := range pol.Grants { cg, err := pol.compileOneGrant(grant, users, nodes) if err != nil { log.Trace().Err(err).Msg("compiling grant") @@ -258,6 +253,24 @@ func (pol *Policy) compileGrants( } } + for _, acl := range pol.ACLs { + // An ACL becomes one grant per destination, all with the ACL's + // sources. Resolve them once and share the result, or both the + // work and the memory kept are len(src) × len(dst). + srcs, err := resolveGrantSources(pol, acl.Sources, users, nodes) + if err != nil { + log.Trace().Err(err).Msg("compiling grant") + + continue + } + + for _, grant := range aclToGrants(acl) { + if cg := pol.compileResolvedGrant(grant, srcs, users, nodes); cg != nil { + compiled = append(compiled, *cg) + } + } + } + return compiled } @@ -265,8 +278,6 @@ func (pol *Policy) compileGrants( // All source resolution happens here. Non-self, non-via destination // resolution also happens here. Per-node data (self dests, via // matching) is stored for deferred compilation. -// -//nolint:gocyclo,cyclop func (pol *Policy) compileOneGrant( grant Grant, users types.Users, @@ -277,6 +288,62 @@ func (pol *Policy) compileOneGrant( return pol.compileOneViaGrant(grant, users, nodes) } + srcs, err := resolveGrantSources(pol, grant.Sources, users, nodes) + if err != nil { + return nil, err + } + + return pol.compileResolvedGrant(grant, srcs, users, nodes), nil +} + +// grantSources is what the sources of a non-via grant resolve to. It is +// read-only once built: the grants of one ACL share it. +type grantSources struct { + resolved []ResolvedAddresses + ipStrings []string + hasWildcard bool + hasDangerAll bool +} + +// resolveGrantSources resolves sources per-alias, tracking non-wildcard +// sources separately so we can preserve their IPs alongside the +// wildcard CGNAT ranges (matching Tailscale SaaS behavior). +func resolveGrantSources( + pol *Policy, + sources Aliases, + users types.Users, + nodes views.Slice[types.NodeView], +) (*grantSources, error) { + resolvedSrcs, nonWildcardSrcs, err := resolveSources( + pol, sources, users, nodes, + ) + if err != nil { + return nil, err + } + + hasWildcard := sourcesHaveWildcard(sources) + hasDangerAll := sourcesHaveDangerAll(sources) + + return &grantSources{ + resolved: resolvedSrcs, + ipStrings: buildSrcIPStrings( + resolvedSrcs, nonWildcardSrcs, + hasWildcard, hasDangerAll, nodes, + ), + hasWildcard: hasWildcard, + hasDangerAll: hasDangerAll, + }, nil +} + +// compileResolvedGrant compiles a non-via grant whose sources have +// already been resolved into srcs. It returns nil when the grant +// produces no rules. +func (pol *Policy) compileResolvedGrant( + grant Grant, + srcs *grantSources, + users types.Users, + nodes views.Slice[types.NodeView], +) *compiledGrant { // Split destinations into self vs other. var autogroupSelfDests, otherDests []Alias @@ -288,43 +355,26 @@ func (pol *Policy) compileOneGrant( } } - // Resolve sources per-alias, tracking non-wildcard sources - // separately so we can preserve their IPs alongside the - // wildcard CGNAT ranges (matching Tailscale SaaS behavior). - resolvedSrcs, nonWildcardSrcs, err := resolveSources( - pol, grant.Sources, users, nodes, - ) - if err != nil { - return nil, err - } - // Literally empty src=[] or dst=[] produces no rules. if len(grant.Sources) == 0 || len(grant.Destinations) == 0 { - return nil, nil //nolint:nilnil + return nil } - if len(resolvedSrcs) == 0 && grant.App == nil { - return nil, nil //nolint:nilnil + if len(srcs.resolved) == 0 && grant.App == nil { + return nil } - hasWildcard := sourcesHaveWildcard(grant.Sources) - hasDangerAll := sourcesHaveDangerAll(grant.Sources) - srcIPStrings := buildSrcIPStrings( - resolvedSrcs, nonWildcardSrcs, - hasWildcard, hasDangerAll, nodes, - ) - cg := &compiledGrant{ - srcIPStrings: srcIPStrings, - hasWildcard: hasWildcard, - hasDangerAll: hasDangerAll, + srcIPStrings: srcs.ipStrings, + hasWildcard: srcs.hasWildcard, + hasDangerAll: srcs.hasDangerAll, } // Compile non-self destination rules (done once, shared). if len(otherDests) > 0 { cg.rules = pol.compileOtherDests( users, nodes, grant, otherDests, - resolvedSrcs, srcIPStrings, + srcs.resolved, srcs.ipStrings, ) } @@ -333,13 +383,13 @@ func (pol *Policy) compileOneGrant( if len(autogroupSelfDests) > 0 { cg.category = grantCategorySelf cg.self = &selfGrantData{ - resolvedSrcs: resolvedSrcs, + resolvedSrcs: srcs.resolved, internetProtocols: grant.InternetProtocols, app: grant.App, } } - return cg, nil + return cg } // mergeResolvedSrcs merges every prefix from the resolved sources into a diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index fcdb7e66e..3dabf5a5c 100644 --- a/hscontrol/policy/v2/filter_test.go +++ b/hscontrol/policy/v2/filter_test.go @@ -4260,3 +4260,26 @@ func TestDestinationsToNetPortRange_AutogroupInternet(t *testing.T) { }) } } + +// TestACLSourcesResolvedOnce pins that the grants an ACL is split into, one +// per destination, share the resolved sources instead of each holding a copy. +func TestACLSourcesResolvedOnce(t *testing.T) { + t.Parallel() + + pol, err := unmarshalPolicy([]byte(`{"acls":[{"action":"accept", + "src":["10.0.0.1","10.0.1.1","10.0.2.1"], + "dst":["100.64.0.1:80","100.64.0.2:443","100.64.0.3:22"]}]}`)) + require.NoError(t, err) + + grants := pol.compileGrants(nil, types.Nodes{}.ViewSlice()) + require.Len(t, grants, 3) + + for _, cg := range grants { + require.Equal(t, []string{"10.0.0.1", "10.0.1.1", "10.0.2.1"}, cg.srcIPStrings) + require.Same(t, &grants[0].srcIPStrings[0], &cg.srcIPStrings[0]) + } + + rules := globalFilterRules(grants) + require.Len(t, rules, 1) + require.Len(t, rules[0].DstPorts, 3) +} diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 339436a41..ede844fbe 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1448,8 +1448,16 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via // Clip so the appends below allocate: callers share pm.pol under RLock, // and writing into its spare capacity races between them. grants := slices.Clip(pm.pol.Grants) + + // Ports never steer routes, so an ACL is one grant here, not one per + // destination that would match its sources once per destination. for _, acl := range pm.pol.ACLs { - grants = append(grants, aclToGrants(acl)...) + dsts := make(Aliases, 0, len(acl.Destinations)) + for _, d := range acl.Destinations { + dsts = append(dsts, d.Alias) + } + + grants = append(grants, Grant{Sources: acl.Sources, Destinations: dsts}) } // matchViewer resolves a grant lazily and at most once: non-via diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 7b94db0fd..3373a4bef 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -1997,6 +1997,72 @@ func TestViaRoutesForPeer(t *testing.T) { require.Empty(t, result.UsePrimary) }) + t.Run("acl_destinations_share_source_match", func(t *testing.T) { + t.Parallel() + + nodes := types.Nodes{ + { + ID: 1, + Hostname: "viewer", + IPv4: ap("100.64.0.1"), + User: new(users[0]), + UserID: new(users[0].ID), + Hostinfo: &tailcfg.Hostinfo{}, + }, + { + ID: 2, + Hostname: "router", + IPv4: ap("100.64.0.2"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:router"}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{mp("10.0.0.0/24")}, + }, + ApprovedRoutes: []netip.Prefix{mp("10.0.0.0/24")}, + }, + } + + // Only the last destination of each ACL covers the route, and only + // the sources of the second ACL match the viewer. + pol := func(secondSrc string) string { + return `{ + "tagOwners": { + "tag:router": ["user1@"] + }, + "acls": [{ + "action": "accept", + "src": ["user2@"], + "dst": ["192.168.0.0/24:*", "10.0.0.0/24:*"] + }, { + "action": "accept", + "src": ["user2@", "` + secondSrc + `"], + "dst": ["192.168.0.0/24:*", "10.0.0.0/24:*"] + }], + "grants": [{ + "src": ["user1@"], + "dst": ["10.0.0.0/24"], + "ip": ["*"], + "via": ["tag:router"] + }] + }` + } + + pm, err := NewPolicyManager([]byte(pol("user1@")), users, nodes.ViewSlice()) + require.NoError(t, err) + + result := pm.ViaRoutesForPeer(nodes[0].View(), nodes[1].View()) + require.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.Include) + require.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.UsePrimary) + + pm, err = NewPolicyManager([]byte(pol("user2@")), users, nodes.ViewSlice()) + require.NoError(t, err) + + result = pm.ViaRoutesForPeer(nodes[0].View(), nodes[1].View()) + require.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.Include) + require.Empty(t, result.UsePrimary) + }) + t.Run("peer_does_not_advertise_destination", func(t *testing.T) { t.Parallel()