From eb9c14b5f23635e8e33b7c081537a7beab26f8ae Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 30 Oct 2026 14:20:12 +0000 Subject: [PATCH] policy/v2: treat an ACL as one grant in ViaRoutesForPeer Ports never steer routes, and one grant per destination matched the sources once per destination. --- hscontrol/policy/v2/filter_test.go | 23 +++++++++++ hscontrol/policy/v2/policy.go | 10 ++++- hscontrol/policy/v2/policy_test.go | 66 ++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 1 deletion(-) diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index 37441c22b..4daa314a0 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 c9b93371b..3f7485ed7 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -1999,6 +1999,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()