From 3a57f3a6e3a0f626434cd5f607d1b2dbf34bfa84 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 13:25:23 +0000 Subject: [PATCH] policy/v2: refuse an empty policy SetPolicy returned a nil error for empty input it ignored, so the v1 API stored it as valid. Reload skips only the nil "no policy" case. --- hscontrol/apiv1_policy_test.go | 27 ++++++++++++++++++ hscontrol/apiv2_compat_test.go | 3 -- hscontrol/policy/v2/policy.go | 5 +++- hscontrol/state/persist_test.go | 50 +++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 10 +++++-- 5 files changed, 88 insertions(+), 7 deletions(-) diff --git a/hscontrol/apiv1_policy_test.go b/hscontrol/apiv1_policy_test.go index 0c4cfefa0..bf8830e28 100644 --- a/hscontrol/apiv1_policy_test.go +++ b/hscontrol/apiv1_policy_test.go @@ -97,6 +97,24 @@ func TestAPIV1PolicySet(t *testing.T) { assert.NotEmpty(t, got.UpdatedAt) }) + t.Run("empty rejected", func(t *testing.T) { + h := newAPIV1Harness(t) + + body, err := json.Marshal(map[string]string{"policy": validPolicy}) + require.NoError(t, err) + require.Equal(t, http.StatusOK, + h.callHuma(http.MethodPut, "/api/v1/policy", body).status) + + for _, body := range []string{`{"policy":""}`, `{}`} { + res := h.callHuma(http.MethodPut, "/api/v1/policy", []byte(body)) + assertStatus(t, res, http.StatusBadRequest) + } + + stored, err := h.app.state.GetPolicy() + require.NoError(t, err) + assert.JSONEq(t, validPolicy, stored.Data) + }) + t.Run("not-db mode rejected", func(t *testing.T) { // Policy updates are only valid in DB mode; file mode rejects with 400. // createTestApp is DB mode, so build a file-mode app here. @@ -132,6 +150,15 @@ func TestAPIV1PolicyCheck(t *testing.T) { assertStatus(t, res, http.StatusBadRequest) }) + t.Run("empty rejected", func(t *testing.T) { + h := newAPIV1Harness(t) + + res := h.callHuma( + http.MethodPost, "/api/v1/policy/check", []byte(`{"policy":""}`), + ) + assertStatus(t, res, http.StatusBadRequest) + }) + t.Run("valid returns empty object", func(t *testing.T) { h := newAPIV1Harness(t) diff --git a/hscontrol/apiv2_compat_test.go b/hscontrol/apiv2_compat_test.go index 14518564f..19fab84f4 100644 --- a/hscontrol/apiv2_compat_test.go +++ b/hscontrol/apiv2_compat_test.go @@ -193,8 +193,6 @@ var apiv2CompatSkipReasons = map[string]string{ "policy-api/P7/0/body": hujsonFormatted, "policy-api/P7/1/body": hujsonFormatted, - - "policy-api/P2/0/v1-verdict": v1CheckAcceptsEmpty, } // apiv2CompatNotCompared lists, by key prefix, checks left out: SaaS's @@ -232,7 +230,6 @@ const ( taggedAllClientAnyTag = "TAGGED_ALL_CLIENT_ANY_TAG: SaaS lets a tagged all-scope client's token ask only for the client's tags; headscale lets it ask for any tag" retagRemovesUnownedTag = "RETAG_REMOVES_UNOWNED_TAG: SaaS refuses a token's retag of a device holding a tag outside the token's grant with 400 cannot remove current tags; headscale replaces the tags" sshTestHostnames = "SSH_TEST_HOSTNAMES: headscale names the nodes behind a failing sshTests destination; SaaS names the destination as written" - v1CheckAcceptsEmpty = "V1_CHECK_ACCEPTS_EMPTY: v1 policy check accepts a zero-length policy; SaaS's validate refuses it" ) // Differences no planned change removes yet. diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index dbefc5bd0..f2d5d5afd 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -29,6 +29,9 @@ import ( // ErrInvalidTagOwner is returned when a tag owner is not an [Alias] type. var ErrInvalidTagOwner = errors.New("tag owner is not an Alias") +// ErrPolicyEmpty is returned by [PolicyManager.SetPolicy] for a zero-length policy. +var ErrPolicyEmpty = errors.New(`policy is empty, use "{}" to allow all`) + type PolicyManager struct { // RWMutex, not Mutex, so concurrent map generation does not serialise on // reads. The per-node caches are xsync.Maps so a read can fill them without @@ -658,7 +661,7 @@ func (pm *PolicyManager) sshRuleAllowsUser(rule SSH, srcNode types.NodeView, loc func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { if len(polB) == 0 { - return false, nil + return false, ErrPolicyEmpty } pol, err := unmarshalPolicy(polB) diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index b1710f7bd..6fd2f9218 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -6,6 +6,7 @@ import ( "maps" "net/http" "net/netip" + "os" "path/filepath" "slices" "strconv" @@ -17,6 +18,7 @@ import ( "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/policy" + policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" "github.com/juanfont/headscale/hscontrol/types/testcapture" @@ -1216,6 +1218,54 @@ func TestReloadPolicyOverlappingWrites(t *testing.T) { assert.Equal(t, want, got, "the policy in force must be the one stored last") } +// TestReloadPolicyEmpty: with no policy configured a reload has nothing to +// install; an emptied policy file is refused. Both keep the live policy. +func TestReloadPolicyEmpty(t *testing.T) { + const pol = `{"acls": [{"action": "accept", "src": ["persist-user@"], "dst": ["persist-user@:22"]}]}` + + t.Run("no policy configured", func(t *testing.T) { + _, s, _ := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + _, err := s.SetPolicy([]byte(pol)) + require.NoError(t, err) + + before, _ := s.Filter() + + _, err = s.ReloadPolicy() + require.NoError(t, err) + + after, _ := s.Filter() + assert.Equal(t, before, after) + }) + + t.Run("empty policy file", func(t *testing.T) { + _, s, _ := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + s.cfg.Policy = types.PolicyConfig{ + Mode: types.PolicyModeFile, + Path: filepath.Join(t.TempDir(), "policy.hujson"), + } + + require.NoError(t, os.WriteFile(s.cfg.Policy.Path, []byte(pol), 0o600)) + + _, err := s.ReloadPolicy() + require.NoError(t, err) + + before, _ := s.Filter() + + // os.ReadFile gives an empty, non-nil slice here. + require.NoError(t, os.WriteFile(s.cfg.Policy.Path, nil, 0o600)) + + _, err = s.ReloadPolicy() + require.ErrorIs(t, err, policyv2.ErrPolicyEmpty) + + after, _ := s.Filter() + assert.Equal(t, before, after) + }) +} + // TestNodeWriteChangeWhenPolicyRefreshFails fails both the NodeStore // writer's SetNodes and the caller's. The write still reached the // NodeStore, so the change returned with the error must resend the node to diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index e64f8b973..b9549229e 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -367,9 +367,13 @@ func (s *State) ReloadPolicy() ([]change.Change, error) { return nil, fmt.Errorf("loading policy: %w", err) } - policyChanged, err := s.polMan.SetPolicy(pol) - if err != nil { - return nil, fmt.Errorf("setting policy: %w", err) + // nil means no policy is configured: nothing to install. + var policyChanged bool + if pol != nil { + policyChanged, err = s.polMan.SetPolicy(pol) + if err != nil { + return nil, fmt.Errorf("setting policy: %w", err) + } } // Clear SSH check auth times when policy changes to ensure stale