diff --git a/CHANGELOG.md b/CHANGELOG.md index 57404c588..9d5873850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - Add the v2 `acl/validate` endpoint, which Terraform provider 0.29.3 and newer call to check `tailscale_acl` at plan time; before it, those releases failed with `invalid character 'p' after top-level value` - A route that is a single address inside the node ranges, in either spelling, is approved and listed but never served; wider routes and `::ffff:` prefixes are still served - Require a valid pre-auth key to re-register an expired tagged node +- Another user's key or login on a tagged machine now creates a new device for that user instead of re-authenticating the tagged node ## 0.29.7 (202x-xx-xx) diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 28cde5d62..380ee1f83 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -4120,9 +4120,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() @@ -4195,19 +4196,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/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index 4e69668ff..04886740b 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -60,32 +60,6 @@ var reauthSkipReasons = map[string]string{ "reauth-user/a1-spent-key/u-spent/expiry": sameKeyRenewsExpiry, "reauth-user/a2-valid-key/u-valid/expiry": sameKeyRenewsExpiry, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/devices": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/owner": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/node-key": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/expiry": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/ephemeral": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/peer-obs": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/obs/filter": otherUserKeyReauthsTagged, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/login-devices": loginTakesTaggedDevice, - "reauth-foreign/t-reauth-foreign-relogin/t-reauth-foreign/login-owner": loginTakesTaggedDevice, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/devices": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/owner": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/node-key": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/expiry": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/ephemeral": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/peer-obs": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/obs/filter": otherUserKeyReauthsTagged, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/login-devices": loginTakesTaggedDevice, - "reauth-foreign/t-exp-foreign-relogin/t-exp-foreign/login-owner": loginTakesTaggedDevice, - - "reauth-foreign/t-eph-spent-foreign-key/t-eph/ephemeral": spentKeyRebindsNode, - "reauth-foreign/t-eph-logout/t-eph/devices": spentKeyRebindsNode, - "reauth-foreign/t-eph-logout/t-eph/owner": spentKeyRebindsNode, - "reauth-foreign/t-eph-logout/t-eph/expiry": spentKeyRebindsNode, - "reauth-foreign/t-eph-logout/t-eph/ephemeral": spentKeyRebindsNode, - "reauth-foreign/t-eph-logout/t-eph/peer-obs": spentKeyRebindsNode, - "reauth-foreign/t-logout-logout/t-logout/expiry": taggedLogoutNotExpired, "reauth-foreign/t-logout-logout/t-logout/peer-obs": taggedLogoutNotExpired, "reauth-foreign/t-logout-relogin/t-logout/expiry": taggedLogoutNotExpired, @@ -105,10 +79,6 @@ var reauthSkipReasons = map[string]string{ "identity/w-tagreq-new-join/w-tagreq-new/peer-obs": keyLoginTagsRefused, "identity/w-tagreq-new-join/w-tagreq-new/peer-key-obs": keyLoginTagsRefused, - "reauth-owner/u-tagged-reauth-relogin/u-tagged-reauth/owner": adminTagsKeptOnUserLogin, - "reauth-owner/u-tagged-reauth-relogin/u-tagged-reauth/expiry": adminTagsKeptOnUserLogin, - "reauth-owner/u-tagged-reauth-relogin/obs/filter": adminTagsKeptOnUserLogin, - "reauth/late-pak-spent-key/late-pak/text": refusalText, "reauth/late-pak-spent-key/late-pak/legacy-text": refusalText, } @@ -120,16 +90,12 @@ const ( ) const ( - expiredKeyNotRotated = "EXPIRED_KEY_NOT_ROTATED: headscale answers an expired node's key login without asking for a new node key; SaaS answers NodeKeyExpired first, so the client rotates it" - sameKeyRenewsExpiry = "SAME_KEY_LOGIN_RENEWS_EXPIRY: headscale renews a user node's expiry on a key login that keeps its node key; SaaS renews it only with a new node key" - otherUserKeyReauthsTagged = "OTHER_USER_KEY_REAUTHS_TAGGED: headscale re-registers a tagged device in place when another user's key logs in anew on its machine; SaaS keeps the tagged device and adds one for that user" - loginTakesTaggedDevice = "LOGIN_TAKES_TAGGED_DEVICE: headscale's interactive login gives another user the tagged device on the machine; SaaS's key login keeps it and adds a device for that user" - spentKeyRebindsNode = "SPENT_KEY_REBINDS_NODE: headscale binds a live node to the spent key it logs in with, so an ephemeral node stops being ephemeral and survives logout; SaaS keeps the node as it was" - taggedLogoutNotExpired = "TAGGED_LOGOUT_NOT_EXPIRED: headscale leaves a tagged node unexpired when it logs out; SaaS expires it" - wakeWithTagsRefused = "WAKE_WITH_TAGS_REFUSED: headscale refuses a key login that requests tags the key does not carry even when the node keeps its node key and tags; SaaS accepts it and changes nothing" - keyLoginTagsRefused = "KEY_LOGIN_TAGS_REFUSED: headscale refuses a user's key login that requests tags the user owns; SaaS tags the device, converting it in place or creating it tagged" - adminTagsKeptOnUserLogin = "ADMIN_TAGS_KEPT_ON_USER_LOGIN: headscale keeps an admin-tagged device tagged when its user's key logs in anew; SaaS turns it back into that user's device" - refusalText = "REFUSAL_TEXT: SaaS refuses a key by naming its ID; headscale says why the key is refused" + expiredKeyNotRotated = "EXPIRED_KEY_NOT_ROTATED: headscale answers an expired node's key login without asking for a new node key; SaaS answers NodeKeyExpired first, so the client rotates it" + sameKeyRenewsExpiry = "SAME_KEY_LOGIN_RENEWS_EXPIRY: headscale renews a user node's expiry on a key login that keeps its node key; SaaS renews it only with a new node key" + taggedLogoutNotExpired = "TAGGED_LOGOUT_NOT_EXPIRED: headscale leaves a tagged node unexpired when it logs out; SaaS expires it" + wakeWithTagsRefused = "WAKE_WITH_TAGS_REFUSED: headscale refuses a key login that requests tags the key does not carry even when the node keeps its node key and tags; SaaS accepts it and changes nothing" + keyLoginTagsRefused = "KEY_LOGIN_TAGS_REFUSED: headscale refuses a user's key login that requests tags the user owns; SaaS tags the device, converting it in place or creating it tagged" + refusalText = "REFUSAL_TEXT: SaaS refuses a key by naming its ID; headscale says why the key is refused" ) // reauthNotCompared lists, by key prefix, checks left out: SaaS's answer diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 05a1bb728..18cc6d36e 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -11,6 +11,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" "gorm.io/gorm" "tailscale.com/tailcfg" @@ -382,34 +383,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 @@ -465,12 +543,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") @@ -706,8 +788,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. @@ -720,12 +802,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.GetPreAuthKeyByID(ephKey.ID) require.NoError(t, err) seeded, ok := n.s.nodeStore.UpdateNode(n.id, func(nd *types.Node) { + nd.RegisterMethod = util.RegisterMethodAuthKey nd.AuthKey = pak.AsCredential() nd.AuthKeyID = &pak.ID }) @@ -1745,7 +1828,7 @@ func (c pakReregCase) lookup(t *testing.T, keyStr string) (types.NodeView, *type pak, err := c.s.GetPreAuthKey(keyStr) require.NoError(t, err) - view, ok, err := c.s.findExistingNodeForPAK(c.machineKey, pak) + view, ok, err := c.s.findExistingNodeForPAK(c.machineKey, pak, c.regReq.NodeKey) require.NoError(t, err) require.True(t, ok) require.True(t, pakSkipsValidation(view, pak, c.regReq.NodeKey, time.Now()), @@ -2244,3 +2327,136 @@ func TestPreAuthKeyRegistrationOwnsHostinfo(t *testing.T) { }) } } + +// 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) { + 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) + + c := registerForPAKRereg(t, s, original) + if adminTagged { + _, err = s.SetPolicy([]byte(`{"tagOwners":{"tag:prod":["former-owner@"]}}`)) + require.NoError(t, err) + _, _, err = s.SetNodeTags(c.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 := c.regReq + req.Auth = &tailcfg.RegisterResponseAuth{AuthKey: presented.Key} + + switch mode { + case "expired": + past := time.Now().Add(-time.Hour) + _, _, err = s.SetNodeExpiry(c.node.ID(), &past) + require.NoError(t, err) + case "rotated": + req.NodeKey = key.NewNode().Public() + } + + beforeCache, beforeDB := c.fields(t) + + got, _, err := s.HandleNodeFromPreAuthKey(req, c.machineKey) + + afterCache, afterDB := c.fields(t) + assert.Equal(t, afterCache, afterDB, "NodeStore and database agree") + + storedKey, keyErr := s.GetPreAuthKeyByID(presented.ID) + 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, c.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, c.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) + } + } + }) + } + } +} + +// TestPAKWakeRechecksAuthorityAtMutation: another user's key that wakes a +// live tagged node is refused when an expiry lands before the writer runs. +func TestPAKWakeRechecksAuthorityAtMutation(t *testing.T) { + s := newRetagTestState(t) + original, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:prod"}) + require.NoError(t, err) + c := registerForPAKRereg(t, s, original) + + guest := s.CreateUserForTest("guest") + guestKey, err := s.CreatePreAuthKey(guest.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + view, pak := c.lookup(t, guestKey.Key) + + past := time.Now().Add(-time.Hour) + _, _, err = s.SetNodeExpiry(c.node.ID(), &past) + require.NoError(t, err) + + beforeCache, beforeDB := c.fields(t) + _, err = c.reregister(view, pak, c.regReq) + require.EqualError(t, err, "authkey cannot re-authenticate this node") + + afterCache, afterDB := c.fields(t) + assert.Equal(t, beforeCache, afterCache) + assert.Equal(t, beforeDB, afterDB) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index dd10f4581..3a4ebf412 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2501,10 +2501,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 @@ -2517,14 +2514,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{}, s.policyChangeSince(genBefore), ErrAmbiguousNodeOwnership - } - // Create logger with common fields for all auth operations logger := log.With(). Str(zf.RegistrationID, authID.String()). @@ -2548,15 +2537,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{}, s.policyChangeSince(genBefore), err } - } else if existingNodeIsTagged { - updateParams.ExistingNode = taggedNode + } else if nodeExists { + updateParams.ExistingNode = existingNode updateParams.IsConvertFromTag = true finalNode, _, err = s.applyAuthNodeUpdate(updateParams) @@ -2652,31 +2641,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); + // pakReregister 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 @@ -2714,6 +2699,31 @@ func (s *State) findExistingNodeForPAK( return types.NodeView{}, false, nil } +// LoginNode returns the node that a login by userID on machineKey +// re-authenticates. False means the login adds a new device. +func (s *State) LoginNode(machineKey key.MachinePublic, userID types.UserID) (types.NodeView, bool) { + return loginNode(s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey), userID) +} + +// 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 +} + // pakSkipsValidation reports whether re-registering n with pak may skip key // validation at now. The machine key proves identity and a pre-auth key only // authorises the initial join, so a plain restart (container "tailscale up @@ -2745,14 +2755,22 @@ func pakSkipsValidation(n types.NodeView, pak *types.PreAuthKey, nodeKey key.Nod !isOwnershipConversion && !isRetag } -// pakReregisterErr is the authorisation decision for re-registering n with -// pak at now: nil when validation may be skipped, else the key's validity. -func pakReregisterErr(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) error { +// pakReregister is the authorisation decision for re-registering n with pak +// at now: whether the key re-authenticates n, and nil when validation may be +// skipped, else the key's validity. A key that does not speak for n only +// wakes it under its own node key. +func pakReregister(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) (bool, error) { + reauth := pak.CanReauthenticate(n) if pakSkipsValidation(n, pak, nodeKey, now) { - return nil + return reauth, nil } - return pak.ValidAt(now) + err := pak.ValidAt(now) + if err == nil && !reauth { + err = types.PAKError("authkey cannot re-authenticate this node") + } + + return reauth, err } //nolint:gocyclo // sequential validation/update/create paths with security-sensitive ordering @@ -2791,7 +2809,7 @@ func (s *State) HandleNodeFromPreAuthKey( } } - existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak) + existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak, regReq.NodeKey) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } @@ -2991,6 +3009,7 @@ func (s *State) reregisterNodeWithPAK( var ( prior *types.Node authErr error + reauth bool ) // Update existing node - NodeStore first, then database @@ -3000,12 +3019,11 @@ func (s *State) reregisterNodeWithPAK( prior = node.Clone() - authErr = pakReregisterErr(node.View(), pak, regReq.NodeKey, now) + reauth, authErr = pakReregister(node.View(), pak, regReq.NodeKey, now) if authErr != nil { return } - node.NodeKey = regReq.NodeKey node.Hostname = hostname // TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics @@ -3015,6 +3033,16 @@ func (s *State) reregisterNodeWithPAK( node.Hostinfo = validHostinfo node.Hostinfo.NetInfo = preserveNetInfo(prior.View(), prior.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(now) + + if !reauth { + return + } + + node.NodeKey = regReq.NodeKey + node.RegisterMethod = util.RegisterMethodAuthKey // Tags from a PreAuthKey are applied on initial registration and @@ -3046,6 +3074,13 @@ func (s *State) reregisterNodeWithPAK( if wasUserOwned || node.IsExpiredAt(now) { node.Expiry = nil } + } else if node.IsTagged() && !pak.IsTagged() && prior.NodeKey != regReq.NodeKey { + // 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.AsCredential() @@ -3056,9 +3091,6 @@ func (s *State) reregisterNodeWithPAK( if !pak.Reusable && !pak.Used { node.AuthKey.Used = true } - // Preserve online state during re-registration so a live node does - // not appear offline before the client restarts its map stream. - node.LastSeen = new(now) // Tagged nodes keep their existing expiry (disabled). // User-owned nodes update expiry from the client request, @@ -3107,7 +3139,11 @@ func (s *State) reregisterNodeWithPAK( // re-registration presents a key just loaded from the database, 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 { @@ -3119,7 +3155,7 @@ func (s *State) reregisterNodeWithPAK( // atomic compare-and-set in [hsdb.UsePreAuthKey] would otherwise // reject it as "authkey already used", locking out containers // that 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 e85f80b57..50c029302 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -963,6 +963,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 6adad5dd4..12ada70e3 100644 --- a/hscontrol/types/preauth_key.go +++ b/hscontrol/types/preauth_key.go @@ -157,6 +157,29 @@ func (pak *PreAuthKey) RetagsNode(node NodeView) bool { (!node.AuthKeyID().Valid() || node.AuthKeyID().Get() != pak.ID) } +// 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 c7e33af8b..905a36ca4 100644 --- a/hscontrol/types/preauth_key_test.go +++ b/hscontrol/types/preauth_key_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/juanfont/headscale/hscontrol/util" "github.com/stretchr/testify/require" ) @@ -199,3 +200,50 @@ func TestPreAuthKeyTagChanges(t *testing.T) { require.False(t, pak.ConvertsNodeToTagged(NodeView{})) require.False(t, pak.RetagsNode(NodeView{})) } + +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: &Credential{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: &Credential{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 5940187cd..4becf7a70 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 string + // 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].Id t.Logf("Initial state - Node ID: %s, Tags: %v", nodes[0].Id, nodes[0].Tags) } }, 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].User.Name, - "Node ownership should be returned to user after untagging") - t.Logf("After conversion - Node ID: %s, Tags: %v, User: %s", - nodes[0].Id, nodes[0].Tags, nodes[0].User.Name) + for _, node := range nodes { + if node.Id == taggedID { + assertNodeHasTagsWithCollect(c, node, []string{"tag:valid-owned"}) + + continue + } + + assertNodeHasNoTagsWithCollect(c, node) + assert.Equal(c, tagTestUser, node.User.Name, "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