From 6275e3a3563bb9302cade9cf78f7b13e90e8ed7e Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 22 Jul 2026 06:44:22 +0000 Subject: [PATCH] policy,state: authorize reauth tags against the authenticating user Re-authenticating a tagged node with --advertise-tags checked the tag-owned node, not the authenticating user, so every tag was rejected. Fixes #3374 --- .github/workflows/test-integration.yaml | 2 + CHANGELOG.md | 1 + hscontrol/policy/pm.go | 6 + hscontrol/policy/v2/policy.go | 31 ++ hscontrol/state/auth_tagged_expiry_test.go | 439 +++++++++++++++++++++ hscontrol/state/state.go | 77 +++- integration/tags_test.go | 248 ++++++++++++ 7 files changed, 800 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index f2f28c3b..ebd5f556 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -383,6 +383,8 @@ jobs: - TestTagsAuthKeyConvertToUserViaCLIRegister - TestTaggedNodeLogoutReloginSingleUseKeyOnline - TestTaggedNodeLogoutReloginReusableKeyOnline + - TestTagsOIDCReauthAddOwnedTag + - TestTagsReauthEmptyTagsReturnsToUserSurvives - TestTS2021WebSocketGET - TestTS2021WASMClientUnderNode - TestTailscaleRustAxum diff --git a/CHANGELOG.md b/CHANGELOG.md index 700ed9b8..25247030 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ keys remain all-access. - 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) +- Fix re-authenticating an already-tagged node with `--advertise-tags` being rejected when the authenticating user owns the tags [#3394](https://github.com/juanfont/headscale/pull/3394) ## 0.29.2 (2026-07-01) diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index ffde6361..99e88807 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -30,6 +30,12 @@ type PolicyManager interface { // NodeCanHaveTag reports whether the given node can have the given tag. NodeCanHaveTag(node types.NodeView, tag string) bool + // UserCanHaveTag reports whether the given user owns the given tag, i.e. + // is listed (directly or via a group) in the tag's tagOwners. This is the + // user half of NodeCanHaveTag, used to authorise re-auth tag changes + // against the authenticating user rather than the node's stale ownership. + UserCanHaveTag(user types.UserView, tag string) bool + // TagExists reports whether the given tag is defined in the policy. TagExists(tag string) bool diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index ae76ad26..d59f44b5 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -972,6 +972,37 @@ func (pm *PolicyManager) NodeCanHaveTag(node types.NodeView, tag string) bool { return false } +// UserCanHaveTag reports whether the given user is one of the tag's owners +// (directly or via a group). It is the user half of [PolicyManager.NodeCanHaveTag]: +// re-authentication authorises requested tags against the authenticating user, +// because a tag-owned node carries no user and its IP is not in any owner set, +// so only the user presenting the credential can prove ownership. +func (pm *PolicyManager) UserCanHaveTag(user types.UserView, tag string) bool { + if pm == nil || !user.Valid() { + return false + } + + pm.mu.RLock() + defer pm.mu.RUnlock() + + if pm.pol == nil { + return false + } + + owners, exists := pm.pol.TagOwners[Tag(tag)] + if !exists { + return false + } + + for _, owner := range owners { + if pm.userMatchesOwner(user, owner) { + return true + } + } + + return false +} + // TagOwnedByTags reports whether a credential holding ownerTags is authorised to // apply tag. It is true when tag is one of ownerTags, or when tag's tagOwners // chain (tag-to-tag ownership) transitively includes one of ownerTags. This is diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index dde41502..e4011107 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -408,6 +408,445 @@ func TestAuthPathRejectsTaggedAndUserCoexistence(t *testing.T) { require.ErrorIs(t, err, ErrAmbiguousNodeOwnership) } +// seededTaggedNode holds the identity of a node seeded into the genuinely +// tag-owned state, so re-auth tests can drive HandleNodeFromAuthPath and then +// re-read the node to assert its post-conditions. +type seededTaggedNode struct { + s *State + regData *types.RegistrationData + id types.NodeID + cfg *types.Config +} + +// seedTagOwnedNode registers a node under a seed user, then rewrites it into the +// genuinely tag-owned state that `tailscale up --advertise-tags` produces: +// tagged, with neither UserID nor User set. This is the shape the issue #3374 +// DB dump shows (`user_id: None`), and the shape that CreateRegisteredNodeForTest +// alone does not produce (it leaves a real user attached). +// +// createdByName controls the retained "created by" user: "" reproduces the pure +// tag-owned state (issue #3374); a non-empty name reproduces a node that kept a +// created-by User (UserID still nil) — used to prove authorisation keys off the +// authenticating user, not the created-by user. When set, that user is created +// in the DB so it resolves in policy. +func seedTagOwnedNode(t *testing.T, tags []string, createdByName string) seededTaggedNode { + t.Helper() + + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + seedUser := database.CreateUserForTest("seed") + node := database.CreateRegisteredNodeForTest(seedUser, "tagged-node") + + var createdBy *types.User + if createdByName != "" { + createdBy = database.CreateUserForTest(createdByName) + } + + regData := &types.RegistrationData{ + MachineKey: node.MachineKey, + NodeKey: node.NodeKey, + DiscoKey: node.DiscoKey, + Hostname: "tagged-node", + } + + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // 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. + seeded, ok := s.nodeStore.UpdateNode(node.ID, func(n *types.Node) { + n.Tags = tags + n.UserID = nil + n.User = createdBy + n.Expiry = nil + }) + require.True(t, ok) + require.True(t, seeded.IsTagged(), "precondition: node must be tagged") + + return seededTaggedNode{s: s, regData: regData, id: node.ID, cfg: cfg} +} + +// reopen closes the current State and reloads it from the same database, so a +// test can assert that a mutation was actually persisted (not just applied to +// the in-memory NodeStore). Returns the reloaded node view. +func (n seededTaggedNode) reopen(t *testing.T) types.NodeView { + t.Helper() + + require.NoError(t, n.s.Close()) + + s2, err := NewState(n.cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + v, ok := s2.GetNodeByID(n.id) + require.True(t, ok, "node must reload from DB after restart") + + return v +} + +// reauth replays an interactive re-auth (OIDC/CLI) of the seeded tag-owned +// machine as authUser, advertising requestTags. clientExpiry is the expiry the +// tailscale client requests (nil for none); it must be ignored while the node +// stays tagged. +func (n seededTaggedNode) reauth(t *testing.T, authUser *types.User, requestTags []string, clientExpiry *time.Time) (types.NodeView, error) { + t.Helper() + + // Sync the policy manager's user cache. Production keeps it current because + // State.CreateUser refreshes it on every user creation; the CreateUserForTest + // DB helper does not, so tests that create the authenticating user after + // NewState must sync before tag authorisation can resolve them as a tag owner. + require.NoError(t, n.s.UpdatePolicyManagerUsersForTest()) + + rd := *n.regData + rd.Hostinfo = &tailcfg.Hostinfo{ + Hostname: n.regData.Hostname, + RequestTags: requestTags, + } + rd.Expiry = clientExpiry + + authID := types.MustAuthID() + n.s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&rd)) + + node, _, err := n.s.HandleNodeFromAuthPath( + authID, + types.UserID(authUser.ID), + nil, + util.RegisterMethodOIDC, + ) + + return node, err +} + +// get re-reads the seeded node from the store. +func (n seededTaggedNode) get(t *testing.T) types.NodeView { + t.Helper() + + v, ok := n.s.GetNodeByID(n.id) + require.True(t, ok, "seeded node must still exist") + + return v +} + +// TestTaggedReauthAddTagAsOwner reproduces issue #3374 with the issue's own +// scenario: a tag-owned node holding tag:tag1 re-authenticates with +// --advertise-tags=tag:tag1,tag:tag2, and the authenticating user (ci-admin, +// via group:ci) owns BOTH tags. Today headscale rejects the whole set as +// "invalid or not permitted", leaving the re-keyed node logged out. +// +// This is stronger than re-advertising the already-held set: it exercises BOTH +// authorization checks on the reauth path, which both ask "can this NODE have +// the tag" instead of "can the authenticating USER apply it": +// +// 1. the pre-check validateRequestTags (state.go, before NodeStore.UpdateNode); +// 2. the apply-time re-check inside processReauthTags (state.go), whose +// rejection return is discarded (`_ =`) on the assumption that the +// pre-check already passed. +// +// A fix that only addresses (1) makes the call return success while +// processReauthTags silently drops the newly-requested tag — so this test +// asserts on the resulting tag SET, not merely on the absence of an error. +// +// https://github.com/juanfont/headscale/issues/3374 +func TestTaggedReauthAddTagAsOwner(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:tag1"}, "") + + admin := n.s.CreateUserForTest("ci-admin") + + // Mirror the issue's acl.json: group:ci = [ci-admin], and group:ci owns + // both tags. Ownership through a group is part of the reported scenario. + policy := fmt.Sprintf( + `{"groups":{"group:ci":["%s@"]},"tagOwners":{"tag:tag1":["group:ci"],"tag:tag2":["group:ci"]}}`, + admin.Name, + ) + _, err := n.s.SetPolicy([]byte(policy)) + require.NoError(t, err) + + // Client requests an expiry; a node that stays tagged must ignore it. + clientExpiry := time.Now().Add(180 * 24 * time.Hour) + finalNode, err := n.reauth(t, admin, []string{"tag:tag1", "tag:tag2"}, &clientExpiry) + require.NoError(t, err, + "tag owner re-authenticating a tagged node with owned tags must be permitted (issue #3374)") + require.True(t, finalNode.Valid()) + require.True(t, finalNode.IsTagged(), "node must remain tagged") + + // The whole point of the re-auth: the node must actually carry both tags. + // A fix that only silences the pre-check leaves tag:tag2 dropped here. + require.ElementsMatch(t, []string{"tag:tag1", "tag:tag2"}, finalNode.Tags().AsSlice(), + "node must hold the full re-advertised owned tag set, not a silently truncated one") + + // Tag ownership invariants matching the issue's DB dump (user_id None, + // expiry None): a tagged node stays user-less and never expires. + require.False(t, finalNode.User().Valid(), "tagged node must carry no user (user_id None)") + require.Nil(t, finalNode.AsStruct().Expiry, + "tagged node keeps nil key expiry; the client-requested expiry must be ignored") +} + +// TestTaggedReauthSameTagAsOwner covers the issue's minimal claim: "at minimum, +// a device should be able to re-authenticate with the same tag set it already +// holds, which is also rejected today." Node is tag-owned (no user), tagger owns +// the tag, re-auth re-advertises exactly the held tag. +func TestTaggedReauthSameTagAsOwner(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + tagger := n.s.CreateUserForTest("tagger") + _, err := n.s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:foo":["%s@"]}}`, tagger.Name)) + require.NoError(t, err) + + finalNode, err := n.reauth(t, tagger, []string{"tag:foo"}, nil) + require.NoError(t, err, + "tagged node re-advertising the same owned tag must be permitted (issue #3374)") + require.True(t, finalNode.IsTagged(), "node must remain tagged") + require.ElementsMatch(t, []string{"tag:foo"}, finalNode.Tags().AsSlice()) + require.False(t, finalNode.User().Valid(), "tagged node must carry no user") + require.Nil(t, finalNode.AsStruct().Expiry, "tagged node keeps nil key expiry") +} + +// TestTaggedReauthRemoveTagAsOwner is a P0 silent-data-loss guard: a tag-owned +// node holding [tag:tag1, tag:tag2] re-auths advertising only [tag:tag1]. The +// dropped tag must actually be removed. A fix that only relaxes the pre-check +// leaves processReauthTags rejecting every tag, so the node silently retains +// both — a success return with the wrong tag set. +func TestTaggedReauthRemoveTagAsOwner(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:tag1", "tag:tag2"}, "") + + owner := n.s.CreateUserForTest("owner") + policy := fmt.Sprintf( + `{"tagOwners":{"tag:tag1":["%s@"],"tag:tag2":["%s@"]}}`, + owner.Name, owner.Name, + ) + _, err := n.s.SetPolicy([]byte(policy)) + require.NoError(t, err) + + finalNode, err := n.reauth(t, owner, []string{"tag:tag1"}, nil) + require.NoError(t, err, "owner narrowing an owned tag set must be permitted") + require.True(t, finalNode.IsTagged(), "node still tagged (tag:tag1 remains)") + require.ElementsMatch(t, []string{"tag:tag1"}, finalNode.Tags().AsSlice(), + "tag:tag2 must be removed, not silently retained") + require.Nil(t, finalNode.AsStruct().Expiry, "tagged node keeps nil key expiry") + require.Equal(t, 1, n.s.ListNodes().Len(), "machine must map to exactly one node") +} + +// TestTaggedReauthReplaceTagSetAsOwner is a P0 silent-data-loss guard: a +// tag-owned node holding [tag:tag1] re-auths advertising an entirely different +// owned tag [tag:tag2]. The node must end up on tag:tag2 only, not silently keep +// tag:tag1. +func TestTaggedReauthReplaceTagSetAsOwner(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:tag1"}, "") + + owner := n.s.CreateUserForTest("owner") + policy := fmt.Sprintf( + `{"tagOwners":{"tag:tag1":["%s@"],"tag:tag2":["%s@"]}}`, + owner.Name, owner.Name, + ) + _, err := n.s.SetPolicy([]byte(policy)) + require.NoError(t, err) + + finalNode, err := n.reauth(t, owner, []string{"tag:tag2"}, nil) + require.NoError(t, err, "owner replacing one owned tag with another must be permitted") + require.True(t, finalNode.IsTagged()) + require.ElementsMatch(t, []string{"tag:tag2"}, finalNode.Tags().AsSlice(), + "node must switch to tag:tag2 only, not silently keep tag:tag1") +} + +// TestTaggedReauthAuthUserOwnsNotCreatedBy is the purest statement of the bug: +// the node retains a created-by User who does NOT own the tag, while the +// authenticating user DOES. Authorisation must key off the authenticating user, +// so this must be permitted. (Before the fix, NodeCanHaveTag consults the +// created-by user and rejects.) +func TestTaggedReauthAuthUserOwnsNotCreatedBy(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "creator") + + admin := n.s.CreateUserForTest("admin") + + // Only admin owns tag:foo; the created-by "creator" does not. + _, err := n.s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:foo":["%s@"]}}`, admin.Name)) + require.NoError(t, err) + + finalNode, err := n.reauth(t, admin, []string{"tag:foo"}, nil) + require.NoError(t, err, + "authorisation must key off the authenticating user, not the created-by user") + require.True(t, finalNode.IsTagged()) + require.ElementsMatch(t, []string{"tag:foo"}, finalNode.Tags().AsSlice()) +} + +// TestTaggedReauthUntagReturnsToAuthUser covers tagged->user: re-auth with empty +// RequestTags untags the node and returns ownership to the authenticating user, +// with the client-requested expiry now applied (tagged nodes have no expiry; +// user-owned nodes do). The untagging user need not own the tag. +func TestTaggedReauthUntagReturnsToAuthUser(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + alice := n.s.CreateUserForTest("alice") + // alice does not own tag:foo; untagging (empty RequestTags) is always allowed. + _, err := n.s.SetPolicy([]byte(`{"tagOwners":{"tag:foo":["someone-else@"]}}`)) + require.NoError(t, err) + + clientExpiry := time.Now().Add(90 * 24 * time.Hour) + finalNode, err := n.reauth(t, alice, []string{}, &clientExpiry) + require.NoError(t, err, "untagging via empty RequestTags must always be permitted") + require.False(t, finalNode.IsTagged(), "node must become user-owned") + require.Empty(t, finalNode.Tags().AsSlice()) + require.True(t, finalNode.User().Valid(), "ownership returns to a user") + require.Equal(t, alice.ID, finalNode.User().ID(), + "ownership returns to the authenticating user") + require.NotNil(t, finalNode.AsStruct().Expiry, + "a now-user-owned node takes the client-requested expiry") +} + +// 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 +// 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. +func TestTaggedReauthUntagClearsEphemeralAuthKey(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + alice := n.s.CreateUserForTest("alice") + _, err := n.s.SetPolicy([]byte(`{"tagOwners":{"tag:foo":["someone-else@"]}}`)) + require.NoError(t, err) + + // 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"}) + 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.AuthKey = pak + nd.AuthKeyID = &pak.ID + }) + require.True(t, ok) + require.True(t, seeded.IsEphemeral(), "precondition: node is ephemeral (tagged ephemeral key)") + + // Untag via interactive re-auth (empty RequestTags). + finalNode, err := n.reauth(t, alice, []string{}, nil) + require.NoError(t, err) + require.False(t, finalNode.IsTagged(), "node must become user-owned") + require.False(t, finalNode.IsEphemeral(), + "untagged node must not keep the ephemeral auth-key reference") + require.False(t, finalNode.AuthKeyID().Valid(), + "auth key reference must be cleared when a node is untagged") + + // Confirm the cleared reference is PERSISTED, not just applied in memory: + // reload the State from the database and assert the node is still + // non-ephemeral. This is what fails if AuthKeyID is not written on the + // untag path — the node reloads as ephemeral and gets GC-deleted. + reloaded := n.reopen(t) + require.False(t, reloaded.IsEphemeral(), + "untagged node must remain non-ephemeral after a control-plane restart") + require.False(t, reloaded.AuthKeyID().Valid(), + "cleared auth key reference must persist across restart") +} + +// TestTaggedReauthRejectsUnownedTag pins the security boundary: the fix +// authorises the authenticating user, it must not skip authorisation. A tag the +// authenticating user does not own is still rejected, and the node is left +// byte-for-byte unchanged — same tags, same node key (NOT rotated), no user, +// nil expiry — matching the issue's post-rejection DB/nodes-list state. +func TestTaggedReauthRejectsUnownedTag(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + tagger := n.s.CreateUserForTest("tagger") + other := n.s.CreateUserForTest("other") + + // tagger owns tag:foo; tag:bar is owned by a different user. + policy := fmt.Sprintf( + `{"tagOwners":{"tag:foo":["%s@"],"tag:bar":["%s@"]}}`, + tagger.Name, other.Name, + ) + _, err := n.s.SetPolicy([]byte(policy)) + require.NoError(t, err) + + before := n.get(t) + beforeKey := before.NodeKey() + + _, err = n.reauth(t, tagger, []string{"tag:foo", "tag:bar"}, nil) + require.Error(t, err, + "a tag the authenticating user does not own must still be rejected") + require.ErrorIs(t, err, ErrRequestedTagsInvalidOrNotPermitted) + + // The rejected re-auth must not have mutated the node. Validation runs before + // NodeStore.UpdateNode, so tags, ownership, expiry and the node key are all + // preserved (issue's nodes-list-after-failed-reauth still shows the old key). + after := n.get(t) + require.ElementsMatch(t, []string{"tag:foo"}, after.Tags().AsSlice(), "tags unchanged") + require.True(t, after.IsTagged()) + require.False(t, after.User().Valid(), "still no user (user_id None)") + require.Nil(t, after.AsStruct().Expiry, "expiry unchanged") + require.Equal(t, beforeKey, after.NodeKey(), + "node key must not be rotated on a rejected re-auth") + require.Equal(t, 1, n.s.ListNodes().Len()) +} + +// TestTaggedReauthUndefinedTagRejected covers the distinct NodeCanHaveTag branch +// where a requested tag is not defined in the policy at all (vs. defined but +// owned by another user). The owned tag alone would pass; the undefined tag must +// force rejection of the whole set. +func TestTaggedReauthUndefinedTagRejected(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + owner := n.s.CreateUserForTest("owner") + _, err := n.s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:foo":["%s@"]}}`, owner.Name)) + require.NoError(t, err) + + _, err = n.reauth(t, owner, []string{"tag:foo", "tag:undefined"}, nil) + require.Error(t, err, "a tag not defined in policy must be rejected") + require.ErrorIs(t, err, ErrRequestedTagsInvalidOrNotPermitted) + + after := n.get(t) + require.ElementsMatch(t, []string{"tag:foo"}, after.Tags().AsSlice(), "node unchanged on rejection") +} + +// TestTaggedReauthDuplicateTagsDeduped guards the slices.Compact in +// processReauthTags: an owner re-advertising the same tag twice must succeed and +// the node must hold a single copy. +func TestTaggedReauthDuplicateTagsDeduped(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + owner := n.s.CreateUserForTest("owner") + _, err := n.s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:foo":["%s@"]}}`, owner.Name)) + require.NoError(t, err) + + finalNode, err := n.reauth(t, owner, []string{"tag:foo", "tag:foo"}, nil) + require.NoError(t, err) + require.ElementsMatch(t, []string{"tag:foo"}, finalNode.Tags().AsSlice(), + "duplicate requested tags must be deduped to a single tag") +} + +// TestTaggedReauthPreservesOnlineAndLastSeen pins the reauth side-effect +// invariants documented at applyAuthNodeUpdate: online status is owned by the +// poll lifecycle and must not be reset here, and LastSeen is refreshed. +func TestTaggedReauthPreservesOnlineAndLastSeen(t *testing.T) { + n := seedTagOwnedNode(t, []string{"tag:foo"}, "") + + owner := n.s.CreateUserForTest("owner") + _, err := n.s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:foo":["%s@"]}}`, owner.Name)) + require.NoError(t, err) + + // Mark the node online before reauth. + _, ok := n.s.nodeStore.UpdateNode(n.id, func(nd *types.Node) { + nd.IsOnline = new(true) + }) + require.True(t, ok) + + finalNode, err := n.reauth(t, owner, []string{"tag:foo"}, nil) + require.NoError(t, err) + require.NotNil(t, finalNode.IsOnline().Get()) + require.True(t, finalNode.IsOnline().Get(), + "re-auth must not reset online status (owned by the poll lifecycle)") + require.NotNil(t, finalNode.LastSeen().Get(), "LastSeen must be set on reauth") +} + // TestIssue3371_TaggedNodeInteractiveReloginAfterLogout reproduces the // interactive/OIDC arm of https://github.com/juanfont/headscale/issues/3371 // ("With no key (interactive): the register URL is printed and the login never diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 7aee9adc..88a38887 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1707,7 +1707,17 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView // Validate tags BEFORE calling [NodeStore.UpdateNode] to ensure we don't modify // [NodeStore] if validation fails. This maintains consistency between [NodeStore] // and database. - rejectedTags := s.validateRequestTags(params.ExistingNode, requestTags) + // + // A tag-owned node carries no user and its IP is not in any tag owner's set, + // so checking the node alone rejects every tag on re-auth (#3374). Authorise + // against the authenticating user too: they are the one presenting the + // credential and may own the requested tags. + var authUser types.UserView + if params.User != nil { + authUser = params.User.View() + } + + rejectedTags := s.validateRequestTagsForReauth(params.ExistingNode, authUser, requestTags) if len(rejectedTags) > 0 { return types.NodeView{}, fmt.Errorf( "%w %v are invalid or not permitted", @@ -1829,8 +1839,21 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView // Persist to database. // Explicitly select all node columns so GORM includes nil/zero-value fields // (see nodeUpdateColumns comment). + // + // AuthKeyID is excluded from nodeUpdateColumns (#2862: never persist a + // possibly-deleted key's stale reference on the shared update path). But + // when a re-auth untags a node it clears AuthKeyID to nil (above), and that + // must persist or the node reloads as tagged/ephemeral after a restart and + // is garbage-collected. Writing NULL can never cause an FK error, so include + // the column only in that clearing case; the other transitions keep the + // #2862-safe column set untouched. + updateColumns := nodeUpdateColumns + if !updatedNodeView.AuthKeyID().Valid() { + updateColumns = append(slices.Clone(nodeUpdateColumns), "AuthKeyID") + } + _, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { - err := tx.Select(nodeUpdateColumns).Updates(updatedNodeView.AsStruct()).Error + err := tx.Select(updateColumns).Updates(updatedNodeView.AsStruct()).Error if err != nil { return nil, fmt.Errorf("saving node: %w", err) } @@ -2034,6 +2057,34 @@ func (s *State) validateRequestTags(node types.NodeView, requestTags []string) [ return rejectedTags } +// validateRequestTagsForReauth authorises re-auth request tags against the +// existing node OR the authenticating user. A tag-owned node (#3374) has no +// user and its IP is in no owner set, so NodeCanHaveTag alone rejects every +// tag; the authenticating user who owns the tags must also be consulted. +// Tags neither the node nor the user owns are still rejected, so this +// authorises the user, it does not skip authorisation. +func (s *State) validateRequestTagsForReauth(node types.NodeView, authUser types.UserView, requestTags []string) []string { + if len(requestTags) == 0 { + return nil + } + + var rejectedTags []string + + for _, tag := range requestTags { + if s.polMan.NodeCanHaveTag(node, tag) { + continue + } + + if authUser.Valid() && s.polMan.UserCanHaveTag(authUser, tag) { + continue + } + + rejectedTags = append(rejectedTags, tag) + } + + return rejectedTags +} + // processReauthTags handles tag changes during node re-authentication. // It processes RequestTags from the client and updates node tags accordingly. // Returns rejected tags (if any) for post-validation error handling. @@ -2068,16 +2119,34 @@ func (s *State) processReauthTags( node.Tags = []string{} node.UserID = &user.ID node.User = user + + // The node is no longer tagged, so it must not keep a reference to + // the tagged auth key. Leaving AuthKey set means a node created by a + // tagged+ephemeral key stays IsEphemeral() after converting to + // user-owned and is garbage-collected on its next disconnect, + // silently deleting the user's just-claimed device. Clearing the + // reference is persisted via the AuthKeyID column added to this + // path's write below. + node.AuthKey = nil + node.AuthKeyID = nil } return nil } - // Non-empty RequestTags: validate and apply + // Non-empty RequestTags: validate and apply. Authorise each tag against the + // node OR the authenticating user, matching the pre-check in + // validateRequestTagsForReauth. Without the user half, a tag-owned node + // (#3374) has every tag rejected here even after the pre-check passed, so + // this returns the tags as rejected and the re-advertised tag is silently + // dropped despite a success response. + authUser := user.View() + var approvedTags, rejectedTags []string for _, tag := range requestTags { - if s.polMan.NodeCanHaveTag(node.View(), tag) { + if s.polMan.NodeCanHaveTag(node.View(), tag) || + (authUser.Valid() && s.polMan.UserCanHaveTag(authUser, tag)) { approvedTags = append(approvedTags, tag) } else { rejectedTags = append(rejectedTags, tag) diff --git a/integration/tags_test.go b/integration/tags_test.go index caff2e5d..4967db44 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -11,6 +11,7 @@ import ( "github.com/juanfont/headscale/integration/hsic" "github.com/juanfont/headscale/integration/integrationutil" "github.com/juanfont/headscale/integration/tsic" + "github.com/oauth2-proxy/mockoidc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" @@ -3609,3 +3610,250 @@ func TestTaggedNodeLogoutReloginReusableKeyOnline(t *testing.T) { t.Logf("Test #3371 PASS: tagged node logged out and re-authenticated online with a reusable key") } + +// TestTagsOIDCReauthAddOwnedTag reproduces issue #3374 through the interactive +// OIDC path with a real client. A node is registered tag-owned (no user) via +// --advertise-tags, then re-authenticates via OIDC advertising an ADDITIONAL +// owned tag. Because a tag-owned node has no user and its IP is in no owner +// set, the pre-fix authorization (which asked only "can this NODE have the +// tag") rejected the whole set, leaving the node logged out. The fix also +// authorises against the authenticating user, so the added owned tag is +// accepted. +// +// This is the OIDC/interactive twin of the #3370 PAK-path retag tests, and it +// hard-asserts the resulting tag SET (the pre-existing web-auth add-tag test +// only logs), so it catches the silent-drop where the pre-check passes but the +// apply-time re-check in processReauthTags still rejects. +// +// https://github.com/juanfont/headscale/issues/3374 +func TestTagsOIDCReauthAddOwnedTag(t *testing.T) { + IntegrationSkip(t) + + oidcUser := "oidcuser" + + // Each OIDC login consumes one mock user, so the same identity must be + // listed twice: once for the initial login and once for the reauth. + spec := ScenarioSpec{ + NodesPerUser: 0, + OIDCUsers: []mockoidc.MockUser{ + oidcMockUser(oidcUser, true), + oidcMockUser(oidcUser, true), + }, + } + + scenario, err := NewScenario(spec) + + require.NoError(t, err) + defer scenario.ShutdownAssertNoPanics(t) + + oidcMap := map[string]string{ + "HEADSCALE_OIDC_ISSUER": scenario.mockOIDC.Issuer(), + "HEADSCALE_OIDC_CLIENT_ID": scenario.mockOIDC.ClientID(), + "CREDENTIALS_DIRECTORY_TEST": "/tmp", + "HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret", + } + + // The OIDC user owns both tags. Ownership is what authorises the reauth tag + // change once the node is tag-owned. Reference the user by email; it already + // contains an "@", so no trailing "@" is added (that suffix is only for + // non-email usernames). + owner := new(policyv2.Username(oidcUser + "@headscale.net")) + policy := &policyv2.Policy{ + TagOwners: policyv2.TagOwners{ + "tag:valid-owned": policyv2.Owners{owner}, + "tag:second": policyv2.Owners{owner}, + }, + ACLs: []policyv2.ACL{ + { + Action: "accept", + Sources: []policyv2.Alias{policyv2.Wildcard}, + Destinations: []policyv2.AliasWithPorts{{Alias: policyv2.Wildcard, Ports: []tailcfg.PortRange{tailcfg.PortRangeAny}}}, + }, + }, + } + + err = scenario.CreateHeadscaleEnvWithLoginURL( + []tsic.Option{ + tsic.WithExtraLoginArgs([]string{"--advertise-tags=tag:valid-owned"}), + }, + hsic.WithTestName("tags-oidc-addtag"), + hsic.WithConfigEnv(oidcMap), + hsic.WithFileInContainer("/tmp/hs_client_oidc_secret", []byte(scenario.mockOIDC.ClientSecret())), + hsic.WithACLPolicy(policy), + ) + requireNoErrHeadscaleEnv(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + client, err := scenario.CreateTailscaleNode( + "unstable", + tsic.WithNetwork(scenario.networks[scenario.testDefaultNetwork]), + tsic.WithExtraLoginArgs([]string{"--advertise-tags=tag:valid-owned"}), + ) + require.NoError(t, err) + + // Initial OIDC login advertising tag:valid-owned. + u, err := client.LoginWithURL(headscale.GetEndpoint()) + require.NoError(t, err) + + _, err = doLoginURL(client.Hostname(), u) + 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:valid-owned"}) + } + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "waiting for initial tag-owned registration") + + // Re-authenticate advertising an ADDITIONAL owned tag via --force-reauth, + // which drives the register/auth path (HandleNodeFromAuthPath), not a poll. + // Parse the login URL from this command's own output; issuing a separate + // `tailscale up` while a reauth is pending errors server-side. + command := []string{ + "tailscale", "up", + "--login-server=" + headscale.GetEndpoint(), + "--hostname=" + client.Hostname(), + "--advertise-tags=tag:valid-owned,tag:second", + "--force-reauth", + } + + stdout, stderr, _ := client.Execute(command) + t.Logf("reauth command output: stdout=%s stderr=%s", stdout, stderr) + + loginURL, err := util.ParseLoginURLFromCLILogin(stdout + stderr) + require.NoError(t, err, "failed to parse login URL from reauth command") + + _, err = doLoginURL(client.Hostname(), loginURL) + require.NoError(t, err) + + // Both tags must be present — not rejected, not silently dropped. + 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 { + assert.Equal(c, initialNodeID, mustParseID(nodes[0].Id), "node ID must be unchanged") + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned", "tag:second"}) + } + }, integrationutil.ScaledTimeout(30*time.Second), integrationutil.SlowPoll, "#3374: added owned tag must be accepted on OIDC reauth") + + t.Logf("Test #3374 PASS: OIDC reauth added an owned tag to a tag-owned node") +} + +// TestTagsReauthEmptyTagsReturnsToUserSurvives covers the #3374 untag path with +// a real client: a tag-owned node created by a tagged+ephemeral key +// re-authenticates via user login with an EMPTY tag set. It must return to the +// user, and — crucially — survive: clearing the tags must also clear the +// node's reference to the ephemeral auth key, or the node stays IsEphemeral() +// and is garbage-collected on its next disconnect, silently deleting the +// user's just-claimed device. +// +// https://github.com/juanfont/headscale/issues/3374 +func TestTagsReauthEmptyTagsReturnsToUserSurvives(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.CreateHeadscaleEnvWithLoginURL( + []tsic.Option{}, + hsic.WithACLPolicy(tagsTestPolicy()), + hsic.WithTestName("tags-untag-survive"), + ) + requireNoErrHeadscaleEnv(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + userMap, err := headscale.MapUsers() + require.NoError(t, err) + + userID := mustParseID(userMap[tagTestUser].Id) + + // A tagged + EPHEMERAL key: the node is tag-owned and ephemeral. + key, err := scenario.CreatePreAuthKeyWithTags(userID, false, true, []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(), key.Key) + require.NoError(t, err) + + err = client.WaitForRunning(integrationutil.PeerSyncTimeout()) + 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:valid-owned"}) + } + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "waiting for initial tag-owned ephemeral registration") + + // Re-authenticate with an EMPTY tag set via --force-reauth. An + // already-authenticated node only emits a fresh login URL when forced, so + // parse the URL from this command's own output rather than issuing a + // second `tailscale up` (which would error with "no URL found"). + command := []string{ + "tailscale", "up", + "--login-server=" + headscale.GetEndpoint(), + "--hostname=" + client.Hostname(), + "--advertise-tags=", + "--force-reauth", + } + + stdout, stderr, _ := client.Execute(command) + t.Logf("reauth command output: stdout=%s stderr=%s", stdout, stderr) + + loginURL, err := util.ParseLoginURLFromCLILogin(stdout + stderr) + require.NoError(t, err, "failed to parse login URL from reauth command") + + body, err := doLoginURL(client.Hostname(), loginURL) + require.NoError(t, err) + + // CLI user-login registration untags the node and returns it to the user. + err = scenario.runHeadscaleRegister(tagTestUser, body) + require.NoError(t, err) + + // Node returns to the user, keeps its ID, and does NOT vanish. + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1, "#3374: untagged node must survive, not be GC'd as ephemeral") + + if len(nodes) == 1 { + assert.Equal(c, initialNodeID, mustParseID(nodes[0].Id), "node ID must be unchanged") + assert.Empty(c, nodes[0].Tags, "#3374: node must have no tags after untag") + // A user-owned node reports its real user; a tagged node would + // report the special "tagged-devices" user instead. + assert.Equal(c, tagTestUser, nodes[0].User.Name, "#3374: untagged node must return to the authenticating user") + } + }, integrationutil.ScaledTimeout(30*time.Second), integrationutil.SlowPoll, "#3374: empty-tags reauth returns node to user and it survives") + + t.Logf("Test #3374 PASS: empty-tags reauth returned the ephemeral tag-owned node to its user and it survived") +}