From bec665726cda8aec203bdf14f85aafe0225cf8b9 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 22 Oct 2026 09:28:45 +0000 Subject: [PATCH] state: apply requested tags on a key login that owns them A requested tag the key's user owns directly, or the key's tags own one hop away, replaces the key's tags instead of refusing the login. --- hscontrol/servertest/reauth_compat_test.go | 33 +----------- hscontrol/state/auth_tagged_expiry_test.go | 62 +++++++++++++++++++++- hscontrol/state/autoapprove_test.go | 2 +- hscontrol/state/state.go | 62 ++++++++++++++++------ integration/tags_test.go | 38 ++++++------- 5 files changed, 125 insertions(+), 72 deletions(-) diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index 8be1c0b22..d050864ea 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -39,35 +39,6 @@ import ( var reauthSkipReasons = map[string]string{ "owner-routes/r-expired-retag/expire/online": expiredPeerOffline, - "identity/w-tagreq-relogin-tags/w-tagreq/owner": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/outcome": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/node-key": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/w-tagreq/expiry": keyLoginTagsRefused, - "identity/w-tagreq-relogin-tags/obs/filter": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/devices": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/owner": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/outcome": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/expiry": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/ephemeral": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/peer-obs": keyLoginTagsRefused, - "identity/w-tagreq-new-join/w-tagreq-new/peer-key-obs": keyLoginTagsRefused, - - "tag-owner-chain/u-base-join/u-base/devices": keyLoginTagsRefused, - "tag-owner-chain/u-base-join/u-base/owner": keyLoginTagsRefused, - "tag-owner-chain/u-base-join/u-base/outcome": keyLoginTagsRefused, - "tag-owner-chain/u-base-join/u-base/expiry": keyLoginTagsRefused, - "tag-owner-chain/u-base-join/u-base/ephemeral": keyLoginTagsRefused, - "tag-owner-chain/u-base-join/u-base/peer-obs": keyLoginTagsRefused, - "tag-owner-chain/u-base-join/u-base/peer-key-obs": keyLoginTagsRefused, - - "tag-owner-chain/t-mid-join/t-mid/devices": taggedKeyOwnedTagRefused, - "tag-owner-chain/t-mid-join/t-mid/owner": taggedKeyOwnedTagRefused, - "tag-owner-chain/t-mid-join/t-mid/outcome": taggedKeyOwnedTagRefused, - "tag-owner-chain/t-mid-join/t-mid/expiry": taggedKeyOwnedTagRefused, - "tag-owner-chain/t-mid-join/t-mid/ephemeral": taggedKeyOwnedTagRefused, - "tag-owner-chain/t-mid-join/t-mid/peer-obs": taggedKeyOwnedTagRefused, - "tag-owner-chain/t-mid-join/t-mid/peer-key-obs": taggedKeyOwnedTagRefused, - "reauth/late-pak-spent-key/late-pak/text": refusalText, "reauth/late-pak-spent-key/late-pak/legacy-text": refusalText, @@ -78,9 +49,7 @@ var reauthSkipReasons = map[string]string{ } const ( - keyLoginTagsRefused = "KEY_LOGIN_TAGS_REFUSED: headscale refuses a user's key login that requests tags the user owns; SaaS tags the device, converting it in place or creating it tagged" - taggedKeyOwnedTagRefused = "TAGGED_KEY_OWNED_TAG_REFUSED: headscale refuses a tagged key's login that requests a tag the key's tags own; SaaS gives the device the requested tags" - refusalText = "REFUSAL_TEXT: SaaS refuses a key by naming its ID; headscale says why the key is refused" + refusalText = "REFUSAL_TEXT: SaaS refuses a key by naming its ID; headscale says why the key is refused" ) // reauthNotCompared lists, by key prefix, checks left out: SaaS's answer diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 28b575d8c..3232fa4f1 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -1853,7 +1853,7 @@ func (c pakReregCase) reregister( regReq tailcfg.RegisterRequest, ) (types.NodeView, error) { hi := regReq.Hostinfo.Clone() - node, _, err := c.s.reregisterNodeWithPAK(view, pak, regReq, c.machineKey, hi.Hostname, hi) + node, _, err := c.s.reregisterNodeWithPAK(view, pak, nil, regReq, c.machineKey, hi.Hostname, hi) return node, err } @@ -3044,10 +3044,70 @@ func TestPAKWakeAcceptsRequestedTags(t *testing.T) { assert.Equal(t, "h1", ns.Hostname) req.NodeKey = key.NewNode().Public() + req.Hostinfo = &tailcfg.Hostinfo{Hostname: "h1", RequestTags: []string{"tag:other"}} _, _, err = s.HandleNodeFromPreAuthKey(req, c.machineKey) require.ErrorIs(t, err, ErrRequestedTagsInvalidOrNotPermitted) } +// TestPAKLoginAppliesOwnedRequestTags: a key login gets the tags it requests +// when a user's key's user owns each directly, or a tagged key's tags own it +// one hop away; the requested tags replace the key's. Anything else is +// refused. +func TestPAKLoginAppliesOwnedRequestTags(t *testing.T) { + s := newRetagTestState(t) + user, _, err := s.CreateUser(types.User{Name: "tag-req-user"}) + require.NoError(t, err) + + _, err = s.SetPolicy([]byte(`{"tagOwners": { + "tag:base": ["tag-req-user@"], + "tag:mid": ["tag:base"], + "tag:top": ["tag:mid"] + }}`)) + require.NoError(t, err) + + userKey, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + baseKey, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:base"}) + require.NoError(t, err) + + join := func(k *types.PreAuthKeyNew, tags ...string) (types.NodeView, error) { + node, _, err := s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: k.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "h", RequestTags: tags}, + }, key.NewMachine().Public()) + + return node, err + } + + node, err := join(userKey, "tag:base") + require.NoError(t, err) + assert.Equal(t, []string{"tag:base"}, node.Tags().AsSlice()) + assert.False(t, node.UserID().Valid()) + assert.False(t, node.Expiry().Valid()) + + node, err = join(baseKey, "tag:mid") + require.NoError(t, err) + assert.Equal(t, []string{"tag:mid"}, node.Tags().AsSlice()) + + _, err = join(userKey, "tag:mid") + require.ErrorIs(t, err, ErrRequestedTagsInvalidOrNotPermitted, "a user owns only its tags") + _, err = join(baseKey, "tag:top") + require.ErrorIs(t, err, ErrRequestedTagsInvalidOrNotPermitted, "two hops") + + c := registerForPAKRereg(t, s, userKey) + req := c.regReq + req.NodeKey = key.NewNode().Public() + req.Hostinfo = &tailcfg.Hostinfo{Hostname: "h1", RequestTags: []string{"tag:base"}} + _, _, err = s.HandleNodeFromPreAuthKey(req, c.machineKey) + require.NoError(t, err) + + ns, row := c.fields(t) + assert.Equal(t, []string{"tag:base"}, ns.Tags, "converted in place") + assert.Equal(t, []string{"tag:base"}, row.Tags) + assert.Nil(t, ns.UserID) +} + // TestUserKeyOnTaggedMachine: a user key on a tagged machine re-authenticates // the tagged node only when it speaks for it (the user an admin tagged it // from); another user's key wakes it or gets a device of its own beside it diff --git a/hscontrol/state/autoapprove_test.go b/hscontrol/state/autoapprove_test.go index 2e87682c9..a4ad5846c 100644 --- a/hscontrol/state/autoapprove_test.go +++ b/hscontrol/state/autoapprove_test.go @@ -199,7 +199,7 @@ func TestRegistrationApprovalComparesReplacedNode(t *testing.T) { req := c.regReq req.Hostinfo = hi.Clone() - node, replaced, err := s.reregisterNodeWithPAK(stale, stalePAK, req, c.machineKey, hi.Hostname, req.Hostinfo) + node, replaced, err := s.reregisterNodeWithPAK(stale, stalePAK, nil, req, c.machineKey, hi.Hostname, req.Hostinfo) require.NoError(t, err) _, _, err = s.approveRoutesAfterRegistration(replaced, node, change.Change{}) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index a4eda1f1a..e6a153d63 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2288,6 +2288,8 @@ type newNodeParams struct { // Optional: Pre-auth key specific fields PreAuthKey *types.PreAuthKey + // Tags the key login was allowed to request; they replace the key's. + Tags []string // Optional: Existing node for netinfo preservation ExistingNodeForNetinfo types.NodeView @@ -2551,10 +2553,15 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro // Assign ownership based on PreAuthKey if params.PreAuthKey != nil { - if params.PreAuthKey.IsTagged() { + tags := params.PreAuthKey.Tags + if params.Tags != nil { + tags = params.Tags + } + + if len(tags) > 0 { // Tagged nodes are owned by their tags, not a user. // UserID is intentionally left nil. - nodeToRegister.Tags = params.PreAuthKey.Tags + nodeToRegister.Tags = tags // Tagged nodes have key expiry disabled. nodeToRegister.Expiry = nil @@ -3194,24 +3201,35 @@ func (s *State) HandleNodeFromPreAuthKey( } } - // A pre-auth key node's tags come from the key, never from RequestTags. // Advertising a subset of the key's tags is redundant, not an escalation, - // and the tailscale client's OAuth authkey flow always does it; any other - // tag is rejected, ahead of the key's own error, so new nodes and + // and the tailscale client's OAuth authkey flow always does it. Other + // requested tags replace the key's when a tagged key's tags own each one, + // or a user's key's user owns it directly, as on Tailscale SaaS; otherwise + // they are refused, ahead of the key's own error, so new nodes and // re-registrations are held to the same rule. - var tagsErr error + var ( + requestedTags []string + tagsErr error + ) - if regReq.Hostinfo != nil { - var extraTags []string + if regReq.Hostinfo != nil && slices.ContainsFunc(regReq.Hostinfo.RequestTags, func(tag string) bool { + return !slices.Contains(pak.Tags, tag) + }) { + var refused []string for _, tag := range regReq.Hostinfo.RequestTags { - if !slices.Contains(pak.Tags, tag) { - extraTags = append(extraTags, tag) + owned := s.polMan.TagOwnedByTags(tag, pak.Tags) || + (!pak.IsTagged() && pak.User != nil && s.polMan.UserCanHaveTag(pak.User.View(), tag)) + if !owned { + refused = append(refused, tag) } } - if len(extraTags) > 0 { - tagsErr = fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, extraTags) + if len(refused) > 0 { + tagsErr = fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, refused) + } else { + // Sort a copy: RequestTags is the node's reported Hostinfo. + requestedTags = slices.Compact(slices.Sorted(slices.Values(regReq.Hostinfo.RequestTags))) } } @@ -3270,7 +3288,7 @@ func (s *State) HandleNodeFromPreAuthKey( // a tagged node (findExistingNodeForPAK matches it under UserID 0); for a // user-owned key it is true when the same user already has the node. if existsSameUser && existingNodeSameUser.Valid() { - finalNode, before, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo) + finalNode, before, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, requestedTags, regReq, machineKey, hostname, validHostinfo) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } @@ -3349,6 +3367,7 @@ func (s *State) HandleNodeFromPreAuthKey( Expiry: reqExpiry, RegisterMethod: util.RegisterMethodAuthKey, PreAuthKey: pak, + Tags: requestedTags, ExistingNodeForNetinfo: differentUserNode, }) if err != nil { @@ -3411,6 +3430,7 @@ func (s *State) approveRoutesAfterRegistration( func (s *State) reregisterNodeWithPAK( existingNodeSameUser types.NodeView, pak *types.PreAuthKey, + requestedTags []string, regReq tailcfg.RegisterRequest, machineKey key.MachinePublic, hostname string, @@ -3492,10 +3512,19 @@ func (s *State) reregisterNodeWithPAK( // node.AuthKeyID still holds the prior key's ID here (it is reassigned // below), and SetNodeTags leaves AuthKeyID intact, so an admin retag // cannot masquerade as a new key. - if pak.ConvertsNodeToTagged(node.View()) || pak.RetagsNode(node.View()) { + // + // Tags the login was allowed to request replace the node's instead. + tags := pak.Tags + retag := pak.ConvertsNodeToTagged(node.View()) || pak.RetagsNode(node.View()) + + if requestedTags != nil { + tags, retag = requestedTags, !slices.Equal(node.Tags, requestedTags) + } + + if retag { wasUserOwned := !node.IsTagged() - node.Tags = pak.Tags + node.Tags = tags node.UserID = nil node.User = nil @@ -3509,7 +3538,8 @@ func (s *State) reregisterNodeWithPAK( if wasUserOwned || node.IsExpiredAt(now) { node.Expiry = nil } - } else if node.IsTagged() && !pak.IsTagged() && prior.NodeKey != regReq.NodeKey { + } else if requestedTags == nil && node.IsTagged() && !pak.IsTagged() && + prior.NodeKey != regReq.NodeKey { // A user key that speaks for a tagged node, with a new node key, // takes it back as that user's device, as on Tailscale SaaS. A // relogin under the same node key keeps it tagged. diff --git a/integration/tags_test.go b/integration/tags_test.go index b3b845d0e..2b9efa89c 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -913,13 +913,15 @@ func TestTagsAuthKeyWithTagCLICannotModifyAdminTags(t *testing.T) { // Test Suite 3: Auth Key WITHOUT Tags // ============================================================================= -// TestTagsAuthKeyWithoutTagCannotRequestTags tests that nodes cannot request tags -// when using an auth key that has no tags. +// TestTagsAuthKeyWithoutTagCanRequestOwnedTag tests that a node registering +// with a user's auth key that has no tags gets the tags it requests when the +// user owns them, as on Tailscale SaaS. Tags the user does not own are still +// refused (Test 3.8). // -// Test 3.1: Cannot request tags with tagless key +// Test 3.1: Request an owned tag with a tagless key // Setup: Run `tailscale up --advertise-tags="tag:valid-owned" --auth-key AUTH_KEY_WITHOUT_TAG` -// Expected: Registration fails with error containing "requested tags [tag:valid-owned] are invalid or not permitted". -func TestTagsAuthKeyWithoutTagCannotRequestTags(t *testing.T) { +// Expected: Registration succeeds and the node is tagged tag:valid-owned. +func TestTagsAuthKeyWithoutTagCanRequestOwnedTag(t *testing.T) { IntegrationSkip(t) policy := tagsTestPolicy() @@ -962,26 +964,18 @@ func TestTagsAuthKeyWithoutTagCannotRequestTags(t *testing.T) { ) require.NoError(t, err) - // Login should fail because the auth key has no tags err = client.Login(headscale.GetEndpoint(), authKey.Key) - if err != nil { - t.Logf("Test 3.1 PASS: Registration correctly rejected: %v", err) - assert.ErrorContains(t, err, "requested tags") - } else { - // If it succeeded, document this unexpected behavior - t.Logf("Test 3.1 UNEXPECTED: Registration succeeded when it should have failed") + require.NoError(t, err) - assert.EventuallyWithT(t, func(c *assert.CollectT) { - nodes, err := headscale.ListNodes() - assert.NoError(c, err) + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1) - if len(nodes) == 1 { - t.Logf("Node registered with tags: %v (expected rejection)", nodes[0].Tags) - } - }, integrationutil.ScaledTimeout(10*time.Second), integrationutil.SlowPoll, "checking node state") - - t.Fail() - } + if len(nodes) == 1 { + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"}) + } + }, integrationutil.ScaledTimeout(10*time.Second), integrationutil.SlowPoll, "node should carry the owned tag it requested") } // TestTagsAuthKeyWithoutTagRegisterNoTags tests that registering with a tagless auth key