mirror of
https://github.com/juanfont/headscale.git
synced 2026-02-07 20:04:00 +01:00
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
This commit is contained in:
Kristoffer Dalby
parent
a09b0d1d69
commit
306aabbbce
@ -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())
|
||||
}
|
||||
|
||||
@ -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
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user