diff --git a/hscontrol/api/v2/keys.go b/hscontrol/api/v2/keys.go index f44c3b890..9f4c67243 100644 --- a/hscontrol/api/v2/keys.go +++ b/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() } diff --git a/hscontrol/apiv2_compat_test.go b/hscontrol/apiv2_compat_test.go index 1170ecde9..646e281f0 100644 --- a/hscontrol/apiv2_compat_test.go +++ b/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" diff --git a/hscontrol/apiv2_oauth_test.go b/hscontrol/apiv2_oauth_test.go index a4d23b1e0..08150ed09 100644 --- a/hscontrol/apiv2_oauth_test.go +++ b/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) { diff --git a/hscontrol/db/users.go b/hscontrol/db/users.go index 84fd3649e..0fa381d9a 100644 --- a/hscontrol/db/users.go +++ b/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 diff --git a/hscontrol/db/users_test.go b/hscontrol/db/users_test.go index b33a3acd2..ab7379250 100644 --- a/hscontrol/db/users_test.go +++ b/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) { diff --git a/hscontrol/servertest/apiv2_test.go b/hscontrol/servertest/apiv2_test.go index ddebf76d6..438f94a30 100644 --- a/hscontrol/servertest/apiv2_test.go +++ b/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") diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index b8afc06d5..6dcfdd482 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/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 diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 29680e0f3..0fc1d38d8 100644 --- a/hscontrol/state/state.go +++ b/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 } diff --git a/integration/cli_nodes_test.go b/integration/cli_nodes_test.go index bbf02331f..9dc0d65c6 100644 --- a/integration/cli_nodes_test.go +++ b/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"