From 21ae79091a5c2c836f1db8e1bd2e7870a6d2024c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 13:31:52 +0000 Subject: [PATCH] state: install a written policy only after it is stored Writers validated by installing the policy live, so a store failure left a refused or unstored policy enforced. Validate on a throwaway manager, store, then reload from the stored row. --- hscontrol/api/v1/errors.go | 3 +- hscontrol/api/v1/policy.go | 49 ++--------------------- hscontrol/api/v2/acl.go | 24 +----------- hscontrol/api/v2/errors.go | 4 ++ hscontrol/apiv1_policy_test.go | 23 +++++++++++ hscontrol/apiv2_acl_test.go | 36 ++++++++++++++++- hscontrol/state/persist_test.go | 27 +++++++++++++ hscontrol/state/state.go | 69 ++++++++++++++++++++++++++++++++- 8 files changed, 164 insertions(+), 71 deletions(-) diff --git a/hscontrol/api/v1/errors.go b/hscontrol/api/v1/errors.go index 87ef93de2..0724aaf78 100644 --- a/hscontrol/api/v1/errors.go +++ b/hscontrol/api/v1/errors.go @@ -38,7 +38,8 @@ func mapError(msg string, err error) error { errors.Is(err, db.ErrUserStillHasNodes), errors.Is(err, db.ErrCannotChangeOIDCUser), errors.Is(err, db.ErrPreAuthKeyNotTaggedOrOwned), - errors.Is(err, db.ErrSingleUseAuthKeyHasBeenUsed): + errors.Is(err, db.ErrSingleUseAuthKeyHasBeenUsed), + errors.Is(err, state.ErrInvalidPolicy): return huma.Error400BadRequest(msg, err) case errors.Is(err, state.ErrNodeKeyInUse), diff --git a/hscontrol/api/v1/policy.go b/hscontrol/api/v1/policy.go index bfae2d139..2588b435b 100644 --- a/hscontrol/api/v1/policy.go +++ b/hscontrol/api/v1/policy.go @@ -9,7 +9,6 @@ import ( "time" "github.com/danielgtaylor/huma/v2" - policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" ) @@ -116,37 +115,11 @@ func registerPolicy(api huma.API, b Backend) { ) } - p := in.Body.Policy - - // Reject policy that would fail when building a map response. SSH rule - // validation needs a node, so a server with no nodes can't catch every - // case here. - nodes := b.State.ListNodes() - - _, err := b.State.SetPolicy([]byte(p)) - if err != nil { - return nil, huma.Error400BadRequest("setting policy", err) - } - - if nodes.Len() > 0 { - _, err = b.State.SSHPolicy(nodes.At(0)) - if err != nil { - return nil, huma.Error400BadRequest("verifying SSH rules", err) - } - } - - updated, err := b.State.SetPolicyInDB(p) - if err != nil { - return nil, huma.Error500InternalServerError("setting policy", err) - } - - // Reload even when content is unchanged: routes manually disabled before - // may now qualify for auto-approval, so they must be re-evaluated. - cs, err := b.State.ReloadPolicy() + updated, cs, err := b.State.UpdatePolicy([]byte(in.Body.Policy)) b.Change(cs...) if err != nil { - return nil, huma.Error500InternalServerError("reloading policy", err) + return nil, mapError("setting policy", err) } out := &setPolicyOutput{} @@ -165,23 +138,9 @@ func registerPolicy(api huma.API, b Backend) { Tags: []string{"Policy"}, Security: bearerAuth, }, func(ctx context.Context, in *checkPolicyInput) (*checkPolicyOutput, error) { - polB := []byte(in.Body.Policy) - - users, err := b.State.ListAllUsers() + err := b.State.ValidatePolicy([]byte(in.Body.Policy)) if err != nil { - return nil, huma.Error500InternalServerError("loading users", err) - } - - nodes := b.State.ListNodes() - - pm, err := policyv2.NewPolicyManager(polB, users, nodes) - if err != nil { - return nil, huma.Error400BadRequest(err.Error(), err) - } - - _, err = pm.SetPolicy(polB) - if err != nil { - return nil, huma.Error400BadRequest(err.Error(), err) + return nil, mapError(err.Error(), err) } return &checkPolicyOutput{}, nil diff --git a/hscontrol/api/v2/acl.go b/hscontrol/api/v2/acl.go index 3d6329fe6..3c7dd28fb 100644 --- a/hscontrol/api/v2/acl.go +++ b/hscontrol/api/v2/acl.go @@ -113,31 +113,11 @@ func registerACL(api huma.API, b Backend) { } } - // Mirror the v1 setPolicy flow: validate, SSH-check, persist, reload. - nodes := b.State.ListNodes() - - _, err = b.State.SetPolicy(in.RawBody) - if err != nil { - return nil, huma.Error400BadRequest("setting policy", err) - } - - if nodes.Len() > 0 { - _, err = b.State.SSHPolicy(nodes.At(0)) - if err != nil { - return nil, huma.Error400BadRequest("verifying SSH rules", err) - } - } - - updated, err := b.State.SetPolicyInDB(string(in.RawBody)) - if err != nil { - return nil, huma.Error500InternalServerError("setting policy", err) - } - - cs, err := b.State.ReloadPolicy() + updated, cs, err := b.State.UpdatePolicy(in.RawBody) b.Change(cs...) if err != nil { - return nil, huma.Error500InternalServerError("reloading policy", err) + return nil, mapError("setting policy", err) } return streamPolicy([]byte(updated.Data), aclContentType(in.Accept)), nil diff --git a/hscontrol/api/v2/errors.go b/hscontrol/api/v2/errors.go index 3a918eda4..82c389bc9 100644 --- a/hscontrol/api/v2/errors.go +++ b/hscontrol/api/v2/errors.go @@ -79,6 +79,10 @@ func mapError(msg string, err error) error { errors.Is(err, state.ErrRequestedTagsInvalidOrNotPermitted): return huma.Error400BadRequest(msg, err) + // A refused policy is answered with its own reason as the message. + case errors.Is(err, state.ErrInvalidPolicy): + return huma.Error400BadRequest(err.Error(), err) + default: return huma.Error500InternalServerError(msg, err) } diff --git a/hscontrol/apiv1_policy_test.go b/hscontrol/apiv1_policy_test.go index bf8830e28..b48047233 100644 --- a/hscontrol/apiv1_policy_test.go +++ b/hscontrol/apiv1_policy_test.go @@ -97,6 +97,29 @@ func TestAPIV1PolicySet(t *testing.T) { assert.NotEmpty(t, got.UpdatedAt) }) + t.Run("persist failure keeps live policy", func(t *testing.T) { + h := newAPIV1Harness(t) + + body, err := json.Marshal(map[string]string{"policy": `{"acls":[]}`}) + require.NoError(t, err) + require.Equal(t, http.StatusOK, + h.callHuma(http.MethodPut, "/api/v1/policy", body).status) + + before, _ := h.app.state.Filter() + + require.NoError(t, h.app.state.DB().DB. + Exec("ALTER TABLE policies RENAME TO policies_away").Error) + + body, err = json.Marshal(map[string]string{"policy": validPolicy}) + require.NoError(t, err) + + res := h.callHuma(http.MethodPut, "/api/v1/policy", body) + assertStatus(t, res, http.StatusInternalServerError) + + after, _ := h.app.state.Filter() + assert.Equal(t, before, after, "an unstored policy must not be enforced") + }) + t.Run("empty rejected", func(t *testing.T) { h := newAPIV1Harness(t) diff --git a/hscontrol/apiv2_acl_test.go b/hscontrol/apiv2_acl_test.go index 8f21ed525..7ba2797cf 100644 --- a/hscontrol/apiv2_acl_test.go +++ b/hscontrol/apiv2_acl_test.go @@ -4,6 +4,7 @@ import ( "bytes" "crypto/sha256" "encoding/hex" + "encoding/json" "errors" "net/http" "net/http/httptest" @@ -190,9 +191,21 @@ func TestAPIv2ACLInvalidPolicyAtomicity(t *testing.T) { goodETag := good.Header().Get("Etag") // Malformed body -> 400, and the stored policy is unchanged (atomicity). - bad := setACL(t, api, `{ this is not valid`, "application/json") + const malformed = `{ this is not valid` + + bad := setACL(t, api, malformed, "application/json") assert.Equal(t, http.StatusBadRequest, bad.Code) - assert.Contains(t, bad.Body.String(), `"message"`) + + var answer struct { + Message string `json:"message"` + } + require.NoError(t, json.Unmarshal(bad.Body.Bytes(), &answer)) + + reason := app.state.ValidatePolicy([]byte(malformed)) + require.Error(t, reason) + assert.Equal(t, reason.Error(), answer.Message, + "the answer gives the parser's reason as it is") + assert.NotContains(t, answer.Message, "invalid policy") assert.JSONEq(t, allowAllPolicy, storedPolicy(t, app)) // Wire-level: GET still serves the good policy + its ETag (no drift). @@ -201,6 +214,25 @@ func TestAPIv2ACLInvalidPolicyAtomicity(t *testing.T) { assert.JSONEq(t, allowAllPolicy, storedPolicy(t, app)) } +func TestAPIv2ACLPersistFailureKeepsLivePolicy(t *testing.T) { + app := createTestApp(t) + api := registerAPIV2(t, app) + + require.Equal(t, http.StatusOK, + setACL(t, api, `{"acls":[]}`, "application/json").Code) + + before, _ := app.state.Filter() + + require.NoError(t, + app.state.DB().DB.Exec("ALTER TABLE policies RENAME TO policies_away").Error) + + assert.Equal(t, http.StatusInternalServerError, + setACL(t, api, allowAllPolicy, "application/json").Code) + + after, _ := app.state.Filter() + assert.Equal(t, before, after, "an unstored policy must not be enforced") +} + func TestAPIv2ACLNonDefaultTailnet404(t *testing.T) { api := registerAPIV2(t, createTestApp(t)) diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 85e35248e..042db6381 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1314,6 +1314,33 @@ func TestReloadPolicyOverlappingWrites(t *testing.T) { assert.Equal(t, want, got, "the policy in force must be the one stored last") } +// TestUpdatePolicyStoreFailure refuses the write at the database and checks +// the live policy was not changed by it. +func TestUpdatePolicyStoreFailure(t *testing.T) { + _, s, _ := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + const ( + open = `{"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]}` + ssh = `{"acls": [{"action": "accept", "src": ["persist-user@"], "dst": ["persist-user@:22"]}]}` + ) + + _, _, err := s.UpdatePolicy([]byte(ssh)) + require.NoError(t, err) + + before, _ := s.Filter() + + require.NoError(t, + s.db.DB.Exec("ALTER TABLE policies RENAME TO policies_away").Error) + + _, _, err = s.UpdatePolicy([]byte(open)) + require.Error(t, err) + require.NotErrorIs(t, err, ErrInvalidPolicy) + + after, _ := s.Filter() + assert.Equal(t, before, after, "an unstored policy must not be installed") +} + // 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) { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 762d4ca57..01d9ef3c1 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -67,6 +67,17 @@ const ( // ErrUnsupportedPolicyMode is returned for invalid policy modes. Valid modes are "file" and "db". var ErrUnsupportedPolicyMode = errors.New("unsupported policy mode") +// ErrInvalidPolicy is returned by [State.UpdatePolicy] and +// [State.ValidatePolicy] for a policy they refuse. +var ErrInvalidPolicy = errors.New("invalid policy") + +// invalidPolicyError marks a refusal as [ErrInvalidPolicy] and keeps the +// reason's own text, which API answers show as is. +type invalidPolicyError struct{ reason error } + +func (e invalidPolicyError) Error() string { return e.reason.Error() } +func (e invalidPolicyError) Unwrap() []error { return []error{ErrInvalidPolicy, e.reason} } + // ErrNodeNotFound is returned when a node cannot be found by its ID. var ErrNodeNotFound = errors.New("node not found") @@ -1404,7 +1415,8 @@ func (s *State) NodeCanHaveTag(node types.NodeView, tag string) bool { return s.polMan.NodeCanHaveTag(node, tag) } -// SetPolicy updates the policy configuration. +// SetPolicy installs pol without storing it. Policy writes use +// [State.UpdatePolicy]. func (s *State) SetPolicy(pol []byte) (bool, error) { changed, err := s.polMan.SetPolicy(pol) if err != nil { @@ -1466,6 +1478,61 @@ func (s *State) SetPolicyInDB(data string) (*types.Policy, error) { return s.db.SetPolicy(data) } +// ValidatePolicy reports whether [State.UpdatePolicy] would accept pol. It +// compiles pol against the current users and nodes on a throwaway policy +// manager; the live policy is not touched. +func (s *State) ValidatePolicy(pol []byte) error { + users, err := s.ListAllUsers() + if err != nil { + return fmt.Errorf("loading users: %w", err) + } + + nodes := s.ListNodes() + + pm, err := policy.NewPolicyManager(nil, users, nodes) + if err != nil { + return fmt.Errorf("creating policy manager: %w", err) + } + + _, err = pm.SetPolicy(pol) + if err != nil { + return invalidPolicyError{err} + } + + // SSH rules compile per node, so a server without nodes cannot catch + // every case here. + if nodes.Len() > 0 { + _, err = pm.SSHPolicy(s.cfg.ServerURL, nodes.At(0)) + if err != nil { + return invalidPolicyError{fmt.Errorf("verifying SSH rules: %w", err)} + } + } + + return nil +} + +// UpdatePolicy validates, stores and installs pol. The live policy is only +// ever installed from what was stored, so a refused or unstored write leaves +// it untouched. The changes are returned alongside a reload error, as from +// [State.ReloadPolicy]. +func (s *State) UpdatePolicy(pol []byte) (*types.Policy, []change.Change, error) { + err := s.ValidatePolicy(pol) + if err != nil { + return nil, nil, err + } + + stored, err := s.db.SetPolicy(string(pol)) + if err != nil { + return nil, nil, fmt.Errorf("storing policy: %w", err) + } + + // Reload even when content is unchanged: routes manually disabled before + // may now qualify for auto-approval, so they must be re-evaluated. + cs, err := s.ReloadPolicy() + + return stored, cs, err +} + // GetNodePrimaryRoutes returns the primary routes for a node. func (s *State) GetNodePrimaryRoutes(nodeID types.NodeID) []netip.Prefix { return s.nodeStore.PrimaryRoutesForNode(nodeID)