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.
This commit is contained in:
Kristoffer Dalby committed 2026-10-28 16:35:21 +00:00
1 parent b102973c77
commit 13a4c22af0
5 files changed
+88 -7

No files matched your search

+27
View File
@@ -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)
-3
View File
@@ -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.
+4 -1
View File
@@ -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)
+50
View File
@@ -6,6 +6,7 @@ import (
"maps"
"net/http"
"net/netip"
"os"
"path/filepath"
"runtime"
"slices"
@@ -18,6 +19,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"
@@ -1312,6 +1314,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
+7 -3
View File
@@ -411,9 +411,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