From 61a626eab7cf14429b16e369509cc868a71d0b6a Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 27 Jun 2026 10:12:29 +0000 Subject: [PATCH] state: accept advertise-tags subset of a pre-auth key's tags tailscale client's OAuth authkey flow re-advertises the key's tags. Reject only tags the key lacks, for new nodes and re-registrations. --- hscontrol/auth_test.go | 232 ++++++++++++++++++++++++++++++++++++++- hscontrol/state/state.go | 25 ++++- integration/tags_test.go | 4 +- 3 files changed, 249 insertions(+), 12 deletions(-) diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index a4c758eb..3ecf399f 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -50,6 +50,7 @@ func TestAuthenticationFlows(t *testing.T) { machineKey func() key.MachinePublic wantAuth bool wantError bool + wantErrMsg string wantAuthURL bool wantExpired bool validate func(*testing.T, *tailcfg.RegisterResponse, *Headscale) @@ -958,11 +959,12 @@ func TestAuthenticationFlows(t *testing.T) { // === ADVERTISE-TAGS (RequestTags) SCENARIOS === // Tests for client-provided tags via --advertise-tags flag - // TEST: PreAuthKey registration rejects client-provided RequestTags - // WHAT: Tests that PreAuthKey registrations cannot use client-provided tags - // INPUT: PreAuthKey registration with [tailcfg.Hostinfo.RequestTags] set + // TEST: Untagged PreAuthKey registration rejects client-provided RequestTags + // WHAT: Tests that PreAuthKey registrations cannot claim tags the key lacks + // INPUT: Untagged PreAuthKey registration with [tailcfg.Hostinfo.RequestTags] set // EXPECTED: Registration fails with "requested tags [...] are invalid or not permitted" error - // WHY: PreAuthKey nodes get their tags from the key itself, not from client requests + // WHY: PreAuthKey nodes get their tags from the key; only a subset of the + // key's own tags may be advertised { name: "preauth_key_rejects_request_tags", setupFunc: func(t *testing.T, app *Headscale) (string, error) { //nolint:thelper @@ -1031,6 +1033,222 @@ func TestAuthenticationFlows(t *testing.T) { wantError: true, // RequestTags rejected for PreAuthKey registrations }, + // TEST: Tagged PreAuthKey accepts RequestTags that are a subset of the key's tags + // WHAT: The tailscale client's OAuth authkey flow re-advertises the key's + // own tags via --advertise-tags. + // INPUT: Tagged PreAuthKey ([tag:authorized, tag:other]) with RequestTags [tag:authorized] + // EXPECTED: Registration succeeds; node carries all of the key's tags. + // WHY: Advertising tags the key already grants is not an escalation; tags + // still come from the key, not the request. + { + name: "tagged_preauth_key_accepts_subset_request_tags", + setupFunc: func(t *testing.T, app *Headscale) (string, error) { //nolint:thelper + t.Helper() + + user := app.state.CreateUserForTest("tagged-pak-subsettags-user") + + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, false, nil, []string{"tag:authorized", "tag:other"}) + if err != nil { + return "", err + } + + return pak.Key, nil + }, + request: func(authKey string) tailcfg.RegisterRequest { + return tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: authKey, + }, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-pak-subsettags-node", + RequestTags: []string{"tag:authorized"}, + }, + Expiry: time.Now().Add(24 * time.Hour), + } + }, + machineKey: machineKey1.Public, + wantAuth: true, + validate: func(t *testing.T, _ *tailcfg.RegisterResponse, app *Headscale) { + t.Helper() + + node, found := app.state.GetNodeByNodeKey(nodeKey1.Public()) + require.True(t, found) + assert.True(t, node.IsTagged()) + assert.ElementsMatch(t, []string{"tag:authorized", "tag:other"}, node.Tags().AsSlice()) + assert.False(t, node.UserID().Valid(), "tagged node must not be user-owned") + }, + }, + + // TEST: Tagged PreAuthKey rejects RequestTags that go beyond the key's tags + // WHAT: A request mixing a key tag with a foreign tag + // INPUT: Tagged PreAuthKey ([tag:authorized]) with RequestTags [tag:authorized, tag:client-wants-this] + // EXPECTED: Registration fails naming only the foreign tag + // WHY: One matching tag must not smuggle in others + { + name: "tagged_preauth_key_rejects_partially_foreign_request_tags", + setupFunc: func(t *testing.T, app *Headscale) (string, error) { //nolint:thelper + t.Helper() + + user := app.state.CreateUserForTest("tagged-pak-mixedtags-user") + + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, false, nil, []string{"tag:authorized"}) + if err != nil { + return "", err + } + + return pak.Key, nil + }, + request: func(authKey string) tailcfg.RegisterRequest { + return tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: authKey, + }, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-pak-mixedtags-node", + RequestTags: []string{"tag:authorized", "tag:client-wants-this"}, + }, + Expiry: time.Now().Add(24 * time.Hour), + } + }, + machineKey: machineKey1.Public, + wantError: true, + wantErrMsg: "[tag:client-wants-this] are invalid", + }, + + // TEST: Existing tagged node re-registering with foreign RequestTags is rejected + // WHAT: Same machine and node key re-run `tailscale up --authkey` with a tag + // the key does not carry, hitting the skip-validation re-registration path + // INPUT: Node registered with tagged PreAuthKey ([tag:authorized]), then the + // same key with RequestTags [tag:client-wants-this] + // EXPECTED: Re-registration fails + // WHY: New and existing nodes are held to the same advertise-tags rule + { + name: "existing_tagged_node_reregister_rejects_foreign_request_tags", + setupFunc: func(t *testing.T, app *Headscale) (string, error) { //nolint:thelper + t.Helper() + + user := app.state.CreateUserForTest("tagged-pak-rereg-user") + + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, false, nil, []string{"tag:authorized"}) + if err != nil { + return "", err + } + + _, err = app.handleRegisterWithAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "tagged-pak-rereg-node"}, + Expiry: time.Now().Add(24 * time.Hour), + }, machineKey1.Public()) + if err != nil { + return "", err + } + + require.EventuallyWithT(t, func(c *assert.CollectT) { + _, found := app.state.GetNodeByNodeKey(nodeKey1.Public()) + assert.True(c, found) + }, 1*time.Second, 50*time.Millisecond) + + return pak.Key, nil + }, + request: func(authKey string) tailcfg.RegisterRequest { + return tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: authKey, + }, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-pak-rereg-node", + RequestTags: []string{"tag:client-wants-this"}, + }, + Expiry: time.Now().Add(24 * time.Hour), + } + }, + machineKey: machineKey1.Public, + wantError: true, + wantErrMsg: "[tag:client-wants-this] are invalid", + }, + + // TEST: Existing tagged node re-registers with a spent single-use key advertising a subset + // WHAT: Container restart re-running `tailscale up --authkey` with the key's + // own tag after an admin retagged the node + // INPUT: Single-use PreAuthKey ([tag:authorized, tag:other]) registers, admin + // sets tags [tag:admin], same key and node key re-register with RequestTags [tag:authorized] + // EXPECTED: Re-registration succeeds; admin's tags preserved + // WHY: The subset check must not break the same-key restart path, and the + // same key must not undo an admin override + { + name: "existing_tagged_node_reregister_spent_key_subset_tags_keeps_admin_tags", + setupFunc: func(t *testing.T, app *Headscale) (string, error) { //nolint:thelper + t.Helper() + + _, err := app.state.SetPolicy([]byte(`{"tagOwners":{"tag:admin":[],"tag:authorized":[],"tag:other":[]}}`)) + require.NoError(t, err) + + user := app.state.CreateUserForTest("tagged-pak-spent-user") + + pak, err := app.state.CreatePreAuthKey(user.TypedID(), false, false, nil, []string{"tag:authorized", "tag:other"}) + if err != nil { + return "", err + } + + _, err = app.handleRegisterWithAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-pak-spent-node", + RequestTags: []string{"tag:authorized"}, + }, + Expiry: time.Now().Add(24 * time.Hour), + }, machineKey1.Public()) + if err != nil { + return "", err + } + + var node types.NodeView + + require.EventuallyWithT(t, func(c *assert.CollectT) { + var found bool + + node, found = app.state.GetNodeByNodeKey(nodeKey1.Public()) + assert.True(c, found) + }, 1*time.Second, 50*time.Millisecond) + + _, _, err = app.state.SetNodeTags(node.ID(), []string{"tag:admin"}) + require.NoError(t, err) + + spent, err := app.state.GetPreAuthKey(pak.Key) + require.NoError(t, err) + require.True(t, spent.Used, "precondition: single-use key must be spent") + + return pak.Key, nil + }, + request: func(authKey string) tailcfg.RegisterRequest { + return tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: authKey, + }, + NodeKey: nodeKey1.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-pak-spent-node", + RequestTags: []string{"tag:authorized"}, + }, + Expiry: time.Now().Add(24 * time.Hour), + } + }, + machineKey: machineKey1.Public, + wantAuth: true, + validate: func(t *testing.T, _ *tailcfg.RegisterResponse, app *Headscale) { + t.Helper() + + node, found := app.state.GetNodeByNodeKey(nodeKey1.Public()) + require.True(t, found) + assert.Equal(t, []string{"tag:admin"}, node.Tags().AsSlice()) + }, + }, + // === RE-AUTHENTICATION SCENARIOS === // TEST: Existing node re-authenticates with new pre-auth key // WHAT: Tests that existing node can re-authenticate using new pre-auth key @@ -2534,6 +2752,11 @@ func TestAuthenticationFlows(t *testing.T) { // Validate error expectations if tt.wantError { assert.Error(t, err, "expected error but got none") + + if tt.wantErrMsg != "" { + require.ErrorContains(t, err, tt.wantErrMsg) + } + return } @@ -2574,6 +2797,7 @@ func runInteractiveWorkflowTest(t *testing.T, tt struct { machineKey func() key.MachinePublic wantAuth bool wantError bool + wantErrMsg string wantAuthURL bool wantExpired bool validate func(*testing.T, *tailcfg.RegisterResponse, *Headscale) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 6703b39e..aef53a96 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2024,12 +2024,6 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro nodeToRegister.Tags = nil } - // Reject advertise-tags for PreAuthKey registrations early, before any resource allocation. - // PreAuthKey nodes get their tags from the key itself, not from client requests. - if params.PreAuthKey != nil && params.Hostinfo != nil && len(params.Hostinfo.RequestTags) > 0 { - return types.NodeView{}, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, params.Hostinfo.RequestTags) - } - // Process RequestTags (from tailscale up --advertise-tags) ONLY for non-PreAuthKey registrations. // Validate early before IP allocation to avoid resource leaks on failure. if params.PreAuthKey == nil && params.Hostinfo != nil && len(params.Hostinfo.RequestTags) > 0 { @@ -2542,6 +2536,25 @@ func (s *State) HandleNodeFromPreAuthKey( return types.NodeView{}, change.Change{}, err } + // 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. Checked before key validation or resource allocation so + // new nodes and re-registrations are held to the same rule. + if regReq.Hostinfo != nil { + var extraTags []string + + for _, tag := range regReq.Hostinfo.RequestTags { + if !slices.Contains(pak.Tags, tag) { + extraTags = append(extraTags, tag) + } + } + + if len(extraTags) > 0 { + return types.NodeView{}, change.Change{}, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, extraTags) + } + } + // Helper to get username for logging (handles nil User for tags-only keys) pakUsername := func() string { if pak.User != nil { diff --git a/integration/tags_test.go b/integration/tags_test.go index 4967db44..5940187c 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -3234,8 +3234,8 @@ func TestTagsAuthKeyWithoutUserInheritsTags(t *testing.T) { } // TestTagsAuthKeyWithoutUserRejectsAdvertisedTags tests that when an auth key without -// a user (tags-only) is used WITH --advertise-tags, the registration is rejected. -// PreAuthKey registrations do not allow client-requested tags. +// a user (tags-only) is used WITH --advertise-tags naming a tag the key lacks, the +// registration is rejected. Only a subset of the key's own tags may be advertised. // // Test 5.2: Auth key without user, with --advertise-tags (should be rejected) // Setup: Run `tailscale up --advertise-tags="tag:second" --auth-key AUTH_KEY_WITH_TAGS_NO_USER`