From fc16cc6905e921566b84c3b4d5895e7c812c6d19 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 22 Jul 2026 06:32:08 +0000 Subject: [PATCH] state: apply a new pre-auth key's tags on re-registration Re-registering a tagged node with a different key discarded the new key's tags and left a stale auth-key reference; retag on key change and persist it. Fixes #3370 --- .github/workflows/test-integration.yaml | 2 + CHANGELOG.md | 1 + hscontrol/state/auth_tagged_expiry_test.go | 552 +++++++++++++++++++++ hscontrol/state/state.go | 64 ++- integration/tags_test.go | 207 ++++++++ 5 files changed, 813 insertions(+), 13 deletions(-) diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index 2c9913dc..f2f28c3b 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -352,6 +352,8 @@ jobs: - TestTagsAuthKeyWithTagCannotAddViaCLI - TestTagsAuthKeyWithTagCannotChangeViaCLI - TestTagsAuthKeyWithTagAdminOverrideReauthPreserves + - TestTagsReauthDifferentKeyRetagsNode + - TestTagsReauthDifferentKeyRemovesTag - TestTagsAuthKeyWithTagCLICannotModifyAdminTags - TestTagsAuthKeyWithoutTagCannotRequestTags - TestTagsAuthKeyWithoutTagRegisterNoTags diff --git a/CHANGELOG.md b/CHANGELOG.md index 180f6772..700ed9b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ keys remain all-access. ### Changes - Fix tagged node stuck expired after `tailscale logout`, unable to re-authenticate [#3394](https://github.com/juanfont/headscale/pull/3394) +- Re-registering a tagged node with a different pre-auth key now applies the new key's tags instead of silently keeping the old ones [#3394](https://github.com/juanfont/headscale/pull/3394) ## 0.29.2 (2026-07-01) diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 51e1a6dd..dde41502 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -553,6 +553,558 @@ func TestIssue3371_TaggedNodePastExpirySelfHealsOnReregister(t *testing.T) { require.Equal(t, nodeID, healed.ID(), "must be the same node") } +// TestIssue3371_ExpiredTaggedNodeSameSpentKeyNotRevalidated is the gating test +// for the isExpired-gate exclusion (state.go: `&& !existingNodeSameUser.IsTagged()`). +// A tagged node carrying a stale PAST expiry, re-registering with the SAME +// single-use key and the SAME node key (no rotation), must take the +// skip-validation fast path — otherwise the spent key is re-validated and +// rejected with "authkey already used", the exact lockout #3371 fixes. Without +// the tagged exclusion this fails; with it the node self-heals. The neighbours +// all rotate the node key or use a reusable key, so validation runs regardless +// and none probes this fast-path exclusion. +func TestIssue3371_ExpiredTaggedNodeSameSpentKeyNotRevalidated(t *testing.T) { + s := newRetagTestState(t) + + // Single-use tagged key. + pak, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:foo"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "spent-tagged"}, + } + node, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.True(t, node.IsTagged()) + + // Stale past (logout) expiry, and the single-use key is now spent. + past := time.Now().Add(-1 * time.Hour) + _, ok := s.nodeStore.UpdateNode(node.ID(), func(n *types.Node) { + n.Expiry = &past + }) + require.True(t, ok) + + // Re-register with the SAME key and the SAME node key (no rotation). The + // tagged exclusion from the isExpired gate must let this skip validation, so + // the spent single-use key is not rejected. + healed, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err, + "a tagged node with a stale past expiry must skip re-validation of its spent key") + require.False(t, healed.IsExpired(), "stale expiry cleared") + require.Nil(t, healed.AsStruct().Expiry, "tagged node key-expiry disabled") + require.Equal(t, node.ID(), healed.ID()) +} + +// TestTaggedPAKReauthRetagsExistingTaggedNode reproduces issue #3370: an +// already-tagged node re-authenticating with a *fresh, valid* tagged pre-auth +// key carrying *different* tags has that key validated and consumed, but its +// tags are silently discarded — the node keeps its old tags. +// +// Root cause: the in-place re-registration path in HandleNodeFromPreAuthKey +// only applies a key's tags when a tagged key converts a user-owned node +// (`pak.IsTagged() && !node.IsTagged()`). An already-tagged node fails +// `!node.IsTagged()`, so a differently-tagged key leaves the tags unchanged — +// while the single-use key is still marked used below (hsdb.UsePreAuthKey). +// +// Tagged-PAK tags are authorised by possession of the key (only syntactic +// `tag:` validation at creation; no tagOwners policy is required — the bug +// reproduces with an empty policy). Re-keying is Tailscale's documented method +// for changing an auth-key device's tags, so a fresh key's tags must be +// applied on re-registration. +// +// https://github.com/juanfont/headscale/issues/3370 +func TestTaggedPAKReauthRetagsExistingTaggedNode(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // Empty policy: tagged-PAK tags need no tagOwners entry (issue reproduces + // with a completely empty policy). + + // KEY1: single-use tags-only key carrying tag:tag1. + key1, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag1"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: key1.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "retag-node"}, + } + + // Initial registration: node comes up tagged tag:tag1. + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.True(t, first.IsTagged(), "precondition: node registered tagged") + require.Equal(t, []string{"tag:tag1"}, first.Tags().AsSlice()) + nodeID := first.ID() + firstIPv4 := first.IPv4() + firstIPv6 := first.IPv6() + + // KEY2: fresh single-use tags-only key carrying tag:tag2. + key2, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag2"}) + require.NoError(t, err) + + // `tailscale up --force-reauth --auth-key KEY2`: same machine, rotated node + // key, fresh valid key. This validates and consumes KEY2. + reReg := regReq + reReg.Auth = &tailcfg.RegisterResponseAuth{AuthKey: key2.Key} + reReg.NodeKey = key.NewNode().Public() + + second, _, err := s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, nodeID, second.ID(), "must update in place, not duplicate") + require.Equal(t, 1, s.ListNodes().Len(), "machine must map to exactly one node") + require.True(t, second.IsTagged(), "node must remain tagged") + + // KEY2 was validated and consumed regardless of the outcome. + consumed, err := s.GetPreAuthKeyByID(key2.ID) + require.NoError(t, err) + require.True(t, consumed.Used, "single-use key was consumed by the re-auth") + + // The consumed key's tags must be applied: re-keying retags the device + // (Tailscale's documented behaviour). This is the assertion that fails + // before the fix — the node keeps ["tag:tag1"]. + require.Equal(t, []string{"tag:tag2"}, second.Tags().AsSlice(), + "re-authenticating with a differently-tagged key must retag the node, "+ + "not silently discard the tags of the consumed key") + + // Identity continuity: the reporter stresses "same node, same IP". The + // re-auth updates in place, so the machine key and both Tailscale IPs are + // preserved while the node key rotates. A retag must not re-allocate IPs or + // duplicate the node. + require.Equal(t, machineKey.Public(), second.MachineKey(), + "machine key is the stable identity across re-auth") + require.Equal(t, firstIPv4, second.IPv4(), "IPv4 preserved across retag") + require.Equal(t, firstIPv6, second.IPv6(), "IPv6 preserved across retag") + require.NotEqual(t, first.NodeKey(), second.NodeKey(), + "--force-reauth rotates the node key") + + // A retagged node stays a tagged node: user-less and key-expiry disabled. + require.Nil(t, second.AsStruct().Expiry, "tagged node keeps nil key expiry") +} + +// TestTaggedPAKReauthSameKeyPreservesTags is the counterpart constraint to +// #3370: re-authenticating with the *same* tagged key must NOT clobber the +// node's current tags, even after an admin retagged it via +// `headscale nodes tag`. This is the unit-level guard for the integration +// test TestTagsAuthKeyWithTagAdminOverrideReauthPreserves (admin decisions are +// authoritative), and it is why the retag discriminator must key on the +// pre-auth key's *identity* (a different key) rather than on "validation ran" +// (a --force-reauth with the same key also runs validation). This test passes +// today and must keep passing after the #3370 fix. +func TestTaggedPAKReauthSameKeyPreservesTags(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // tag:admin must be permitted for the admin SetNodeTags call. + _, err = s.SetPolicy([]byte(`{"tagOwners":{"tag:admin":["admin@"]}}`)) + require.NoError(t, err) + + // Reusable tagged key (the shape used by the admin-override integration + // test) so the same key can be presented twice. + pak, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:orig"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "sticky-node"}, + } + + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, []string{"tag:orig"}, first.Tags().AsSlice()) + nodeID := first.ID() + + // Admin retags the node out-of-band. + tagged, _, err := s.SetNodeTags(nodeID, []string{"tag:admin"}) + require.NoError(t, err) + require.Equal(t, []string{"tag:admin"}, tagged.Tags().AsSlice()) + + // `--force-reauth` with the SAME key: rotate the node key so validation + // runs, exactly like the client does. + reReg := regReq + reReg.NodeKey = key.NewNode().Public() + + second, _, err := s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, nodeID, second.ID()) + require.Equal(t, []string{"tag:admin"}, second.Tags().AsSlice(), + "re-authenticating with the same key must preserve the admin-assigned tags") +} + +// TestTaggedPAKReauthSpentKeySameNodeKeyRejected pins the authorization boundary +// the retag must respect: presenting a *spent* single-use tagged key to retag an +// already-tagged node must be rejected, even when the client reuses its node key +// (the skip-validation fast path). Without forcing validation on a retag, a +// used/revoked/expired tagged credential could still apply its tags — replaying +// a dead key to escalate a machine onto a tag, and defeating key revocation. The +// existing isExpired comment already declares the boundary "must not depend on +// the client rotating its key"; retag must honour the same rule. +func TestTaggedPAKReauthSpentKeySameNodeKeyRejected(t *testing.T) { + s := newRetagTestState(t) + + // KEY1: single-use tag:tag1, used to register. + key1, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag1"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: key1.Key}, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "retag-node"}, + } + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, []string{"tag:tag1"}, first.Tags().AsSlice()) + + // KEY2: single-use tag:tag2, spent elsewhere so it is already Used. + key2, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag2"}) + require.NoError(t, err) + _, _, err = s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: key2.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "other-node"}, + }, key.NewMachine().Public()) + require.NoError(t, err) + + // Re-register the FIRST node with the now-spent KEY2, REUSING its node key + // (no rotation) so it targets the skip-validation fast path. A spent key must + // be rejected, and the node's tags must be unchanged. + reReg := regReq + reReg.Auth = &tailcfg.RegisterResponseAuth{AuthKey: key2.Key} + // reReg.NodeKey stays nodeKey (same) -> fast path. + + _, _, err = s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.Error(t, err, "a spent tagged key must not be able to retag on the fast path") + require.Contains(t, err.Error(), "authkey already used") + + after, ok := s.GetNodeByID(first.ID()) + require.True(t, ok) + require.Equal(t, []string{"tag:tag1"}, after.Tags().AsSlice(), + "a rejected spent key must not have applied its tags") +} + +// retagReauthCase drives the #3370 retag scenario with configurable key shapes +// so the sibling cases below stay a few lines each. It registers a node with +// key1, then re-registers the same machine with key2, and returns the +// re-registered node. rotateNodeKey mirrors --force-reauth (the client rotates +// its node key); when false the client reuses its node key. +func retagReauthCase( + t *testing.T, + s *State, + key1Tags, key2Tags []string, + key1User, key2User *types.UserID, + reusable, rotateNodeKey bool, +) (types.NodeView, types.NodeView) { + t.Helper() + + key1, err := s.CreatePreAuthKey(key1User, reusable, false, nil, key1Tags) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: key1.Key}, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "retag-node"}, + } + + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + + key2, err := s.CreatePreAuthKey(key2User, reusable, false, nil, key2Tags) + require.NoError(t, err) + + reReg := regReq + + reReg.Auth = &tailcfg.RegisterResponseAuth{AuthKey: key2.Key} + if rotateNodeKey { + reReg.NodeKey = key.NewNode().Public() + } + + second, _, err := s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, first.ID(), second.ID(), "must update in place, not duplicate") + + return first, second +} + +// TestTaggedPAKReauthReusableKeyRetags: a *reusable* differently-tagged key +// must retag too. The retag must key on key identity, not on the single-use +// `Used` flag, so it cannot be coupled to single-use semantics. +func TestTaggedPAKReauthReusableKeyRetags(t *testing.T) { + s := newRetagTestState(t) + _, second := retagReauthCase(t, s, + []string{"tag:tag1"}, []string{"tag:tag2"}, nil, nil, true /*reusable*/, true) + require.Equal(t, []string{"tag:tag2"}, second.Tags().AsSlice()) +} + +// TestTaggedPAKReauthDifferentKeySameNodeKey: retag must fire even when the +// client reuses its node key (no rotation). The trigger is key identity, +// decoupled from NodeKey rotation. +func TestTaggedPAKReauthDifferentKeySameNodeKey(t *testing.T) { + s := newRetagTestState(t) + _, second := retagReauthCase(t, s, + []string{"tag:tag1"}, []string{"tag:tag2"}, nil, nil, false, false /*same node key*/) + require.Equal(t, []string{"tag:tag2"}, second.Tags().AsSlice()) +} + +// TestTaggedPAKReauthSameSingleUseKeySameNodeKeyPreservesTags guards the #2830 +// container-restart permutation for a tags-only single-use key: presenting the +// SAME (already-used) single-use key with the SAME node key must take the +// skip-validation fast path, succeed, and keep the node's tags — not reject +// with "authkey already used" and not spuriously retag. This confirms the +// keyChanged/isRetag additions never fire for a same-key restart, the exact +// property #2830/#3312 depend on. (The existing #3312 test uses a user-owned +// one-shot key; this covers the tags-only single-use key.) +func TestTaggedPAKReauthSameSingleUseKeySameNodeKeyPreservesTags(t *testing.T) { + s := newRetagTestState(t) + + pak, err := s.CreatePreAuthKey(nil, false /*single-use*/, false, nil, []string{"tag:tag1"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "restart-node"}, + } + + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, []string{"tag:tag1"}, first.Tags().AsSlice()) + + // Container restart: same key, same node key, same machine. The single-use + // key is already consumed, so re-validation would reject it. + second, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err, + "same-key restart of a tagged node must skip validation, not reject a spent single-use key") + require.Equal(t, first.ID(), second.ID(), "must update in place") + require.Equal(t, []string{"tag:tag1"}, second.Tags().AsSlice(), + "same-key restart must preserve tags (no spurious retag)") + require.Equal(t, 1, s.ListNodes().Len(), "no duplicate node") +} + +// TestTaggedPAKReauthUserScopedKeyRetags: a user-scoped tagged key +// (User != nil, the `headscale preauthkeys create -u --tags` shape) +// exercises the pak.User != nil branch of findExistingNodeForPAK, a different +// lookup path from tags-only keys. It must still retag. +func TestTaggedPAKReauthUserScopedKeyRetags(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("owner") + uid := user.TypedID() + _, second := retagReauthCase(t, s, + []string{"tag:tag1"}, []string{"tag:tag2"}, uid, uid, false, true) + require.Equal(t, []string{"tag:tag2"}, second.Tags().AsSlice()) + require.True(t, second.IsTagged()) +} + +// TestTaggedPAKReauthReplacesNotMerges pins the Tailscale KB 1068 rule that +// re-keying *replaces* the tag set (it does not union). Re-keying a +// {tag1,tag2} node with a {tag1} key must drop tag2. +func TestTaggedPAKReauthReplacesNotMerges(t *testing.T) { + s := newRetagTestState(t) + _, second := retagReauthCase(t, s, + []string{"tag:tag1", "tag:tag2"}, []string{"tag:tag1"}, nil, nil, false, true) + require.Equal(t, []string{"tag:tag1"}, second.Tags().AsSlice(), + "re-keying replaces the tag set; tag:tag2 must be removed, not merged") +} + +// TestExpiredTaggedNodeReauthRetags: a tagged node that looks expired (a stale +// logout stamp — see #3371) re-authenticating with a fresh differently-tagged +// key must still retag. Combines the isExpired validation path with the retag. +func TestExpiredTaggedNodeReauthRetags(t *testing.T) { + s := newRetagTestState(t) + + key1, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag1"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: key1.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "retag-node"}, + } + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + + // Force a stale past expiry onto the tagged node. + past := time.Now().Add(-1 * time.Hour) + _, ok := s.nodeStore.UpdateNode(first.ID(), func(n *types.Node) { + n.Expiry = &past + }) + require.True(t, ok) + + key2, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag2"}) + require.NoError(t, err) + + reReg := regReq + reReg.Auth = &tailcfg.RegisterResponseAuth{AuthKey: key2.Key} + reReg.NodeKey = key.NewNode().Public() + + second, _, err := s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, []string{"tag:tag2"}, second.Tags().AsSlice(), + "an expired (stale logout stamp) tagged node must retag on fresh-key re-auth") + require.Nil(t, second.AsStruct().Expiry, + "retag must clear a stale expiry: the node is tagged and tagged nodes never expire") + require.False(t, second.IsExpired(), + "a retagged node must not remain expired") +} + +// TestTaggedPAKReauthRetagPreservesFutureAdminExpiry pins the composition +// decision between #3370 (retag) and #3371 (never wrongly expire a tagged +// node): a deliberate FUTURE expiry set by an admin via `headscale nodes expire` +// is a node property, not tied to the auth key, so re-keying an already-tagged +// node with a different key must retag it WITHOUT wiping that future expiry. +// Only a stale PAST expiry is cleared (see TestExpiredTaggedNodeReauthRetags). +// Symmetric with the same-key relogin path. +func TestTaggedPAKReauthRetagPreservesFutureAdminExpiry(t *testing.T) { + s := newRetagTestState(t) + + key1, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag1"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: key1.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "retag-node"}, + } + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + + // Admin sets a deliberate future expiry on the tagged node + // (`headscale nodes expire`), which SetNodeExpiry permits. + future := time.Now().Add(30 * 24 * time.Hour) + _, _, err = s.SetNodeExpiry(first.ID(), &future) + require.NoError(t, err) + + // Re-key with a different tagged key. + key2, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:tag2"}) + require.NoError(t, err) + + reReg := regReq + reReg.Auth = &tailcfg.RegisterResponseAuth{AuthKey: key2.Key} + reReg.NodeKey = key.NewNode().Public() + + second, _, err := s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, []string{"tag:tag2"}, second.Tags().AsSlice(), "node retags") + require.NotNil(t, second.AsStruct().Expiry, + "a deliberate future admin expiry must survive a different-key retag") + require.Equal(t, future.Unix(), second.AsStruct().Expiry.Unix(), + "the admin expiry value must be unchanged") +} + +// newRetagTestState builds a State with an empty policy (tagged-PAK tags need +// no tagOwners entry). +func newRetagTestState(t *testing.T) *State { + t.Helper() + + cfg := persistTestConfig(t.TempDir() + "/headscale.db") + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + return s +} + +// TestPreAuthKeyReauthPersistsAuthKeyID pins the root cause behind the +// ephemeral-flag revert bug found while auditing #3370: on re-registration +// with a *different* key, HandleNodeFromPreAuthKey updates node.AuthKeyID in +// the NodeStore (state.go, in the in-place closure) but AuthKeyID is excluded +// from nodeUpdateColumns (added for #2862 to avoid persisting a *deleted* key's +// stale reference on MapRequest). The exclusion is correct for MapRequest, but +// on the re-registration path the presented key is freshly loaded and valid, so +// the new auth_key_id must be persisted. Otherwise a control-plane restart +// reloads the OLD key, and any key-scoped property (notably Ephemeral) silently +// reverts — an ephemeral->non-ephemeral re-auth leaves a node that looks +// persistent in memory but is ephemeral again after restart and can be GC'd. +// +// Drives the real handler twice, then reopens the DB to prove the association +// survives the reload. +func TestPreAuthKeyReauthPersistsAuthKeyID(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + s, err := NewState(cfg) + require.NoError(t, err) + + user := s.CreateUserForTest("rekey-user") + + // KEY A: ephemeral, single-use. + keyA, err := s.CreatePreAuthKey(user.TypedID(), false, true /*ephemeral*/, nil, nil) + require.NoError(t, err) + + machineKey := key.NewMachine() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: keyA.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "rekey-node"}, + Expiry: time.Now().Add(24 * time.Hour), + } + + first, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + + nodeID := first.ID() + require.True(t, first.IsEphemeral(), "precondition: registered with an ephemeral key") + + // KEY B: non-ephemeral, single-use, same user. `--force-reauth` rotates the + // node key so this is a genuine re-registration that consumes KEY B. + keyB, err := s.CreatePreAuthKey(user.TypedID(), false, false /*non-ephemeral*/, nil, nil) + require.NoError(t, err) + + reReg := regReq + reReg.Auth = &tailcfg.RegisterResponseAuth{AuthKey: keyB.Key} + reReg.NodeKey = key.NewNode().Public() + + second, _, err := s.HandleNodeFromPreAuthKey(reReg, machineKey.Public()) + require.NoError(t, err) + require.Equal(t, nodeID, second.ID(), "must update in place") + + // In-memory, the node now tracks KEY B (non-ephemeral). + require.NotNil(t, second.AuthKeyID().Get()) + require.Equal(t, keyB.ID, second.AuthKeyID().Get(), + "NodeStore must reference the key just used to re-authenticate") + require.False(t, second.IsEphemeral(), + "re-auth with a non-ephemeral key must make the node non-ephemeral") + + // Restart the control plane: reload state from the database only. + require.NoError(t, s.Close()) + + s2, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + reloaded, ok := s2.GetNodeByID(nodeID) + require.True(t, ok, "node must reload from DB after restart") + + // This is the assertion that fails today: auth_key_id was never persisted on + // re-registration, so the reload resurrects KEY A and the node is ephemeral + // again — at risk of GC on the next disconnect. + require.NotNil(t, reloaded.AuthKeyID().Get()) + require.Equal(t, keyB.ID, reloaded.AuthKeyID().Get(), + "auth_key_id must persist across restart, not revert to the old key") + require.False(t, reloaded.IsEphemeral(), + "ephemerality must not silently revert after a control-plane restart") +} + // TestTaggedNodeCanHaveKeyExpiry matches Tailscale: a tagged node has key // expiry disabled by default, but it can still be set explicitly (e.g. via // `headscale nodes expire`). diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index e8784551..7aee9adc 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2443,7 +2443,17 @@ func (s *State) HandleNodeFromPreAuthKey( isOwnershipConversion := existsSameUser && existingNodeSameUser.Valid() && pak.IsTagged() && !existingNodeSameUser.IsTagged() - if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && !isOwnershipConversion { + // A tagged key that differs from the one the node last authed with retags + // the node (see the in-place update below). Applying a key's tags is an + // authorisation decision, so the key must be validated rather than ride the + // skip-validation fast-path; otherwise a spent, revoked or expired tagged + // key could still retag a node that reuses its node key. Like isExpired, + // this boundary must not depend on the client rotating its key. + isRetag := existsSameUser && existingNodeSameUser.Valid() && + pak.IsTagged() && existingNodeSameUser.IsTagged() && + (!existingNodeSameUser.AuthKeyID().Valid() || existingNodeSameUser.AuthKeyID().Get() != pak.ID) + + 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. // Critical for containers that run "tailscale up --authkey=KEY" on every @@ -2491,8 +2501,10 @@ func (s *State) HandleNodeFromPreAuthKey( var finalNode types.NodeView - // If this node exists for this user, update the node in place. - // Note: For tags-only keys (pak.User == nil), existsSameUser is always false. + // If this node exists for this user, update the node in place. For a + // tags-only key (pak.User == nil) this is true when the machine already has + // a tagged node (findExistingNodeForPAK matches it under UserID 0); for a + // user-owned key it is true when the same user already has the node. if existsSameUser && existingNodeSameUser.Valid() { log.Trace(). Caller(). @@ -2534,17 +2546,37 @@ func (s *State) HandleNodeFromPreAuthKey( node.RegisterMethod = util.RegisterMethodAuthKey - // Tags from PreAuthKey are only applied during initial registration. - // On re-registration the node keeps its existing tags and ownership, - // except when a tagged key converts a user-owned node: that adopts - // the key's tags and drops user ownership (tagged nodes are - // user-less and never expire). Only update AuthKey reference - // otherwise. - if pak.IsTagged() && !node.IsTagged() { + // Tags from a PreAuthKey are applied on initial registration and + // re-applied whenever a *different* key is presented on + // re-registration: re-keying is Tailscale's documented way to change + // an auth-key device's tags (KB 1068 - "generate a new auth key with + // the new set of tags ... doing so replaces the device's existing + // tags"). Presenting the SAME key again (container restart, + // #2830/#3312) preserves the node's current tags and any admin + // override. A tagged key presented for a user-owned node also converts + // it, dropping user ownership. + // + // node.AuthKeyID still holds the prior key's ID here (it is reassigned + // below), and SetNodeTags leaves AuthKeyID intact, so an admin retag + // cannot masquerade as a new key. + keyChanged := node.AuthKeyID == nil || *node.AuthKeyID != pak.ID + if pak.IsTagged() && (!node.IsTagged() || keyChanged) { + wasUserOwned := !node.IsTagged() + node.Tags = pak.Tags node.UserID = nil node.User = nil - node.Expiry = nil + + // Converting a user-owned node to tagged drops the user's key + // expiry (tagged nodes never expire). But retagging an + // already-tagged node must preserve a deliberate FUTURE expiry + // set via `headscale nodes expire` - that is a node property, not + // tied to the auth key - and only clear a stale PAST expiry. This + // keeps the retag path symmetric with the same-key relogin path + // (#3371) rather than silently overriding an admin decision. + if wasUserOwned || node.IsExpired() { + node.Expiry = nil + } } node.AuthKey = pak @@ -2586,8 +2618,14 @@ func (s *State) HandleNodeFromPreAuthKey( _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { // Explicitly select all node columns so GORM includes nil/zero-value fields - // (see nodeUpdateColumns comment). - err := tx.Select(nodeUpdateColumns).Updates(updatedNodeView.AsStruct()).Error + // (see nodeUpdateColumns comment). AuthKeyID is normally excluded to + // avoid persisting a deleted key's stale reference on MapRequest + // (#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") + + err := tx.Select(reregColumns).Updates(updatedNodeView.AsStruct()).Error if err != nil { return nil, fmt.Errorf("saving node: %w", err) } diff --git a/integration/tags_test.go b/integration/tags_test.go index 81b5154b..caff2e5d 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -579,6 +579,213 @@ func TestTagsAuthKeyWithTagAdminOverrideReauthPreserves(t *testing.T) { t.Logf("Test 2.5 PASS: Admin tags preserved through reauth (admin decisions are authoritative)") } +// TestTagsReauthDifferentKeyRetagsNode reproduces issue #3370 end-to-end with a +// real tailscale client: a node registered with a single-use tag:valid-owned +// key is re-authenticated via `tailscale up --force-reauth` with a *fresh* +// single-use tag:second key. Tailscale's documented behaviour (KB 1068) is that +// re-keying replaces the device's tags, verified by the reporter against SaaS on +// the same node/IP. Before the fix headscale consumes the new key but keeps the +// old tag; after the fix the node retags in place. +// +// Unlike Test 2.5 (same reusable key + admin override -> tags preserved), this +// presents a *different* key, so it exercises the opposite arm of the retag +// discriminator. It also asserts what a state-unit test cannot: the new tag +// propagates to the node's own Self view and netmap, and the node ID and IPs are +// unchanged. +// +// https://github.com/juanfont/headscale/issues/3370 +func TestTagsReauthDifferentKeyRetagsNode(t *testing.T) { + IntegrationSkip(t) + + spec := ScenarioSpec{ + NodesPerUser: 0, + Users: []string{tagTestUser}, + } + + scenario, err := NewScenario(spec) + + require.NoError(t, err) + defer scenario.ShutdownAssertNoPanics(t) + + err = scenario.CreateHeadscaleEnv( + []tsic.Option{}, + hsic.WithACLPolicy(tagsTestPolicy()), + hsic.WithTestName("tags-rekey-retag"), + ) + requireNoErrHeadscaleEnv(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + userMap, err := headscale.MapUsers() + require.NoError(t, err) + + userID := mustParseID(userMap[tagTestUser].Id) + + // KEY1: single-use tag:valid-owned. + key1, err := scenario.CreatePreAuthKeyWithTags(userID, false, false, []string{"tag:valid-owned"}) + require.NoError(t, err) + + client, err := scenario.CreateTailscaleNode( + "head", + tsic.WithNetwork(scenario.networks[scenario.testDefaultNetwork]), + ) + require.NoError(t, err) + + err = client.Login(headscale.GetEndpoint(), key1.Key) + require.NoError(t, err) + + var ( + initialNodeID uint64 + initialIPs []string + ) + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1) + + if len(nodes) == 1 { + initialNodeID = mustParseID(nodes[0].Id) + initialIPs = nodes[0].IpAddresses + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"}) + } + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "waiting for initial registration") + + t.Logf("Step 1: node %d registered with tag:valid-owned, IPs %v", initialNodeID, initialIPs) + + // KEY2: fresh single-use tag:second. Re-key via --force-reauth. + key2, err := scenario.CreatePreAuthKeyWithTags(userID, false, false, []string{"tag:second"}) + require.NoError(t, err) + + //nolint:errcheck // result is verified via EventuallyWithT below + client.Execute([]string{ + "tailscale", "up", + "--login-server=" + headscale.GetEndpoint(), + "--hostname=" + client.Hostname(), + "--authkey=" + key2.Key, + "--force-reauth", + }) + + // Server-side: node retagged in place, same node ID and IPs, no duplicate. + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1, "must not duplicate the node") + + if len(nodes) == 1 { + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:second"}) + assert.Equal(c, initialNodeID, mustParseID(nodes[0].Id), "node ID must be unchanged") + assert.ElementsMatch(c, initialIPs, nodes[0].IpAddresses, "IPs must be preserved") + } + }, integrationutil.ScaledTimeout(20*time.Second), integrationutil.SlowPoll, "server must reflect the retag") + + // Node self view: the new tag propagates to the client (issue #2978 surface). + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assertNodeSelfHasTagsWithCollect(c, client, []string{"tag:second"}) + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "node self view must reflect the retag") + + // Netmap: independent serialization surface. + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assertNetmapSelfHasTagsWithCollect(c, client, []string{"tag:second"}) + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "netmap self must reflect the retag") + + t.Logf("Test #3370 PASS: re-keying with a different tagged key retagged the node in place") +} + +// TestTagsReauthDifferentKeyRemovesTag is the sharpest proof of the KB 1068 +// "replaces, not merges" rule at the integration level: a node registered with +// a two-tag key is re-keyed with a single-tag key, and the second tag must be +// *removed*, not retained. Tag removal is the highest-risk propagation path +// (peers must stop seeing the removed tag), so it is worth a real-client test. +// +// https://github.com/juanfont/headscale/issues/3370 +func TestTagsReauthDifferentKeyRemovesTag(t *testing.T) { + IntegrationSkip(t) + + spec := ScenarioSpec{ + NodesPerUser: 0, + Users: []string{tagTestUser}, + } + + scenario, err := NewScenario(spec) + + require.NoError(t, err) + defer scenario.ShutdownAssertNoPanics(t) + + err = scenario.CreateHeadscaleEnv( + []tsic.Option{}, + hsic.WithACLPolicy(tagsTestPolicy()), + hsic.WithTestName("tags-rekey-remove"), + ) + requireNoErrHeadscaleEnv(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + userMap, err := headscale.MapUsers() + require.NoError(t, err) + + userID := mustParseID(userMap[tagTestUser].Id) + + // KEY1: single-use with BOTH tags. + key1, err := scenario.CreatePreAuthKeyWithTags(userID, false, false, []string{"tag:valid-owned", "tag:second"}) + require.NoError(t, err) + + client, err := scenario.CreateTailscaleNode( + "head", + tsic.WithNetwork(scenario.networks[scenario.testDefaultNetwork]), + ) + require.NoError(t, err) + + err = client.Login(headscale.GetEndpoint(), key1.Key) + require.NoError(t, err) + + var initialNodeID uint64 + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1) + + if len(nodes) == 1 { + initialNodeID = mustParseID(nodes[0].Id) + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:second", "tag:valid-owned"}) + } + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "waiting for initial registration") + + // KEY2: fresh single-use with ONLY tag:valid-owned. Re-key. + key2, err := scenario.CreatePreAuthKeyWithTags(userID, false, false, []string{"tag:valid-owned"}) + require.NoError(t, err) + + //nolint:errcheck // result is verified via EventuallyWithT below + client.Execute([]string{ + "tailscale", "up", + "--login-server=" + headscale.GetEndpoint(), + "--hostname=" + client.Hostname(), + "--authkey=" + key2.Key, + "--force-reauth", + }) + + // tag:second must be gone on every surface. + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1) + + if len(nodes) == 1 { + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"}) + assert.Equal(c, initialNodeID, mustParseID(nodes[0].Id)) + } + }, integrationutil.ScaledTimeout(20*time.Second), integrationutil.SlowPoll, "re-keying must replace (remove tag:second), not merge") + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assertNodeSelfHasTagsWithCollect(c, client, []string{"tag:valid-owned"}) + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "removed tag must clear from node self view") + + t.Logf("Test #3370 PASS: re-keying replaced the tag set (tag:second removed)") +} + // TestTagsAuthKeyWithTagCLICannotModifyAdminTags tests that the client CLI // cannot modify admin-assigned tags. //