From 5e382e2a897468cb15b6bcb95acf33e9228c1efc Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 09:38:51 +0000 Subject: [PATCH] state: give another user's key on a tagged machine a new device Only a key that speaks for a tagged node re-authenticates it: a tagged key, or one of the user it was tagged from, which takes it back with a new node key. Another user's key or login adds that user's own device. --- hscontrol/auth_test.go | 21 +- hscontrol/servertest/reauth_compat_test.go | 62 ++--- hscontrol/state/auth_tagged_expiry_test.go | 263 +++++++++++++++++++-- hscontrol/state/state.go | 89 ++++--- hscontrol/types/node.go | 14 ++ hscontrol/types/preauth_key.go | 23 ++ hscontrol/types/preauth_key_test.go | 48 ++++ 7 files changed, 408 insertions(+), 112 deletions(-) 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 3458ffe44..c863cda27 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -57,43 +57,23 @@ var reauthSkipReasons = map[string]string{ "reauth-user/a5-expired-valid-key/u-valid/outcome": expiredKeyNotRotated, "reauth-user/a5-expired-valid-key/u-valid/node-key": expiredKeyAccepted, - "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-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, "reauth-foreign/t-logout-relogin/t-logout/peer-obs": taggedLogoutNotExpired, - "reauth-foreign/t-wake-foreign-key-tags/t-wake/outcome": wakeWithTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/owner": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/outcome": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/node-key": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/expiry": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/obs/filter": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/devices": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/owner": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/outcome": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/expiry": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/ephemeral": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/peer-obs": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/peer-key-obs": keyLoginTagsRefused, + "identity/w-tagreq-relogin-tags/w-tagreq/owner": keyLoginTagsRefused, + "identity/w-tagreq-relogin-tags/w-tagreq/outcome": keyLoginTagsRefused, + "identity/w-tagreq-relogin-tags/w-tagreq/node-key": keyLoginTagsRefused, + "identity/w-tagreq-relogin-tags/w-tagreq/expiry": keyLoginTagsRefused, + "identity/w-tagreq-relogin-tags/obs/filter": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/devices": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/owner": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/outcome": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/expiry": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/ephemeral": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/peer-obs": keyLoginTagsRefused, + "identity/w-tagreq-new-join/w-tagreq-new/peer-key-obs": keyLoginTagsRefused, "tag-owner-chain/u-base-join/u-base/devices": keyLoginTagsRefused, "tag-owner-chain/u-base-join/u-base/owner": keyLoginTagsRefused, @@ -111,10 +91,6 @@ var reauthSkipReasons = map[string]string{ "tag-owner-chain/t-mid-join/t-mid/peer-obs": taggedKeyOwnedTagRefused, "tag-owner-chain/t-mid-join/t-mid/peer-key-obs": taggedKeyOwnedTagRefused, - "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, @@ -131,15 +107,11 @@ 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" - 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" - 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" - taggedKeyOwnedTagRefused = "TAGGED_KEY_OWNED_TAG_REFUSED: headscale refuses a tagged key's login that requests a tag the key's tags own; SaaS gives the device the requested tags" - 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" + taggedLogoutNotExpired = "TAGGED_LOGOUT_NOT_EXPIRED: headscale leaves a tagged node unexpired when it logs out; SaaS expires it" + 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" + taggedKeyOwnedTagRefused = "TAGGED_KEY_OWNED_TAG_REFUSED: headscale refuses a tagged key's login that requests a tag the key's tags own; SaaS gives the device the requested tags" + 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 1ac355b32..1aa1abc8b 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -388,34 +388,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 @@ -471,12 +548,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") @@ -712,8 +793,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. @@ -726,12 +807,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 }) @@ -1751,7 +1833,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()), @@ -2957,3 +3039,136 @@ func TestPAKWakeAcceptsRequestedTags(t *testing.T) { _, _, err = s.HandleNodeFromPreAuthKey(req, c.machineKey) require.ErrorIs(t, err, ErrRequestedTagsInvalidOrNotPermitted) } + +// 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 e40484856..38910df02 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2564,10 +2564,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 @@ -2580,14 +2577,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()). @@ -2611,15 +2600,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) @@ -2715,31 +2704,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 @@ -2777,6 +2762,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 @@ -2813,7 +2823,11 @@ func pakSkipsValidation(n types.NodeView, pak *types.PreAuthKey, nodeKey key.Nod func pakReregister(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) (bool, error) { err := pak.ValidAt(now) if err == nil { - return true, nil + if pak.CanReauthenticate(n) { + return true, nil + } + + err = types.PAKError("authkey cannot re-authenticate this node") } if pakSkipsValidation(n, pak, nodeKey, now) { return false, nil @@ -2872,7 +2886,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 } @@ -3136,6 +3150,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() diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index ad7d25601..756b906aa 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -1022,6 +1022,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)) + }) + } +}