state: update node in place on pre-auth-key re-registration

A reusable key on a converted node, or a tagged key on a user-owned node,
fell through to new-node creation, leaving two nodes per machine. Match by
machine key and update or convert in place.

Updates #3312

hscontrol/state/auth_tagged_expiry_test.go

@@ -233,3 +233,88 @@ func TestExpiredUserNodeReusedOneShotKey_SameNodeKey(t *testing.T) {
 		"expired node re-registering with the same node key must re-validate its key")
 	require.Contains(t, err.Error(), "authkey already used")
 }
+
+// TestReusableUserPAKReauthOnTaggedNodeNoDuplicate guards against a reusable
+// user pre-auth key creating a second node when it is re-presented for a node
+// that has since been converted to tagged. The node must be updated in place.
+func TestReusableUserPAKReauthOnTaggedNodeNoDuplicate(t *testing.T) {
+	dbPath := t.TempDir() + "/headscale.db"
+	cfg := persistTestConfig(dbPath)
+
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	user := s.CreateUserForTest("reusable-user")
+
+	policy := fmt.Sprintf(`{"tagOwners":{"tag:foo":["%s@"]}}`, user.Name)
+	_, err = s.SetPolicy([]byte(policy))
+	require.NoError(t, err)
+
+	pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+	require.NoError(t, err)
+
+	machineKey := key.NewMachine()
+	regReq := tailcfg.RegisterRequest{
+		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: pak.Key},
+		NodeKey:  key.NewNode().Public(),
+		Hostinfo: &tailcfg.Hostinfo{Hostname: "reusable-node"},
+		Expiry:   time.Now().Add(24 * time.Hour),
+	}
+
+	first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public())
+	require.NoError(t, err)
+
+	_, _, err = s.SetNodeTags(first.ID(), []string{"tag:foo"})
+	require.NoError(t, err)
+
+	second, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public())
+	require.NoError(t, err)
+	require.True(t, second.IsTagged())
+	require.Equal(t, first.ID(), second.ID(), "must update in place, not duplicate")
+	require.Equal(t, 1, s.ListNodes().Len(), "machine must map to exactly one node")
+}
+
+// TestTaggedPAKReauthConvertsUserOwnedNode ensures presenting a tagged pre-auth
+// key for a machine that already has a user-owned node converts that node in
+// place (same machine, new ownership) rather than creating a duplicate.
+func TestTaggedPAKReauthConvertsUserOwnedNode(t *testing.T) {
+	dbPath := t.TempDir() + "/headscale.db"
+	cfg := persistTestConfig(dbPath)
+
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	user := s.CreateUserForTest("owner")
+
+	userPak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+	require.NoError(t, err)
+
+	machineKey := key.NewMachine()
+	nodeKey := key.NewNode()
+	regReq := tailcfg.RegisterRequest{
+		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: userPak.Key},
+		NodeKey:  nodeKey.Public(),
+		Hostinfo: &tailcfg.Hostinfo{Hostname: "owned-node"},
+		Expiry:   time.Now().Add(24 * time.Hour),
+	}
+
+	owned, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public())
+	require.NoError(t, err)
+	require.False(t, owned.IsTagged(), "precondition: node is user-owned")
+
+	// A tags-only key re-registers the same machine (same node key).
+	taggedPak, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:foo"})
+	require.NoError(t, err)
+
+	convReq := regReq
+	convReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: taggedPak.Key}
+
+	converted, _, err := s.HandleNodeFromPreAuthKey(convReq, machineKey.Public())
+	require.NoError(t, err)
+	require.Equal(t, owned.ID(), converted.ID(), "must convert in place, not duplicate")
+	require.True(t, converted.IsTagged(), "node must become tagged")
+	require.Equal(t, []string{"tag:foo"}, converted.Tags().AsSlice())
+	require.Equal(t, 1, s.ListNodes().Len(), "machine must map to exactly one node")
+}

hscontrol/state/state.go

@@ -2187,10 +2187,13 @@ func (s *State) findExistingNodeForPAK(
 		}
 	}
 
-	// Tagged nodes have nil UserID, so they are indexed under UserID(0)
+	// A tagged key re-registers the same machine regardless of how that machine
-	// in nodesByMachineKey. Check there for tagged PAK re-registration.
+	// is currently owned: a tagged node (indexed under UserID(0)) is a plain
+	// re-registration, while a user-owned node is converted to tagged in place
+	// (handled by the caller). Match on the machine key alone so neither case
+	// creates a duplicate node.
 	if pak.IsTagged() {
-		return s.nodeStore.GetNodeByMachineKey(machineKey, 0)
+		return s.nodeStore.GetNodeByMachineKeyAnyUser(machineKey)
 	}
 
 	return types.NodeView{}, false
@@ -2241,7 +2244,13 @@ func (s *State) HandleNodeFromPreAuthKey(
 	isExpired := existsSameUser && existingNodeSameUser.Valid() &&
 		existingNodeSameUser.IsExpired()
 
-	if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired {
+	// A tagged key presented for a currently user-owned node converts that node
+	// to tagged. That is an ownership change, not a plain refresh, so it must
+	// present a valid key rather than ride the skip-validation fast-path.
+	isOwnershipConversion := existsSameUser && existingNodeSameUser.Valid() &&
+		pak.IsTagged() && !existingNodeSameUser.IsTagged()
+
+	if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && !isOwnershipConversion {
 		// Existing, still-valid node re-registering with same NodeKey: skip
 		// validation. Pre-auth keys are only needed for initial authentication.
 		// Critical for containers that run "tailscale up --authkey=KEY" on every
@@ -2327,8 +2336,17 @@ func (s *State) HandleNodeFromPreAuthKey(
 			node.RegisterMethod = util.RegisterMethodAuthKey
 
 			// Tags from PreAuthKey are only applied during initial registration.
-			// On re-registration the node keeps its existing tags and ownership.
+			// On re-registration the node keeps its existing tags and ownership,
-			// Only update AuthKey reference.
+			// except when a tagged key converts a user-owned node: that adopts
+			// the key's tags and drops user ownership (tagged nodes are
+			// user-less and never expire). Only update AuthKey reference
+			// otherwise.
+			if pak.IsTagged() && !node.IsTagged() {
+				node.Tags = pak.Proto().GetAclTags()
+				node.UserID = nil
+				node.User = nil
+				node.Expiry = nil
+			}
 			node.AuthKey = pak
 			node.AuthKeyID = &pak.ID
 			// Do NOT reset IsOnline here. Online status is managed exclusively by