diff --git a/hscontrol/servertest/apiv2_devices_test.go b/hscontrol/servertest/apiv2_devices_test.go index b1bf43501..b244f7671 100644 --- a/hscontrol/servertest/apiv2_devices_test.go +++ b/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 { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index c68202067..34c53d639 100644 --- a/hscontrol/state/state.go +++ b/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.