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()