From 0451dd47181bb802b54f26b2e97dca05a4bd3ab7 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 14 Jan 2026 12:22:02 +0000 Subject: [PATCH] state: allow untagging nodes via reauth with empty RequestTags When a node re-authenticates via OIDC/web auth with empty RequestTags (from `tailscale up --advertise-tags= --force-reauth`), remove all tags and return ownership to the authenticating user. This allows nodes to transition from any tagged state (including nodes originally registered with a tagged pre-auth key) back to user-owned. Fixes #2979 --- CHANGELOG.md | 5 +- hscontrol/auth_test.go | 184 +++++++++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 117 ++++++++++++++++++++++++- 3 files changed, 302 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b22fce4e..6cb76f0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ tags rather than users, making them suitable for servers and infrastructure. App ownership. See the [Tailscale tags documentation](https://tailscale.com/kb/1068/tags) for details on how tags work. User-owned nodes can now request tags during registration using `--advertise-tags`. Tags are validated against the `tagOwners` policy -and applied at registration time. Tags can be managed via the CLI or API after registration. +and applied at registration time. Tags can be managed via the CLI or API after registration. Tagged nodes can return to user-owned +by re-authenticating with `tailscale up --advertise-tags= --force-reauth`. ### Smarter map updates @@ -41,7 +42,7 @@ sequentially through each stable release, selecting the latest patch version ava - **API**: The Node message in the gRPC/REST API has been simplified - the `ForcedTags`, `InvalidTags`, and `ValidTags` fields have been removed and replaced with a single `Tags` field that contains the node's applied tags [#2993](https://github.com/juanfont/headscale/pull/2993) - API clients should use the `Tags` field instead of `ValidTags` - The `headscale nodes list` CLI command now always shows a Tags column and the `--tags` flag has been removed -- **Tags**: The gRPC `SetTags` endpoint now allows converting user-owned nodes to tagged nodes by setting tags. Once a node is tagged, it cannot be converted back to a user-owned node. [#2885](https://github.com/juanfont/headscale/pull/2885) +- **Tags**: The gRPC `SetTags` endpoint now allows converting user-owned nodes to tagged nodes by setting tags. [#2885](https://github.com/juanfont/headscale/pull/2885) - **Tags**: Tags are now resolved from the node's stored Tags field only [#2931](https://github.com/juanfont/headscale/pull/2931) - `--advertise-tags` is processed during registration, not on every policy evaluation - PreAuthKey tagged devices ignore `--advertise-tags` from clients diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 46225f45..1677642f 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -3541,3 +3541,187 @@ func TestWebAuthRejectsUnauthorizedRequestTags(t *testing.T) { _, found := app.state.GetNodeByNodeKey(nodeKey.Public()) require.False(t, found, "Node should not be created when tags are unauthorized") } + +// TestWebAuthReauthWithEmptyTagsRemovesAllTags tests that when an existing tagged node +// reauths with empty RequestTags, all tags are removed and ownership returns to user. +// This is the fix for issue #2979. +func TestWebAuthReauthWithEmptyTagsRemovesAllTags(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + + // Create a user + user := app.state.CreateUserForTest("reauth-untag-user") + + // Update policy manager to recognize the new user + // This is necessary because CreateUserForTest doesn't update the policy manager + err := app.state.UpdatePolicyManagerUsersForTest() + require.NoError(t, err, "Failed to update policy manager users") + + // Set up policy that allows the user to own these tags + policy := `{ + "tagOwners": { + "tag:valid-owned": ["reauth-untag-user@"], + "tag:second": ["reauth-untag-user@"] + }, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }` + _, err = app.state.SetPolicy([]byte(policy)) + require.NoError(t, err, "Failed to set policy") + + machineKey := key.NewMachine() + nodeKey1 := key.NewNode() + + // Step 1: Initial registration with tags + registrationID1 := types.MustRegistrationID() + regEntry1 := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), + NodeKey: nodeKey1.Public(), + Hostname: "reauth-untag-node", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "reauth-untag-node", + RequestTags: []string{"tag:valid-owned", "tag:second"}, + }, + }) + app.state.SetRegistrationCacheEntry(registrationID1, regEntry1) + + // Complete initial registration with tags + node, _, err := app.state.HandleNodeFromAuthPath( + registrationID1, + types.UserID(user.ID), + nil, + "webauth", + ) + require.NoError(t, err, "Initial registration should succeed") + require.True(t, node.IsTagged(), "Node should be tagged after initial registration") + require.ElementsMatch(t, []string{"tag:valid-owned", "tag:second"}, node.Tags().AsSlice()) + t.Logf("Initial registration complete - Node ID: %d, Tags: %v, IsTagged: %t", + node.ID().Uint64(), node.Tags().AsSlice(), node.IsTagged()) + + // Step 2: Reauth with EMPTY tags to untag + nodeKey2 := key.NewNode() // New node key for reauth + registrationID2 := types.MustRegistrationID() + regEntry2 := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), // Same machine key + NodeKey: nodeKey2.Public(), // Different node key (rotation) + Hostname: "reauth-untag-node", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "reauth-untag-node", + RequestTags: []string{}, // EMPTY - should untag + }, + }) + app.state.SetRegistrationCacheEntry(registrationID2, regEntry2) + + // Complete reauth with empty tags + nodeAfterReauth, _, err := app.state.HandleNodeFromAuthPath( + registrationID2, + types.UserID(user.ID), + nil, + "webauth", + ) + require.NoError(t, err, "Reauth should succeed") + + // Verify tags were removed + require.False(t, nodeAfterReauth.IsTagged(), "Node should NOT be tagged after reauth with empty tags") + require.Empty(t, nodeAfterReauth.Tags().AsSlice(), "Node should have no tags") + + // Verify ownership returned to user + require.True(t, nodeAfterReauth.UserID().Valid(), "Node should have a user ID") + require.Equal(t, user.ID, nodeAfterReauth.UserID().Get(), "Node should be owned by the user again") + + // Verify it's the same node (not a new one) + require.Equal(t, node.ID(), nodeAfterReauth.ID(), "Should be the same node after reauth") + + t.Logf("Reauth complete - Node ID: %d, Tags: %v, IsTagged: %t, UserID: %d", + nodeAfterReauth.ID().Uint64(), nodeAfterReauth.Tags().AsSlice(), + nodeAfterReauth.IsTagged(), nodeAfterReauth.UserID().Get()) +} + +// TestAuthKeyTaggedToUserOwnedViaReauth tests that a node originally registered +// with a tagged pre-auth key can transition to user-owned by re-authenticating +// via web auth with empty RequestTags. This ensures authkey-tagged nodes are +// not permanently locked to being tagged. +func TestAuthKeyTaggedToUserOwnedViaReauth(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + + // Create a user + user := app.state.CreateUserForTest("authkey-to-user") + + // Create a tagged pre-auth key + authKeyTags := []string{"tag:server", "tag:prod"} + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, false, nil, authKeyTags) + require.NoError(t, err, "Failed to create tagged pre-auth key") + + machineKey := key.NewMachine() + nodeKey1 := key.NewNode() + + // Step 1: Initial registration with tagged pre-auth key + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: pak.Key, + }, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "authkey-tagged-node", + }, + Expiry: time.Now().Add(24 * time.Hour), + } + + resp, err := app.handleRegisterWithAuthKey(regReq, machineKey.Public()) + require.NoError(t, err, "Initial registration should succeed") + require.True(t, resp.MachineAuthorized, "Node should be authorized") + + // Verify initial state: node is tagged via authkey + node, found := app.state.GetNodeByNodeKey(nodeKey1.Public()) + require.True(t, found, "Node should be found") + require.True(t, node.IsTagged(), "Node should be tagged after authkey registration") + require.ElementsMatch(t, authKeyTags, node.Tags().AsSlice(), "Node should have authkey tags") + require.NotNil(t, node.AuthKey(), "Node should have AuthKey reference") + require.Positive(t, node.AuthKey().Tags().Len(), "AuthKey should have tags") + + t.Logf("Initial registration complete - Node ID: %d, Tags: %v, IsTagged: %t, AuthKey.Tags.Len: %d", + node.ID().Uint64(), node.Tags().AsSlice(), node.IsTagged(), node.AuthKey().Tags().Len()) + + // Step 2: Reauth via web auth with EMPTY tags to transition to user-owned + nodeKey2 := key.NewNode() // New node key for reauth + registrationID := types.MustRegistrationID() + regEntry := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), // Same machine key + NodeKey: nodeKey2.Public(), // Different node key (rotation) + Hostname: "authkey-tagged-node", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "authkey-tagged-node", + RequestTags: []string{}, // EMPTY - should untag + }, + }) + app.state.SetRegistrationCacheEntry(registrationID, regEntry) + + // Complete reauth with empty tags + nodeAfterReauth, _, err := app.state.HandleNodeFromAuthPath( + registrationID, + types.UserID(user.ID), + nil, + "webauth", + ) + require.NoError(t, err, "Reauth should succeed") + + // Verify tags were removed (authkey-tagged → user-owned transition) + require.False(t, nodeAfterReauth.IsTagged(), "Node should NOT be tagged after reauth with empty tags") + require.Empty(t, nodeAfterReauth.Tags().AsSlice(), "Node should have no tags") + + // Verify ownership returned to user + require.True(t, nodeAfterReauth.UserID().Valid(), "Node should have a user ID") + require.Equal(t, user.ID, nodeAfterReauth.UserID().Get(), "Node should be owned by the user") + + // Verify it's the same node (not a new one) + require.Equal(t, node.ID(), nodeAfterReauth.ID(), "Should be the same node after reauth") + + // AuthKey reference should still exist (for audit purposes) + require.NotNil(t, nodeAfterReauth.AuthKey(), "AuthKey reference should be preserved") + + t.Logf("Reauth complete - Node ID: %d, Tags: %v, IsTagged: %t, UserID: %d", + nodeAfterReauth.ID().Uint64(), nodeAfterReauth.Tags().AsSlice(), + nodeAfterReauth.IsTagged(), nodeAfterReauth.UserID().Get()) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 03ba6e90..3b916ad6 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1246,6 +1246,93 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro return s.nodeStore.PutNode(*savedNode), nil } +// 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. +func (s *State) processReauthTags( + node *types.Node, + requestTags []string, + user *types.User, + oldTags []string, +) []string { + wasAuthKeyTagged := node.AuthKey != nil && node.AuthKey.IsTagged() + + logEvent := log.Debug(). + Uint64("node.id", uint64(node.ID)). + Str("node.name", node.Hostname). + Strs("request.tags", requestTags). + Strs("current.tags", node.Tags). + Bool("is.tagged", node.IsTagged()). + Bool("was.authkey.tagged", wasAuthKeyTagged) + logEvent.Msg("Processing RequestTags during reauth") + + // Empty RequestTags means untag node (transition to user-owned) + if len(requestTags) == 0 { + if node.IsTagged() { + log.Info(). + Uint64("node.id", uint64(node.ID)). + Str("node.name", node.Hostname). + Strs("removed.tags", node.Tags). + Str("user.name", user.Name). + Bool("was.authkey.tagged", wasAuthKeyTagged). + Msg("Reauth: removing all tags, returning node ownership to user") + + node.Tags = []string{} + node.UserID = &user.ID + } + + return nil + } + + // Non-empty RequestTags: validate and apply + var approvedTags, rejectedTags []string + + for _, tag := range requestTags { + if s.polMan.NodeCanHaveTag(node.View(), tag) { + approvedTags = append(approvedTags, tag) + } else { + rejectedTags = append(rejectedTags, tag) + } + } + + if len(rejectedTags) > 0 { + log.Warn(). + Uint64("node.id", uint64(node.ID)). + Str("node.name", node.Hostname). + Strs("rejected.tags", rejectedTags). + Msg("Reauth: requested tags are not permitted") + + return rejectedTags + } + + if len(approvedTags) > 0 { + slices.Sort(approvedTags) + approvedTags = slices.Compact(approvedTags) + + wasTagged := node.IsTagged() + node.Tags = approvedTags + + // Note: UserID is preserved as "created by" tracking, consistent with SetNodeTags + if !wasTagged { + log.Info(). + Uint64("node.id", uint64(node.ID)). + Str("node.name", node.Hostname). + Strs("new.tags", approvedTags). + Str("old.user", user.Name). + Msg("Reauth: applying tags, transferring node to tagged-devices") + } else { + log.Info(). + Uint64("node.id", uint64(node.ID)). + Str("node.name", node.Hostname). + Strs("old.tags", oldTags). + Strs("new.tags", approvedTags). + Msg("Reauth: updating tags on already-tagged node") + } + } + + return nil +} + // HandleNodeFromAuthPath handles node registration through authentication flow (like OIDC). func (s *State) HandleNodeFromAuthPath( registrationID types.RegistrationID, @@ -1291,14 +1378,26 @@ func (s *State) HandleNodeFromAuthPath( // If this node exists for this user, update the node in place. if existsSameUser && existingNodeSameUser.Valid() { - log.Debug(). + log.Info(). Caller(). Str("registration_id", registrationID.String()). Str("user.name", user.Name). Str("registrationMethod", registrationMethod). Str("node.name", existingNodeSameUser.Hostname()). Uint64("node.id", existingNodeSameUser.ID().Uint64()). - Msg("Updating existing node registration") + Interface("hostinfo", regEntry.Node.Hostinfo). + Msg("Updating existing node registration via reauth") + + // Process RequestTags during reauth (#2979) + // Due to json:",omitempty", we treat empty/nil as "clear tags" + var requestTags []string + if regEntry.Node.Hostinfo != nil { + requestTags = regEntry.Node.Hostinfo.RequestTags + } + + oldTags := existingNodeSameUser.Tags().AsSlice() + + var rejectedTags []string // Update existing node - NodeStore first, then database updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) { @@ -1327,12 +1426,18 @@ func (s *State) HandleNodeFromAuthPath( node.Expiry = regEntry.Node.Expiry } } + + rejectedTags = s.processReauthTags(node, requestTags, user, oldTags) }) if !ok { return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID()) } + if len(rejectedTags) > 0 { + return types.NodeView{}, change.Change{}, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, rejectedTags) + } + _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { // Use Updates() to preserve fields not modified by UpdateNode. err := tx.Updates(updatedNodeView.AsStruct()).Error @@ -1686,6 +1791,14 @@ func (s *State) updatePolicyManagerUsers() (change.Change, error) { return change.Change{}, nil } +// UpdatePolicyManagerUsersForTest updates the policy manager's user cache. +// This is exposed for testing purposes to sync the policy manager after +// creating test users via CreateUserForTest(). +func (s *State) UpdatePolicyManagerUsersForTest() error { + _, err := s.updatePolicyManagerUsers() + return err +} + // updatePolicyManagerNodes updates the policy manager with current nodes. // Returns true if the policy changed and notifications should be sent. // TODO(kradalby): This is a temporary stepping stone, ultimately we should