api/v2: show userId on admin-made tagged keys

An admin key's user is recorded as the creator of the tagged keys it
makes. A tagged key re-registers a machine by its tags, whoever made it.

hscontrol/api/v2/keys.go

@@ -372,7 +372,14 @@ func createAuthKey(ctx context.Context, b Backend, body CreateKeyRequest) (*keyO
 		// A key minted by an OAuth token may only carry tags within the token's
 		// grant (held directly, or owned by a held tag) and defined in policy,
 		// matching SetNodeTags. An admin key keeps the historical behaviour of
-		// validating only tag syntax (db.validateACLTags).
+		// validating only tag syntax (db.validateACLTags), and records its
+		// user as the key's creator.
+		if !isOAuth {
+			if uid, ok := ownerUser(ctx); ok {
+				userID = &uid
+			}
+		}
+
 		if isOAuth {
 			for _, tag := range create.Tags {
 				if !b.State.TagExists(tag) {
@@ -547,8 +554,8 @@ func keyFromNew(pak *types.PreAuthKeyNew, req CreateKeyRequest) Key {
 		key.ExpirySeconds = expirySeconds(pak.CreatedAt, pak.Expiration)
 	}
 
-	// A tagged key presents no owner; a user-owned key reports its user id.
-	if len(pak.Tags) == 0 && pak.User != nil {
+	// A user-owned key reports its owner, a tagged key its creator.
+	if pak.User != nil {
 		key.UserID = pak.User.StringID()
 	}
 
@@ -579,7 +586,7 @@ func keyFromStored(pak *types.PreAuthKey) Key {
 		key.Revoked = pak.Revoked
 	}
 
-	if len(pak.Tags) == 0 && pak.User != nil {
+	if pak.User != nil {
 		key.UserID = pak.User.StringID()
 	}
 

hscontrol/apiv2_compat_test.go

@@ -105,23 +105,6 @@ var apiv2CompatSkipReasons = map[string]string{
 	"oauth/B7/0/message": deletedClientCredentialText,
 	"oauth/B7/1/message": deletedClientCredentialText,
 
-	"oauth/key:J-ci-runner/creator-user-id":   taggedKeyNoUserID,
-	"oauth/key:J-managed/creator-user-id":     taggedKeyNoUserID,
-	"oauth/key:J-mixed/creator-user-id":       taggedKeyNoUserID,
-	"oauth/key:J-prod-db/creator-user-id":     taggedKeyNoUserID,
-	"oauth/key:J-router/creator-user-id":      taggedKeyNoUserID,
-	"oauth/key:K-ci/creator-user-id":          taggedKeyNoUserID,
-	"oauth/key:K-mixed/creator-user-id":       taggedKeyNoUserID,
-	"oauth/key:K-prod/creator-user-id":        taggedKeyNoUserID,
-	"oauth/key:K1/creator-user-id":            taggedKeyNoUserID,
-	"policy-api/key:J-server/creator-user-id": taggedKeyNoUserID,
-
-	"key-list/key:A-db/creator-user-id":          taggedKeyNoUserID,
-	"key-list/key:A-late-prod/creator-user-id":   taggedKeyNoUserID,
-	"key-list/key:A-prod-db/creator-user-id":     taggedKeyNoUserID,
-	"key-list/key:A-prod-server/creator-user-id": taggedKeyNoUserID,
-	"key-list/key:A-prod/creator-user-id":        taggedKeyNoUserID,
-
 	"oauth/device:before/all/device-oauth-client":        deviceOAuthClientID,
 	"oauth/device:ci-runner/all/device-oauth-client":     deviceOAuthClientID,
 	"oauth/device:joiner/all/device-oauth-client":        deviceOAuthClientID,
@@ -207,7 +190,6 @@ const (
 	deletedClientGone           = "DELETED_CLIENT_GONE: SaaS keeps a deleted OAuth client readable as invalid and revoked; headscale removes it"
 	deletedClientCredentialText = "DELETED_CLIENT_CREDENTIAL_TEXT: SaaS refuses a deleted client's secret and tokens with API token invalid; headscale says invalid client credentials and unauthorized"
 	missingScopeText            = "MISSING_SCOPE_TEXT: SaaS refuses a delete outside the token's scopes with calling actor does not have enough permissions; headscale names the missing scope"
-	taggedKeyNoUserID           = "TAGGED_KEY_NO_USER_ID: SaaS shows the creating user's userId on a tagged auth key the admin made; headscale leaves userId off every tagged key"
 	deviceOAuthClientID         = "DEVICE_OAUTH_CLIENT_ID: SaaS device bodies carry oauthClientId, naming the join key or the OAuth client that made it; headscale's have no such field"
 	taggedDeviceUser            = "TAGGED_DEVICE_USER: SaaS shows a tagged device's user as the login that made its key, empty for a key a token made; headscale shows tagged-devices"
 	emptyRouteListsOmitted      = "EMPTY_ROUTE_LISTS_OMITTED: SaaS sends advertisedRoutes and enabledRoutes as [] for a device without routes under fields=all; headscale leaves both out"

hscontrol/apiv2_oauth_test.go

@@ -15,9 +15,12 @@ import (
 	clientv2 "github.com/juanfont/headscale/gen/client/v2"
 	apiv2 "github.com/juanfont/headscale/hscontrol/api/v2"
 	"github.com/juanfont/headscale/hscontrol/scope"
+	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/types/testcapture"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
+	tskey "tailscale.com/types/key"
 )
 
 // newOAuthTestServer builds the full v2 API (the auth+scope middleware and the
@@ -275,6 +278,66 @@ func TestAPIv2OAuth_ValidationErrorsBounded(t *testing.T) {
 	assert.Equal(t, "and 980 more", out.Data[0].Errors[20])
 }
 
+// TestAPIv2OAuth_TaggedKeyCreator: an admin key's user is the creator of the
+// tagged keys it makes and shows as their userId; a node joined with such a
+// key is tagged. A token's tagged key has no creator.
+func TestAPIv2OAuth_TaggedKeyCreator(t *testing.T) {
+	app, baseURL, admin := newOAuthTestServer(t)
+
+	owner := app.state.CreateUserForTest("odin")
+	ownerKey, apiKey, err := app.state.CreateAPIKey(nil)
+	require.NoError(t, err)
+	require.NoError(t, app.state.SetAPIKeyUser(apiKey.ID, types.UserID(owner.ID)))
+
+	create := func(bearer string) apiv2.Key {
+		t.Helper()
+
+		status, body := apiPost(t, baseURL+"/api/v2/tailnet/-/keys", bearer, apiv2.CreateKeyRequest{
+			Capabilities: &apiv2.KeyCapabilities{Devices: apiv2.KeyDeviceCapabilities{
+				Create: apiv2.KeyDeviceCreateCapabilities{Reusable: true, Tags: []string{"tag:ci"}},
+			}},
+		})
+		require.Equalf(t, http.StatusOK, status, "body: %s", body)
+
+		var key apiv2.Key
+		require.NoError(t, json.Unmarshal(body, &key))
+
+		return key
+	}
+
+	key := create(ownerKey)
+	assert.Equal(t, owner.StringID(), key.UserID)
+
+	status, body := apiReq(t, http.MethodGet, baseURL+"/api/v2/tailnet/-/keys/"+key.ID, admin, nil)
+	require.Equal(t, http.StatusOK, status)
+	assert.Contains(t, string(body), `"userId":"`+owner.StringID()+`"`)
+
+	node, _, err := app.state.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{
+		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: key.Key},
+		NodeKey:  tskey.NewNode().Public(),
+		Hostinfo: &tailcfg.Hostinfo{Hostname: "made-by-odin"},
+	}, tskey.NewMachine().Public())
+	require.NoError(t, err)
+	assert.True(t, node.IsTagged())
+	assert.Equal(t, []string{"tag:ci"}, node.Tags().AsSlice())
+
+	tok := mintScopedToken(t, baseURL, admin, scope.AuthKeys)
+	assert.Empty(t, create(tok).UserID)
+
+	// The key and the node's copy of it outlive the creator.
+	_, err = app.state.DeleteUser(*owner.TypedID())
+	require.NoError(t, err)
+
+	status, body = apiReq(t, http.MethodGet, baseURL+"/api/v2/tailnet/-/keys/"+key.ID, admin, nil)
+	require.Equalf(t, http.StatusOK, status, "body: %s", body)
+	assert.NotContains(t, string(body), `"userId"`)
+
+	nv, ok := app.state.GetNodeByID(node.ID())
+	require.True(t, ok)
+	require.True(t, nv.AuthKey().Valid())
+	assert.False(t, nv.AuthKey().User().Valid())
+}
+
 // TestAPIv2OAuth_UnknownKeyTypeRefused: a keyType other than auth or client
 // is refused, not taken as an auth key.
 func TestAPIv2OAuth_UnknownKeyTypeRefused(t *testing.T) {

hscontrol/db/users.go

@@ -79,6 +79,12 @@ func DestroyUser(tx *gorm.DB, uid types.UserID) error {
 	}
 
 	for _, key := range keys {
+		// A tagged key belongs to its tags: deleting the user that made it
+		// only nulls the key's user.
+		if key.IsTagged() {
+			continue
+		}
+
 		err = DestroyPreAuthKey(tx, key.ID)
 		if err != nil {
 			return err

hscontrol/db/users_test.go

@@ -41,7 +41,7 @@ func TestDestroyUserKeepsTaggedNodeEphemeral(t *testing.T) {
 
 	retained, err := db.GetPreAuthKeyByID(pak.ID)
 	require.NoError(t, err)
-	require.NotNil(t, retained.Revoked)
+	require.Nil(t, retained.Revoked, "a tagged key outlives the user that made it")
 	require.Nil(t, retained.UserID)
 }
 
@@ -97,6 +97,27 @@ func TestDestroyUserErrors(t *testing.T) {
 				assert.ErrorIs(t, result.Error, gorm.ErrRecordNotFound)
 			},
 		},
+		{
+			// A tagged key belongs to its tags, so it outlives the user that
+			// made it and only loses the reference.
+			name: "success_keeps_tagged_preauthkeys",
+			test: func(t *testing.T, db *HSDatabase) {
+				t.Helper()
+
+				user := db.CreateUserForTest("test")
+
+				tagged, err := db.CreatePreAuthKey(user.TypedID(), true, false, nil, []string{"tag:ci"})
+				require.NoError(t, err)
+
+				err = db.DestroyUser(types.UserID(user.ID))
+				require.NoError(t, err)
+
+				got, err := db.GetPreAuthKeyByID(tagged.ID)
+				require.NoError(t, err)
+				assert.Nil(t, got.UserID)
+				assert.Equal(t, []string{"tag:ci"}, got.Tags)
+			},
+		},
 		{
 			name: "error_user_has_nodes",
 			test: func(t *testing.T, db *HSDatabase) {

hscontrol/servertest/apiv2_test.go

@@ -305,12 +305,13 @@ func apiv2TSCLI(t *testing.T, srv *servertest.TestServer, baseURL, apiKey string
 	assert.NotEmpty(t, created.ID)
 	assert.NotEmpty(t, created.Key, "secret returned on create")
 
-	// Server-side: tagged, reusable, described, no owner.
+	// Server-side: tagged, reusable, described, and the API key's user
+	// recorded as its creator.
 	pak := srvPreAuthKey(t, srv, created.ID)
 	assert.Equal(t, []string{"tag:ci"}, pak.Tags)
 	assert.True(t, pak.Reusable)
 	assert.Equal(t, "tscli", pak.Description)
-	assert.Nil(t, pak.User, "tagged key has no owning user")
+	assert.NotNil(t, pak.User, "the API key's user made the tagged key")
 
 	getOut := run("get", "key", "--key", created.ID, "-o", "json")
 

hscontrol/state/auth_tagged_expiry_test.go

@@ -285,8 +285,19 @@ func TestReusableUserPAKReauthOnTaggedNodeNoDuplicate(t *testing.T) {
 
 // TestTaggedPAKReauthConvertsUserOwnedNode ensures presenting a tagged pre-auth
 // key for a machine that already has a user-owned node converts that node in
-// place (same machine, new ownership) rather than creating a duplicate.
+// place (same machine, new ownership) rather than creating a duplicate, whether
+// or not the key records who created it.
 func TestTaggedPAKReauthConvertsUserOwnedNode(t *testing.T) {
+	for _, withCreator := range []bool{false, true} {
+		t.Run(fmt.Sprintf("creator=%t", withCreator), func(t *testing.T) {
+			testTaggedPAKReauthConvertsUserOwnedNode(t, withCreator)
+		})
+	}
+}
+
+func testTaggedPAKReauthConvertsUserOwnedNode(t *testing.T, withCreator bool) {
+	t.Helper()
+
 	dbPath := t.TempDir() + "/headscale.db"
 	cfg := persistTestConfig(dbPath)
 
@@ -296,6 +307,11 @@ func TestTaggedPAKReauthConvertsUserOwnedNode(t *testing.T) {
 
 	user := s.CreateUserForTest("owner")
 
+	var creator *types.UserID
+	if withCreator {
+		creator = s.CreateUserForTest("admin").TypedID()
+	}
+
 	userPak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
 	require.NoError(t, err)
 
@@ -312,8 +328,8 @@ func TestTaggedPAKReauthConvertsUserOwnedNode(t *testing.T) {
 	require.NoError(t, err)
 	require.False(t, owned.IsTagged(), "precondition: node is user-owned")
 
-	// A tags-only key re-registers the same machine (same node key).
-	taggedPak, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:foo"})
+	// A tagged key re-registers the same machine (same node key).
+	taggedPak, err := s.CreatePreAuthKey(creator, true, false, nil, []string{"tag:foo"})
 	require.NoError(t, err)
 
 	convReq := regReq
@@ -371,19 +387,33 @@ func registerTwoUsersOnOneMachine(t *testing.T) (*State, key.MachinePublic, type
 // so the registration is rejected rather than converting an arbitrary one and
 // orphaning the rest.
 func TestTaggedPAKReauthRejectsAmbiguousMultiUserNode(t *testing.T) {
-	s, mk, _ := registerTwoUsersOnOneMachine(t)
+	for _, withCreator := range []bool{false, true} {
+		t.Run(fmt.Sprintf("creator=%t", withCreator), func(t *testing.T) {
+			s, mk, n1 := registerTwoUsersOnOneMachine(t)
 
-	taggedPak, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:foo"})
-	require.NoError(t, err)
+			// A key created by one of the two owners is still a tagged key.
+			var creator *types.UserID
 
-	_, _, err = s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{
-		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: taggedPak.Key},
-		NodeKey:  key.NewNode().Public(),
-		Hostinfo: &tailcfg.Hostinfo{Hostname: "multi"},
-		Expiry:   time.Now().Add(24 * time.Hour),
-	}, mk)
-	require.ErrorIs(t, err, ErrAmbiguousNodeOwnership)
-	require.Equal(t, 2, s.ListNodes().Len(), "no node created or converted on rejection")
+			if withCreator {
+				nv, ok := s.GetNodeByID(n1)
+				require.True(t, ok)
+
+				creator = new(types.UserID(nv.UserID().Get()))
+			}
+
+			taggedPak, err := s.CreatePreAuthKey(creator, true, false, nil, []string{"tag:foo"})
+			require.NoError(t, err)
+
+			_, _, err = s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{
+				Auth:     &tailcfg.RegisterResponseAuth{AuthKey: taggedPak.Key},
+				NodeKey:  key.NewNode().Public(),
+				Hostinfo: &tailcfg.Hostinfo{Hostname: "multi"},
+				Expiry:   time.Now().Add(24 * time.Hour),
+			}, mk)
+			require.ErrorIs(t, err, ErrAmbiguousNodeOwnership)
+			require.Equal(t, 2, s.ListNodes().Len(), "no node created or converted on rejection")
+		})
+	}
 }
 
 // TestAuthPathOnTaggedMachine: a login on a machine with a tagged node and

hscontrol/state/state.go

@@ -685,6 +685,30 @@ func (s *State) DeleteUser(userID types.UserID) (change.Change, error) {
 		return change.Change{}, err
 	}
 
+	// Tagged keys outlive the user that made them; their NodeStore copies
+	// must stop naming it, as the database already does.
+	dropCreator := func(n *types.Node) {
+		k := n.AuthKey
+		if k == nil || !k.IsTagged() || k.UserID == nil || types.UserID(*k.UserID) != userID {
+			return
+		}
+
+		key := *k
+		key.UserID, key.User = nil, nil
+		n.AuthKey = &key
+	}
+
+	updates := make(map[types.NodeID]UpdateNodeFunc)
+
+	for _, n := range s.nodeStore.ListNodes().All() {
+		if k := n.AuthKey(); k.Valid() && k.Tags().Len() > 0 && k.UserID().Valid() &&
+			types.UserID(k.UserID().Get()) == userID {
+			updates[n.ID()] = dropCreator
+		}
+	}
+
+	s.nodeStore.UpdateNodes(updates)
+
 	// Update policy manager with the new user list (without the deleted user)
 	// This ensures that if the policy references the deleted user, it gets
 	// re-evaluated immediately rather than when some other operation triggers it.
@@ -3158,21 +3182,8 @@ func (s *State) findExistingNodeForPAK(
 ) (types.NodeView, bool, error) {
 	all := s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey)
 
-	if pak.User != nil {
-		node, ok := all[types.UserID(pak.User.ID)]
-		if !ok {
-			// The machine's tagged node, when the key speaks for it. Any other
-			// key under its own node key is the device restarting (#3312);
-			// pakReregister lets that key only wake it.
-			node, ok = all[0]
-			ok = ok && node.IsTagged() && (pak.CanReauthenticate(node) || node.NodeKey() == nodeKey)
-		}
-
-		return node, ok, nil
-	}
-
-	// A tagged key re-registers the same machine regardless of how it is
-	// currently owned. An existing tagged node is a plain re-registration. A
+	// A tagged key, whether or not it records its creator, re-registers the
+	// same machine regardless of how it is currently owned. An existing tagged node is a plain re-registration. A
 	// single user-owned node is converted to tagged in place (handled by the
 	// caller). More than one user-owned node is ambiguous - we cannot know
 	// which to convert - so reject rather than convert an arbitrary one and
@@ -3203,6 +3214,19 @@ func (s *State) findExistingNodeForPAK(
 		}
 	}
 
+	if pak.User != nil {
+		node, ok := all[types.UserID(pak.User.ID)]
+		if !ok {
+			// The machine's tagged node, when the key speaks for it. Any other
+			// key under its own node key is the device restarting (#3312);
+			// pakReregister lets that key only wake it.
+			node, ok = all[0]
+			ok = ok && node.IsTagged() && (pak.CanReauthenticate(node) || node.NodeKey() == nodeKey)
+		}
+
+		return node, ok, nil
+	}
+
 	return types.NodeView{}, false, nil
 }
 

integration/cli_nodes_test.go

@@ -657,8 +657,14 @@ func TestPreAuthKeyCorrectUserLoggedInCommand(t *testing.T) {
 		listNodes, err = headscale.ListNodes()
 		assert.NoError(ct, err)
 		assert.Len(ct, listNodes, 1, "Should have exactly 1 node for user1")
-		assert.Equal(ct, user1, listNodes[0].User.Name, "Node should belong to user1")
+
+		if len(listNodes) == 1 {
+			assert.Equal(ct, user1, listNodes[0].User.Name, "Node should belong to user1")
+		}
 	}, integrationutil.ScaledTimeout(15*time.Second), 1*time.Second)
+	require.Len(t, listNodes, 1)
+
+	userNodeID := listNodes[0].Id
 
 	allClients, err := scenario.ListTailscaleClients()
 	requireNoErrListClients(t, err)
@@ -698,10 +704,15 @@ func TestPreAuthKeyCorrectUserLoggedInCommand(t *testing.T) {
 
 		listNodes, err = headscale.ListNodes()
 		assert.NoError(ct, err)
-		assert.Len(ct, listNodes, 2, "Should have 2 nodes after re-login")
-		assert.Equal(ct, user1, listNodes[0].User.Name, "First node should belong to user1")
-		// Second node is tagged (created with tagged PreAuthKey), so it shows as "tagged-devices"
-		assert.Equal(ct, "tagged-devices", listNodes[1].User.Name, "Second node should be tagged-devices")
+		// A tagged key converts the machine's user-owned node in place,
+		// whoever made the key, so it shows as "tagged-devices".
+		assert.Len(ct, listNodes, 1, "Should still have 1 node after re-login with a tagged key")
+
+		if len(listNodes) == 1 {
+			assert.Equal(ct, userNodeID, listNodes[0].Id, "The node should be converted, not replaced")
+			assert.Equal(ct, "tagged-devices", listNodes[0].User.Name, "The node should be tagged-devices")
+			assert.ElementsMatch(ct, []string{"tag:test1", "tag:test2"}, listNodes[0].Tags)
+		}
 	}, integrationutil.ScaledTimeout(20*time.Second), 1*time.Second)
 }
 
@@ -758,6 +769,18 @@ func TestTaggedNodesCLIOutput(t *testing.T) {
 		assert.NoError(c, err)
 	}, integrationutil.ScaledTimeout(10*time.Second), integrationutil.FastPoll, "Waiting for user2 tagged preauth key creation")
 
+	var userNodeID string
+
+	assert.EventuallyWithT(t, func(ct *assert.CollectT) {
+		nodes, err := headscale.ListNodes()
+		assert.NoError(ct, err)
+		assert.Len(ct, nodes, 1, "Should have exactly 1 node for user1")
+
+		if len(nodes) == 1 {
+			userNodeID = nodes[0].Id
+		}
+	}, integrationutil.ScaledTimeout(15*time.Second), 1*time.Second)
+
 	allClients, err := scenario.ListTailscaleClients()
 	requireNoErrListClients(t, err)
 
@@ -791,17 +814,16 @@ func TestTaggedNodesCLIOutput(t *testing.T) {
 		assert.Equal(ct, "userid:2147455555", status.Self.UserID.String(), "Expected node to be logged in as tagged-devices user")
 	}, integrationutil.StatusReadyTimeout, 2*time.Second)
 
-	// Wait for the second node to appear
-	var listNodes []*clientv1.Node
-
+	// A tagged key converts the machine's user-owned node in place.
 	assert.EventuallyWithT(t, func(ct *assert.CollectT) {
-		var err error
-
-		listNodes, err = headscale.ListNodes()
+		listNodes, err := headscale.ListNodes()
 		assert.NoError(ct, err)
-		assert.Len(ct, listNodes, 2, "Should have 2 nodes after re-login with tagged key")
-		assert.Equal(ct, user1, listNodes[0].User.Name, "First node should belong to user1")
-		assert.Equal(ct, "tagged-devices", listNodes[1].User.Name, "Second node should be tagged-devices")
+		assert.Len(ct, listNodes, 1, "Should still have 1 node after re-login with tagged key")
+
+		if len(listNodes) == 1 {
+			assert.Equal(ct, userNodeID, listNodes[0].Id, "The node should be converted, not replaced")
+			assert.Equal(ct, "tagged-devices", listNodes[0].User.Name, "The node should be tagged-devices")
+		}
 	}, integrationutil.ScaledTimeout(20*time.Second), 1*time.Second)
 
 	// Test: tailscale status output should show "tagged-devices" not "userid:2147455555"