diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index cc1baa5e..fa1bab53 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -233,3 +233,88 @@ func TestExpiredUserNodeReusedOneShotKey_SameNodeKey(t *testing.T) { "expired node re-registering with the same node key must re-validate its key") require.Contains(t, err.Error(), "authkey already used") } + +// TestReusableUserPAKReauthOnTaggedNodeNoDuplicate guards against a reusable +// user pre-auth key creating a second node when it is re-presented for a node +// that has since been converted to tagged. The node must be updated in place. +func TestReusableUserPAKReauthOnTaggedNodeNoDuplicate(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + user := s.CreateUserForTest("reusable-user") + + policy := fmt.Sprintf(`{"tagOwners":{"tag:foo":["%s@"]}}`, user.Name) + _, err = s.SetPolicy([]byte(policy)) + require.NoError(t, err) + + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "reusable-node"}, + Expiry: time.Now().Add(24 * time.Hour), + } + + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + + _, _, err = s.SetNodeTags(first.ID(), []string{"tag:foo"}) + require.NoError(t, err) + + second, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.True(t, second.IsTagged()) + require.Equal(t, first.ID(), second.ID(), "must update in place, not duplicate") + require.Equal(t, 1, s.ListNodes().Len(), "machine must map to exactly one node") +} + +// TestTaggedPAKReauthConvertsUserOwnedNode ensures presenting a tagged pre-auth +// key for a machine that already has a user-owned node converts that node in +// place (same machine, new ownership) rather than creating a duplicate. +func TestTaggedPAKReauthConvertsUserOwnedNode(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + user := s.CreateUserForTest("owner") + + userPak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: userPak.Key}, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "owned-node"}, + Expiry: time.Now().Add(24 * time.Hour), + } + + owned, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.False(t, owned.IsTagged(), "precondition: node is user-owned") + + // A tags-only key re-registers the same machine (same node key). + taggedPak, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:foo"}) + require.NoError(t, err) + + convReq := regReq + convReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: taggedPak.Key} + + converted, _, err := s.HandleNodeFromPreAuthKey(convReq, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, owned.ID(), converted.ID(), "must convert in place, not duplicate") + require.True(t, converted.IsTagged(), "node must become tagged") + require.Equal(t, []string{"tag:foo"}, converted.Tags().AsSlice()) + require.Equal(t, 1, s.ListNodes().Len(), "machine must map to exactly one node") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index f5427184..f2803fde 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2187,10 +2187,13 @@ func (s *State) findExistingNodeForPAK( } } - // Tagged nodes have nil UserID, so they are indexed under UserID(0) - // in nodesByMachineKey. Check there for tagged PAK re-registration. + // 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.GetNodeByMachineKey(machineKey, 0) + return s.nodeStore.GetNodeByMachineKeyAnyUser(machineKey) } return types.NodeView{}, false @@ -2241,7 +2244,13 @@ func (s *State) HandleNodeFromPreAuthKey( isExpired := existsSameUser && existingNodeSameUser.Valid() && existingNodeSameUser.IsExpired() - if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired { + // A tagged key presented for a currently user-owned node converts that node + // to tagged. That is an ownership change, not a plain refresh, so it must + // present a valid key rather than ride the skip-validation fast-path. + isOwnershipConversion := existsSameUser && existingNodeSameUser.Valid() && + pak.IsTagged() && !existingNodeSameUser.IsTagged() + + if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && !isOwnershipConversion { // Existing, still-valid node re-registering with same NodeKey: skip // validation. Pre-auth keys are only needed for initial authentication. // Critical for containers that run "tailscale up --authkey=KEY" on every @@ -2327,8 +2336,17 @@ func (s *State) HandleNodeFromPreAuthKey( node.RegisterMethod = util.RegisterMethodAuthKey // Tags from PreAuthKey are only applied during initial registration. - // On re-registration the node keeps its existing tags and ownership. - // Only update AuthKey reference. + // On re-registration the node keeps its existing tags and ownership, + // except when a tagged key converts a user-owned node: that adopts + // the key's tags and drops user ownership (tagged nodes are + // user-less and never expire). Only update AuthKey reference + // otherwise. + if pak.IsTagged() && !node.IsTagged() { + node.Tags = pak.Proto().GetAclTags() + node.UserID = nil + node.User = nil + node.Expiry = nil + } node.AuthKey = pak node.AuthKeyID = &pak.ID // Do NOT reset IsOnline here. Online status is managed exclusively by