From 2e1a716a9aacf4a77bf848fd50ba146fd065cae0 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Tue, 28 Apr 2026 09:21:18 +0000 Subject: [PATCH] policy/v2: fix empty grants/acls returning FilterAllowAll compileFilterRules, compileGrants, and updateLocked guarded the "no rules so allow all" fallback with len(pol.Grants) == 0, which matches both an absent grants field and an explicit empty array. JSON {"grants": []} unmarshals to a non-nil empty slice; it should compile to zero filter rules (deny all) to match Tailscale SaaS, but the length check sent it down the FilterAllowAll path. Distinguish absent (nil) from explicit-empty by switching the guard to pol.Grants == nil, the same asymmetry already used for ACLs. {} keeps allowing all; {"acls": []} and {"grants": []} now both deny all. Fixes #3211 --- hscontrol/policy/v2/compiled.go | 2 +- hscontrol/policy/v2/filter.go | 2 +- hscontrol/policy/v2/filter_test.go | 19 +++++++++++++++++-- hscontrol/policy/v2/policy.go | 2 +- hscontrol/policy/v2/policy_test.go | 14 ++++++++++++++ hscontrol/policy/v2/types_test.go | 29 +++++++++++++++++++++++++++++ 6 files changed, 63 insertions(+), 5 deletions(-) diff --git a/hscontrol/policy/v2/compiled.go b/hscontrol/policy/v2/compiled.go index 010382d4..935ccf03 100644 --- a/hscontrol/policy/v2/compiled.go +++ b/hscontrol/policy/v2/compiled.go @@ -101,7 +101,7 @@ func (pol *Policy) compileGrants( users types.Users, nodes views.Slice[types.NodeView], ) []compiledGrant { - if pol == nil || (pol.ACLs == nil && len(pol.Grants) == 0) { + if pol == nil || (pol.ACLs == nil && pol.Grants == nil) { return nil } diff --git a/hscontrol/policy/v2/filter.go b/hscontrol/policy/v2/filter.go index 47534272..9bdaa7d2 100644 --- a/hscontrol/policy/v2/filter.go +++ b/hscontrol/policy/v2/filter.go @@ -138,7 +138,7 @@ func (pol *Policy) compileFilterRules( users types.Users, nodes views.Slice[types.NodeView], ) ([]tailcfg.FilterRule, error) { - if pol == nil || (pol.ACLs == nil && len(pol.Grants) == 0) { + if pol == nil || (pol.ACLs == nil && pol.Grants == nil) { return tailcfg.FilterAllowAll, nil } diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index 333d4c41..ccf8d793 100644 --- a/hscontrol/policy/v2/filter_test.go +++ b/hscontrol/policy/v2/filter_test.go @@ -3537,11 +3537,26 @@ func TestFilterAllowAllFix(t *testing.T) { wantFilterAllow: true, }, { - name: "nil ACLs and empty grants returns FilterAllowAll", + name: "nil ACLs and empty grants denies all", pol: &Policy{ Grants: []Grant{}, }, - wantFilterAllow: true, + wantFilterAllow: false, + }, + { + name: "empty ACLs and nil grants denies all", + pol: &Policy{ + ACLs: []ACL{}, + }, + wantFilterAllow: false, + }, + { + name: "empty ACLs and empty grants denies all", + pol: &Policy{ + ACLs: []ACL{}, + Grants: []Grant{}, + }, + wantFilterAllow: false, }, { name: "both ACLs and grants should not return FilterAllowAll", diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 49546da8..892082c2 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -109,7 +109,7 @@ func (pm *PolicyManager) updateLocked() (bool, error) { pm.needsPerNodeFilter = hasPerNodeGrants(pm.compiledGrants) var filter []tailcfg.FilterRule - if pm.pol == nil || (pm.pol.ACLs == nil && len(pm.pol.Grants) == 0) { + if pm.pol == nil || (pm.pol.ACLs == nil && pm.pol.Grants == nil) { filter = tailcfg.FilterAllowAll } else { filter = globalFilterRules(pm.compiledGrants) diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index ee8af79f..a11ea416 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -44,6 +44,20 @@ func TestPolicyManager(t *testing.T) { wantFilter: tailcfg.FilterAllowAll, wantMatchers: matcher.MatchesFromFilterRules(tailcfg.FilterAllowAll), }, + { + name: "empty-acls-denies-all", + pol: `{"acls": []}`, + nodes: types.Nodes{}, + wantFilter: nil, + wantMatchers: matcher.MatchesFromFilterRules(nil), + }, + { + name: "empty-grants-denies-all", + pol: `{"grants": []}`, + nodes: types.Nodes{}, + wantFilter: nil, + wantMatchers: matcher.MatchesFromFilterRules(nil), + }, } for _, tt := range tests { diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index a90014db..ec2ad751 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -5212,3 +5212,32 @@ func TestGrantMarshalJSON(t *testing.T) { }) } } + +// TestUnmarshalPolicyEmptyArrays locks in the JSON unmarshal contract +// the FilterAllowAll guards depend on: an absent "acls" or "grants" +// field produces a nil slice, while an explicit empty array produces +// a non-nil empty slice. The compileFilterRules guard distinguishes +// these to differentiate "no policy → allow all" from "explicit +// empty rule set → deny all" (matching Tailscale SaaS). +func TestUnmarshalPolicyEmptyArrays(t *testing.T) { + cases := []struct { + name string + input string + aclsIsNil bool + grantsIsNil bool + }{ + {name: "empty-object", input: "{}", aclsIsNil: true, grantsIsNil: true}, + {name: "empty-acls", input: `{"acls": []}`, aclsIsNil: false, grantsIsNil: true}, + {name: "empty-grants", input: `{"grants": []}`, aclsIsNil: true, grantsIsNil: false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pol, err := unmarshalPolicy([]byte(tc.input)) + require.NoError(t, err) + require.NotNil(t, pol) + require.Equal(t, tc.aclsIsNil, pol.ACLs == nil, "ACLs nil-ness") + require.Equal(t, tc.grantsIsNil, pol.Grants == nil, "Grants nil-ness") + }) + } +}