diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 4eb028a3..ed294f3c 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -1699,7 +1699,7 @@ func TestAuthenticationFlows(t *testing.T) { assert.False(t, resp.NodeKeyExpired) // Verify NEW node was created for user2 - node2, found := app.state.GetNodeByMachineKey(machineKey1.Public(), types.UserID(2)) + node2, found := app.state.GetNodesByMachineKeyAllUsers(machineKey1.Public())[types.UserID(2)] require.True(t, found, "new node should exist for user2") assert.Equal(t, uint(2), node2.UserID().Get(), "new node should belong to user2") @@ -1707,7 +1707,7 @@ func TestAuthenticationFlows(t *testing.T) { assert.Equal(t, "user2-context", user.Name(), "new node should show user2 username") // Verify original node still exists for user1 - node1, found := app.state.GetNodeByMachineKey(machineKey1.Public(), types.UserID(1)) + node1, found := app.state.GetNodesByMachineKeyAllUsers(machineKey1.Public())[types.UserID(1)] require.True(t, found, "original node should still exist for user1") assert.Equal(t, uint(1), node1.UserID().Get(), "original node should still belong to user1") @@ -1775,13 +1775,13 @@ func TestAuthenticationFlows(t *testing.T) { validateCompleteResponse: true, validate: func(t *testing.T, resp *tailcfg.RegisterResponse, app *Headscale) { //nolint:thelper // User1's original node should STILL exist (not transferred) - node1, found1 := app.state.GetNodeByMachineKey(machineKey1.Public(), types.UserID(1)) + node1, found1 := app.state.GetNodesByMachineKeyAllUsers(machineKey1.Public())[types.UserID(1)] require.True(t, found1, "user1's original node should still exist") assert.Equal(t, uint(1), node1.UserID().Get(), "user1's node should still belong to user1") assert.Equal(t, nodeKey1.Public(), node1.NodeKey(), "user1's node should have original node key") // User2 should have a NEW node created - node2, found2 := app.state.GetNodeByMachineKey(machineKey1.Public(), types.UserID(2)) + node2, found2 := app.state.GetNodesByMachineKeyAllUsers(machineKey1.Public())[types.UserID(2)] require.True(t, found2, "user2 should have new node created") assert.Equal(t, uint(2), node2.UserID().Get(), "user2's node should belong to user2") @@ -2914,7 +2914,7 @@ func TestPreAuthKeyLogoutAndReloginDifferentUser(t *testing.T) { for i := range 2 { node := nodes[i] // User1's original nodes should still be owned by user1 - registeredNode, found := app.state.GetNodeByMachineKey(node.machineKey.Public(), types.UserID(user1.ID)) + registeredNode, found := app.state.GetNodesByMachineKeyAllUsers(node.machineKey.Public())[types.UserID(user1.ID)] require.True(t, found, "User1's original node %s should still exist", node.hostname) require.Equal(t, user1.ID, registeredNode.UserID().Get(), "Node %s should still belong to user1", node.hostname) t.Logf("✓ User1's original node %s (ID=%d) still owned by user1", node.hostname, registeredNode.ID().Uint64()) @@ -2923,7 +2923,7 @@ func TestPreAuthKeyLogoutAndReloginDifferentUser(t *testing.T) { for i := 2; i < 4; i++ { node := nodes[i] // User2's original nodes should still be owned by user2 - registeredNode, found := app.state.GetNodeByMachineKey(node.machineKey.Public(), types.UserID(user2.ID)) + registeredNode, found := app.state.GetNodesByMachineKeyAllUsers(node.machineKey.Public())[types.UserID(user2.ID)] require.True(t, found, "User2's original node %s should still exist", node.hostname) require.Equal(t, user2.ID, registeredNode.UserID().Get(), "Node %s should still belong to user2", node.hostname) t.Logf("✓ User2's original node %s (ID=%d) still owned by user2", node.hostname, registeredNode.ID().Uint64()) @@ -2935,7 +2935,7 @@ func TestPreAuthKeyLogoutAndReloginDifferentUser(t *testing.T) { for i := 2; i < 4; i++ { node := nodes[i] // Should be able to find a node with user1 and this machine key (the new one) - newNode, found := app.state.GetNodeByMachineKey(node.machineKey.Public(), types.UserID(user1.ID)) + newNode, found := app.state.GetNodesByMachineKeyAllUsers(node.machineKey.Public())[types.UserID(user1.ID)] require.True(t, found, "Should have created new node for user1 with machine key from %s", node.hostname) require.Equal(t, user1.ID, newNode.UserID().Get(), "New node should belong to user1") t.Logf("✓ New node created for user1 with machine key from %s (ID=%d)", node.hostname, newNode.ID().Uint64()) @@ -2984,7 +2984,7 @@ func TestWebFlowReauthDifferentUser(t *testing.T) { require.True(t, resp1.MachineAuthorized, "Should be authorized via pre-auth key") // Verify node exists for user1 - user1Node, found := app.state.GetNodeByMachineKey(machineKey.Public(), types.UserID(user1.ID)) + user1Node, found := app.state.GetNodesByMachineKeyAllUsers(machineKey.Public())[types.UserID(user1.ID)] require.True(t, found, "Node should exist for user1") require.Equal(t, user1.ID, user1Node.UserID().Get(), "Node should belong to user1") user1NodeID := user1Node.ID() @@ -3000,7 +3000,7 @@ func TestWebFlowReauthDifferentUser(t *testing.T) { require.NoError(t, err) // Verify node is expired - user1Node, found = app.state.GetNodeByMachineKey(machineKey.Public(), types.UserID(user1.ID)) + user1Node, found = app.state.GetNodesByMachineKeyAllUsers(machineKey.Public())[types.UserID(user1.ID)] require.True(t, found, "Node should still exist after logout") require.True(t, user1Node.IsExpired(), "Node should be expired after logout") t.Logf("✓ User1 node expired (logged out)") @@ -3041,7 +3041,7 @@ func TestWebFlowReauthDifferentUser(t *testing.T) { t.Run("user1_original_node_still_exists", func(t *testing.T) { // User1's original node should STILL exist (not transferred to user2) - user1NodeAfter, found1 := app.state.GetNodeByMachineKey(machineKey.Public(), types.UserID(user1.ID)) + user1NodeAfter, found1 := app.state.GetNodesByMachineKeyAllUsers(machineKey.Public())[types.UserID(user1.ID)] assert.True(t, found1, "User1's original node should still exist (not transferred)") if !found1 { @@ -3056,7 +3056,7 @@ func TestWebFlowReauthDifferentUser(t *testing.T) { t.Run("user2_has_new_node_created", func(t *testing.T) { // User2 should have a NEW node created (not transfer from user1) - user2Node, found2 := app.state.GetNodeByMachineKey(machineKey.Public(), types.UserID(user2.ID)) + user2Node, found2 := app.state.GetNodesByMachineKeyAllUsers(machineKey.Public())[types.UserID(user2.ID)] assert.True(t, found2, "User2 should have a new node created") if !found2 { @@ -3080,8 +3080,8 @@ func TestWebFlowReauthDifferentUser(t *testing.T) { t.Run("both_nodes_share_machine_key", func(t *testing.T) { // Both nodes should have the same machine key (same physical device) - user1NodeFinal, found1 := app.state.GetNodeByMachineKey(machineKey.Public(), types.UserID(user1.ID)) - user2NodeFinal, found2 := app.state.GetNodeByMachineKey(machineKey.Public(), types.UserID(user2.ID)) + user1NodeFinal, found1 := app.state.GetNodesByMachineKeyAllUsers(machineKey.Public())[types.UserID(user1.ID)] + user2NodeFinal, found2 := app.state.GetNodesByMachineKeyAllUsers(machineKey.Public())[types.UserID(user2.ID)] require.True(t, found1, "User1 node should exist") require.True(t, found2, "User2 node should exist") diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 0544e049..4eef8fee 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -319,6 +319,95 @@ func TestTaggedPAKReauthConvertsUserOwnedNode(t *testing.T) { require.Equal(t, 1, s.ListNodes().Len(), "machine must map to exactly one node") } +// registerTwoUsersOnOneMachine registers two user-owned nodes that share a +// machine key (the "create new, do not transfer" multi-user device state) and +// returns the State and the shared machine key. +func registerTwoUsersOnOneMachine(t *testing.T) (*State, key.MachinePublic, types.NodeID) { + t.Helper() + + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + u1 := s.CreateUserForTest("u1") + u2 := s.CreateUserForTest("u2") + mk := key.NewMachine() + + reg := func(pakUser *types.User) types.NodeView { + pak, err := s.CreatePreAuthKey(pakUser.TypedID(), true, false, nil, nil) + require.NoError(t, err) + n, _, err := s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "multi"}, + Expiry: time.Now().Add(24 * time.Hour), + }, mk.Public()) + require.NoError(t, err) + + return n + } + + n1 := reg(u1) + n2 := reg(u2) + require.NotEqual(t, n1.ID(), n2.ID(), "precondition: two distinct nodes share the machine key") + require.Equal(t, 2, s.ListNodes().Len()) + + return s, mk.Public(), n1.ID() +} + +// TestTaggedPAKReauthRejectsAmbiguousMultiUserNode: a tagged pre-auth key on a +// machine that has more than one user-owned node cannot know which to convert, +// so the registration is rejected rather than converting an arbitrary one and +// orphaning the rest. +func TestTaggedPAKReauthRejectsAmbiguousMultiUserNode(t *testing.T) { + s, mk, _ := registerTwoUsersOnOneMachine(t) + + taggedPak, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:foo"}) + require.NoError(t, err) + + _, _, err = s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: taggedPak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "multi"}, + Expiry: time.Now().Add(24 * time.Hour), + }, mk) + require.ErrorIs(t, err, ErrAmbiguousNodeOwnership) + require.Equal(t, 2, s.ListNodes().Len(), "no node created or converted on rejection") +} + +// TestAuthPathRejectsTaggedAndUserCoexistence: if a machine key ends up with +// both a tagged node and a user-owned node (impossible per validateNodeOwnership, +// but reachable by tagging one node of a multi-user device via the admin path), +// an OIDC re-auth must reject rather than silently converting the tagged node +// and orphaning the user-owned one. +func TestAuthPathRejectsTaggedAndUserCoexistence(t *testing.T) { + s, mk, n1 := registerTwoUsersOnOneMachine(t) + + // Tag one of the two user-owned nodes -> {0: tagged, u2: user-owned} coexist. + _, err := s.SetPolicy([]byte(`{"tagOwners":{"tag:foo":["u1@"]}}`)) + require.NoError(t, err) + tagged, _, err := s.SetNodeTags(n1, []string{"tag:foo"}) + require.NoError(t, err) + require.True(t, tagged.IsTagged()) + + // A third user authenticates the same machine via OIDC. + u3 := s.CreateUserForTest("u3") + regData := &types.RegistrationData{ + MachineKey: mk, + NodeKey: key.NewNode().Public(), + Hostname: "multi", + Hostinfo: &tailcfg.Hostinfo{Hostname: "multi"}, + } + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(regData)) + + _, _, err = s.HandleNodeFromAuthPath(authID, types.UserID(u3.ID), nil, util.RegisterMethodOIDC) + require.ErrorIs(t, err, ErrAmbiguousNodeOwnership) +} + // TestTaggedNodeCanHaveKeyExpiry matches Tailscale: a tagged node has key // expiry disabled by default, but it can still be set explicitly (e.g. via // `headscale nodes expire`). diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index b997ea2e..e1e17a89 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -787,53 +787,27 @@ func (s *NodeStore) GetNodeByNodeKey(nodeKey key.NodePublic) (types.NodeView, bo return nodeView, exists } -// GetNodeByMachineKey returns a node by its machine key and user ID. The bool indicates if the node exists. -func (s *NodeStore) GetNodeByMachineKey(machineKey key.MachinePublic, userID types.UserID) (types.NodeView, bool) { - timer := prometheus.NewTimer(nodeStoreOperationDuration.WithLabelValues("get_by_machine_key")) - defer timer.ObserveDuration() - - nodeStoreOperations.WithLabelValues("get_by_machine_key").Inc() - - snapshot := s.data.Load() - if userMap, exists := snapshot.nodesByMachineKey[machineKey]; exists { - if node, exists := userMap[userID]; exists { - return node, true - } - } - - return types.NodeView{}, false -} - -// GetNodeByMachineKeyAnyUser returns a node with the given machine key, -// regardless of which user it belongs to. This is useful for scenarios like -// transferring a node to a different user when re-authenticating with a -// different user's auth key. +// GetNodesByMachineKeyAllUsers returns every node sharing machineKey, keyed by +// owning UserID. Tagged nodes are indexed under UserID(0) (the tagged sentinel); +// user-owned nodes under their owning UserID. Returns an empty map if none. // -// When more than one node shares the machine key (e.g. a tagged node and a -// user-owned node), the choice is deterministic: the tagged node is preferred, -// otherwise the node with the lowest ID. A stable pick keeps re-auth branch -// selection (convert vs create) from depending on map iteration order. -func (s *NodeStore) GetNodeByMachineKeyAnyUser(machineKey key.MachinePublic) (types.NodeView, bool) { - timer := prometheus.NewTimer(nodeStoreOperationDuration.WithLabelValues("get_by_machine_key_any_user")) +// One machine key can map to several nodes (the same device registered by +// different users via the "create new, do not transfer" path). Exposing the +// whole set lets callers decide with full context — index [userID] for an exact +// match, [0] for a tagged node, or reject when the set is ambiguous — rather +// than guessing from a single arbitrary pick. +func (s *NodeStore) GetNodesByMachineKeyAllUsers(machineKey key.MachinePublic) map[types.UserID]types.NodeView { + timer := prometheus.NewTimer(nodeStoreOperationDuration.WithLabelValues("get_nodes_by_machine_key_all_users")) defer timer.ObserveDuration() - nodeStoreOperations.WithLabelValues("get_by_machine_key_any_user").Inc() + nodeStoreOperations.WithLabelValues("get_nodes_by_machine_key_all_users").Inc() - snapshot := s.data.Load() + userMap := s.data.Load().nodesByMachineKey[machineKey] - var best types.NodeView + out := make(map[types.UserID]types.NodeView, len(userMap)) + maps.Copy(out, userMap) - if userMap, exists := snapshot.nodesByMachineKey[machineKey]; exists { - for _, node := range userMap { - if !best.Valid() || - (node.IsTagged() && !best.IsTagged()) || - (node.IsTagged() == best.IsTagged() && node.ID() < best.ID()) { - best = node - } - } - } - - return best, best.Valid() + return out } // DebugString returns debug information about the [NodeStore]. diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index f3c517d0..04d4290b 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -1322,42 +1322,42 @@ func TestRebuildPeerMapsWithChangedPeersFunc(t *testing.T) { assert.Equal(t, 1, peers2.Len(), "ListPeers for node2 should return 1") } -// TestGetNodeByMachineKeyAnyUserDeterministic ensures the any-user machine-key -// lookup returns a stable, well-defined node when more than one node shares a -// machine key: the tagged node if present, otherwise the lowest node ID. A -// nondeterministic pick makes re-auth branch choice (convert vs create) depend -// on map iteration order. -func TestGetNodeByMachineKeyAnyUserDeterministic(t *testing.T) { +// TestGetNodesByMachineKeyAllUsers ensures the lookup returns every node sharing +// a machine key keyed by owning UserID (tagged nodes under UserID(0)), so callers +// see the full set instead of a single arbitrary pick. +func TestGetNodesByMachineKeyAllUsers(t *testing.T) { mk := key.NewMachine().Public() - assertStablePick := func(t *testing.T, store *NodeStore, want types.NodeID) { - t.Helper() - - for range 50 { - got, ok := store.GetNodeByMachineKeyAnyUser(mk) - require.True(t, ok) - require.Equal(t, want, got.ID(), "pick must be stable and well-defined") - } - } - - t.Run("prefers lowest node ID", func(t *testing.T) { + t.Run("empty when absent", func(t *testing.T) { store := NewNodeStore(nil, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) store.Start() defer store.Stop() - n2 := createTestNode(2, 2, "user2", "node2") - n2.MachineKey = mk - n1 := createTestNode(1, 1, "user1", "node1") - n1.MachineKey = mk - - store.PutNode(n2) - store.PutNode(n1) - - assertStablePick(t, store, 1) + require.Empty(t, store.GetNodesByMachineKeyAllUsers(mk)) }) - t.Run("prefers tagged node", func(t *testing.T) { + t.Run("returns all user-owned nodes keyed by user", func(t *testing.T) { + store := NewNodeStore(nil, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) + + store.Start() + defer store.Stop() + + n1 := createTestNode(1, 1, "user1", "node1") + n1.MachineKey = mk + n2 := createTestNode(2, 2, "user2", "node2") + n2.MachineKey = mk + + store.PutNode(n1) + store.PutNode(n2) + + all := store.GetNodesByMachineKeyAllUsers(mk) + require.Len(t, all, 2) + require.Equal(t, types.NodeID(1), all[types.UserID(1)].ID()) + require.Equal(t, types.NodeID(2), all[types.UserID(2)].ID()) + }) + + t.Run("tagged node indexed under UserID(0)", func(t *testing.T) { store := NewNodeStore(nil, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) store.Start() @@ -1374,6 +1374,10 @@ func TestGetNodeByMachineKeyAnyUserDeterministic(t *testing.T) { store.PutNode(owned) store.PutNode(tagged) - assertStablePick(t, store, 3) + all := store.GetNodesByMachineKeyAllUsers(mk) + require.Len(t, all, 2) + require.Equal(t, types.NodeID(1), all[types.UserID(1)].ID()) + require.True(t, all[types.UserID(0)].IsTagged()) + require.Equal(t, types.NodeID(3), all[types.UserID(0)].ID()) }) } diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 94acac73..edbed2e5 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -118,6 +118,13 @@ var ErrRegistrationExpired = errors.New("registration expired") // binding. var ErrNodeKeyInUse = errors.New("node key already in use by another machine") +// ErrAmbiguousNodeOwnership is returned when a machine key maps to a set of +// nodes from which the correct one to update or convert cannot be determined: +// multiple user-owned candidates for a tagged conversion, or a tagged node and +// a user-owned node coexisting (impossible per validateNodeOwnership). The +// registration is rejected rather than mutating an arbitrarily-picked node. +var ErrAmbiguousNodeOwnership = errors.New("machine key maps to ambiguous node ownership") + // sshCheckPair identifies a (source, destination) node pair for // SSH check auth tracking. type sshCheckPair struct { @@ -745,12 +752,11 @@ func (s *State) GetNodeByNodeKey(nodeKey key.NodePublic) (types.NodeView, bool) return s.nodeStore.GetNodeByNodeKey(nodeKey) } -// GetNodeByMachineKey retrieves a node by its machine key and user ID. -// The bool indicates if the node exists or is available (like "err not found"). -// The NodeView might be invalid, so it must be checked with .Valid(), which must be used to ensure -// it isn't an invalid node (this is more of a node error or node is broken). -func (s *State) GetNodeByMachineKey(machineKey key.MachinePublic, userID types.UserID) (types.NodeView, bool) { - return s.nodeStore.GetNodeByMachineKey(machineKey, userID) +// GetNodesByMachineKeyAllUsers returns every node sharing the machine key, +// keyed by owning UserID (tagged nodes under UserID(0)). See +// [NodeStore.GetNodesByMachineKeyAllUsers]. +func (s *State) GetNodesByMachineKeyAllUsers(machineKey key.MachinePublic) map[types.UserID]types.NodeView { + return s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) } // ResolveNode looks up a node by numeric ID, IPv4/IPv6 address, given @@ -2051,16 +2057,32 @@ func (s *State) HandleNodeFromAuthPath( // resolve to a single node rather than racing the find-then-create section. defer s.lockRegistration(machineKey)() - existingNodeSameUser, _ := s.nodeStore.GetNodeByMachineKey(machineKey, types.UserID(user.ID)) - existingNodeAnyUser, _ := s.nodeStore.GetNodeByMachineKeyAnyUser(machineKey) + all := s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) - // Named conditions - describe WHAT we found, not HOW we check it - nodeExistsForSameUser := existingNodeSameUser.Valid() - nodeExistsForAnyUser := existingNodeAnyUser.Valid() - existingNodeIsTagged := nodeExistsForAnyUser && existingNodeAnyUser.IsTagged() - existingNodeOwnedByOtherUser := nodeExistsForAnyUser && - !existingNodeIsTagged && - existingNodeAnyUser.UserID().Get() != user.ID + // Named conditions - describe WHAT we found, not HOW we check it. + existingNodeSameUser, nodeExistsForSameUser := all[types.UserID(user.ID)] + + taggedNode, hasTagged := all[0] + existingNodeIsTagged := hasTagged && taggedNode.IsTagged() + + var existingNodeOtherUser types.NodeView + + existingNodeOwnedByOtherUser := false + + for uid, n := range all { + if uid != 0 && uid != types.UserID(user.ID) && !n.IsTagged() { + existingNodeOtherUser = n + existingNodeOwnedByOtherUser = true + } + } + + // A tagged node and a user-owned node cannot legitimately share a machine + // key (validateNodeOwnership enforces tags XOR user ownership). If both are + // present the machine key is in a corrupt/ambiguous state; reject rather + // than converting an arbitrary node and orphaning the other. + if existingNodeIsTagged && (nodeExistsForSameUser || existingNodeOwnedByOtherUser) { + return types.NodeView{}, change.Change{}, ErrAmbiguousNodeOwnership + } // Create logger with common fields for all auth operations logger := log.With(). @@ -2090,7 +2112,7 @@ func (s *State) HandleNodeFromAuthPath( return types.NodeView{}, change.Change{}, err } } else if existingNodeIsTagged { - updateParams.ExistingNode = existingNodeAnyUser + updateParams.ExistingNode = taggedNode updateParams.IsConvertFromTag = true finalNode, err = s.applyAuthNodeUpdate(updateParams) @@ -2098,7 +2120,7 @@ func (s *State) HandleNodeFromAuthPath( return types.NodeView{}, change.Change{}, err } } else if existingNodeOwnedByOtherUser { - oldUser := existingNodeAnyUser.User() + oldUser := existingNodeOtherUser.User() oldUserName := "" if oldUser.Valid() { @@ -2106,14 +2128,14 @@ func (s *State) HandleNodeFromAuthPath( } logger.Info(). - Str(zf.ExistingNodeName, existingNodeAnyUser.Hostname()). - Uint64(zf.ExistingNodeID, existingNodeAnyUser.ID().Uint64()). + Str(zf.ExistingNodeName, existingNodeOtherUser.Hostname()). + Uint64(zf.ExistingNodeID, existingNodeOtherUser.ID().Uint64()). Str(zf.OldUser, oldUserName). Msg("Creating new node for different user (same machine key exists for another user)") finalNode, err = s.createNewNodeFromAuth( logger, user, regData, hostname, hostinfo, - expiry, registrationMethod, existingNodeAnyUser, + expiry, registrationMethod, existingNodeOtherUser, ) if err != nil { return types.NodeView{}, change.Change{}, err @@ -2192,11 +2214,12 @@ func (s *State) createNewNodeFromAuth( func (s *State) findExistingNodeForPAK( machineKey key.MachinePublic, pak *types.PreAuthKey, -) (types.NodeView, bool) { +) (types.NodeView, bool, error) { + all := s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) + if pak.User != nil { - node, exists := s.nodeStore.GetNodeByMachineKey(machineKey, types.UserID(pak.User.ID)) - if exists { - return node, true + if node, ok := all[types.UserID(pak.User.ID)]; ok { + return node, true, nil } // The node may have been converted to a tagged node since it first @@ -2205,21 +2228,46 @@ func (s *State) findExistingNodeForPAK( // it for re-registration instead of re-validating the spent key or // creating a duplicate node. Re-registration preserves the node's tagged // ownership. See https://github.com/juanfont/headscale/issues/3312. - if node, exists := s.nodeStore.GetNodeByMachineKey(machineKey, 0); exists && node.IsTagged() { - return node, true + if node, ok := all[0]; ok && node.IsTagged() { + return node, true, nil + } + + return types.NodeView{}, false, nil + } + + // A tagged key re-registers the same machine regardless of how it is + // currently owned. An existing tagged node is a plain re-registration. A + // single user-owned node is converted to tagged in place (handled by the + // caller). More than one user-owned node is ambiguous - we cannot know + // which to convert - so reject rather than convert an arbitrary one and + // orphan the rest. + if pak.IsTagged() { + if node, ok := all[0]; ok && node.IsTagged() { + return node, true, nil + } + + var userOwned types.NodeView + + count := 0 + + for uid, node := range all { + if uid != 0 && !node.IsTagged() { + userOwned = node + count++ + } + } + + switch count { + case 0: + return types.NodeView{}, false, nil + case 1: + return userOwned, true, nil + default: + return types.NodeView{}, false, ErrAmbiguousNodeOwnership } } - // A tagged key re-registers the same machine regardless of how that machine - // 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.GetNodeByMachineKeyAnyUser(machineKey) - } - - return types.NodeView{}, false + return types.NodeView{}, false, nil } //nolint:gocyclo // sequential validation/update/create paths with security-sensitive ordering @@ -2245,7 +2293,10 @@ func (s *State) HandleNodeFromPreAuthKey( return types.TaggedDevices.Name } - existingNodeSameUser, existsSameUser := s.findExistingNodeForPAK(machineKey, pak) + existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak) + if err != nil { + return types.NodeView{}, change.Change{}, err + } // For existing nodes, skip validation if: // 1. MachineKey matches (cryptographic proof of machine identity) @@ -2455,24 +2506,29 @@ func (s *State) HandleNodeFromPreAuthKey( finalNode = updatedNodeView } else { - // Node does not exist for this user with this machine key - // Check if node exists with this machine key for a different user - existingNodeAnyUser, existsAnyUser := s.nodeStore.GetNodeByMachineKeyAnyUser(machineKey) + // Node does not exist for this user with this machine key. + // For a user-owned key, check whether the machine key is already held + // by a node belonging to a different user (tags-only keys skip this; + // tagged nodes have no owning user). Any such node yields the same + // outcome - create a new node for the new user, do not transfer - so a + // single representative is enough. + var differentUserNode types.NodeView - // For user-owned keys, check if node exists for a different user. - // Tags-only keys (pak.User == nil) skip this check. - // Tagged nodes are also skipped since they have no owning user. - existingIsUserOwned := existsAnyUser && - existingNodeAnyUser.Valid() && - !existingNodeAnyUser.IsTagged() - belongsToDifferentUser := pak.User != nil && - existingIsUserOwned && - existingNodeAnyUser.UserID().Get() != pak.User.ID + belongsToDifferentUser := false + + if pak.User != nil { + for uid, node := range s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) { + if uid != 0 && !node.IsTagged() && uid != types.UserID(pak.User.ID) { + differentUserNode = node + belongsToDifferentUser = true + } + } + } if belongsToDifferentUser { // Node exists but belongs to a different user. // Create a new node for the new user (do not transfer). - oldUser := existingNodeAnyUser.User() + oldUser := differentUserNode.User() oldUserName := "" if oldUser.Valid() { @@ -2481,8 +2537,8 @@ func (s *State) HandleNodeFromPreAuthKey( log.Info(). Caller(). - Str(zf.ExistingNodeName, existingNodeAnyUser.Hostname()). - Uint64(zf.ExistingNodeID, existingNodeAnyUser.ID().Uint64()). + Str(zf.ExistingNodeName, differentUserNode.Hostname()). + Uint64(zf.ExistingNodeID, differentUserNode.ID().Uint64()). Str(zf.MachineKey, machineKey.ShortString()). Str(zf.OldUser, oldUserName). Str(zf.NewUser, pakUsername()). @@ -2522,7 +2578,7 @@ func (s *State) HandleNodeFromPreAuthKey( Expiry: reqExpiry, RegisterMethod: util.RegisterMethodAuthKey, PreAuthKey: pak, - ExistingNodeForNetinfo: cmp.Or(existingNodeAnyUser, types.NodeView{}), + ExistingNodeForNetinfo: cmp.Or(differentUserNode, types.NodeView{}), }) if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("creating new node: %w", err)