From ea53078dde76cbd0b90cd3ccb88f032e7da06575 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 2 Feb 2026 14:53:27 +0000 Subject: [PATCH] =?UTF-8?q?integration:=20add=20test=20for=20tagged?= =?UTF-8?q?=E2=86=92user-owned=20conversion=20panic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add TestTagsAuthKeyConvertToUserViaCLIRegister that reproduces the exact panic from #3038: register a node with a tags-only PreAuthKey (no user), force reauth with empty tags, then register via CLI with a user. The mapper panics on node.Owner().Model().ID when User is nil. The critical detail is using a tags-only PreAuthKey (User: nil). When the key is created under a user, the node inherits the User pointer from createAndSaveNewNode and the bug is masked. Also add Owner() validity assertions to the existing unit test TestTaggedNodeWithoutUserToDifferentUser to catch the nil pointer at the unit test level. Updates #3038 --- .github/workflows/test-integration.yaml | 1 + hscontrol/auth_test.go | 7 ++ integration/tags_test.go | 119 ++++++++++++++++++++++++ 3 files changed, 127 insertions(+) diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index 82b40044..3a16c68f 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -247,6 +247,7 @@ jobs: - TestTagsUserLoginReauthWithEmptyTagsRemovesAllTags - TestTagsAuthKeyWithoutUserInheritsTags - TestTagsAuthKeyWithoutUserRejectsAdvertisedTags + - TestTagsAuthKeyConvertToUserViaCLIRegister uses: ./.github/workflows/integration-test-template.yml secrets: inherit with: diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 8e947f1f..7967eee3 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -3916,6 +3916,13 @@ func TestTaggedNodeWithoutUserToDifferentUser(t *testing.T) { require.False(t, nodeAfterReauth.IsTagged(), "Node should no longer be tagged") require.Empty(t, nodeAfterReauth.Tags().AsSlice(), "Node should have no tags") + // Verify Owner() works without panicking - this is what the mapper's + // generateUserProfiles calls, and it would panic with a nil pointer + // dereference if node.User was not set during the tag→user conversion. + owner := nodeAfterReauth.Owner() + require.True(t, owner.Valid(), "Owner should be valid after conversion (mapper would panic if nil)") + require.Equal(t, alice.ID, owner.Model().ID, "Owner should be alice") + t.Logf("Re-registration complete - Node ID: %d, Tags: %v, IsTagged: %t, UserID: %d", nodeAfterReauth.ID().Uint64(), nodeAfterReauth.Tags().AsSlice(), nodeAfterReauth.IsTagged(), nodeAfterReauth.UserID().Get()) diff --git a/integration/tags_test.go b/integration/tags_test.go index 5dad36e5..16105ea2 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -3116,3 +3116,122 @@ func TestTagsAuthKeyWithoutUserRejectsAdvertisedTags(t *testing.T) { t.Fail() } } + +// ============================================================================= +// Test Suite 6: Tagged→User Conversion via CLI Register (#3038) +// ============================================================================= + +// TestTagsAuthKeyConvertToUserViaCLIRegister reproduces the panic from +// issue #3038: register a node with a tags-only preauthkey (no user), then +// convert it to a user-owned node via "headscale nodes register --user --key ...". +// The crash happens in the mapper's generateUserProfiles when node.User is nil +// after the tag→user conversion in processReauthTags. +// +// The key detail is using a tags-only PreAuthKey (User: nil). When created under +// a user, the node inherits User from the PreAuthKey and the bug is masked. +func TestTagsAuthKeyConvertToUserViaCLIRegister(t *testing.T) { + IntegrationSkip(t) + + policy := tagsTestPolicy() + + spec := ScenarioSpec{ + NodesPerUser: 0, + Users: []string{tagTestUser}, + } + + scenario, err := NewScenario(spec) + + require.NoError(t, err) + defer scenario.ShutdownAssertNoPanics(t) + + err = scenario.CreateHeadscaleEnvWithLoginURL( + []tsic.Option{}, + hsic.WithACLPolicy(policy), + hsic.WithTestName("tags-authkey-to-user-cli-3038"), + hsic.WithTLS(), + ) + requireNoErrHeadscaleEnv(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + // Step 1: Create a tags-only preauthkey WITHOUT a user. + // This is the critical detail: when PreAuthKey.UserID is nil, the node + // enters the NodeStore with node.User == nil. The processReauthTags + // conversion then sets UserID but not User, leaving it nil for the mapper. + authKey, err := scenario.CreatePreAuthKeyWithOptions(hsic.AuthKeyOptions{ + User: nil, + Reusable: false, + Ephemeral: false, + Tags: []string{"tag:valid-owned"}, + }) + require.NoError(t, err) + t.Logf("Created tags-only PreAuthKey (no user) with tags: %v", authKey.GetAclTags()) + + client, err := scenario.CreateTailscaleNode( + "head", + tsic.WithNetwork(scenario.networks[scenario.testDefaultNetwork]), + ) + require.NoError(t, err) + + err = client.Login(headscale.GetEndpoint(), authKey.GetKey()) + require.NoError(t, err) + + err = client.WaitForRunning(120 * time.Second) + require.NoError(t, err) + + // Verify initial state: node is tagged + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1) + + if len(nodes) == 1 { + assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"}) + t.Logf("Initial state - Node ID: %d, Tags: %v", nodes[0].GetId(), nodes[0].GetTags()) + } + }, 30*time.Second, 500*time.Millisecond, "node should be tagged initially") + + // Step 2: Force reauth with empty tags (triggers web auth flow) + command := []string{ + "tailscale", "up", + "--login-server=" + headscale.GetEndpoint(), + "--hostname=" + client.Hostname(), + "--advertise-tags=", + "--force-reauth", + } + + stdout, stderr, _ := client.Execute(command) + t.Logf("Reauth command output: stdout=%s stderr=%s", stdout, stderr) + + loginURL, err := util.ParseLoginURLFromCLILogin(stdout + stderr) + require.NoError(t, err, "Failed to parse login URL from reauth command") + + body, err := doLoginURL(client.Hostname(), loginURL) + require.NoError(t, err) + + // Step 3: Register via CLI with user (this is the exact step that triggers the panic) + err = scenario.runHeadscaleRegister(tagTestUser, body) + require.NoError(t, err) + + err = client.WaitForRunning(120 * time.Second) + require.NoError(t, err) + + // Step 4: Verify node is now user-owned and the mapper didn't panic. + // The panic would occur when the mapper builds the MapResponse and calls + // node.Owner().Model().ID with a nil User pointer. + // ShutdownAssertNoPanics in the defer catches any panics in headscale logs. + assert.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Len(c, nodes, 1) + + if len(nodes) == 1 { + assertNodeHasNoTagsWithCollect(c, nodes[0]) + assert.Equal(c, tagTestUser, nodes[0].GetUser().GetName(), + "Node ownership should be returned to user after untagging") + t.Logf("After conversion - Node ID: %d, Tags: %v, User: %s", + nodes[0].GetId(), nodes[0].GetTags(), nodes[0].GetUser().GetName()) + } + }, 60*time.Second, 1*time.Second, "node should be user-owned after conversion via CLI register") +}