diff --git a/hscontrol/state/persist_clobber_test.go b/hscontrol/state/persist_clobber_test.go new file mode 100644 index 00000000..b2ec961d --- /dev/null +++ b/hscontrol/state/persist_clobber_test.go @@ -0,0 +1,71 @@ +package state + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" +) + +// TestPersistNodeDoesNotClobberConcurrentAdminWrite ensures that persisting a +// node snapshot captured earlier (as UpdateNodeFromMapRequest does at the top +// of its body) cannot overwrite a concurrent admin write (SetNodeTags) that +// landed in between. NodeStore is the source of truth; the database row must +// converge on it rather than reverting to the stale snapshot. +func TestPersistNodeDoesNotClobberConcurrentAdminWrite(t *testing.T) { + dbPath, s, nodeID := persistTestSetup(t) + + pol := `{ + "tagOwners": {"tag:foo": ["persist-user@"]}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }` + _, err := s.SetPolicy([]byte(pol)) + require.NoError(t, err) + + before, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.False(t, before.IsTagged(), "node should start user-owned") + require.True(t, before.UserID().Valid(), "node should start with a UserID") + + // (1) Map-request captures the node snapshot (the stale view). + staleView, ok := s.nodeStore.GetNode(nodeID) + require.True(t, ok) + + staleNode := staleView.AsStruct() + staleNode.Hostinfo = &tailcfg.Hostinfo{Hostname: "persist-node"} + staleView = staleNode.View() + + // (2) Concurrent admin SetNodeTags lands in the window: NodeStore + DB + // become tagged and user ownership is cleared. + _, _, err = s.SetNodeTags(nodeID, []string{"tag:foo"}) + require.NoError(t, err) + + dbAfterAdmin, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + require.Equal(t, []string{"tag:foo"}, dbAfterAdmin.Tags.List(), + "precondition: admin SetNodeTags must have written the tag to the DB") + + // (3) Map-request persists its stale snapshot. + _, _, err = s.persistNodeToDB(staleView) + require.NoError(t, err) + + // The admin write must survive. + dbFinal, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + assert.Equal(t, []string{"tag:foo"}, dbFinal.Tags.List(), + "DB tags must reflect the admin SetNodeTags, not the stale persist") + assert.Nil(t, dbFinal.UserID, + "DB UserID must stay nil after tagging (tags XOR user ownership)") + + // Restart: the divergence would surface here in production. + require.NoError(t, s.Close()) + s2 := persistTestReopen(t, dbPath) + + reloaded, ok := s2.GetNodeByID(nodeID) + require.True(t, ok, "node should reload from DB after restart") + assert.True(t, reloaded.IsTagged(), + "after restart the node must still be tagged") + assert.Equal(t, []string{"tag:foo"}, reloaded.AsStruct().Tags.List(), + "after restart the node must still carry tag:foo") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 4daf18f3..be38c152 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -170,6 +170,11 @@ type State struct { // Ref: https://github.com/tailscale/tailscale/issues/7125 sshCheckAuth map[sshCheckPair]time.Time sshCheckMu sync.RWMutex + + // persistMu serialises the re-read-and-write critical section in + // persistNodeToDB so the database row always converges on [NodeStore] + // rather than being clobbered by a stale caller snapshot. + persistMu sync.Mutex } // NewState creates and initializes a new [State] instance, setting up the database, @@ -487,13 +492,23 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha return types.NodeView{}, change.Change{}, ErrInvalidNodeView } - // Verify the node still exists in [NodeStore] before persisting to database. - // Without this check, we could hit a race condition where [NodeStore.UpdateNode] - // returns a valid node from a batch update, then the node gets deleted (e.g., - // ephemeral node logout), and persistNodeToDB would incorrectly re-insert the - // deleted node into the database. - _, exists := s.nodeStore.GetNode(node.ID()) + // [NodeStore] is the source of truth and every caller updates it before + // persisting. Re-read the authoritative node under persistMu and write + // that, rather than the caller's `node` view which may have been captured + // earlier (e.g. at the top of UpdateNodeFromMapRequest) and gone stale + // behind a concurrent admin write such as SetNodeTags. Serialising the + // read+write keeps the database row converging on [NodeStore] instead of + // reverting it to an out-of-date column set. + // + // The same re-read also guards against the node having been deleted (e.g. + // ephemeral logout) between the caller's update and this persist: a missing + // node means we must not re-insert it. + s.persistMu.Lock() + + fresh, exists := s.nodeStore.GetNode(node.ID()) if !exists { + s.persistMu.Unlock() + log.Warn(). EmbedObject(node). Bool("is_ephemeral", node.IsEphemeral()). @@ -502,13 +517,14 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, node.ID()) } - nodePtr := node.AsStruct() + nodePtr := fresh.AsStruct() // Explicitly select all node columns so GORM includes nil/zero-value // fields (e.g. UserID=nil when converting a user-owned node to tagged). // Omit "Expiry" here: expiry is only updated through explicit // SetNodeExpiry calls or re-registration, not during MapRequest updates. err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error + s.persistMu.Unlock() if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("saving node: %w", err) } @@ -516,14 +532,14 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha // Check if policy manager needs updating c, err := s.updatePolicyManagerNodes() if err != nil { - return nodePtr.View(), change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err) + return fresh, change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err) } if c.IsEmpty() { c = change.NodeAdded(node.ID()) } - return node, c, nil + return fresh, c, nil } func (s *State) SaveNode(node types.NodeView) (types.NodeView, change.Change, error) {