state: send nothing for a policy write that changes nothing

As SaaS. Every accepted write sent all nodes a map and cleared SSH check
approvals, so a tool that re-posts the same file prompted users again.

hscontrol/servertest/apiv2_devices_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"net/url"
 	"os"
 	"os/exec"
@@ -484,6 +485,74 @@ func TestAPIV2TaggedDeviceUserCompat(t *testing.T) {
 	})
 }
 
+// TestNoChangePolicyWrite: SaaS sent nothing for the accepted writes in
+// policy-api that changed nothing. An approval to skip an SSH check must also
+// outlive such a write, as any rewrite by a gitops tool would otherwise drop it.
+func TestNoChangePolicyWrite(t *testing.T) {
+	t.Parallel()
+
+	const (
+		policy = `{"tagOwners":{"tag:server":["odin@"]},"acls":[{"action":"accept","src":["*"],"dst":["*:*"]}]}`
+		other  = `{"tagOwners":{"tag:server":["odin@"]},"acls":[{"action":"accept","src":["*"],"dst":["tag:server:22"]}]}`
+	)
+
+	srv := servertest.NewServer(t)
+	odin := srv.CreateUser(t, "odin")
+	key := srv.CreateAPIKey(t, odin)
+
+	post := func(body string) {
+		req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/v2/tailnet/-/acl", strings.NewReader(body))
+		req.Header.Set("Authorization", "Bearer "+key)
+		req.Header.Set("Content-Type", "application/hujson")
+
+		rec := httptest.NewRecorder()
+		srv.App.HTTPHandler().ServeHTTP(rec, req)
+		require.Equal(t, http.StatusOK, rec.Code, rec.Body.String())
+	}
+
+	post(policy)
+
+	desk := servertest.NewClient(t, srv, "desk", servertest.WithUser(odin))
+	server := servertest.NewClient(t, srv, "server", servertest.WithUser(odin), servertest.WithTags("tag:server"))
+	desk.WaitForPeers(t, 1, 20*time.Second)
+	server.WaitForPeers(t, 1, 20*time.Second)
+
+	src := types.NodeID(desk.Netmap().SelfNode.ID())   //nolint:gosec // test node IDs are small
+	dst := types.NodeID(server.Netmap().SelfNode.ID()) //nolint:gosec // test node IDs are small
+	srv.State().SetLastSSHAuth(src, dst)
+
+	// Late frames from the joins settle first.
+	var frames, steady int
+
+	require.Eventually(t, func() bool {
+		if n := desk.Frames(); n != frames {
+			frames, steady = n, 0
+
+			return false
+		}
+
+		steady++
+
+		return steady >= 10
+	}, 20*time.Second, 200*time.Millisecond, "frames settle after the joins")
+
+	post(policy)
+
+	assert.Never(t, func() bool { return desk.Frames() != frames }, 2*time.Second, 100*time.Millisecond,
+		"frames after a write that changes nothing")
+
+	_, kept := srv.State().GetLastSSHAuth(src, dst)
+	assert.True(t, kept, "SSH check approval after a write that changes nothing")
+
+	post(other)
+
+	assert.Eventually(t, func() bool { return desk.Frames() > frames }, 10*time.Second, 100*time.Millisecond,
+		"a write that changes the policy still reaches clients")
+
+	_, kept = srv.State().GetLastSSHAuth(src, dst)
+	assert.False(t, kept, "a changed policy drops SSH check approvals")
+}
+
 // deviceListChecks compares, per device in the step's device list, its user,
 // SaaS emails mapped to user names, and its tags.
 func deviceListChecks(t *testing.T, r *stepReplay, step testcapture.Step) []stepCheck {

hscontrol/state/state.go

@@ -511,14 +511,22 @@ func (s *State) ReloadPolicy() ([]change.Change, error) {
 	s.policyMu.Lock()
 	defer s.policyMu.Unlock()
 
-	return s.reloadPolicyLocked()
+	_, err := s.reloadPolicyLocked()
+	if err != nil {
+		return nil, err
+	}
+
+	// Nodes whose CapMap shifted get their self refresh from
+	// [State.DrainSelfRefreshes] when these changes are dispatched.
+	return []change.Change{change.PolicyChange()}, nil
 }
 
-// reloadPolicyLocked is [State.ReloadPolicy] for callers holding policyMu.
-func (s *State) reloadPolicyLocked() ([]change.Change, error) {
+// reloadPolicyLocked is [State.ReloadPolicy] for callers holding policyMu. It
+// reports whether the policy changed.
+func (s *State) reloadPolicyLocked() (bool, error) {
 	pol, err := hsdb.PolicyBytes(s.db.DB, s.cfg)
 	if err != nil {
-		return nil, fmt.Errorf("loading policy: %w", err)
+		return false, fmt.Errorf("loading policy: %w", err)
 	}
 
 	// nil means no policy is configured: nothing to install.
@@ -526,14 +534,10 @@ func (s *State) reloadPolicyLocked() ([]change.Change, error) {
 	if pol != nil {
 		policyChanged, err = s.polMan.SetPolicy(pol)
 		if err != nil {
-			return nil, fmt.Errorf("setting policy: %w", err)
+			return false, fmt.Errorf("setting policy: %w", err)
 		}
 	}
 
-	// Clear SSH check auth times when policy changes to ensure stale
-	// approvals don't persist if checkPeriod rules are modified or removed.
-	s.ClearSSHCheckAuth()
-
 	// Rebuild peer maps after policy changes because the peersFunc in [NodeStore]
 	// uses the [policy.PolicyManager]'s filters. Without this, nodes won't see
 	// newly allowed peers until a node is added/removed, causing autogroup:self
@@ -547,16 +551,14 @@ func (s *State) reloadPolicyLocked() ([]change.Change, error) {
 	// the policy with [State.SetPolicy], which rebuilds, store it, and
 	// reload the very same bytes.
 	if policyChanged {
+		// Stale approvals must not persist if checkPeriod rules are modified
+		// or removed.
+		s.ClearSSHCheckAuth()
 		s.nodeStore.RebuildPeerMaps()
-	}
-
-	if policyChanged {
 		log.Info().Msg("Policy reload completed with changes")
 	}
 
-	// Nodes whose CapMap shifted get their self refresh from
-	// [State.DrainSelfRefreshes] when these changes are dispatched.
-	return []change.Change{change.PolicyChange()}, nil
+	return policyChanged, nil
 }
 
 // CreateUser creates a new user and updates the policy manager.
@@ -1720,12 +1722,10 @@ func (s *State) SetPolicy(pol []byte) (bool, error) {
 		return changed, err
 	}
 
-	// Clear SSH check auth times when policy changes.
-	s.ClearSSHCheckAuth()
-
 	// Payload-only writes reuse the cached adjacency, so a policy swap
 	// must rebuild it here rather than wait for the next relation write.
 	if changed {
+		s.ClearSSHCheckAuth()
 		s.nodeStore.RebuildPeerMaps()
 	}
 
@@ -1870,11 +1870,17 @@ func (s *State) UpdatePolicy(pol []byte) (*types.Policy, []change.Change, error)
 		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.reloadPolicyLocked()
+	changed, err := s.reloadPolicyLocked()
+	if err != nil {
+		return stored, nil, err
+	}
 
-	return stored, cs, err
+	// As on SaaS, a write that changes nothing sends clients nothing.
+	if !changed {
+		return stored, nil, nil
+	}
+
+	return stored, []change.Change{change.PolicyChange()}, nil
 }
 
 // GetNodePrimaryRoutes returns the primary routes for a node.