diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 859f9a48..8e947f1f 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -3832,3 +3832,91 @@ func TestDeletedPreAuthKeyNotRecreatedOnNodeUpdate(t *testing.T) { t.Log("SUCCESS: PreAuthKey remained deleted after node update") } + +// TestTaggedNodeWithoutUserToDifferentUser tests that a node registered with a +// tags-only PreAuthKey (no user) can be re-registered to a different user +// without panicking. This reproduces the issue reported in #3038. +func TestTaggedNodeWithoutUserToDifferentUser(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + + // Step 1: Create a tags-only PreAuthKey (no user, only tags) + // This is valid for tagged nodes where ownership is defined by tags, not users + tags := []string{"tag:server", "tag:prod"} + pak, err := app.state.CreatePreAuthKey(nil, true, false, nil, tags) + require.NoError(t, err, "Failed to create tags-only pre-auth key") + require.Nil(t, pak.User, "Tags-only PAK should have nil User") + + machineKey := key.NewMachine() + nodeKey1 := key.NewNode() + + // Step 2: Register node with tags-only PreAuthKey + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: pak.Key, + }, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-orphan-node", + }, + Expiry: time.Now().Add(24 * time.Hour), + } + + resp, err := app.handleRegisterWithAuthKey(regReq, machineKey.Public()) + require.NoError(t, err, "Initial registration should succeed") + require.True(t, resp.MachineAuthorized, "Node should be authorized") + + // Verify initial state: node is tagged with no UserID + node, found := app.state.GetNodeByNodeKey(nodeKey1.Public()) + require.True(t, found, "Node should be found") + require.True(t, node.IsTagged(), "Node should be tagged") + require.ElementsMatch(t, tags, node.Tags().AsSlice(), "Node should have tags from PAK") + require.False(t, node.UserID().Valid(), "Node should NOT have a UserID (tags-only PAK)") + require.False(t, node.User().Valid(), "Node should NOT have a User (tags-only PAK)") + + t.Logf("Initial registration complete - Node ID: %d, Tags: %v, IsTagged: %t, UserID valid: %t", + node.ID().Uint64(), node.Tags().AsSlice(), node.IsTagged(), node.UserID().Valid()) + + // Step 3: Create a new user (alice) to re-register the node to + alice := app.state.CreateUserForTest("alice") + require.NotNil(t, alice, "Alice user should be created") + + // Step 4: Re-register the node to alice via HandleNodeFromAuthPath + // This is what happens when running: headscale nodes register --user alice --key ... + nodeKey2 := key.NewNode() + registrationID := types.MustRegistrationID() + regEntry := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), // Same machine key as the tagged node + NodeKey: nodeKey2.Public(), + Hostname: "tagged-orphan-node", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-orphan-node", + RequestTags: []string{}, // Empty - transition to user-owned + }, + }) + app.state.SetRegistrationCacheEntry(registrationID, regEntry) + + // This should NOT panic - before the fix, this would panic with: + // panic: runtime error: invalid memory address or nil pointer dereference + // at UserView.Name() because the existing node has no User + nodeAfterReauth, _, err := app.state.HandleNodeFromAuthPath( + registrationID, + types.UserID(alice.ID), + nil, + "cli", + ) + require.NoError(t, err, "Re-registration to alice should succeed without panic") + + // Verify the existing tagged node was converted to be owned by alice (same node ID) + require.True(t, nodeAfterReauth.Valid(), "Node should be valid") + require.True(t, nodeAfterReauth.UserID().Valid(), "Node should have a UserID") + require.Equal(t, alice.ID, nodeAfterReauth.UserID().Get(), "Node should be owned by alice") + require.Equal(t, node.ID(), nodeAfterReauth.ID(), "Should be the same node (converted, not new)") + require.False(t, nodeAfterReauth.IsTagged(), "Node should no longer be tagged") + require.Empty(t, nodeAfterReauth.Tags().AsSlice(), "Node should have no tags") + + t.Logf("Re-registration complete - Node ID: %d, Tags: %v, IsTagged: %t, UserID: %d", + nodeAfterReauth.ID().Uint64(), nodeAfterReauth.Tags().AsSlice(), + nodeAfterReauth.IsTagged(), nodeAfterReauth.UserID().Get()) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index d774a8a1..5c8757ed 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1460,14 +1460,93 @@ func (s *State) HandleNodeFromAuthPath( 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 (for netinfo preservation) + // Check if node exists with this machine key for a different user/owner existingNodeAnyUser, existsAnyUser := s.nodeStore.GetNodeByMachineKeyAnyUser(regEntry.Node.MachineKey) - if existsAnyUser && existingNodeAnyUser.Valid() && existingNodeAnyUser.UserID().Get() != user.ID { - // Node exists but belongs to a different user + // If an existing TAGGED node is found (regardless of UserID), update it to be owned by + // the new user. This handles the case where a node was registered with a tags-only + // PreAuthKey and is now being re-registered to a user. + if existsAnyUser && existingNodeAnyUser.Valid() && existingNodeAnyUser.IsTagged() { + log.Info(). + Caller(). + Str("existing.node.name", existingNodeAnyUser.Hostname()). + Uint64("existing.node.id", existingNodeAnyUser.ID().Uint64()). + Str("machine.key", regEntry.Node.MachineKey.ShortString()). + Strs("old.tags", existingNodeAnyUser.Tags().AsSlice()). + Str("new.user", user.Name). + Str("method", registrationMethod). + Msg("Converting tagged node to user-owned node") + + // Process RequestTags during conversion + var requestTags []string + if regEntry.Node.Hostinfo != nil { + requestTags = regEntry.Node.Hostinfo.RequestTags + } + + oldTags := existingNodeAnyUser.Tags().AsSlice() + + var rejectedTags []string + + // Update existing node - convert from tagged to user-owned + updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeAnyUser.ID(), func(node *types.Node) { + node.NodeKey = regEntry.Node.NodeKey + node.DiscoKey = regEntry.Node.DiscoKey + node.Hostname = hostname + node.Hostinfo = validHostinfo + node.Hostinfo.NetInfo = preserveNetInfo(existingNodeAnyUser, existingNodeAnyUser.ID(), validHostinfo) + node.Endpoints = regEntry.Node.Endpoints + node.RegisterMethod = registrationMethod + node.IsOnline = ptr.To(false) + node.LastSeen = ptr.To(time.Now()) + + // Set expiry for user-owned node + if expiry != nil { + node.Expiry = expiry + } else { + node.Expiry = regEntry.Node.Expiry + } + + rejectedTags = s.processReauthTags(node, requestTags, user, oldTags) + }) + + if !ok { + return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeAnyUser.ID()) + } + + if len(rejectedTags) > 0 { + return types.NodeView{}, change.Change{}, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, rejectedTags) + } + + _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { + node := updatedNodeView.AsStruct() + + err := tx.Omit("AuthKeyID", "AuthKey").Updates(node).Error + if err != nil { + return nil, fmt.Errorf("failed to save node: %w", err) + } + + return node, nil + }) + if err != nil { + return types.NodeView{}, change.Change{}, err + } + + log.Trace(). + Caller(). + Str("node.name", updatedNodeView.Hostname()). + Uint64("node.id", updatedNodeView.ID().Uint64()). + Str("machine.key", regEntry.Node.MachineKey.ShortString()). + Str("node.key", updatedNodeView.NodeKey().ShortString()). + Str("user.name", user.Name). + Msg("Tagged node converted to user-owned") + + finalNode = updatedNodeView + } else if existsAnyUser && existingNodeAnyUser.Valid() && existingNodeAnyUser.UserID().Get() != user.ID { + // Node exists but belongs to a different user (user-owned by someone else) // Create a NEW node for the new user (do not transfer) // This allows the same machine to have separate node identities per user oldUser := existingNodeAnyUser.User() + log.Info(). Caller(). Str("existing.node.name", existingNodeAnyUser.Hostname()). @@ -1477,33 +1556,60 @@ func (s *State) HandleNodeFromAuthPath( Str("new.user", user.Name). Str("method", registrationMethod). Msg("Creating new node for different user (same machine key exists for another user)") - } - // Create a completely new node - log.Debug(). - Caller(). - Str("registration_id", registrationID.String()). - Str("user.name", user.Name). - Str("registrationMethod", registrationMethod). - Str("expiresAt", fmt.Sprintf("%v", expiry)). - Msg("Registering new node from auth callback") + // Create a completely new node + log.Debug(). + Caller(). + Str("registration_id", registrationID.String()). + Str("user.name", user.Name). + Str("registrationMethod", registrationMethod). + Str("expiresAt", fmt.Sprintf("%v", expiry)). + Msg("Registering new node from auth callback") - // Create and save new node - var err error - finalNode, err = s.createAndSaveNewNode(newNodeParams{ - User: *user, - MachineKey: regEntry.Node.MachineKey, - NodeKey: regEntry.Node.NodeKey, - DiscoKey: regEntry.Node.DiscoKey, - Hostname: hostname, - Hostinfo: validHostinfo, - Endpoints: regEntry.Node.Endpoints, - Expiry: cmp.Or(expiry, regEntry.Node.Expiry), - RegisterMethod: registrationMethod, - ExistingNodeForNetinfo: cmp.Or(existingNodeAnyUser, types.NodeView{}), - }) - if err != nil { - return types.NodeView{}, change.Change{}, err + var err error + + finalNode, err = s.createAndSaveNewNode(newNodeParams{ + User: *user, + MachineKey: regEntry.Node.MachineKey, + NodeKey: regEntry.Node.NodeKey, + DiscoKey: regEntry.Node.DiscoKey, + Hostname: hostname, + Hostinfo: validHostinfo, + Endpoints: regEntry.Node.Endpoints, + Expiry: cmp.Or(expiry, regEntry.Node.Expiry), + RegisterMethod: registrationMethod, + ExistingNodeForNetinfo: existingNodeAnyUser, + }) + if err != nil { + return types.NodeView{}, change.Change{}, err + } + } else { + // No existing node found - create a completely new node + log.Debug(). + Caller(). + Str("registration_id", registrationID.String()). + Str("user.name", user.Name). + Str("registrationMethod", registrationMethod). + Str("expiresAt", fmt.Sprintf("%v", expiry)). + Msg("Registering new node from auth callback") + + var err error + + finalNode, err = s.createAndSaveNewNode(newNodeParams{ + User: *user, + MachineKey: regEntry.Node.MachineKey, + NodeKey: regEntry.Node.NodeKey, + DiscoKey: regEntry.Node.DiscoKey, + Hostname: hostname, + Hostinfo: validHostinfo, + Endpoints: regEntry.Node.Endpoints, + Expiry: cmp.Or(expiry, regEntry.Node.Expiry), + RegisterMethod: registrationMethod, + ExistingNodeForNetinfo: cmp.Or(existingNodeAnyUser, types.NodeView{}), + }) + if err != nil { + return types.NodeView{}, change.Change{}, err + } } }