diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 596a41bab..92b7463bb 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -673,10 +673,11 @@ func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { return false, fmt.Errorf("parsing policy: %w", err) } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + users, nodes := pm.users, pm.nodes + pm.mu.RUnlock() - err = validateUserReferences(pol, pm.users) + err = validateUserReferences(pol, users) if err != nil { return false, fmt.Errorf("validating policy user references: %w", err) } @@ -686,17 +687,25 @@ func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { // they fail, return without mutating the live PolicyManager so the // failed write does not knock the running config offline. // + // They run before the write lock is taken: how long they take is up + // to the policy author, and every map response and node write needs + // the lock. A user or node that changes meanwhile counts as changed + // right after the write. + // // Aggregate ACL and SSH test failures via multierr so operators // see both classes in a single response instead of having to // fix-and-retry to discover the second one. testErr := multierr.New( - evaluateTests(pol, pm.users, pm.nodes), - evaluateSSHTests(pol, pm.users, pm.nodes), + evaluateTests(pol, users, nodes), + evaluateSSHTests(pol, users, nodes), ) if testErr != nil { return false, testErr } + pm.mu.Lock() + defer pm.mu.Unlock() + // Log policy metadata for debugging log.Debug(). Int("policy.bytes", len(polB)). diff --git a/hscontrol/policy/v2/test_test.go b/hscontrol/policy/v2/test_test.go index 366a080fa..583350a4e 100644 --- a/hscontrol/policy/v2/test_test.go +++ b/hscontrol/policy/v2/test_test.go @@ -6,6 +6,7 @@ import ( "slices" "strings" "testing" + "time" "github.com/juanfont/headscale/hscontrol/types" "github.com/stretchr/testify/assert" @@ -439,6 +440,48 @@ func TestRunTestsManyAssertionsAndDestinations(t *testing.T) { require.NoError(t, evaluateTests(pol, users, nodes.ViewSlice())) } +// TestSetPolicyEvaluatesTestsUnlocked asserts that a policy's tests run +// before SetPolicy takes the write lock, so map generation and node writes +// do not wait for them: a reader holding the lock does not hold up the +// evaluation, and a rejected policy never needs the write lock at all. +func TestSetPolicyEvaluatesTestsUnlocked(t *testing.T) { + users := policyTestUsers() + nodes := policyTestNodes(users) + + pm, err := NewPolicyManager([]byte(`{}`), users, nodes.ViewSlice()) + require.NoError(t, err) + + bad := `{ + "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "acls": [{ + "action": "accept", + "src": ["alice@headscale.net"], + "dst": ["tag:server:22"] + }], + "tests": [{ + "src": "bob@headscale.net", + "accept": ["tag:server:22"] + }] + }` + + pm.mu.RLock() + defer pm.mu.RUnlock() + + done := make(chan error, 1) + + go func() { + _, err := pm.SetPolicy([]byte(bad)) + done <- err + }() + + select { + case err := <-done: + require.ErrorIs(t, err, errPolicyTestsFailed) + case <-time.After(10 * time.Second): + t.Fatal("SetPolicy waited for the write lock before evaluating the tests") + } +} + // TestSetPolicyRejectsFailingTests asserts that SetPolicy is the user-write // boundary: a policy whose tests fail must be rejected without mutating the // live PolicyManager. NewPolicyManager (boot path) does not run tests.