Compare commits

...

7 Commits

Author SHA1 Message Date
Kristoffer Dalby
ea53078dde integration: add test for tagged→user-owned conversion panic
Add TestTagsAuthKeyConvertToUserViaCLIRegister that reproduces the
exact panic from #3038: register a node with a tags-only PreAuthKey
(no user), force reauth with empty tags, then register via CLI with
a user. The mapper panics on node.Owner().Model().ID when User is nil.

The critical detail is using a tags-only PreAuthKey (User: nil). When
the key is created under a user, the node inherits the User pointer
from createAndSaveNewNode and the bug is masked.

Also add Owner() validity assertions to the existing unit test
TestTaggedNodeWithoutUserToDifferentUser to catch the nil pointer
at the unit test level.

Updates #3038
2026-02-02 14:53:27 +00:00
Kristoffer Dalby
80a34ec3c1 state: set User pointer during tagged→user-owned conversion
processReauthTags sets UserID when converting a tagged node to
user-owned, but does not set the User pointer. When the node was
registered with a tags-only PreAuthKey (User: nil), the in-memory
NodeStore cache holds a node with User=nil. The mapper's
generateUserProfiles then calls node.Owner().Model().ID, which
dereferences the nil pointer and panics.

Set node.User alongside node.UserID in processReauthTags. Also add
defensive nil checks in generateUserProfiles to gracefully handle
nodes with invalid owners rather than panicking.

Fixes #3038
2026-02-02 14:52:47 +00:00
Kristoffer Dalby
2cbbfc4319 state: inline reauthExistingNode and convertTaggedNodeToUser
These were thin wrappers around applyAuthNodeUpdate that only added
logging. Move the logging into applyAuthNodeUpdate and call it directly
from HandleNodeFromAuthPath.

This simplifies the code structure without changing behavior.

Updates #3038
2026-01-28 15:34:18 +00:00
Kristoffer Dalby
32203accbe state: validate tags before UpdateNode to ensure consistency
Move tag validation before the UpdateNode callback in applyAuthNodeUpdate.
Previously, tag validation happened inside the callback, and the error
check occurred after UpdateNode had already committed changes to the
NodeStore. This left the NodeStore in an inconsistent state when tags
were rejected.

Now validation happens first, and UpdateNode is only called when we know
the operation will succeed. This follows the principle that UpdateNode
should only be called when we have all information and are ready to commit.

Also extract validateRequestTags as a reusable function and use it in
createAndSaveNewNode to deduplicate the tag validation logic.

Updates #3038
Updates #3048
2026-01-28 15:34:18 +00:00
Kristoffer Dalby
7b6990f63e state: fix expiry handling during node tag conversion
Previously, expiry handling ran BEFORE processReauthTags(), using the
old tagged status to determine whether to set/clear expiry. This caused:

- Personal → Tagged: Expiry remained set (should be cleared to nil)
- Tagged → Personal: Expiry remained nil (should be set from client)

Move expiry handling after tag processing and handle all four transition
cases based on the new tagged status:

- Tagged → Personal: Set expiry from client request
- Personal → Tagged: Clear expiry (tagged nodes don't expire)
- Personal → Personal: Update expiry from client
- Tagged → Tagged: Keep existing nil expiry

Fixes #3048
2026-01-28 15:34:18 +00:00
Kristoffer Dalby
0694caf4d2 state: refactor HandleNodeFromAuthPath for clarity
Reorganize HandleNodeFromAuthPath (~300 lines) into a cleaner structure
with named conditions and extracted helper functions.

Changes:
- Add authNodeUpdateParams struct for shared update logic
- Extract applyAuthNodeUpdate for common reauth/convert operations
- Extract reauthExistingNode and convertTaggedNodeToUser handlers
- Extract createNewNodeFromAuth for new node creation
- Use named boolean conditions (nodeExistsForSameUser, existingNodeIsTagged,
  existingNodeOwnedByOtherUser) instead of compound if conditions
- Create logger with common fields (registration_id, user.name, machine.key,
  method) to reduce log statement verbosity

Updates #3038
2026-01-28 15:34:18 +00:00
Kristoffer Dalby
b066f05945 state: fix nil pointer panic when re-registering tagged node without user
When a node was registered with a tags-only PreAuthKey (no user
associated), the node had User=nil and UserID=nil. When attempting to
re-register this node to a different user via HandleNodeFromAuthPath,
two issues occurred:

1. The code called oldUser.Name() without checking if oldUser was valid,
   causing a nil pointer dereference panic.

2. The existing node lookup logic didn't find the tagged node because it
   searched by (machineKey, userID), but tagged nodes have no userID.
   This caused a new node to be created instead of updating the existing
   tagged node.

Fix this by restructuring HandleNodeFromAuthPath to:
1. First check if a node exists for the same user (existing behavior)
2. If not found, check if an existing TAGGED node exists with the same
   machine key (regardless of userID)
3. If a tagged node exists, UPDATE it to convert from tagged to
   user-owned (preserving the node ID)
4. Only create a new node if the existing node is user-owned by a
   different user

This ensures consistent behavior between:
- personal → tagged → personal (same node, same owner)
- tagged (no user) → personal (same node, new owner)

Add a test that reproduces the panic and conversion scenario by:
1. Creating a tags-only PreAuthKey (no user)
2. Registering a node with that key
3. Re-registering the same machine to a different user
4. Verifying the node ID stays the same (conversion, not creation)

Fixes #3038
2026-01-28 08:27:03 +00:00
6 changed files with 670 additions and 138 deletions

View File

@@ -247,6 +247,7 @@ jobs:
- TestTagsUserLoginReauthWithEmptyTagsRemovesAllTags
- TestTagsAuthKeyWithoutUserInheritsTags
- TestTagsAuthKeyWithoutUserRejectsAdvertisedTags
- TestTagsAuthKeyConvertToUserViaCLIRegister
uses: ./.github/workflows/integration-test-template.yml
secrets: inherit
with:

View File

@@ -625,6 +625,152 @@ func TestTaggedNodeReauthPreservesDisabledExpiry(t *testing.T) {
"Tagged node should have expiry PRESERVED as disabled after re-auth")
}
// TestExpiryDuringPersonalToTaggedConversion tests that when a personal node
// is converted to tagged via reauth with RequestTags, the expiry is cleared to nil.
// BUG #3048: Previously expiry was NOT cleared because expiry handling ran
// BEFORE processReauthTags.
func TestExpiryDuringPersonalToTaggedConversion(t *testing.T) {
app := createTestApp(t)
user := app.state.CreateUserForTest("expiry-test-user")
// Update policy to allow user to own tags
err := app.state.UpdatePolicyManagerUsersForTest()
require.NoError(t, err)
policy := `{
"tagOwners": {
"tag:server": ["expiry-test-user@"]
},
"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
}`
_, err = app.state.SetPolicy([]byte(policy))
require.NoError(t, err)
machineKey := key.NewMachine()
nodeKey1 := key.NewNode()
// Step 1: Create user-owned node WITH expiry set
clientExpiry := time.Now().Add(24 * time.Hour)
registrationID1 := types.MustRegistrationID()
regEntry1 := types.NewRegisterNode(types.Node{
MachineKey: machineKey.Public(),
NodeKey: nodeKey1.Public(),
Hostname: "personal-to-tagged",
Hostinfo: &tailcfg.Hostinfo{
Hostname: "personal-to-tagged",
RequestTags: []string{}, // No tags - user-owned
},
Expiry: &clientExpiry,
})
app.state.SetRegistrationCacheEntry(registrationID1, regEntry1)
node, _, err := app.state.HandleNodeFromAuthPath(
registrationID1, types.UserID(user.ID), nil, "webauth",
)
require.NoError(t, err)
require.False(t, node.IsTagged(), "Node should be user-owned initially")
require.True(t, node.Expiry().Valid(), "User-owned node should have expiry set")
// Step 2: Re-auth with tags (Personal → Tagged conversion)
nodeKey2 := key.NewNode()
registrationID2 := types.MustRegistrationID()
regEntry2 := types.NewRegisterNode(types.Node{
MachineKey: machineKey.Public(),
NodeKey: nodeKey2.Public(),
Hostname: "personal-to-tagged",
Hostinfo: &tailcfg.Hostinfo{
Hostname: "personal-to-tagged",
RequestTags: []string{"tag:server"}, // Adding tags
},
Expiry: &clientExpiry, // Client still sends expiry
})
app.state.SetRegistrationCacheEntry(registrationID2, regEntry2)
nodeAfter, _, err := app.state.HandleNodeFromAuthPath(
registrationID2, types.UserID(user.ID), nil, "webauth",
)
require.NoError(t, err)
require.True(t, nodeAfter.IsTagged(), "Node should be tagged after conversion")
// CRITICAL ASSERTION: Tagged nodes should NOT have expiry
assert.False(t, nodeAfter.Expiry().Valid(),
"Tagged node should have expiry cleared to nil")
}
// TestExpiryDuringTaggedToPersonalConversion tests that when a tagged node
// is converted to personal via reauth with empty RequestTags, expiry is set
// from the client request.
// BUG #3048: Previously expiry was NOT set because expiry handling ran
// BEFORE processReauthTags (node was still tagged at check time).
func TestExpiryDuringTaggedToPersonalConversion(t *testing.T) {
app := createTestApp(t)
user := app.state.CreateUserForTest("expiry-test-user2")
// Update policy to allow user to own tags
err := app.state.UpdatePolicyManagerUsersForTest()
require.NoError(t, err)
policy := `{
"tagOwners": {
"tag:server": ["expiry-test-user2@"]
},
"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
}`
_, err = app.state.SetPolicy([]byte(policy))
require.NoError(t, err)
machineKey := key.NewMachine()
nodeKey1 := key.NewNode()
// Step 1: Create tagged node (expiry should be nil)
registrationID1 := types.MustRegistrationID()
regEntry1 := types.NewRegisterNode(types.Node{
MachineKey: machineKey.Public(),
NodeKey: nodeKey1.Public(),
Hostname: "tagged-to-personal",
Hostinfo: &tailcfg.Hostinfo{
Hostname: "tagged-to-personal",
RequestTags: []string{"tag:server"}, // Tagged node
},
})
app.state.SetRegistrationCacheEntry(registrationID1, regEntry1)
node, _, err := app.state.HandleNodeFromAuthPath(
registrationID1, types.UserID(user.ID), nil, "webauth",
)
require.NoError(t, err)
require.True(t, node.IsTagged(), "Node should be tagged initially")
require.False(t, node.Expiry().Valid(), "Tagged node should have nil expiry")
// Step 2: Re-auth with empty tags (Tagged → Personal conversion)
nodeKey2 := key.NewNode()
clientExpiry := time.Now().Add(48 * time.Hour)
registrationID2 := types.MustRegistrationID()
regEntry2 := types.NewRegisterNode(types.Node{
MachineKey: machineKey.Public(),
NodeKey: nodeKey2.Public(),
Hostname: "tagged-to-personal",
Hostinfo: &tailcfg.Hostinfo{
Hostname: "tagged-to-personal",
RequestTags: []string{}, // Empty tags - convert to user-owned
},
Expiry: &clientExpiry, // Client requests expiry
})
app.state.SetRegistrationCacheEntry(registrationID2, regEntry2)
nodeAfter, _, err := app.state.HandleNodeFromAuthPath(
registrationID2, types.UserID(user.ID), nil, "webauth",
)
require.NoError(t, err)
require.False(t, nodeAfter.IsTagged(), "Node should be user-owned after conversion")
// CRITICAL ASSERTION: User-owned nodes should have expiry from client
assert.True(t, nodeAfter.Expiry().Valid(),
"User-owned node should have expiry set")
assert.WithinDuration(t, clientExpiry, nodeAfter.Expiry().Get(), 5*time.Second,
"Expiry should match client request")
}
// TestReAuthWithDifferentMachineKey tests the edge case where a node attempts
// to re-authenticate with the same NodeKey but a DIFFERENT MachineKey.
// This scenario should be handled gracefully (currently creates a new node).

View File

@@ -3832,3 +3832,98 @@ 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")
// Verify Owner() works without panicking - this is what the mapper's
// generateUserProfiles calls, and it would panic with a nil pointer
// dereference if node.User was not set during the tag→user conversion.
owner := nodeAfterReauth.Owner()
require.True(t, owner.Valid(), "Owner should be valid after conversion (mapper would panic if nil)")
require.Equal(t, alice.ID, owner.Model().ID, "Owner should be alice")
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())
}

View File

@@ -77,11 +77,22 @@ func generateUserProfiles(
userMap := make(map[uint]*types.UserView)
ids := make([]uint, 0, len(userMap))
user := node.Owner()
if !user.Valid() {
log.Error().
Uint64("node.id", node.ID().Uint64()).
Str("node.name", node.Hostname()).
Msg("node has no valid owner, skipping user profile generation")
return nil
}
userID := user.Model().ID
userMap[userID] = &user
ids = append(ids, userID)
for _, peer := range peers.All() {
peerUser := peer.Owner()
if !peerUser.Valid() {
continue
}
peerUserID := peerUser.Model().ID
userMap[peerUserID] = &peerUser
ids = append(ids, peerUserID)

View File

@@ -22,6 +22,7 @@ import (
"github.com/juanfont/headscale/hscontrol/types"
"github.com/juanfont/headscale/hscontrol/types/change"
"github.com/juanfont/headscale/hscontrol/util"
"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
"golang.org/x/sync/errgroup"
"gorm.io/gorm"
@@ -135,6 +136,7 @@ func NewState(cfg *types.Config) (*State, error) {
for _, node := range nodes {
node.IsOnline = ptr.To(false)
}
users, err := db.ListUsers()
if err != nil {
return nil, fmt.Errorf("loading users: %w", err)
@@ -156,6 +158,7 @@ func NewState(cfg *types.Config) (*State, error) {
if batchSize == 0 {
batchSize = defaultNodeStoreBatchSize
}
batchTimeout := cfg.Tuning.NodeStoreBatchTimeout
if batchTimeout == 0 {
batchTimeout = defaultNodeStoreBatchTimeout
@@ -189,7 +192,8 @@ func NewState(cfg *types.Config) (*State, error) {
func (s *State) Close() error {
s.nodeStore.Stop()
if err := s.db.Close(); err != nil {
err := s.db.Close()
if err != nil {
return fmt.Errorf("closing database: %w", err)
}
@@ -572,12 +576,14 @@ func (s *State) ListNodes(nodeIDs ...types.NodeID) views.Slice[types.NodeView] {
// Filter nodes by the requested IDs
allNodes := s.nodeStore.ListNodes()
nodeIDSet := make(map[types.NodeID]struct{}, len(nodeIDs))
for _, id := range nodeIDs {
nodeIDSet[id] = struct{}{}
}
var filteredNodes []types.NodeView
for _, node := range allNodes.All() {
if _, exists := nodeIDSet[node.ID()]; exists {
filteredNodes = append(filteredNodes, node)
@@ -600,12 +606,14 @@ func (s *State) ListPeers(nodeID types.NodeID, peerIDs ...types.NodeID) views.Sl
// For specific peerIDs, filter from all nodes
allNodes := s.nodeStore.ListNodes()
nodeIDSet := make(map[types.NodeID]struct{}, len(peerIDs))
for _, id := range peerIDs {
nodeIDSet[id] = struct{}{}
}
var filteredNodes []types.NodeView
for _, node := range allNodes.All() {
if _, exists := nodeIDSet[node.ID()]; exists {
filteredNodes = append(filteredNodes, node)
@@ -618,6 +626,7 @@ func (s *State) ListPeers(nodeID types.NodeID, peerIDs ...types.NodeID) views.Sl
// ListEphemeralNodes retrieves all ephemeral (temporary) nodes in the system.
func (s *State) ListEphemeralNodes() views.Slice[types.NodeView] {
allNodes := s.nodeStore.ListNodes()
var ephemeralNodes []types.NodeView
for _, node := range allNodes.All() {
@@ -749,7 +758,8 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t
// RenameNode changes the display name of a node.
func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, change.Change, error) {
if err := util.ValidateHostname(newName); err != nil {
err := util.ValidateHostname(newName)
if err != nil {
return types.NodeView{}, change.Change{}, fmt.Errorf("renaming node: %w", err)
}
@@ -1079,6 +1089,7 @@ func preserveNetInfo(existingNode types.NodeView, nodeID types.NodeID, validHost
if existingNode.Valid() {
existingHostinfo = existingNode.Hostinfo().AsStruct()
}
return netInfoFromMapRequest(nodeID, existingHostinfo, validHostinfo)
}
@@ -1101,6 +1112,167 @@ type newNodeParams struct {
ExistingNodeForNetinfo types.NodeView
}
// authNodeUpdateParams contains parameters for updating an existing node during auth.
type authNodeUpdateParams struct {
// Node to update; must be valid and in NodeStore.
ExistingNode types.NodeView
// Client data: keys, hostinfo, endpoints.
RegEntry *types.RegisterNode
// Pre-validated hostinfo; NetInfo preserved from ExistingNode.
ValidHostinfo *tailcfg.Hostinfo
// Hostname from hostinfo, or generated from keys if client omits it.
Hostname string
// Auth user; may differ from ExistingNode.User() on conversion.
User *types.User
// Overrides RegEntry.Node.Expiry; ignored for tagged nodes.
Expiry *time.Time
// Only used when IsConvertFromTag=true.
RegisterMethod string
// Set true for tagged->user conversion. Affects RegisterMethod and expiry.
IsConvertFromTag bool
}
// applyAuthNodeUpdate applies common update logic for re-authenticating or converting
// an existing node. It updates the node in NodeStore, processes RequestTags, and
// persists changes to the database.
func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, error) {
// Log the operation type
if params.IsConvertFromTag {
log.Info().
Str("node.name", params.ExistingNode.Hostname()).
Uint64("node.id", params.ExistingNode.ID().Uint64()).
Strs("old.tags", params.ExistingNode.Tags().AsSlice()).
Msg("Converting tagged node to user-owned node")
} else {
log.Info().
Str("node.name", params.ExistingNode.Hostname()).
Uint64("node.id", params.ExistingNode.ID().Uint64()).
Interface("hostinfo", params.RegEntry.Node.Hostinfo).
Msg("Updating existing node registration via reauth")
}
// Process RequestTags during reauth (#2979)
// Due to json:",omitempty", we treat empty/nil as "clear tags"
var requestTags []string
if params.RegEntry.Node.Hostinfo != nil {
requestTags = params.RegEntry.Node.Hostinfo.RequestTags
}
oldTags := params.ExistingNode.Tags().AsSlice()
// Validate tags BEFORE calling UpdateNode to ensure we don't modify NodeStore
// if validation fails. This maintains consistency between NodeStore and database.
rejectedTags := s.validateRequestTags(params.ExistingNode, requestTags)
if len(rejectedTags) > 0 {
return types.NodeView{}, fmt.Errorf(
"%w %v are invalid or not permitted",
ErrRequestedTagsInvalidOrNotPermitted,
rejectedTags,
)
}
// Update existing node in NodeStore - validation passed, safe to mutate
updatedNodeView, ok := s.nodeStore.UpdateNode(params.ExistingNode.ID(), func(node *types.Node) {
node.NodeKey = params.RegEntry.Node.NodeKey
node.DiscoKey = params.RegEntry.Node.DiscoKey
node.Hostname = params.Hostname
// Preserve NetInfo from existing node when re-registering
node.Hostinfo = params.ValidHostinfo
node.Hostinfo.NetInfo = preserveNetInfo(
params.ExistingNode,
params.ExistingNode.ID(),
params.ValidHostinfo,
)
node.Endpoints = params.RegEntry.Node.Endpoints
node.IsOnline = ptr.To(false)
node.LastSeen = ptr.To(time.Now())
// Set RegisterMethod - for conversion this is the new method,
// for reauth we preserve the existing one from regEntry
if params.IsConvertFromTag {
node.RegisterMethod = params.RegisterMethod
} else {
node.RegisterMethod = params.RegEntry.Node.RegisterMethod
}
// Track tagged status BEFORE processing tags
wasTagged := node.IsTagged()
// Process tags - may change node.Tags and node.UserID
// Tags were pre-validated, so this will always succeed (no rejected tags)
_ = s.processReauthTags(node, requestTags, params.User, oldTags)
// Handle expiry AFTER tag processing, based on transition
// This ensures expiry is correctly set/cleared based on the NEW tagged status
isTagged := node.IsTagged()
switch {
case wasTagged && !isTagged:
// Tagged → Personal: set expiry from client request
if params.Expiry != nil {
node.Expiry = params.Expiry
} else {
node.Expiry = params.RegEntry.Node.Expiry
}
case !wasTagged && isTagged:
// Personal → Tagged: clear expiry (tagged nodes don't expire)
node.Expiry = nil
case params.IsConvertFromTag:
// Explicit conversion from tagged to user-owned: set expiry from client request
if params.Expiry != nil {
node.Expiry = params.Expiry
} else {
node.Expiry = params.RegEntry.Node.Expiry
}
case !isTagged:
// Personal → Personal: update expiry from client
if params.Expiry != nil {
node.Expiry = params.Expiry
} else {
node.Expiry = params.RegEntry.Node.Expiry
}
}
// Tagged → Tagged: keep existing expiry (nil) - no action needed
})
if !ok {
return types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID())
}
// Persist to database
// Omit AuthKeyID/AuthKey to prevent stale PreAuthKey references from causing FK errors.
_, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) {
err := tx.Omit("AuthKeyID", "AuthKey").Updates(updatedNodeView.AsStruct()).Error
if err != nil {
return nil, fmt.Errorf("failed to save node: %w", err)
}
return nil, nil //nolint:nilnil // side-effect only write
})
if err != nil {
return types.NodeView{}, err
}
// Log completion
if params.IsConvertFromTag {
log.Trace().
Str("node.name", updatedNodeView.Hostname()).
Uint64("node.id", updatedNodeView.ID().Uint64()).
Str("node.key", updatedNodeView.NodeKey().ShortString()).
Msg("Tagged node converted to user-owned")
} else {
log.Trace().
Str("node.name", updatedNodeView.Hostname()).
Uint64("node.id", updatedNodeView.ID().Uint64()).
Str("node.key", updatedNodeView.NodeKey().ShortString()).
Msg("Node re-authorized")
}
return updatedNodeView, nil
}
// createAndSaveNewNode creates a new node, allocates IPs, saves to DB, and adds to NodeStore.
// It preserves netinfo from an existing node if one is provided (for faster DERP connectivity).
func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, error) {
@@ -1148,6 +1320,7 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro
nodeToRegister.User = params.PreAuthKey.User
nodeToRegister.Tags = nil
}
nodeToRegister.AuthKey = params.PreAuthKey
nodeToRegister.AuthKeyID = &params.PreAuthKey.ID
} else {
@@ -1166,21 +1339,14 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro
// Process RequestTags (from tailscale up --advertise-tags) ONLY for non-PreAuthKey registrations.
// Validate early before IP allocation to avoid resource leaks on failure.
if params.PreAuthKey == nil && params.Hostinfo != nil && len(params.Hostinfo.RequestTags) > 0 {
var approvedTags, rejectedTags []string
for _, tag := range params.Hostinfo.RequestTags {
if s.polMan.NodeCanHaveTag(nodeToRegister.View(), tag) {
approvedTags = append(approvedTags, tag)
} else {
rejectedTags = append(rejectedTags, tag)
}
}
// Reject registration if any requested tags are unauthorized
// Validate all tags before applying - reject if any tag is not permitted
rejectedTags := s.validateRequestTags(nodeToRegister.View(), params.Hostinfo.RequestTags)
if len(rejectedTags) > 0 {
return types.NodeView{}, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, rejectedTags)
}
// All tags are approved - apply them
approvedTags := params.Hostinfo.RequestTags
if len(approvedTags) > 0 {
nodeToRegister.Tags = approvedTags
slices.Sort(nodeToRegister.Tags)
@@ -1217,12 +1383,14 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro
if err != nil {
return types.NodeView{}, fmt.Errorf("failed to ensure unique given name: %w", err)
}
nodeToRegister.GivenName = givenName
}
// New node - database first to get ID, then NodeStore
savedNode, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) {
if err := tx.Save(&nodeToRegister).Error; err != nil {
err := tx.Save(&nodeToRegister).Error
if err != nil {
return nil, fmt.Errorf("failed to save node: %w", err)
}
@@ -1243,6 +1411,26 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro
return s.nodeStore.PutNode(*savedNode), nil
}
// validateRequestTags validates that the requested tags are permitted for the node.
// This should be called BEFORE UpdateNode to ensure we don't modify NodeStore
// if validation fails. Returns the list of rejected tags (empty if all valid).
func (s *State) validateRequestTags(node types.NodeView, requestTags []string) []string {
// Empty tags = clear tags, always permitted
if len(requestTags) == 0 {
return nil
}
var rejectedTags []string
for _, tag := range requestTags {
if !s.polMan.NodeCanHaveTag(node, tag) {
rejectedTags = append(rejectedTags, tag)
}
}
return rejectedTags
}
// processReauthTags handles tag changes during node re-authentication.
// It processes RequestTags from the client and updates node tags accordingly.
// Returns rejected tags (if any) for post-validation error handling.
@@ -1276,6 +1464,7 @@ func (s *State) processReauthTags(
node.Tags = []string{}
node.UserID = &user.ID
node.User = user
}
return nil
@@ -1368,140 +1557,75 @@ func (s *State) HandleNodeFromAuthPath(
regEntry.Node.Hostinfo,
)
// Lookup existing nodes
machineKey := regEntry.Node.MachineKey
existingNodeSameUser, _ := s.nodeStore.GetNodeByMachineKey(machineKey, types.UserID(user.ID))
existingNodeAnyUser, _ := s.nodeStore.GetNodeByMachineKeyAnyUser(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
// Create logger with common fields for all auth operations
logger := log.With().
Str("registration_id", registrationID.String()).
Str("user.name", user.Name).
Str("machine.key", machineKey.ShortString()).
Str("method", registrationMethod).
Logger()
// Common params for update operations
updateParams := authNodeUpdateParams{
RegEntry: regEntry,
ValidHostinfo: validHostinfo,
Hostname: hostname,
User: user,
Expiry: expiry,
RegisterMethod: registrationMethod,
}
var finalNode types.NodeView
// Check if node already exists with same machine key for this user
existingNodeSameUser, existsSameUser := s.nodeStore.GetNodeByMachineKey(regEntry.Node.MachineKey, types.UserID(user.ID))
if nodeExistsForSameUser {
updateParams.ExistingNode = existingNodeSameUser
// If this node exists for this user, update the node in place.
if existsSameUser && existingNodeSameUser.Valid() {
log.Info().
Caller().
Str("registration_id", registrationID.String()).
Str("user.name", user.Name).
Str("registrationMethod", registrationMethod).
Str("node.name", existingNodeSameUser.Hostname()).
Uint64("node.id", existingNodeSameUser.ID().Uint64()).
Interface("hostinfo", regEntry.Node.Hostinfo).
Msg("Updating existing node registration via reauth")
// Process RequestTags during reauth (#2979)
// Due to json:",omitempty", we treat empty/nil as "clear tags"
var requestTags []string
if regEntry.Node.Hostinfo != nil {
requestTags = regEntry.Node.Hostinfo.RequestTags
}
oldTags := existingNodeSameUser.Tags().AsSlice()
var rejectedTags []string
// Update existing node - NodeStore first, then database
updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) {
node.NodeKey = regEntry.Node.NodeKey
node.DiscoKey = regEntry.Node.DiscoKey
node.Hostname = hostname
// TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics
// when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest).
// Preserve NetInfo from existing node when re-registering
node.Hostinfo = validHostinfo
node.Hostinfo.NetInfo = preserveNetInfo(existingNodeSameUser, existingNodeSameUser.ID(), validHostinfo)
node.Endpoints = regEntry.Node.Endpoints
node.RegisterMethod = regEntry.Node.RegisterMethod
node.IsOnline = ptr.To(false)
node.LastSeen = ptr.To(time.Now())
// Tagged nodes keep their existing expiry (disabled).
// User-owned nodes update expiry from the provided value or registration entry.
if !node.IsTagged() {
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, existingNodeSameUser.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) {
// Use Updates() to preserve fields not modified by UpdateNode.
// Omit AuthKeyID/AuthKey to prevent stale PreAuthKey references from causing FK errors.
err := tx.Omit("AuthKeyID", "AuthKey").Updates(updatedNodeView.AsStruct()).Error
if err != nil {
return nil, fmt.Errorf("failed to save node: %w", err)
}
return nil, nil
})
finalNode, err = s.applyAuthNodeUpdate(updateParams)
if err != nil {
return types.NodeView{}, change.Change{}, err
}
} else if existingNodeIsTagged {
updateParams.ExistingNode = existingNodeAnyUser
updateParams.IsConvertFromTag = true
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("Node re-authorized")
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)
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
// 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()).
Uint64("existing.node.id", existingNodeAnyUser.ID().Uint64()).
Str("machine.key", regEntry.Node.MachineKey.ShortString()).
Str("old.user", oldUser.Name()).
Str("new.user", user.Name).
Str("method", registrationMethod).
Msg("Creating new node for different user (same machine key exists for another user)")
finalNode, err = s.applyAuthNodeUpdate(updateParams)
if err != nil {
return types.NodeView{}, change.Change{}, err
}
} else if existingNodeOwnedByOtherUser {
oldUser := existingNodeAnyUser.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")
logger.Info().
Str("existing.node.name", existingNodeAnyUser.Hostname()).
Uint64("existing.node.id", existingNodeAnyUser.ID().Uint64()).
Str("old.user", oldUser.Name()).
Msg("Creating new node for different user (same machine key exists for another user)")
// 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{}),
})
finalNode, err = s.createNewNodeFromAuth(
logger, user, regEntry, hostname, validHostinfo,
expiry, registrationMethod, existingNodeAnyUser,
)
if err != nil {
return types.NodeView{}, change.Change{}, err
}
} else {
finalNode, err = s.createNewNodeFromAuth(
logger, user, regEntry, hostname, validHostinfo,
expiry, registrationMethod, types.NodeView{},
)
if err != nil {
return types.NodeView{}, change.Change{}, err
}
@@ -1534,6 +1658,37 @@ func (s *State) HandleNodeFromAuthPath(
return finalNode, c, nil
}
// createNewNodeFromAuth creates a new node during auth callback.
// This is used for both new registrations and when a machine already has a node
// for a different user.
func (s *State) createNewNodeFromAuth(
logger zerolog.Logger,
user *types.User,
regEntry *types.RegisterNode,
hostname string,
validHostinfo *tailcfg.Hostinfo,
expiry *time.Time,
registrationMethod string,
existingNodeForNetinfo types.NodeView,
) (types.NodeView, error) {
logger.Debug().
Interface("expiry", expiry).
Msg("Registering new node from auth callback")
return 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: existingNodeForNetinfo,
})
}
// HandleNodeFromPreAuthKey handles node registration using a pre-authentication key.
func (s *State) HandleNodeFromPreAuthKey(
regReq tailcfg.RegisterRequest,
@@ -1753,6 +1908,7 @@ func (s *State) HandleNodeFromPreAuthKey(
}
var err error
finalNode, err = s.createAndSaveNewNode(newNodeParams{
User: pakUser,
MachineKey: machineKey,
@@ -1890,7 +2046,9 @@ func (s *State) autoApproveNodes() ([]change.Change, error) {
}
mu.Lock()
cs = append(cs, c)
mu.Unlock()
}
@@ -1960,6 +2118,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
if hi := req.Hostinfo; hi != nil {
hasNewRoutes = len(hi.RoutableIPs) > 0
}
needsRouteApproval = hostinfoChanged && (routesChanged(currentNode.View(), req.Hostinfo) || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0))
if needsRouteApproval {
// Extract announced routes from request
@@ -2112,6 +2271,7 @@ func hostinfoEqual(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool {
if !oldNode.Valid() || newHI == nil {
return false
}
old := oldNode.AsStruct().Hostinfo
return old.Equal(newHI)

View File

@@ -3116,3 +3116,122 @@ func TestTagsAuthKeyWithoutUserRejectsAdvertisedTags(t *testing.T) {
t.Fail()
}
}
// =============================================================================
// Test Suite 6: Tagged→User Conversion via CLI Register (#3038)
// =============================================================================
// TestTagsAuthKeyConvertToUserViaCLIRegister reproduces the panic from
// issue #3038: register a node with a tags-only preauthkey (no user), then
// convert it to a user-owned node via "headscale nodes register --user <user> --key ...".
// The crash happens in the mapper's generateUserProfiles when node.User is nil
// after the tag→user conversion in processReauthTags.
//
// The key detail is using a tags-only PreAuthKey (User: nil). When created under
// a user, the node inherits User from the PreAuthKey and the bug is masked.
func TestTagsAuthKeyConvertToUserViaCLIRegister(t *testing.T) {
IntegrationSkip(t)
policy := tagsTestPolicy()
spec := ScenarioSpec{
NodesPerUser: 0,
Users: []string{tagTestUser},
}
scenario, err := NewScenario(spec)
require.NoError(t, err)
defer scenario.ShutdownAssertNoPanics(t)
err = scenario.CreateHeadscaleEnvWithLoginURL(
[]tsic.Option{},
hsic.WithACLPolicy(policy),
hsic.WithTestName("tags-authkey-to-user-cli-3038"),
hsic.WithTLS(),
)
requireNoErrHeadscaleEnv(t, err)
headscale, err := scenario.Headscale()
requireNoErrGetHeadscale(t, err)
// Step 1: Create a tags-only preauthkey WITHOUT a user.
// This is the critical detail: when PreAuthKey.UserID is nil, the node
// enters the NodeStore with node.User == nil. The processReauthTags
// conversion then sets UserID but not User, leaving it nil for the mapper.
authKey, err := scenario.CreatePreAuthKeyWithOptions(hsic.AuthKeyOptions{
User: nil,
Reusable: false,
Ephemeral: false,
Tags: []string{"tag:valid-owned"},
})
require.NoError(t, err)
t.Logf("Created tags-only PreAuthKey (no user) with tags: %v", authKey.GetAclTags())
client, err := scenario.CreateTailscaleNode(
"head",
tsic.WithNetwork(scenario.networks[scenario.testDefaultNetwork]),
)
require.NoError(t, err)
err = client.Login(headscale.GetEndpoint(), authKey.GetKey())
require.NoError(t, err)
err = client.WaitForRunning(120 * time.Second)
require.NoError(t, err)
// Verify initial state: node is tagged
assert.EventuallyWithT(t, func(c *assert.CollectT) {
nodes, err := headscale.ListNodes()
assert.NoError(c, err)
assert.Len(c, nodes, 1)
if len(nodes) == 1 {
assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"})
t.Logf("Initial state - Node ID: %d, Tags: %v", nodes[0].GetId(), nodes[0].GetTags())
}
}, 30*time.Second, 500*time.Millisecond, "node should be tagged initially")
// Step 2: Force reauth with empty tags (triggers web auth flow)
command := []string{
"tailscale", "up",
"--login-server=" + headscale.GetEndpoint(),
"--hostname=" + client.Hostname(),
"--advertise-tags=",
"--force-reauth",
}
stdout, stderr, _ := client.Execute(command)
t.Logf("Reauth command output: stdout=%s stderr=%s", stdout, stderr)
loginURL, err := util.ParseLoginURLFromCLILogin(stdout + stderr)
require.NoError(t, err, "Failed to parse login URL from reauth command")
body, err := doLoginURL(client.Hostname(), loginURL)
require.NoError(t, err)
// Step 3: Register via CLI with user (this is the exact step that triggers the panic)
err = scenario.runHeadscaleRegister(tagTestUser, body)
require.NoError(t, err)
err = client.WaitForRunning(120 * time.Second)
require.NoError(t, err)
// Step 4: Verify node is now user-owned and the mapper didn't panic.
// The panic would occur when the mapper builds the MapResponse and calls
// node.Owner().Model().ID with a nil User pointer.
// ShutdownAssertNoPanics in the defer catches any panics in headscale logs.
assert.EventuallyWithT(t, func(c *assert.CollectT) {
nodes, err := headscale.ListNodes()
assert.NoError(c, err)
assert.Len(c, nodes, 1)
if len(nodes) == 1 {
assertNodeHasNoTagsWithCollect(c, nodes[0])
assert.Equal(c, tagTestUser, nodes[0].GetUser().GetName(),
"Node ownership should be returned to user after untagging")
t.Logf("After conversion - Node ID: %d, Tags: %v, User: %s",
nodes[0].GetId(), nodes[0].GetTags(), nodes[0].GetUser().GetName())
}
}, 60*time.Second, 1*time.Second, "node should be user-owned after conversion via CLI register")
}