diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index afa51813b..59e7f7d5f 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -365,7 +365,7 @@ jobs: - TestTagsUserLoginReauthWithEmptyTagsRemovesAllTags - TestTagsAuthKeyWithoutUserInheritsTags - TestTagsAuthKeyWithoutUserRejectsAdvertisedTags - - TestTagsAuthKeyConvertToUserViaCLIRegister + - TestTagsAuthKeyWithoutUserCLIRegisterAddsUserDevice - TestTaggedNodeLogoutReloginSingleUseKeyOnline - TestTaggedNodeLogoutReloginReusableKeyOnline - TestTagsOIDCReauthAddOwnedTag diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index bbf0d2ba2..c7efc162b 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -3900,9 +3900,10 @@ 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. +// TestTaggedNodeWithoutUserToDifferentUser tests that a user's login on the +// machine of a node registered with a tags-only PreAuthKey (no user) works +// without panicking (#3038). No user made the key, so the login gets a device +// of its own and the tagged node stays. func TestTaggedNodeWithoutUserToDifferentUser(t *testing.T) { t.Parallel() @@ -3975,19 +3976,21 @@ func TestTaggedNodeWithoutUserToDifferentUser(t *testing.T) { ) 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") + require.NotEqual(t, node.ID(), nodeAfterReauth.ID(), "alice gets a device of her own") + require.False(t, nodeAfterReauth.IsTagged(), "Node should not be tagged") + + tagged, ok := app.state.GetNodeByID(node.ID()) + require.True(t, ok, "the tagged node stays") + require.ElementsMatch(t, tags, tagged.Tags().AsSlice()) // Verify [types.NodeView.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. + // dereference if node.User was not set. owner := nodeAfterReauth.Owner() - require.True(t, owner.Valid(), "Owner should be valid after conversion (mapper would panic if nil)") + require.True(t, owner.Valid(), "Owner should be valid (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", diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 81452620a..7e26615ed 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -8,6 +8,7 @@ import ( "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" "tailscale.com/types/key" @@ -378,34 +379,111 @@ func TestTaggedPAKReauthRejectsAmbiguousMultiUserNode(t *testing.T) { 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) { +// TestAuthPathOnTaggedMachine: a login on a machine with a tagged node and +// another user's node re-authenticates the tagged node only for the user it +// was tagged from; anyone else gets a device of their own, even under the +// tagged node's node key. +func TestAuthPathOnTaggedMachine(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)) + login := func(user *types.User, nodeKey key.NodePublic) (types.NodeView, error) { + t.Helper() - _, _, err = s.HandleNodeFromAuthPath(authID, types.UserID(u3.ID), nil, util.RegisterMethodOIDC) - require.ErrorIs(t, err, ErrAmbiguousNodeOwnership) + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: mk, + NodeKey: nodeKey, + Hostname: "multi", + Hostinfo: &tailcfg.Hostinfo{Hostname: "multi"}, + })) + + node, _, err := s.HandleNodeFromAuthPath(authID, types.UserID(user.ID), nil, util.RegisterMethodOIDC) + + return node, err + } + + u3 := s.CreateUserForTest("u3") + other, err := login(u3, tagged.NodeKey()) + require.NoError(t, err) + assert.NotEqual(t, n1, other.ID(), "another user gets a device of their own") + assert.Equal(t, u3.ID, other.UserID().Get()) + + current, ok := s.GetNodeByID(n1) + require.True(t, ok) + assert.True(t, current.IsTagged(), "the tagged node is untouched") + + u1, err := s.GetUserByName("u1") + require.NoError(t, err) + + claimed, err := login(u1, key.NewNode().Public()) + require.NoError(t, err) + assert.Equal(t, n1, claimed.ID(), "the user it was tagged from takes it back") + assert.False(t, claimed.IsTagged()) + assert.Equal(t, 3, s.ListNodes().Len()) +} + +// TestUserKeyOnLoginJoinedTaggedNode: an admin-tagged node that joined by +// login records no former owner, so no user key speaks for it. Under its own +// node key a key only wakes it; under a new one each user gets a device of +// their own. +func TestUserKeyOnLoginJoinedTaggedNode(t *testing.T) { + s := newRetagTestState(t) + owner := s.CreateUserForTest("login-owner") + machineKey := key.NewMachine().Public() + nodeKey := key.NewNode().Public() + + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: machineKey, + NodeKey: nodeKey, + Hostname: "login-node", + Hostinfo: &tailcfg.Hostinfo{Hostname: "login-node"}, + })) + joined, _, err := s.HandleNodeFromAuthPath(authID, types.UserID(owner.ID), nil, util.RegisterMethodOIDC) + require.NoError(t, err) + + _, err = s.SetPolicy([]byte(`{"tagOwners":{"tag:prod":["login-owner@"]}}`)) + require.NoError(t, err) + _, _, err = s.SetNodeTags(joined.ID(), []string{"tag:prod"}) + require.NoError(t, err) + + for _, user := range []*types.User{owner, s.CreateUserForTest("login-other")} { + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + register := func(nk key.NodePublic) types.NodeView { + t.Helper() + + node, _, err := s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: nk, + Hostinfo: &tailcfg.Hostinfo{Hostname: "login-node"}, + }, machineKey) + require.NoError(t, err) + + return node + } + + woken := register(nodeKey) + assert.Equal(t, joined.ID(), woken.ID(), "%s: its own node key wakes it", user.Name) + assert.True(t, woken.IsTagged()) + + own := register(key.NewNode().Public()) + assert.NotEqual(t, joined.ID(), own.ID(), "%s: a device of its own", user.Name) + assert.Equal(t, user.ID, own.UserID().Get()) + } + + current, ok := s.GetNodeByID(joined.ID()) + require.True(t, ok) + assert.True(t, current.IsTagged(), "the tagged node is untouched") + assert.Equal(t, nodeKey, current.NodeKey()) + assert.Equal(t, 3, s.ListNodes().Len()) } // seededTaggedNode holds the identity of a node seeded into the genuinely @@ -461,12 +539,16 @@ func seedTagOwnedNode(t *testing.T, tags []string, createdByName string) seededT // Genuinely tag-owned: tags set, UserID nil. UserID nil indexes the node // under userID 0, so HandleNodeFromAuthPath takes the convert-from-tag - // branch — the path the issue exercises. User mirrors createdBy. + // branch — the path the issue exercises. User mirrors createdBy. It joined + // by login, so it carries no auth key. seeded, ok := s.nodeStore.UpdateNode(node.ID, func(n *types.Node) { n.Tags = tags n.UserID = nil n.User = createdBy n.Expiry = nil + n.RegisterMethod = util.RegisterMethodOIDC + n.AuthKeyID = nil + n.AuthKey = nil }) require.True(t, ok) require.True(t, seeded.IsTagged(), "precondition: node must be tagged") @@ -702,8 +784,8 @@ func TestTaggedReauthUntagReturnsToAuthUser(t *testing.T) { } // TestTaggedReauthUntagClearsEphemeralAuthKey guards the interactive-path mirror -// of the #3370 ephemeral bug: a node created by a tagged *ephemeral* pre-auth -// key, then untagged via an interactive re-auth, must not keep the ephemeral key +// of the #3370 ephemeral bug: a node joined with a user's tagged *ephemeral* +// pre-auth key, then untagged via their interactive re-auth, must not keep the ephemeral key // reference. Otherwise it stays IsEphemeral() and is garbage-collected on its // next disconnect, silently deleting the user's just-claimed device — and the // stale reference survives a control-plane restart via the reloaded AuthKeyID. @@ -716,12 +798,13 @@ func TestTaggedReauthUntagClearsEphemeralAuthKey(t *testing.T) { // Attach a tagged, ephemeral auth key to the seeded node (the shape a // `tailscale up --authkey ` node has). - ephKey, err := n.s.CreatePreAuthKey(nil, false, true /*ephemeral*/, nil, []string{"tag:foo"}) + ephKey, err := n.s.CreatePreAuthKey(alice.TypedID(), false, true /*ephemeral*/, nil, []string{"tag:foo"}) require.NoError(t, err) pak, err := n.s.GetPreAuthKey(ephKey.Key) require.NoError(t, err) seeded, ok := n.s.nodeStore.UpdateNode(n.id, func(nd *types.Node) { + nd.RegisterMethod = util.RegisterMethodAuthKey nd.AuthKey = pak nd.AuthKeyID = &pak.ID }) @@ -1622,3 +1705,155 @@ func TestTaggingPreservesNodeExpiry(t *testing.T) { require.NotNil(t, tagged.AsStruct().Expiry, "tag change must not clear expiry") require.Equal(t, expiry.Unix(), tagged.AsStruct().Expiry.Unix()) } + +// TestUserKeyOnTaggedMachine: a user key on a tagged machine re-authenticates +// the tagged node only when it speaks for it (the user an admin tagged it +// from); another user's key wakes it, gets a device of its own beside it with +// a new node key, and cannot lift its expiry. +func TestUserKeyOnTaggedMachine(t *testing.T) { + type nodeFields struct { + NodeKey key.NodePublic + Hostname string + AuthKeyID *uint64 + Expiry int64 + Tags []string + UserID *uint + } + + fieldsOf := func(n *types.Node) nodeFields { + f := nodeFields{ + NodeKey: n.NodeKey, + Hostname: n.Hostname, + AuthKeyID: n.AuthKeyID, + Tags: n.Tags, + UserID: n.UserID, + } + if n.Expiry != nil { + f.Expiry = n.Expiry.UnixNano() + } + + return f + } + + for _, adminTagged := range []bool{false, true} { + for _, mode := range []string{"expired", "rotated", "wake"} { + t.Run(fmt.Sprintf("admin=%t/%s", adminTagged, mode), func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("former-owner") + + var owner *types.UserID + + tags := []string{"tag:prod"} + + if adminTagged { + owner = user.TypedID() + tags = nil + } + + original, err := s.CreatePreAuthKey(owner, true, true, nil, tags) + require.NoError(t, err) + + machineKey := key.NewMachine().Public() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: original.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "h0"}, + Expiry: time.Now().Add(24 * time.Hour), + } + node, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey) + require.NoError(t, err) + + fields := func() (nodeFields, nodeFields) { + t.Helper() + + ns, ok := s.GetNodeByID(node.ID()) + require.True(t, ok) + + dbNode, err := s.db.GetNodeByID(node.ID()) + require.NoError(t, err) + + return fieldsOf(ns.AsStruct()), fieldsOf(dbNode) + } + + if adminTagged { + _, err = s.SetPolicy([]byte(`{"tagOwners":{"tag:prod":["former-owner@"]}}`)) + require.NoError(t, err) + _, _, err = s.SetNodeTags(node.ID(), []string{"tag:prod"}) + require.NoError(t, err) + } else { + user = s.CreateUserForTest("guest") + } + + presented, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + req := regReq + req.Hostinfo = &tailcfg.Hostinfo{Hostname: "h1"} + req.Auth = &tailcfg.RegisterResponseAuth{AuthKey: presented.Key} + + switch mode { + case "expired": + past := time.Now().Add(-time.Hour) + _, _, err = s.SetNodeExpiry(node.ID(), &past) + require.NoError(t, err) + case "rotated": + req.NodeKey = key.NewNode().Public() + } + + beforeCache, beforeDB := fields() + + got, _, err := s.HandleNodeFromPreAuthKey(req, machineKey) + + afterCache, afterDB := fields() + assert.Equal(t, afterCache, afterDB, "NodeStore and database agree") + + storedKey, keyErr := s.GetPreAuthKey(presented.Key) + require.NoError(t, keyErr) + + switch { + case !adminTagged && mode == "expired": + require.EqualError(t, err, "authkey cannot re-authenticate this node") + assert.Equal(t, beforeCache, afterCache) + assert.Equal(t, beforeDB, afterDB) + assert.False(t, storedKey.Used) + assert.Equal(t, 1, s.ListNodes().Len()) + case !adminTagged && mode == "rotated": + require.NoError(t, err) + assert.NotEqual(t, node.ID(), got.ID(), "a device of its own") + assert.False(t, got.IsTagged()) + assert.Equal(t, user.ID, got.UserID().Get()) + assert.Equal(t, beforeCache.NodeKey, afterCache.NodeKey, "the tagged node is untouched") + assert.Equal(t, beforeCache.Tags, afterCache.Tags) + assert.Equal(t, beforeCache.AuthKeyID, afterCache.AuthKeyID) + assert.True(t, storedKey.Used) + assert.Equal(t, 2, s.ListNodes().Len()) + case !adminTagged: + require.NoError(t, err) + assert.Equal(t, original.ID, *afterCache.AuthKeyID) + assert.Equal(t, beforeCache.Tags, afterCache.Tags) + assert.False(t, storedKey.Used) + assert.Equal(t, 1, s.ListNodes().Len()) + case mode == "rotated": + require.NoError(t, err) + assert.Equal(t, node.ID(), got.ID(), "converted in place") + assert.Empty(t, afterCache.Tags) + require.NotNil(t, afterCache.UserID) + assert.Equal(t, user.ID, *afterCache.UserID) + assert.Equal(t, req.NodeKey, afterCache.NodeKey) + assert.Equal(t, 1, s.ListNodes().Len()) + default: + require.NoError(t, err) + assert.Equal(t, []string{"tag:prod"}, afterCache.Tags, "a same-key relogin keeps it tagged") + assert.Equal(t, presented.ID, *afterCache.AuthKeyID) + assert.Equal(t, 1, s.ListNodes().Len()) + + if mode == "expired" { + assert.Zero(t, afterCache.Expiry, "a valid key lifts the past expiry") + } else { + assert.Equal(t, beforeCache.Expiry, afterCache.Expiry) + } + } + }) + } + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1817db698..882edb8ff 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2252,10 +2252,7 @@ func (s *State) HandleNodeFromAuthPath( all := s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) // 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() + existingNode, nodeExists := loginNode(all, types.UserID(user.ID)) var existingNodeOtherUser types.NodeView @@ -2268,14 +2265,6 @@ func (s *State) HandleNodeFromAuthPath( } } - // 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(). Str(zf.RegistrationID, authID.String()). @@ -2299,15 +2288,15 @@ func (s *State) HandleNodeFromAuthPath( keysOnly bool ) - if nodeExistsForSameUser { - updateParams.ExistingNode = existingNodeSameUser + if nodeExists && !existingNode.IsTagged() { + updateParams.ExistingNode = existingNode finalNode, keysOnly, err = s.applyAuthNodeUpdate(updateParams) if err != nil { return types.NodeView{}, change.Change{}, err } - } else if existingNodeIsTagged { - updateParams.ExistingNode = taggedNode + } else if nodeExists { + updateParams.ExistingNode = existingNode updateParams.IsConvertFromTag = true finalNode, _, err = s.applyAuthNodeUpdate(updateParams) @@ -2403,31 +2392,27 @@ func (s *State) createNewNodeFromAuth( // HandleNodeFromPreAuthKey handles node registration using a pre-authentication key. // findExistingNodeForPAK looks up an existing node by machine key, -// matching the PAK's ownership. For user-owned keys it checks the -// user's ID; for tagged keys it checks UserID(0) since tagged nodes -// have no owning user. +// matching the PAK's ownership. A key with a user finds that user's node, +// else the machine's tagged node; a tags-only key checks UserID(0) since +// tagged nodes have no owning user. func (s *State) findExistingNodeForPAK( machineKey key.MachinePublic, pak *types.PreAuthKey, + nodeKey key.NodePublic, ) (types.NodeView, bool, error) { all := s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) if pak.User != nil { - if node, ok := all[types.UserID(pak.User.ID)]; ok { - return node, true, nil + node, ok := all[types.UserID(pak.User.ID)] + if !ok { + // The machine's tagged node, when the key speaks for it. Any other + // key under its own node key is the device restarting (#3312); + // HandleNodeFromPreAuthKey lets that key only wake it. + node, ok = all[0] + ok = ok && node.IsTagged() && (pak.CanReauthenticate(node) || node.NodeKey() == nodeKey) } - // The node may have been converted to a tagged node since it first - // registered (SetNodeTags clears UserID, re-indexing it under UserID(0)). - // It is still the same machine, proven by the machine key, so recognise - // 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, ok := all[0]; ok && node.IsTagged() { - return node, true, nil - } - - return types.NodeView{}, false, nil + return node, ok, nil } // A tagged key re-registers the same machine regardless of how it is @@ -2465,6 +2450,25 @@ func (s *State) findExistingNodeForPAK( return types.NodeView{}, false, nil } +// loginNode picks, from a machine's nodes, the one a login by userID +// re-authenticates: the user's own node, else the machine's tagged node when +// the user can claim it. A tagged node that joined by login records no former +// owner, so any login re-authenticates it, as before; the tags the login asks +// for decide whether it stays tagged. Otherwise another user's login on a +// tagged machine adds a device for that user beside it, as on Tailscale SaaS. +func loginNode(all map[types.UserID]types.NodeView, userID types.UserID) (types.NodeView, bool) { + if node, ok := all[userID]; ok { + return node, true + } + + if node, ok := all[0]; ok && node.IsTagged() && + (node.ClaimableBy(userID) || node.RegisterMethod() != util.RegisterMethodAuthKey) { + return node, true + } + + return types.NodeView{}, false +} + //nolint:gocyclo // sequential validation/update/create paths with security-sensitive ordering func (s *State) HandleNodeFromPreAuthKey( regReq tailcfg.RegisterRequest, @@ -2488,7 +2492,7 @@ func (s *State) HandleNodeFromPreAuthKey( return types.TaggedDevices.Name } - existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak) + existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak, regReq.NodeKey) if err != nil { return types.NodeView{}, change.Change{}, err } @@ -2537,6 +2541,11 @@ func (s *State) HandleNodeFromPreAuthKey( pak.IsTagged() && existingNodeSameUser.IsTagged() && (!existingNodeSameUser.AuthKeyID().Valid() || existingNodeSameUser.AuthKeyID().Get() != pak.ID) + // Only a key that speaks for the node re-authenticates it. Any other key + // (another user's key on a tagged machine, under the node's own node key) + // only wakes it. + reauth := isExistingNodeReregistering && pak.CanReauthenticate(existingNodeSameUser) + if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && !isOwnershipConversion && !isRetag { // Existing, still-valid node re-registering with same NodeKey: skip // validation. Pre-auth keys are only needed for initial authentication. @@ -2561,6 +2570,10 @@ func (s *State) HandleNodeFromPreAuthKey( if err != nil { return types.NodeView{}, change.Change{}, err } + + if isExistingNodeReregistering && !reauth { + return types.NodeView{}, change.Change{}, types.PAKError("authkey cannot re-authenticate this node") + } } // Preserve the raw hostname as reported by the client. Sanitisation @@ -2621,7 +2634,6 @@ func (s *State) HandleNodeFromPreAuthKey( // Update existing node - NodeStore first, then database before, updatedNodeView, ok := s.nodeStore.UpdateNodeDiff(existingNodeSameUser.ID(), func(node *types.Node) { - node.NodeKey = regReq.NodeKey node.Hostname = hostname // TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics @@ -2631,6 +2643,16 @@ func (s *State) HandleNodeFromPreAuthKey( node.Hostinfo = validHostinfo node.Hostinfo.NetInfo = preserveNetInfo(existingNodeSameUser, existingNodeSameUser.ID(), validHostinfo) + // Preserve online state during re-registration so a live node does + // not appear offline before the client restarts its map stream. + node.LastSeen = new(time.Now()) + + if !reauth { + return + } + + node.NodeKey = regReq.NodeKey + node.RegisterMethod = util.RegisterMethodAuthKey // Tags from a PreAuthKey are applied on initial registration and @@ -2664,12 +2686,17 @@ func (s *State) HandleNodeFromPreAuthKey( if wasUserOwned || node.IsExpired() { node.Expiry = nil } + } else if node.IsTagged() && !pak.IsTagged() && isNodeKeyRotation { + // A user key that speaks for a tagged node, with a new node key, + // takes it back as that user's device, as on Tailscale SaaS. A + // relogin under the same node key keeps it tagged. + node.Tags = nil + node.UserID = &pak.User.ID + node.User = pak.User } + node.AuthKey = pak node.AuthKeyID = &pak.ID - // Preserve online state during re-registration so a live node does - // not appear offline before the client restarts its map stream. - node.LastSeen = new(time.Now()) // Tagged nodes keep their existing expiry (disabled). // User-owned nodes update expiry from the client request, @@ -2704,7 +2731,11 @@ func (s *State) HandleNodeFromPreAuthKey( // (#2862), but re-registration presents a freshly-validated key, so // its ID must be persisted here — otherwise a restart reloads the old // key and any key-scoped property (e.g. Ephemeral) silently reverts. - reregColumns := append(slices.Clone(nodeUpdateColumns), "AuthKeyID") + // A wake-up keeps the prior key, which may have been deleted (#2862). + reregColumns := nodeUpdateColumns + if reauth { + reregColumns = append(slices.Clone(nodeUpdateColumns), "AuthKeyID") + } err := tx.Select(reregColumns).Updates(updatedNodeView.AsStruct()).Error if err != nil { @@ -2717,7 +2748,7 @@ func (s *State) HandleNodeFromPreAuthKey( // reject it as "authkey already used". This is the path // behind issue #2830 where containers restart with the // same one-shot key. - if !pak.Reusable && !pak.Used { + if reauth && !pak.Reusable && !pak.Used { err = hsdb.UsePreAuthKey(tx, pak) if err != nil { return nil, fmt.Errorf("using pre auth key: %w", err) diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index cd1a1469f..94bb5fe96 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -1012,6 +1012,20 @@ func (nv NodeView) IsTagged() bool { return nv.ж.IsTagged() } +// ClaimableBy reports whether a login by userID takes this tagged node back +// as that user's device, rather than adding a device beside it: userID owns +// or created the key it joined with. Tagging clears a node's user, so a node +// that joined by login records no former owner and nobody claims it. +func (nv NodeView) ClaimableBy(userID UserID) bool { + if !nv.IsTagged() || nv.RegisterMethod() != util.RegisterMethodAuthKey { + return false + } + + k := nv.AuthKey() + + return k.Valid() && k.UserID().Valid() && UserID(k.UserID().Get()) == userID +} + // IsExpired returns whether the node registration has expired. func (nv NodeView) IsExpired() bool { if !nv.Valid() { diff --git a/hscontrol/types/preauth_key.go b/hscontrol/types/preauth_key.go index a78ff3d71..ae5cddd6d 100644 --- a/hscontrol/types/preauth_key.go +++ b/hscontrol/types/preauth_key.go @@ -150,6 +150,29 @@ func (pak *PreAuthKey) IsTagged() bool { return len(pak.Tags) > 0 } +// CanReauthenticate reports whether this key speaks for node: a tagged key +// (it brings its own tags), a key of the node's user, or a key of the user a +// tagged node can be claimed by (see [NodeView.ClaimableBy]). +func (pak *PreAuthKey) CanReauthenticate(node NodeView) bool { + if pak == nil || !node.Valid() { + return false + } + + if pak.IsTagged() { + return true + } + + if pak.User == nil { + return false + } + + if node.IsTagged() { + return node.ClaimableBy(UserID(pak.User.ID)) + } + + return node.UserID().Valid() && node.UserID().Get() == pak.User.ID +} + // maskedPrefix returns the key prefix in masked format for safe logging. // SECURITY: Never log the full key or hash, only the masked prefix. func (pak *PreAuthKey) maskedPrefix() string { diff --git a/hscontrol/types/preauth_key_test.go b/hscontrol/types/preauth_key_test.go index 1b280149b..f6e05af40 100644 --- a/hscontrol/types/preauth_key_test.go +++ b/hscontrol/types/preauth_key_test.go @@ -6,6 +6,8 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/juanfont/headscale/hscontrol/util" + "github.com/stretchr/testify/require" ) func TestCanUsePreAuthKey(t *testing.T) { @@ -127,3 +129,50 @@ func TestCanUsePreAuthKey(t *testing.T) { }) } } + +func TestPreAuthKeyCanReauthenticate(t *testing.T) { + owner, other := uint(1), uint(2) + userNode := (&Node{ID: 1, UserID: &owner}).View() + + // An admin tagged a node its user joined with a user key. + taggedByUserKey := (&Node{ + ID: 1, Tags: []string{"tag:prod"}, RegisterMethod: util.RegisterMethodAuthKey, + AuthKey: &PreAuthKey{ID: 10, UserID: &owner}, + }).View() + // A user made the tagged key the node joined with. + taggedByTaggedKey := (&Node{ + ID: 1, Tags: []string{"tag:prod"}, RegisterMethod: util.RegisterMethodAuthKey, + AuthKey: &PreAuthKey{ID: 10, UserID: &owner, Tags: []string{"tag:prod"}}, + }).View() + // A node registered by login records no user. + taggedByLogin := (&Node{ + ID: 1, Tags: []string{"tag:prod"}, RegisterMethod: util.RegisterMethodOIDC, + }).View() + + userKey := func(id uint) *PreAuthKey { return &PreAuthKey{ID: 11, User: &User{ID: id}} } + taggedKey := &PreAuthKey{Tags: []string{"tag:ci"}} + + for _, tt := range []struct { + name string + pak *PreAuthKey + node NodeView + want bool + }{ + {"tagged key converts", taggedKey, userNode, true}, + {"tagged key retags", taggedKey, taggedByTaggedKey, true}, + {"same user", userKey(owner), userNode, true}, + {"other user", userKey(other), userNode, false}, + {"former owner after admin tagging", userKey(owner), taggedByUserKey, true}, + {"other user on admin-tagged node", userKey(other), taggedByUserKey, false}, + {"tagged key creator", userKey(owner), taggedByTaggedKey, true}, + {"other user on tagged key node", userKey(other), taggedByTaggedKey, false}, + {"node registered by login", userKey(owner), taggedByLogin, false}, + {"no owner", &PreAuthKey{ID: 11}, userNode, false}, + {"invalid node", taggedKey, NodeView{}, false}, + {"nil key", nil, userNode, false}, + } { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, tt.pak.CanReauthenticate(tt.node)) + }) + } +} diff --git a/integration/tags_test.go b/integration/tags_test.go index 68e7634aa..4033edca6 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -3295,18 +3295,19 @@ func TestTagsAuthKeyWithoutUserRejectsAdvertisedTags(t *testing.T) { } // ============================================================================= -// Test Suite 6: Tagged→User Conversion via CLI Register (#3038) +// Test Suite 6: User Login on a Tagged Machine 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 auth register --auth-id --user ". -// The crash happens in the mapper's generateUserProfiles when [types.Node.User] is nil -// after the tag→user conversion in [State.processReauthTags]. +// TestTagsAuthKeyWithoutUserCLIRegisterAddsUserDevice covers issue #3038: +// register a node with a tags-only preauthkey (no user), then log the machine +// in as a user via "headscale auth register --auth-id --user ". +// The crash was in the mapper's generateUserProfiles when [types.Node.User] +// was nil. // -// 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) { +// No user made the key, so no user can take the tagged node back. As for +// another user's login on a tagged machine on Tailscale SaaS, the login adds a +// device of the user's own beside it. +func TestTagsAuthKeyWithoutUserCLIRegisterAddsUserDevice(t *testing.T) { IntegrationSkip(t) policy := tagsTestPolicy() @@ -3331,10 +3332,8 @@ func TestTagsAuthKeyConvertToUserViaCLIRegister(t *testing.T) { headscale, err := scenario.Headscale() requireNoErrGetHeadscale(t, err) - // Step 1: Create a tags-only preauthkey WITHOUT a user. - // This is the critical detail: when [types.PreAuthKey.UserID] is nil, the node - // enters the [state.NodeStore] with [types.Node.User] == nil. The [state.State.processReauthTags] - // conversion then sets UserID but not User, leaving it nil for the mapper. + // Step 1: Create a tags-only preauthkey WITHOUT a user, so the node enters + // the [state.NodeStore] with [types.Node.User] == nil. authKey, err := scenario.CreatePreAuthKeyWithOptions(hsic.AuthKeyOptions{ User: nil, Reusable: false, @@ -3356,6 +3355,8 @@ func TestTagsAuthKeyConvertToUserViaCLIRegister(t *testing.T) { err = client.WaitForRunning(integrationutil.PeerSyncTimeout()) require.NoError(t, err) + var taggedID uint64 + // Verify initial state: node is tagged assert.EventuallyWithT(t, func(c *assert.CollectT) { nodes, err := headscale.ListNodes() @@ -3364,6 +3365,7 @@ func TestTagsAuthKeyConvertToUserViaCLIRegister(t *testing.T) { if len(nodes) == 1 { assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"}) + taggedID = nodes[0].GetId() t.Logf("Initial state - Node ID: %d, Tags: %v", nodes[0].GetId(), nodes[0].GetTags()) } }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "node should be tagged initially") @@ -3386,30 +3388,32 @@ func TestTagsAuthKeyConvertToUserViaCLIRegister(t *testing.T) { 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) + // Step 3: Register via CLI with user (the step that triggered the panic) err = scenario.runHeadscaleRegister(tagTestUser, body) require.NoError(t, err) err = client.WaitForRunning(integrationutil.PeerSyncTimeout()) 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 [tailcfg.MapResponse] and calls - // [types.Node.Owner].Model().ID with a nil User pointer. - // ShutdownAssertNoPanics in the defer catches any panics in headscale logs. + // Step 4: The tagged node stays and the user gets a device of its own. The + // mapper builds both into the map; ShutdownAssertNoPanics in the defer + // catches a panic in the headscale logs. assert.EventuallyWithT(t, func(c *assert.CollectT) { nodes, err := headscale.ListNodes() assert.NoError(c, err) - assert.Len(c, nodes, 1) + assert.Len(c, nodes, 2) - 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()) + for _, node := range nodes { + if node.GetId() == taggedID { + assertNodeHasTagsWithCollect(c, node, []string{"tag:valid-owned"}) + + continue + } + + assertNodeHasNoTagsWithCollect(c, node) + assert.Equal(c, tagTestUser, node.GetUser().GetName(), "the login's device belongs to the user") } - }, integrationutil.HAConvergeTimeout, 1*time.Second, "node should be user-owned after conversion via CLI register") + }, integrationutil.HAConvergeTimeout, 1*time.Second, "login should add a user device beside the tagged node") } // TestTaggedNodeLogoutReloginSingleUseKeyOnline reproduces issue #3371