From 636f660caf3ca995fad5a9ed6f1b6b0578637b55 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 18 Jun 2026 08:32:06 +0000 Subject: [PATCH 01/15] db: preserve user_id on untagged nodes with tags='null' A nil tags slice marshals to JSON `null`; the clear-tagged migration read that as tagged and cleared user_id. Exclude it, and recover already-detached nodes from their pre-auth key. Fixes #3323 --- CHANGELOG.md | 8 ++ hscontrol/db/db.go | 39 +++++++- hscontrol/db/db_test.go | 97 +++++++++++++++++++ .../null_tags_user_id_migration_test.sql | 85 ++++++++++++++++ ...cover_null_tags_user_id_migration_test.sql | 87 +++++++++++++++++ mkdocs.yml | 2 +- 6 files changed, 316 insertions(+), 2 deletions(-) create mode 100644 hscontrol/db/testdata/sqlite/null_tags_user_id_migration_test.sql create mode 100644 hscontrol/db/testdata/sqlite/recover_null_tags_user_id_migration_test.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 8580865a5..6630e6c3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ **Minimum supported Tailscale client version: v1.xx.0** +## 0.29.1 (2026-06-18) + +**Minimum supported Tailscale client version: v1.80.0** + +### Changes + +- Fix nodes with `tags='null'` losing their assigned user on upgrade [#3325](https://github.com/juanfont/headscale/pull/3325) + ## 0.29.0 (2026-06-17) **Minimum supported Tailscale client version: v1.80.0** diff --git a/hscontrol/db/db.go b/hscontrol/db/db.go index 8c088c492..f247a2fa7 100644 --- a/hscontrol/db/db.go +++ b/hscontrol/db/db.go @@ -706,13 +706,20 @@ AND auth_key_id NOT IN ( // but this prevents deleting users whose nodes have been // tagged, and the ON DELETE CASCADE FK would destroy the // tagged nodes if the user were deleted. + // + // A nil tags slice marshals to the JSON literal 'null', so + // untagged nodes can carry tags='null'. That spelling must be + // excluded alongside '[]' and '' or untagged nodes lose their + // user. Nodes already detached by the earlier version of this + // migration are repaired by the recovery migration below. // Fixes: https://github.com/juanfont/headscale/issues/3077 + // Fixes: https://github.com/juanfont/headscale/issues/3323 ID: "202602201200-clear-tagged-node-user-id", Migrate: func(tx *gorm.DB) error { err := tx.Exec(` UPDATE nodes SET user_id = NULL -WHERE tags IS NOT NULL AND tags != '[]' AND tags != ''; +WHERE tags IS NOT NULL AND tags != '[]' AND tags != '' AND tags != 'null'; `).Error if err != nil { return fmt.Errorf("clearing user_id on tagged nodes: %w", err) @@ -744,6 +751,36 @@ WHERE expiry IS NOT NULL AND expiry < '1900-01-01'; }, Rollback: func(db *gorm.DB) error { return nil }, }, + { + // Recover user_id on untagged nodes detached by the earlier + // version of 202602201200-clear-tagged-node-user-id, which + // treated tags='null' as tagged and cleared the user. This + // repairs databases that already upgraded to 0.29.0; fresh + // upgrades are protected by the fixed migration above and find + // nothing to repair. Recovery is best-effort: the owner is + // re-derived from the node's pre-auth key, so nodes registered + // via CLI/OIDC (no pre-auth key) cannot be recovered and must + // be reassigned manually. + // Fixes: https://github.com/juanfont/headscale/issues/3323 + ID: "202606181200-recover-null-tags-node-user-id", + Migrate: func(tx *gorm.DB) error { + err := tx.Exec(` +UPDATE nodes +SET user_id = ( + SELECT pak.user_id FROM pre_auth_keys pak WHERE pak.id = nodes.auth_key_id +) +WHERE user_id IS NULL + AND auth_key_id IS NOT NULL + AND (tags IS NULL OR tags = '' OR tags = '[]' OR tags = 'null'); + `).Error + if err != nil { + return fmt.Errorf("recovering user_id on untagged nodes: %w", err) + } + + return nil + }, + Rollback: func(db *gorm.DB) error { return nil }, + }, }, ) diff --git a/hscontrol/db/db_test.go b/hscontrol/db/db_test.go index 60abcfa07..98feef09f 100644 --- a/hscontrol/db/db_test.go +++ b/hscontrol/db/db_test.go @@ -198,6 +198,103 @@ func TestSQLiteMigrationAndDataValidation(t *testing.T) { assert.False(t, node5.IsExpired(), "node5 should not be reported as expired") }, }, + // Test for the clear-tagged-node-user-id migration + // (202602201200-clear-tagged-node-user-id). A nil tags slice + // marshals to the JSON literal 'null', so untagged nodes can carry + // tags='null' in the database. The migration must only clear + // user_id on genuinely tagged nodes, not on these untagged ones. + // Fixes: https://github.com/juanfont/headscale/issues/3323 + { + dbPath: "testdata/sqlite/null_tags_user_id_migration_test.sql", + wantFunc: func(t *testing.T, hsdb *HSDatabase) { + t.Helper() + + nodes, err := Read(hsdb.DB, func(rx *gorm.DB) (types.Nodes, error) { + return ListNodes(rx) + }) + require.NoError(t, err) + require.Len(t, nodes, 4, "should have all 4 nodes") + + byHostname := make(map[string]*types.Node, len(nodes)) + for _, n := range nodes { + byHostname[n.Hostname] = n + } + + // Node 1 had tags='null' (untagged) and belonged to user2. + // The migration must NOT clear its user_id. + node1 := byHostname["node1"] + require.NotNil(t, node1, "node1 should exist") + assert.False(t, node1.IsTagged(), "node1 with tags='null' should be untagged") + require.NotNil(t, node1.UserID, "node1 should keep its user assigned") + assert.Equal(t, uint(2), *node1.UserID, "node1 should still belong to user2") + + // Node 2 is genuinely tagged; user_id must be cleared. + node2 := byHostname["node2"] + require.NotNil(t, node2, "node2 should exist") + assert.True(t, node2.IsTagged(), "node2 should be tagged") + assert.Nil(t, node2.UserID, "node2 (tagged) should have user_id cleared") + + // Node 3 had tags='[]' (untagged); user_id preserved. + node3 := byHostname["node3"] + require.NotNil(t, node3, "node3 should exist") + assert.False(t, node3.IsTagged(), "node3 with tags='[]' should be untagged") + require.NotNil(t, node3.UserID, "node3 should keep its user assigned") + assert.Equal(t, uint(1), *node3.UserID, "node3 should still belong to user1") + + // Node 4 had tags='' (untagged); user_id preserved. + node4 := byHostname["node4"] + require.NotNil(t, node4, "node4 should exist") + assert.False(t, node4.IsTagged(), "node4 with tags='' should be untagged") + require.NotNil(t, node4.UserID, "node4 should keep its user assigned") + assert.Equal(t, uint(1), *node4.UserID, "node4 should still belong to user1") + }, + }, + // Test for the null-tags user_id recovery migration. Databases that + // already upgraded to 0.29.0 had user_id wrongly cleared on untagged + // nodes with tags='null'. The recovery migration re-derives user_id + // from the node's pre-auth key where one exists. + // Fixes: https://github.com/juanfont/headscale/issues/3323 + { + dbPath: "testdata/sqlite/recover_null_tags_user_id_migration_test.sql", + wantFunc: func(t *testing.T, hsdb *HSDatabase) { + t.Helper() + + nodes, err := Read(hsdb.DB, func(rx *gorm.DB) (types.Nodes, error) { + return ListNodes(rx) + }) + require.NoError(t, err) + require.Len(t, nodes, 4, "should have all 4 nodes") + + byHostname := make(map[string]*types.Node, len(nodes)) + for _, n := range nodes { + byHostname[n.Hostname] = n + } + + // Node 1: authkey-registered, orphaned by the bug. The recovery + // migration restores user_id from its pre-auth key (user2). + node1 := byHostname["node1"] + require.NotNil(t, node1, "node1 should exist") + require.NotNil(t, node1.UserID, "node1 user_id should be recovered") + assert.Equal(t, uint(2), *node1.UserID, "node1 should be recovered to user2") + + // Node 2: genuinely tagged, correctly cleared. Must stay cleared. + node2 := byHostname["node2"] + require.NotNil(t, node2, "node2 should exist") + assert.True(t, node2.IsTagged(), "node2 should be tagged") + assert.Nil(t, node2.UserID, "node2 (tagged) must remain cleared") + + // Node 3: CLI-registered, no pre-auth key. Unrecoverable. + node3 := byHostname["node3"] + require.NotNil(t, node3, "node3 should exist") + assert.Nil(t, node3.UserID, "node3 has no pre-auth key to recover from") + + // Node 4: never orphaned; user_id must be untouched. + node4 := byHostname["node4"] + require.NotNil(t, node4, "node4 should exist") + require.NotNil(t, node4.UserID, "node4 user_id should be untouched") + assert.Equal(t, uint(1), *node4.UserID, "node4 should still belong to user1") + }, + }, } for _, tt := range tests { diff --git a/hscontrol/db/testdata/sqlite/null_tags_user_id_migration_test.sql b/hscontrol/db/testdata/sqlite/null_tags_user_id_migration_test.sql new file mode 100644 index 000000000..c45d0ff59 --- /dev/null +++ b/hscontrol/db/testdata/sqlite/null_tags_user_id_migration_test.sql @@ -0,0 +1,85 @@ +-- Test SQL dump for the clear-tagged-node-user-id migration +-- (202602201200-clear-tagged-node-user-id) against nodes whose tags +-- column holds the JSON literal 'null'. +-- +-- A nil Strings slice marshals to the JSON literal `null`, so pre-0.29 +-- databases contain untagged nodes with tags='null'. The migration's +-- WHERE clause (tags IS NOT NULL AND tags != '[]' AND tags != '') treats +-- the 4-character string 'null' as "tagged" and wrongly clears user_id, +-- detaching the node from its owning user on upgrade. +-- Fixes: https://github.com/juanfont/headscale/issues/3323 + +PRAGMA foreign_keys=OFF; +BEGIN TRANSACTION; + +-- Migrations table: every entry BEFORE clear-tagged-node-user-id has been +-- applied. That migration is intentionally absent so it runs against this dump. +CREATE TABLE `migrations` (`id` text,PRIMARY KEY (`id`)); +INSERT INTO migrations VALUES('202312101416'); +INSERT INTO migrations VALUES('202312101430'); +INSERT INTO migrations VALUES('202402151347'); +INSERT INTO migrations VALUES('2024041121742'); +INSERT INTO migrations VALUES('202406021630'); +INSERT INTO migrations VALUES('202409271400'); +INSERT INTO migrations VALUES('202407191627'); +INSERT INTO migrations VALUES('202408181235'); +INSERT INTO migrations VALUES('202501221827'); +INSERT INTO migrations VALUES('202501311657'); +INSERT INTO migrations VALUES('202502070949'); +INSERT INTO migrations VALUES('202502131714'); +INSERT INTO migrations VALUES('202502171819'); +INSERT INTO migrations VALUES('202505091439'); +INSERT INTO migrations VALUES('202505141324'); +INSERT INTO migrations VALUES('202507021200'); +INSERT INTO migrations VALUES('202510311551'); +INSERT INTO migrations VALUES('202511101554-drop-old-idx'); +INSERT INTO migrations VALUES('202511011637-preauthkey-bcrypt'); +INSERT INTO migrations VALUES('202511122344-remove-newline-index'); +INSERT INTO migrations VALUES('202511131445-node-forced-tags-to-tags'); +INSERT INTO migrations VALUES('202601121700-migrate-hostinfo-request-tags'); + +-- Users table +CREATE TABLE `users` (`id` integer PRIMARY KEY AUTOINCREMENT,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime,`name` text,`display_name` text,`email` text,`provider_identifier` text,`provider` text,`profile_pic_url` text); +INSERT INTO users VALUES(1,'2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL,'user1','User One','user1@example.com',NULL,NULL,NULL); +INSERT INTO users VALUES(2,'2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL,'user2','User Two','user2@example.com',NULL,NULL,NULL); + +-- Pre-auth keys table +CREATE TABLE `pre_auth_keys` (`id` integer PRIMARY KEY AUTOINCREMENT,`key` text,`user_id` integer,`reusable` numeric,`ephemeral` numeric DEFAULT false,`used` numeric DEFAULT false,`tags` text,`created_at` datetime,`expiration` datetime,`prefix` text,`hash` blob,CONSTRAINT `fk_pre_auth_keys_user` FOREIGN KEY (`user_id`) REFERENCES `users`(`id`) ON DELETE SET NULL); + +-- API keys table +CREATE TABLE `api_keys` (`id` integer PRIMARY KEY AUTOINCREMENT,`prefix` text,`hash` blob,`created_at` datetime,`expiration` datetime,`last_seen` datetime); + +-- Nodes table - current schema (after the tags rename + last_seen/expiry reordering) +CREATE TABLE IF NOT EXISTS "nodes" (`id` integer PRIMARY KEY AUTOINCREMENT,`machine_key` text,`node_key` text,`disco_key` text,`endpoints` text,`host_info` text,`ipv4` text,`ipv6` text,`hostname` text,`given_name` varchar(63),`user_id` integer,`register_method` text,`tags` text,`auth_key_id` integer,`last_seen` datetime,`expiry` datetime,`approved_routes` text,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime,CONSTRAINT `fk_nodes_user` FOREIGN KEY (`user_id`) REFERENCES `users`(`id`) ON DELETE CASCADE,CONSTRAINT `fk_nodes_auth_key` FOREIGN KEY (`auth_key_id`) REFERENCES `pre_auth_keys`(`id`)); + +-- Node 1: tags='null' (untagged, nil slice marshalled to JSON null), owned by user2. +-- After migration: user_id MUST be preserved (this is the bug). +INSERT INTO nodes VALUES(1,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e01','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605501','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57701','[]','{}','100.64.0.1','fd7a:115c:a1e0::1','node1','node1',2,'cli','null',NULL,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Node 2: genuinely tagged, owned by user1. +-- After migration: user_id MUST be cleared to NULL (tagged nodes are owned by tags). +INSERT INTO nodes VALUES(2,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e02','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605502','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57702','[]','{}','100.64.0.2','fd7a:115c:a1e0::2','node2','node2',1,'cli','["tag:server"]',NULL,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Node 3: empty-array tags (untagged), owned by user1. +-- After migration: user_id MUST be preserved. +INSERT INTO nodes VALUES(3,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e03','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605503','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57703','[]','{}','100.64.0.3','fd7a:115c:a1e0::3','node3','node3',1,'cli','[]',NULL,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Node 4: empty-string tags (untagged), owned by user1. +-- After migration: user_id MUST be preserved. +INSERT INTO nodes VALUES(4,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e04','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605504','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57704','[]','{}','100.64.0.4','fd7a:115c:a1e0::4','node4','node4',1,'cli','',NULL,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Policies table (empty) +CREATE TABLE `policies` (`id` integer PRIMARY KEY AUTOINCREMENT,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime,`data` text); + +DELETE FROM sqlite_sequence; +INSERT INTO sqlite_sequence VALUES('users',2); +INSERT INTO sqlite_sequence VALUES('nodes',4); +CREATE INDEX idx_users_deleted_at ON users(deleted_at); +CREATE UNIQUE INDEX idx_api_keys_prefix ON api_keys(prefix); +CREATE INDEX idx_policies_deleted_at ON policies(deleted_at); +CREATE UNIQUE INDEX idx_provider_identifier ON users(provider_identifier) WHERE provider_identifier IS NOT NULL; +CREATE UNIQUE INDEX idx_name_provider_identifier ON users(name, provider_identifier); +CREATE UNIQUE INDEX idx_name_no_provider_identifier ON users(name) WHERE provider_identifier IS NULL; +CREATE UNIQUE INDEX IF NOT EXISTS idx_pre_auth_keys_prefix ON pre_auth_keys(prefix) WHERE prefix IS NOT NULL AND prefix != ''; + +COMMIT; diff --git a/hscontrol/db/testdata/sqlite/recover_null_tags_user_id_migration_test.sql b/hscontrol/db/testdata/sqlite/recover_null_tags_user_id_migration_test.sql new file mode 100644 index 000000000..163dcd352 --- /dev/null +++ b/hscontrol/db/testdata/sqlite/recover_null_tags_user_id_migration_test.sql @@ -0,0 +1,87 @@ +-- Test SQL dump for the null-tags user_id RECOVERY migration. +-- +-- Represents a database that already upgraded to 0.29.0, where the buggy +-- clear-tagged-node-user-id migration (202602201200) already cleared +-- user_id on untagged nodes whose tags column held 'null'. The recovery +-- migration runs against this state and re-derives user_id from the node's +-- pre-auth key where possible. +-- Fixes: https://github.com/juanfont/headscale/issues/3323 + +PRAGMA foreign_keys=OFF; +BEGIN TRANSACTION; + +-- Migrations table: everything through the current last migration has been +-- applied (this DB already ran the buggy clear-tagged migration). The new +-- recovery migration is intentionally absent so it runs against this dump. +CREATE TABLE `migrations` (`id` text,PRIMARY KEY (`id`)); +INSERT INTO migrations VALUES('202312101416'); +INSERT INTO migrations VALUES('202312101430'); +INSERT INTO migrations VALUES('202402151347'); +INSERT INTO migrations VALUES('2024041121742'); +INSERT INTO migrations VALUES('202406021630'); +INSERT INTO migrations VALUES('202409271400'); +INSERT INTO migrations VALUES('202407191627'); +INSERT INTO migrations VALUES('202408181235'); +INSERT INTO migrations VALUES('202501221827'); +INSERT INTO migrations VALUES('202501311657'); +INSERT INTO migrations VALUES('202502070949'); +INSERT INTO migrations VALUES('202502131714'); +INSERT INTO migrations VALUES('202502171819'); +INSERT INTO migrations VALUES('202505091439'); +INSERT INTO migrations VALUES('202505141324'); +INSERT INTO migrations VALUES('202507021200'); +INSERT INTO migrations VALUES('202510311551'); +INSERT INTO migrations VALUES('202511101554-drop-old-idx'); +INSERT INTO migrations VALUES('202511011637-preauthkey-bcrypt'); +INSERT INTO migrations VALUES('202511122344-remove-newline-index'); +INSERT INTO migrations VALUES('202511131445-node-forced-tags-to-tags'); +INSERT INTO migrations VALUES('202601121700-migrate-hostinfo-request-tags'); +INSERT INTO migrations VALUES('202602201200-clear-tagged-node-user-id'); +INSERT INTO migrations VALUES('202605221435-clear-zero-time-node-expiry'); + +-- Users table +CREATE TABLE `users` (`id` integer PRIMARY KEY AUTOINCREMENT,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime,`name` text,`display_name` text,`email` text,`provider_identifier` text,`provider` text,`profile_pic_url` text); +INSERT INTO users VALUES(1,'2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL,'user1','User One','user1@example.com',NULL,NULL,NULL); +INSERT INTO users VALUES(2,'2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL,'user2','User Two','user2@example.com',NULL,NULL,NULL); + +-- Pre-auth keys table. Key 1 belongs to user2, key 2 to user1. +CREATE TABLE `pre_auth_keys` (`id` integer PRIMARY KEY AUTOINCREMENT,`key` text,`user_id` integer,`reusable` numeric,`ephemeral` numeric DEFAULT false,`used` numeric DEFAULT false,`tags` text,`created_at` datetime,`expiration` datetime,`prefix` text,`hash` blob,CONSTRAINT `fk_pre_auth_keys_user` FOREIGN KEY (`user_id`) REFERENCES `users`(`id`) ON DELETE SET NULL); +INSERT INTO pre_auth_keys VALUES(1,NULL,2,1,false,true,NULL,'2024-01-01 00:00:00+00:00',NULL,'pak1',NULL); +INSERT INTO pre_auth_keys VALUES(2,NULL,1,1,false,true,NULL,'2024-01-01 00:00:00+00:00',NULL,'pak2',NULL); + +-- API keys table +CREATE TABLE `api_keys` (`id` integer PRIMARY KEY AUTOINCREMENT,`prefix` text,`hash` blob,`created_at` datetime,`expiration` datetime,`last_seen` datetime); + +-- Nodes table +CREATE TABLE IF NOT EXISTS "nodes" (`id` integer PRIMARY KEY AUTOINCREMENT,`machine_key` text,`node_key` text,`disco_key` text,`endpoints` text,`host_info` text,`ipv4` text,`ipv6` text,`hostname` text,`given_name` varchar(63),`user_id` integer,`register_method` text,`tags` text,`auth_key_id` integer,`last_seen` datetime,`expiry` datetime,`approved_routes` text,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime,CONSTRAINT `fk_nodes_user` FOREIGN KEY (`user_id`) REFERENCES `users`(`id`) ON DELETE CASCADE,CONSTRAINT `fk_nodes_auth_key` FOREIGN KEY (`auth_key_id`) REFERENCES `pre_auth_keys`(`id`)); + +-- Node 1: authkey-registered, tags='null', already orphaned (user_id NULL) by +-- the buggy migration. auth_key_id=1 (user2). Recovery: user_id -> 2. +INSERT INTO nodes VALUES(1,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e01','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605501','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57701','[]','{}','100.64.0.1','fd7a:115c:a1e0::1','node1','node1',NULL,'authkey','null',1,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Node 2: genuinely tagged, user_id correctly cleared. Must stay NULL. +INSERT INTO nodes VALUES(2,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e02','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605502','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57702','[]','{}','100.64.0.2','fd7a:115c:a1e0::2','node2','node2',NULL,'authkey','["tag:server"]',2,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Node 3: CLI-registered, tags='null', orphaned, no auth_key_id. +-- Unrecoverable: must stay NULL. +INSERT INTO nodes VALUES(3,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e03','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605503','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57703','[]','{}','100.64.0.3','fd7a:115c:a1e0::3','node3','node3',NULL,'cli','null',NULL,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Node 4: authkey-registered, untouched (user_id still set). Must stay user1. +INSERT INTO nodes VALUES(4,'mkey:a0ab77456320823945ae0331823e3c0d516fae9585bd42698dfa1ac3d7679e04','nodekey:7c84167ab68f494942de14deb83587fd841843de2bac105b6c670048c1605504','discokey:53075b3c6cad3b62a2a29caea61beeb93f66b8c75cb89dac465236a5bbf57704','[]','{}','100.64.0.4','fd7a:115c:a1e0::4','node4','node4',1,'authkey','null',2,'2024-01-01 00:00:00+00:00',NULL,'[]','2024-01-01 00:00:00+00:00','2024-01-01 00:00:00+00:00',NULL); + +-- Policies table (empty) +CREATE TABLE `policies` (`id` integer PRIMARY KEY AUTOINCREMENT,`created_at` datetime,`updated_at` datetime,`deleted_at` datetime,`data` text); + +DELETE FROM sqlite_sequence; +INSERT INTO sqlite_sequence VALUES('users',2); +INSERT INTO sqlite_sequence VALUES('pre_auth_keys',2); +INSERT INTO sqlite_sequence VALUES('nodes',4); +CREATE INDEX idx_users_deleted_at ON users(deleted_at); +CREATE UNIQUE INDEX idx_api_keys_prefix ON api_keys(prefix); +CREATE INDEX idx_policies_deleted_at ON policies(deleted_at); +CREATE UNIQUE INDEX idx_provider_identifier ON users(provider_identifier) WHERE provider_identifier IS NOT NULL; +CREATE UNIQUE INDEX idx_name_provider_identifier ON users(name, provider_identifier); +CREATE UNIQUE INDEX idx_name_no_provider_identifier ON users(name) WHERE provider_identifier IS NULL; +CREATE UNIQUE INDEX IF NOT EXISTS idx_pre_auth_keys_prefix ON pre_auth_keys(prefix) WHERE prefix IS NOT NULL AND prefix != ''; + +COMMIT; diff --git a/mkdocs.yml b/mkdocs.yml index 18b6fc932..1d08b8330 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -111,7 +111,7 @@ extra: - icon: fontawesome/brands/discord link: https://discord.gg/c84AZQhmpx headscale: - version: 0.29.0 + version: 0.29.1 # Extensions markdown_extensions: From d4f2acf3ab3cada875ab88eab4e0f81e4118cd12 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 09:40:21 +0000 Subject: [PATCH 02/15] policy: take RLock for reads so map generation runs concurrently One exclusive mutex serialized every policy read, so a mass reconnect on autogroup:self/via/relay policies stalled clients into "unexpected EOF" retries. Per-node caches become xsync.Maps for lazy population under RLock. Fixes #3346 (cherry picked from commit 6f317c7576474c5d19c799f4e225d66d2b597e50) --- CHANGELOG.md | 8 ++ hscontrol/policy/v2/policy.go | 136 ++++++++++++++++------------- hscontrol/policy/v2/policy_test.go | 20 +++-- hscontrol/policy/v2/sshtest.go | 4 +- hscontrol/policy/v2/test.go | 4 +- 5 files changed, 98 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6630e6c3a..406f65d4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ **Minimum supported Tailscale client version: v1.xx.0** +## 0.29.2 (202x-xx-xx) + +**Minimum supported Tailscale client version: v1.80.0** + +### Changes + +- Fix map generation serializing on the policy lock, so a mass reconnect on `autogroup:self`, via or relay policies no longer stalls clients into `unexpected EOF` retry loops [#3358](https://github.com/juanfont/headscale/pull/3358) + ## 0.29.1 (2026-06-18) **Minimum supported Tailscale client version: v1.80.0** diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index a7b55a2a6..a98d0191e 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -15,6 +15,7 @@ import ( "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/policy/policyutil" "github.com/juanfont/headscale/hscontrol/types" + "github.com/puzpuzpuz/xsync/v4" "github.com/rs/zerolog/log" "go4.org/netipx" "tailscale.com/net/tsaddr" @@ -28,7 +29,10 @@ import ( var ErrInvalidTagOwner = errors.New("tag owner is not an Alias") type PolicyManager struct { - mu sync.Mutex + // RWMutex, not Mutex, so concurrent map generation does not serialise on + // reads. The per-node caches are xsync.Maps so a read can fill them without + // taking the write lock. + mu sync.RWMutex pol *Policy users []types.User nodes views.Slice[types.NodeView] @@ -55,7 +59,7 @@ type PolicyManager struct { viaTargetTags map[Tag]struct{} // Lazy map of SSH policies - sshPolicyMap map[types.NodeID]*tailcfg.SSHPolicy + sshPolicyMap *xsync.Map[types.NodeID, *tailcfg.SSHPolicy] // compiledGrants are the grants with sources pre-resolved. // The single source of truth for filter compilation. Both @@ -64,12 +68,12 @@ type PolicyManager struct { userNodeIdx userNodeIndex // Lazy map of per-node filter rules (reduced, for packet filters) - filterRulesMap map[types.NodeID][]tailcfg.FilterRule + filterRulesMap *xsync.Map[types.NodeID, []tailcfg.FilterRule] // Lazy map of per-node matchers derived from UNREDUCED filter // rules. Only populated on the slow path when needsPerNodeFilter // is true; the fast path returns pm.matchers directly. - matchersForNodeMap map[types.NodeID][]matcher.Match + matchersForNodeMap *xsync.Map[types.NodeID, []matcher.Match] // needsPerNodeFilter is true when any compiled grant requires // per-node work (autogroup:self or via grants). @@ -197,9 +201,9 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node pol: policy, users: users, nodes: nodes, - sshPolicyMap: make(map[types.NodeID]*tailcfg.SSHPolicy, nodes.Len()), - filterRulesMap: make(map[types.NodeID][]tailcfg.FilterRule, nodes.Len()), - matchersForNodeMap: make(map[types.NodeID][]matcher.Match, nodes.Len()), + sshPolicyMap: xsync.NewMap[types.NodeID, *tailcfg.SSHPolicy](), + filterRulesMap: xsync.NewMap[types.NodeID, []tailcfg.FilterRule](), + matchersForNodeMap: xsync.NewMap[types.NodeID, []matcher.Match](), } _, err = pm.updateLocked() @@ -354,9 +358,9 @@ func (pm *PolicyManager) updateLocked() (bool, error) { // TODO(kradalby): This could potentially be optimized by only clearing the // policies for nodes that have changed. Particularly if the only difference is // that nodes has been added or removed. - clear(pm.sshPolicyMap) - clear(pm.filterRulesMap) - clear(pm.matchersForNodeMap) + pm.sshPolicyMap.Clear() + pm.filterRulesMap.Clear() + pm.matchersForNodeMap.Clear() } // If nothing changed, no need to update nodes @@ -400,8 +404,8 @@ func (pm *PolicyManager) NodeNeedsPeerRecompute(node types.NodeView) bool { return true } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() if pm.relayTargetIPs != nil && node.InIPSet(pm.relayTargetIPs) { return true @@ -422,10 +426,10 @@ func (pm *PolicyManager) NodeNeedsPeerRecompute(node types.NodeView) bool { // /machine/ssh/action/{src}/to/{dst}?local_user={local_user} per the // SaaS wire format. Cache is invalidated on policy reload. func (pm *PolicyManager) SSHPolicy(baseURL string, node types.NodeView) (*tailcfg.SSHPolicy, error) { - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() - if sshPol, ok := pm.sshPolicyMap[node.ID()]; ok { + if sshPol, ok := pm.sshPolicyMap.Load(node.ID()); ok { return sshPol, nil } @@ -434,7 +438,7 @@ func (pm *PolicyManager) SSHPolicy(baseURL string, node types.NodeView) (*tailcf return nil, fmt.Errorf("compiling SSH policy: %w", err) } - pm.sshPolicyMap[node.ID()] = sshPol + pm.sshPolicyMap.Store(node.ID(), sshPol) return sshPol, nil } @@ -450,8 +454,8 @@ func (pm *PolicyManager) SSHPolicy(baseURL string, node types.NodeView) (*tailcf func (pm *PolicyManager) SSHCheckParams( srcNodeID, dstNodeID types.NodeID, ) (time.Duration, bool) { - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() if pm.pol == nil || len(pm.pol.SSHs) == 0 { return 0, false @@ -584,8 +588,8 @@ func (pm *PolicyManager) Filter() ([]tailcfg.FilterRule, []matcher.Match) { return nil, nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() return pm.filter, pm.matchers } @@ -604,8 +608,8 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ return nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() // Precompute each node's subnet routes and exit-node status once; the // O(n^2) pair scans below would otherwise recompute them for every pair. @@ -726,7 +730,7 @@ func (pm *PolicyManager) filterForNodeLocked( return nil } - if rules, ok := pm.filterRulesMap[node.ID()]; ok { + if rules, ok := pm.filterRulesMap.Load(node.ID()); ok { return rules } @@ -738,7 +742,7 @@ func (pm *PolicyManager) filterForNodeLocked( } reduced := policyutil.ReduceFilterRules(node, unreduced) - pm.filterRulesMap[node.ID()] = reduced + pm.filterRulesMap.Store(node.ID(), reduced) return reduced } @@ -755,8 +759,8 @@ func (pm *PolicyManager) FilterForNode(node types.NodeView) ([]tailcfg.FilterRul return nil, nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() return pm.filterForNodeLocked(node), nil } @@ -776,8 +780,8 @@ func (pm *PolicyManager) MatchersForNode(node types.NodeView) ([]matcher.Match, return nil, nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() // For global policies, return the shared global matchers. // Via grants require per-node matchers because the global matchers @@ -786,7 +790,7 @@ func (pm *PolicyManager) MatchersForNode(node types.NodeView) ([]matcher.Match, return pm.matchers, nil } - if cached, ok := pm.matchersForNodeMap[node.ID()]; ok { + if cached, ok := pm.matchersForNodeMap.Load(node.ID()); ok { return cached, nil } @@ -794,7 +798,7 @@ func (pm *PolicyManager) MatchersForNode(node types.NodeView) ([]matcher.Match, // the stored compiled grants for this specific node. unreduced := pm.filterRulesForNodeLocked(node) matchers := matcher.MatchesFromFilterRules(unreduced) - pm.matchersForNodeMap[node.ID()] = matchers + pm.matchersForNodeMap.Store(node.ID(), matchers) return matchers, nil } @@ -813,7 +817,7 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, error) { // Clear SSH policy map when users change to force SSH policy recomputation // This ensures that if SSH policy compilation previously failed due to missing users, // it will be retried with the new user list - clear(pm.sshPolicyMap) + pm.sshPolicyMap.Clear() changed, err := pm.updateLocked() if err != nil { @@ -866,9 +870,9 @@ func (pm *PolicyManager) SetNodes(nodes views.Slice[types.NodeView]) (bool, erro if !needsUpdate { // This ensures fresh filter rules are generated for all nodes - clear(pm.sshPolicyMap) - clear(pm.filterRulesMap) - clear(pm.matchersForNodeMap) + pm.sshPolicyMap.Clear() + pm.filterRulesMap.Clear() + pm.matchersForNodeMap.Clear() } // Always return true when nodes changed, even if filter hash didn't change // (can happen with autogroup:self or when nodes are added but don't affect rules) @@ -921,8 +925,8 @@ func (pm *PolicyManager) NodeCanHaveTag(node types.NodeView, tag string) bool { return false } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() // pm.pol is written by SetPolicy under pm.mu; reading it before the // lock races with concurrent policy reloads. @@ -1010,8 +1014,8 @@ func (pm *PolicyManager) TagExists(tag string) bool { return false } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() // pm.pol is written by SetPolicy under pm.mu; reading it before the // lock races with concurrent policy reloads. @@ -1029,8 +1033,8 @@ func (pm *PolicyManager) NodeCanApproveRoute(node types.NodeView, route netip.Pr return false } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() // If the route to-be-approved is an exit route, then we need to check // if the node is in allowed to approve it. This is treated differently @@ -1092,8 +1096,8 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via return result } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() // pm.pol is written by SetPolicy under pm.mu; reading it before the // lock races with concurrent policy reloads. @@ -1311,8 +1315,8 @@ func (pm *PolicyManager) DebugString() string { // pm.pol, filter, matchers, and the derived maps are all written // under pm.mu by SetPolicy/SetUsers/SetNodes. - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() var sb strings.Builder @@ -1475,7 +1479,7 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S // Clear cache entries for affected users only. // For autogroup:self, we need to clear all nodes belonging to affected users // because autogroup:self rules depend on the entire user's device set. - for nodeID := range pm.filterRulesMap { + pm.filterRulesMap.Range(func(nodeID types.NodeID, _ []tailcfg.FilterRule) bool { // Find the user for this cached node var nodeUserID types.UserID @@ -1518,20 +1522,22 @@ func (pm *PolicyManager) invalidateAutogroupSelfCache(oldNodes, newNodes views.S // If we found the user and they're affected, clear this cache entry if found { if _, affected := affectedUsers[nodeUserID]; affected { - delete(pm.filterRulesMap, nodeID) - delete(pm.matchersForNodeMap, nodeID) + pm.filterRulesMap.Delete(nodeID) + pm.matchersForNodeMap.Delete(nodeID) } } else { // Node not found in either old or new list, clear it - delete(pm.filterRulesMap, nodeID) - delete(pm.matchersForNodeMap, nodeID) + pm.filterRulesMap.Delete(nodeID) + pm.matchersForNodeMap.Delete(nodeID) } - } + + return true + }) if len(affectedUsers) > 0 { log.Debug(). Int("affected_users", len(affectedUsers)). - Int("remaining_cache_entries", len(pm.filterRulesMap)). + Int("remaining_cache_entries", pm.filterRulesMap.Size()). Msg("Selectively cleared autogroup:self cache for affected users") } } @@ -1572,23 +1578,27 @@ func (pm *PolicyManager) invalidateGlobalPolicyCache(newNodes views.Slice[types. } if newNode.HasNetworkChanges(oldNode) { - delete(pm.filterRulesMap, nodeID) - delete(pm.matchersForNodeMap, nodeID) + pm.filterRulesMap.Delete(nodeID) + pm.matchersForNodeMap.Delete(nodeID) } } // Remove deleted nodes from cache - for nodeID := range pm.filterRulesMap { + pm.filterRulesMap.Range(func(nodeID types.NodeID, _ []tailcfg.FilterRule) bool { if _, exists := newNodeMap[nodeID]; !exists { - delete(pm.filterRulesMap, nodeID) + pm.filterRulesMap.Delete(nodeID) } - } - for nodeID := range pm.matchersForNodeMap { + return true + }) + + pm.matchersForNodeMap.Range(func(nodeID types.NodeID, _ []matcher.Match) bool { if _, exists := newNodeMap[nodeID]; !exists { - delete(pm.matchersForNodeMap, nodeID) + pm.matchersForNodeMap.Delete(nodeID) } - } + + return true + }) } // flattenTags resolves nested tag-owner references. Cycles @@ -1770,8 +1780,8 @@ func (pm *PolicyManager) NodeCapMap(id types.NodeID) tailcfg.NodeCapMap { return nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() src := pm.nodeAttrsMap[id] if len(src) == 0 { @@ -1794,8 +1804,8 @@ func (pm *PolicyManager) NodeCapMaps() map[types.NodeID]tailcfg.NodeCapMap { return nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() out := make(map[types.NodeID]tailcfg.NodeCapMap, len(pm.nodeAttrsMap)) maps.Copy(out, pm.nodeAttrsMap) diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 9237f33fe..43febe1a3 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/types" + "github.com/puzpuzpuz/xsync/v4" "github.com/stretchr/testify/require" "gorm.io/gorm" "tailscale.com/net/tsaddr" @@ -122,7 +123,7 @@ func TestInvalidateAutogroupSelfCache(t *testing.T) { require.NoError(t, err) } - require.Len(t, pm.filterRulesMap, len(initialNodes)) + require.Equal(t, len(initialNodes), pm.filterRulesMap.Size()) tests := []struct { name string @@ -207,19 +208,20 @@ func TestInvalidateAutogroupSelfCache(t *testing.T) { } } - pm.filterRulesMap = make(map[types.NodeID][]tailcfg.FilterRule) + pm.filterRulesMap.Clear() + for _, n := range initialNodes { _, err := pm.FilterForNode(n.View()) require.NoError(t, err) } - initialCacheSize := len(pm.filterRulesMap) + initialCacheSize := pm.filterRulesMap.Size() require.Equal(t, len(initialNodes), initialCacheSize) pm.invalidateAutogroupSelfCache(initialNodes.ViewSlice(), tt.newNodes.ViewSlice()) // Verify the expected number of cache entries were cleared - finalCacheSize := len(pm.filterRulesMap) + finalCacheSize := pm.filterRulesMap.Size() clearedEntries := initialCacheSize - finalCacheSize require.Equal(t, tt.expectedCleared, clearedEntries, tt.description) }) @@ -498,15 +500,19 @@ func TestInvalidateGlobalPolicyCache(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { pm := &PolicyManager{ - nodes: tt.oldNodes.ViewSlice(), - filterRulesMap: tt.initialCache, + nodes: tt.oldNodes.ViewSlice(), + filterRulesMap: xsync.NewMap[types.NodeID, []tailcfg.FilterRule](), + matchersForNodeMap: xsync.NewMap[types.NodeID, []matcher.Match](), + } + for id, rules := range tt.initialCache { + pm.filterRulesMap.Store(id, rules) } pm.invalidateGlobalPolicyCache(tt.newNodes.ViewSlice()) // Verify cache state for nodeID, shouldExist := range tt.expectedCacheAfter { - _, exists := pm.filterRulesMap[nodeID] + _, exists := pm.filterRulesMap.Load(nodeID) require.Equal(t, shouldExist, exists, "node %d cache existence mismatch", nodeID) } }) diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index d2b07a631..48089f7c7 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -128,8 +128,8 @@ func (pm *PolicyManager) RunSSHTests() error { return nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() cache := make(map[types.NodeID]*tailcfg.SSHPolicy) results := runSSHPolicyTests(pm.pol, pm.users, pm.nodes, cache) diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 5f1189f2e..e28c2ef6e 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -204,8 +204,8 @@ func (pm *PolicyManager) RunTests() error { return nil } - pm.mu.Lock() - defer pm.mu.Unlock() + pm.mu.RLock() + defer pm.mu.RUnlock() results := runPolicyTests(pm.pol, pm.filter, pm.users, pm.nodes) if results.AllPassed { From 9d1327458fb21b240ea8078e64aa09ed30b59a96 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 09:40:28 +0000 Subject: [PATCH 03/15] mapper,policy: add reconnect-storm and lock-concurrency regression tests TestInitialMapNotStarvedByReconnectStorm reproduces the #3346 stall; TestPolicyManagerConcurrentReads guards the RLock cache access under -race. Updates #3346 (cherry picked from commit d528686f14f309beedc9f2307ca108aad819a4d0) --- hscontrol/mapper/initialmap_storm_test.go | 189 ++++++++++++++++++ .../policy/v2/policy_concurrency_test.go | 103 ++++++++++ 2 files changed, 292 insertions(+) create mode 100644 hscontrol/mapper/initialmap_storm_test.go create mode 100644 hscontrol/policy/v2/policy_concurrency_test.go diff --git a/hscontrol/mapper/initialmap_storm_test.go b/hscontrol/mapper/initialmap_storm_test.go new file mode 100644 index 000000000..877a8d97b --- /dev/null +++ b/hscontrol/mapper/initialmap_storm_test.go @@ -0,0 +1,189 @@ +//go:build !race + +// This is a timing-sensitive performance regression test; the race detector's +// ~10x slowdown makes its wall-clock assertion meaningless, so it is excluded +// from -race builds. The concurrency correctness of the policy lock change it +// guards is covered under -race by TestPolicyManagerConcurrentReads in +// hscontrol/policy/v2. + +package mapper + +import ( + "net/netip" + "runtime" + "slices" + "sync" + "testing" + "time" + + "github.com/juanfont/headscale/hscontrol/db" + "github.com/juanfont/headscale/hscontrol/derp" + "github.com/juanfont/headscale/hscontrol/state" + "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" +) + +// setupStormBatcher builds a real state+batcher with production-default +// NodeStore batching so the reconnect-storm contention is realistic. It mirrors +// setupBatcherWithTestData but lets the test control BatcherWorkers and the +// policy. +func setupStormBatcher(tb testing.TB, nodeCount, workers int, policy string) (*TestData, func()) { + tb.Helper() + + tmpDir := tb.TempDir() + prefixV4 := netip.MustParsePrefix("100.64.0.0/10") + prefixV6 := netip.MustParsePrefix("fd7a:115c:a1e0::/48") + + cfg := &types.Config{ + Database: types.DatabaseConfig{ + Type: types.DatabaseSqlite, + Sqlite: types.SqliteConfig{Path: tmpDir + "/headscale_test.db"}, + }, + PrefixV4: &prefixV4, + PrefixV6: &prefixV6, + IPAllocation: types.IPAllocationStrategySequential, + BaseDomain: "headscale.test", + Policy: types.PolicyConfig{Mode: types.PolicyModeDB}, + DERP: types.DERPConfig{ + ServerEnabled: false, + DERPMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{999: {RegionID: 999}}, + }, + }, + Tuning: types.Tuning{ + BatchChangeDelay: 10 * time.Millisecond, + BatcherWorkers: workers, + // Production defaults: coalesce writes so the storm is not + // exaggerated by an unrealistically small NodeStore batch. + NodeStoreBatchSize: 100, + NodeStoreBatchTimeout: 500 * time.Millisecond, + }, + } + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(tb, err) + + users := database.CreateUsersForTest(1, "testuser") + dbNodes := database.CreateRegisteredNodesForTest(users[0], nodeCount, "node") + + allNodes := make([]node, 0, nodeCount) + for i := range dbNodes { + allNodes = append(allNodes, node{ + n: dbNodes[i], + ch: make(chan *tailcfg.MapResponse, normalBufferSize), + }) + } + + st, err := state.NewState(cfg) + require.NoError(tb, err) + + derpMap, err := derp.GetDERPMap(cfg.DERP) + require.NoError(tb, err) + st.SetDERPMap(derpMap) + + _, err = st.SetPolicy([]byte(policy)) + require.NoError(tb, err) + + batcher := wrapBatcherForTest(NewBatcherAndMapper(cfg, st), st) + batcher.Start() + + td := &TestData{ + Database: database, + Users: users, + Nodes: allNodes, + State: st, + Config: cfg, + Batcher: batcher, + } + + return td, func() { + batcher.Close() + st.Close() + database.Close() + } +} + +// TestInitialMapNotStarvedByReconnectStorm reproduces juanfont/headscale#3346. +// +// When every node redials at once (e.g. after a server upgrade restart), each +// connection writes the NodeStore (UpdateNodeFromMapRequest + Connect) and the +// batcher generates its initial map. All of that reads the policy through the +// PolicyManager. Before the fix the PolicyManager guarded every read with a +// single exclusive mutex, so the NodeStore writer's O(n^2) BuildPeerMap and +// every node's FilterForNode serialised against each other. On a per-node +// filter policy (autogroup:self, via, relay grants) each hold is expensive, so +// under the storm time-to-initial-map grew without bound. +// +// On the production server in #3346 this drove the batcher's per-node +// total.duration from ~4s to ~76s; tailscale clients aborted the map POST +// first and reported +// +// PollNetMap: Post ".../machine/map": unexpected EOF +// +// then redialled, feeding the storm so it never converged. An allow-all policy +// does NOT reproduce this — BuildPeerMap is cheap there; the per-node filter +// path is what makes it expensive, matching a real deployment's ACLs. +// +// The fix makes PolicyManager reads take a shared RLock so map generation runs +// concurrently. AddNode blocks until the initial map is generated and handed to +// the node channel, so its wall-clock duration is the time-to-initial-map the +// client experiences. Without the fix this test's slowest node takes ~10s+ at +// this scale (lock-bound, and more workers do not help); with it, generation +// parallelises across workers and stays well within a client's patience. +func TestInitialMapNotStarvedByReconnectStorm(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive storm regression; skipped in -short") + } + + const ( + nodeCount = 300 + + // A per-node-filter policy: forces BuildPeerMap and FilterForNode onto + // the slow path that recompiles filter rules per node, the same shape + // as a real ACL using autogroup:self / via / relay grants. + perNodeFilterPolicy = `{"acls":[{"action":"accept","src":["autogroup:member"],"dst":["autogroup:self:*"]}]}` + + // Deliberately roomy so it passes on CI's few-core runners, where the + // single-writer BuildPeerMap sets the floor (~10s) whatever the reads + // do. It still trips on a hang or a return to the ~76s serialised + // behaviour; the fine-grained concurrency is verified separately by + // TestPolicyManagerConcurrentReads under -race. + maxAcceptableLatency = 30 * time.Second + ) + + // Use the real available parallelism, as production does. + workers := runtime.NumCPU() + + td, cleanup := setupStormBatcher(t, nodeCount, workers, perNodeFilterPolicy) + defer cleanup() + + latencies := make([]time.Duration, nodeCount) + + var wg sync.WaitGroup + + for i := range td.Nodes { + wg.Go(func() { + n := &td.Nodes[i] + + start := time.Now() + err := td.Batcher.AddNode(n.n.ID, n.ch, tailcfg.CapabilityVersion(100), nil) + latencies[i] = time.Since(start) + + assert.NoError(t, err) //nolint:testifylint // assert (not require) is correct off the test goroutine + }) + } + + wg.Wait() + + slices.Sort(latencies) + p50 := latencies[len(latencies)/2] + p95 := latencies[len(latencies)*95/100] + maxLatency := latencies[len(latencies)-1] + t.Logf("initial-map latency over %d nodes (workers=%d): p50=%s p95=%s max=%s", + nodeCount, workers, p50, p95, maxLatency) + + require.Less(t, maxLatency, maxAcceptableLatency, + "slowest initial map took %s: policy reads are serialising instead of running concurrently (issue #3346)", maxLatency) +} diff --git a/hscontrol/policy/v2/policy_concurrency_test.go b/hscontrol/policy/v2/policy_concurrency_test.go new file mode 100644 index 000000000..b4b759f59 --- /dev/null +++ b/hscontrol/policy/v2/policy_concurrency_test.go @@ -0,0 +1,103 @@ +package v2 + +import ( + "fmt" + "sync" + "testing" + + "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +// TestPolicyManagerConcurrentReads is the correctness guard for the #3346 fix: +// PolicyManager read methods take a shared RLock and populate their per-node +// caches (filterRulesMap, matchersForNodeMap) concurrently. This test hammers +// those reads from many goroutines while a writer mutates the node set, so the +// race detector catches any unsafe access to the shared caches or policy state. +// +// It uses an autogroup:self policy so reads take the per-node filter slow path +// — the same path that made #3346's reconnect storm expensive — which is where +// the lazy caches are written. +func TestPolicyManagerConcurrentReads(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "user1", Email: "user1@headscale.net"}, + {Model: gorm.Model{ID: 2}, Name: "user2", Email: "user2@headscale.net"}, + {Model: gorm.Model{ID: 3}, Name: "user3", Email: "user3@headscale.net"}, + } + + policy := `{ + "acls": [ + { + "action": "accept", + "src": ["autogroup:member"], + "dst": ["autogroup:self:*"] + } + ] + }` + + const nodeCount = 60 + + nodes := make(types.Nodes, 0, nodeCount) + for i := range nodeCount { + n := node( + fmt.Sprintf("node%d", i), + fmt.Sprintf("100.64.0.%d", i+1), + fmt.Sprintf("fd7a:115c:a1e0::%d", i+1), + users[i%len(users)], + ) + n.ID = types.NodeID(i + 1) //nolint:gosec // safe in test + nodes = append(nodes, n) + } + + pm, err := NewPolicyManager([]byte(policy), users, nodes.ViewSlice()) + require.NoError(t, err) + + const ( + readers = 16 + iterations = 60 + mutatorReloads = 30 + ) + + var wg sync.WaitGroup + + // Concurrent readers exercise every converted RLock read path, including + // the two lazily populated per-node caches. Assertions inside the + // goroutines use assert (not require) so a failure does not call + // t.FailNow from a non-test goroutine. + for r := range readers { + wg.Go(func() { + for i := range iterations { + nv := nodes[(r+i)%len(nodes)].View() + + rules, err := pm.FilterForNode(nv) + assert.NoError(t, err) //nolint:testifylint // assert (not require) is correct off the test goroutine + assert.NotNil(t, rules) + + _, err = pm.MatchersForNode(nv) + assert.NoError(t, err) //nolint:testifylint // assert (not require) is correct off the test goroutine + + pm.Filter() + pm.NodeCapMap(nv.ID()) + + // BuildPeerMap is the O(n^2) writer-side read; exercise it + // under RLock too, but not every iteration. + if i%8 == 0 { + assert.NotNil(t, pm.BuildPeerMap(nodes.ViewSlice())) + } + } + }) + } + + // A writer repeatedly re-sets the node set, invalidating and racing the + // caches the readers are populating. + wg.Go(func() { + for range mutatorReloads { + _, err := pm.SetNodes(nodes.ViewSlice()) + assert.NoError(t, err) //nolint:testifylint // assert (not require) is correct off the test goroutine + } + }) + + wg.Wait() +} From ec6719736806e53f1aad1e219670bb8b79476052 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:19 +0000 Subject: [PATCH 04/15] mapper: skip peers with invalid names instead of failing the map A peer whose GivenName fails GetFQDN aborted the whole map for every node that could see it. Drop and log it; SSH policy errors degrade too. Fixes #3346 (cherry picked from commit 08956d51a45c52434b1dbc7aa3d33fd373fb0192) --- hscontrol/mapper/builder.go | 26 +++++++++- hscontrol/mapper/mapper_test.go | 89 +++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index ee3653ae4..e4db98f63 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -9,6 +9,8 @@ import ( "github.com/juanfont/headscale/hscontrol/policy" policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" + "github.com/rs/zerolog/log" "tailscale.com/tailcfg" "tailscale.com/types/views" "tailscale.com/util/multierr" @@ -144,7 +146,15 @@ func (b *MapResponseBuilder) WithSSHPolicy() *MapResponseBuilder { sshPolicy, err := b.mapper.state.SSHPolicy(node) if err != nil { - b.addError(err) + // SSH policy is optional for a node to function. Rather than fail the + // whole map (leaving the node unable to connect), log and continue + // without it; the node still receives a usable netmap. + log.Warn().Caller(). + Err(err). + Uint64(zf.NodeID, node.ID().Uint64()). + Str(zf.NodeHostname, node.Hostname()). + Msg("building map response: skipping SSH policy for node; node will receive a map without SSH rules") + return b } @@ -279,7 +289,19 @@ func (b *MapResponseBuilder) buildTailPeers(peers views.Slice[types.NodeView]) ( return b.mapper.state.RoutesForPeer(node, peer, matchers) }, b.mapper.cfg, allCapMaps[peer.ID()]) if err != nil { - return nil, err + // One peer with invalid data (e.g. an empty or over-long + // GivenName that fails GetFQDN) must not blank out the map for + // every node that can see it. Drop the offending peer, log it + // with the identity an operator needs to fix it, and keep + // building from the remaining valid peers. + log.Warn().Caller(). + Err(err). + Uint64(zf.NodeID, peer.ID().Uint64()). + Str(zf.NodeHostname, peer.Hostname()). + Uint64("map.viewer.node.id", b.nodeID.Uint64()). + Msgf("dropping peer %d from map response: invalid node data; fix with `headscale nodes rename %d `", peer.ID(), peer.ID()) + + continue } // [tailcfg.Node.CapMap] on a peer carries the small set of diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 94880c783..9b19c9498 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -3,6 +3,7 @@ package mapper import ( "fmt" "net/netip" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -536,6 +537,94 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { } } +// TestFullMapResponseSurvivesPeerWithInvalidName proves a single node with an +// FQDN-invalid GivenName must not break map generation for its peers. +// +// A node whose stored GivenName is empty (ErrNodeHasNoGivenName) or yields an +// FQDN longer than MaxHostnameLength (ErrHostnameTooLong) makes GetFQDN, and +// therefore TailNode, return an error. buildTailPeers used to abort the entire +// peer list on the first such error, so MapResponseBuilder.Build() failed for +// every node that could see the bad peer; on the initial-connection path that +// surfaced as "PollNetMap: ... unexpected EOF" and the "Unable to connect to +// the Tailscale coordination server" health warning. A legacy DB row loads +// verbatim (NewNodeStore reads db.ListNodes() without re-sanitising names), so +// the bad peer persists across restart. The build for an unaffected viewer +// must succeed: the bad peer is dropped, valid peers and self survive. +func TestFullMapResponseSurvivesPeerWithInvalidName(t *testing.T) { + for _, tt := range []struct { + name string + badName string + }{ + {"empty given name", ""}, + {"over-long fqdn", strings.Repeat("a", types.MaxHostnameLength+1)}, + } { + t.Run(tt.name, func(t *testing.T) { + tmp := t.TempDir() + p4 := netip.MustParsePrefix("100.64.0.0/10") + p6 := netip.MustParsePrefix("fd7a:115c:a1e0::/48") + cfg := &types.Config{ + Database: types.DatabaseConfig{ + Type: types.DatabaseSqlite, + Sqlite: types.SqliteConfig{Path: tmp + "/h.db"}, + }, + PrefixV4: &p4, + PrefixV6: &p6, + IPAllocation: types.IPAllocationStrategySequential, + BaseDomain: "headscale.test", + Policy: types.PolicyConfig{Mode: types.PolicyModeDB}, + DERP: types.DERPConfig{ + DERPMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{999: {RegionID: 999}}, + }, + }, + Tuning: types.Tuning{ + NodeStoreBatchSize: state.TestBatchSize, + NodeStoreBatchTimeout: state.TestBatchTimeout, + }, + } + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("u1") + n1 := database.CreateRegisteredNodeForTest(user, "n1") // viewer, valid + bad := database.CreateRegisteredNodeForTest(user, "bad") // peer, name corrupted below + good := database.CreateRegisteredNodeForTest(user, "good") // peer, valid control + + // Simulate a legacy/corrupt row that v29 loads verbatim. + require.NoError(t, database.DB. + Model(&types.Node{}). + Where("id = ?", bad.ID). + Update("given_name", tt.badName).Error) + require.NoError(t, database.Close()) + + s, err := state.NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // Allow-all so n1 sees both peers; the bad one must still be dropped. + _, err = s.SetPolicy([]byte(`{"acls":[{"action":"accept","src":["*"],"dst":["*:*"]}]}`)) + require.NoError(t, err) + + m := &mapper{state: s, cfg: cfg} + capVer := tailcfg.CurrentCapabilityVersion + + resp, err := m.fullMapResponse(n1.ID, capVer) + require.NoError(t, err, "n1's map must build despite a peer with an invalid name") + require.NotNil(t, resp) + require.NotNil(t, resp.Node, "n1 must receive its own self node") + + peers := map[tailcfg.NodeID]bool{} + for _, p := range resp.Peers { + peers[p.ID] = true + } + + assert.False(t, peers[bad.ID.NodeID()], "the peer with an invalid name must be dropped") + assert.True(t, peers[good.ID.NodeID()], "valid peers must remain in the map") + }) + } +} + // TestGenerateDNSConfigNilHostinfoNoPanic proves generateDNSConfig does not // panic when a node's Hostinfo is nil (e.g. a legacy DB row with a NULL // host_info column). addNextDNSMetadata dereferenced node.Hostinfo().OS() From 5fb76eb231f105c67ff38316958a889ab4b1534b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:33 +0000 Subject: [PATCH 05/15] poll: return an HTTP error on long-poll setup failure A bare return sent an empty 200 the client read as "unexpected EOF" and retried forever; emit a real error instead. Updates #3346 (cherry picked from commit 4e4512c4b730ddc5f2e71a3f8f3f8dc10a7c8872) --- hscontrol/poll.go | 10 ++++ hscontrol/poll_test.go | 127 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) diff --git a/hscontrol/poll.go b/hscontrol/poll.go index 96f782d44..35657026c 100644 --- a/hscontrol/poll.go +++ b/hscontrol/poll.go @@ -230,6 +230,11 @@ func (m *mapSession) serveLongPoll() { mapReqChange, err := m.h.state.UpdateNodeFromMapRequest(m.node.ID, m.req) if err != nil { m.log.Error().Caller().Err(err).Msg("failed to update node from initial MapRequest") + // Write an explicit error rather than returning silently: a bare + // return leaves net/http to send an empty 200, which the client + // reads as "unexpected EOF" and retries forever (issue #3346). + httpError(m.w, err) + return } @@ -252,6 +257,11 @@ func (m *mapSession) serveLongPoll() { // time between the node connecting and the batcher being ready. if err := m.h.mapBatcher.AddNode(m.node.ID, m.ch, m.capVer, m.stopFromBatcher); err != nil { //nolint:noinlineerr m.log.Error().Caller().Err(err).Msg("failed to add node to batcher") + // Write an explicit error rather than returning silently: a bare + // return leaves net/http to send an empty 200, which the client + // reads as "unexpected EOF" and retries forever (issue #3346). + httpError(m.w, err) + return } diff --git a/hscontrol/poll_test.go b/hscontrol/poll_test.go index e294d5e71..71ce08d9a 100644 --- a/hscontrol/poll_test.go +++ b/hscontrol/poll_test.go @@ -7,8 +7,10 @@ import ( "testing" "time" + "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/mapper" "github.com/juanfont/headscale/hscontrol/state" + "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -89,6 +91,131 @@ func (w *delayedSuccessResponseWriter) WriteCount() int { return w.writeCount } +// recordingResponseWriter records the status code and whether anything was +// written, so a test can tell an explicit error response apart from a handler +// that returned without writing (which net/http turns into an empty 200 the +// client reads as "unexpected EOF"). +type recordingResponseWriter struct { + mu sync.Mutex + header http.Header + status int + writes int +} + +func (w *recordingResponseWriter) Header() http.Header { + if w.header == nil { + w.header = make(http.Header) + } + + return w.header +} + +func (w *recordingResponseWriter) WriteHeader(code int) { + w.mu.Lock() + defer w.mu.Unlock() + + if w.status == 0 { + w.status = code + } +} + +func (w *recordingResponseWriter) Write(data []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + + if w.status == 0 { + w.status = http.StatusOK + } + + w.writes++ + + return len(data), nil +} + +func (w *recordingResponseWriter) Flush() {} + +func (w *recordingResponseWriter) statusCode() int { + w.mu.Lock() + defer w.mu.Unlock() + + return w.status +} + +// TestServeLongPollWritesErrorWhenInitialMapFails proves that when the initial +// map cannot be generated (here: the node's own GivenName is invalid, so +// WithSelfNode fails and AddNode errors), serveLongPoll writes an explicit HTTP +// error instead of returning with no body. Returning empty leaves net/http to +// send an empty 200, which the Tailscale client reports as +// "PollNetMap: ... unexpected EOF" and retries forever (issue #3346). +func TestServeLongPollWritesErrorWhenInitialMapFails(t *testing.T) { + app := createTestApp(t) + user := app.state.CreateUserForTest("self-bad-name-user") + createdNode := app.state.CreateRegisteredNodeForTest(user, "self-bad-name-node") + + // Corrupt the node's stored name to empty so GetFQDN fails for itself, + // then reload state so the bad row enters the NodeStore verbatim. + app.mapBatcher.Close() + require.NoError(t, app.state.Close()) + + database, err := db.NewHeadscaleDatabase(app.cfg) + require.NoError(t, err) + require.NoError(t, database.DB. + Model(&types.Node{}). + Where("id = ?", createdNode.ID). + Update("given_name", "").Error) + require.NoError(t, database.Close()) + + app.state, err = state.NewState(app.cfg) + require.NoError(t, err) + + app.mapBatcher = mapper.NewBatcherAndMapper(app.cfg, app.state) + app.mapBatcher.Start() + + t.Cleanup(func() { + app.mapBatcher.Close() + require.NoError(t, app.state.Close()) + }) + + nodeView, ok := app.state.GetNodeByID(createdNode.ID) + require.True(t, ok) + + node := nodeView.AsStruct() + + ctx, cancel := context.WithCancel(context.Background()) + writer := &recordingResponseWriter{} + session := app.newMapSession(ctx, tailcfg.MapRequest{ + Stream: true, + Version: tailcfg.CapabilityVersion(100), + }, writer, node) + + serveDone := make(chan struct{}) + + go func() { + session.serveLongPoll() + close(serveDone) + }() + + t.Cleanup(func() { + // Break the post-disconnect reconnect wait so the goroutine exits. + dummyCh := make(chan *tailcfg.MapResponse, 1) + _ = app.mapBatcher.AddNode(node.ID, dummyCh, tailcfg.CapabilityVersion(100), nil) + + cancel() + + select { + case <-serveDone: + case <-time.After(2 * time.Second): + } + + _ = app.mapBatcher.RemoveNode(node.ID, dummyCh) + }) + + assert.Eventually(t, func() bool { + return writer.statusCode() >= http.StatusInternalServerError + }, 2*time.Second, 10*time.Millisecond, + "serveLongPoll must write an HTTP error response when the initial map cannot be built, not an empty 200") +} + // TestGitHubIssue3129_TransientlyBlockedWriteDoesNotLeaveLiveStaleSession // tests the scenario reported in // https://github.com/juanfont/headscale/issues/3129. From 9c9206a92b7b3ca416e955f472e7f002e9215b75 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:42 +0000 Subject: [PATCH 06/15] state: reject renames whose FQDN exceeds the hostname limit A valid label can still overflow 255 chars under a long base_domain; gate RenameNode with the new types.ValidateGivenName. Updates #3346 (cherry picked from commit c497612c999597e0ed5fd59fa748f50b00a44a80) --- hscontrol/state/rename_test.go | 40 ++++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 5 ++++- hscontrol/types/node.go | 23 +++++++++++++++++++ hscontrol/types/node_test.go | 27 +++++++++++++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 hscontrol/state/rename_test.go diff --git a/hscontrol/state/rename_test.go b/hscontrol/state/rename_test.go new file mode 100644 index 000000000..b141147be --- /dev/null +++ b/hscontrol/state/rename_test.go @@ -0,0 +1,40 @@ +package state + +import ( + "strings" + "testing" + + "github.com/juanfont/headscale/hscontrol/db" + "github.com/stretchr/testify/require" +) + +// TestRenameNodeRejectsNameExceedingFQDNLimit proves RenameNode rejects a name +// that is a valid DNS label but whose FQDN, under the configured base_domain, +// exceeds MaxHostnameLength. Without the FQDN-length gate such a name persists +// and then breaks map generation for the node and its peers (issue #3346): +// admin-facing writes must not be able to introduce an unmappable name. +func TestRenameNodeRejectsNameExceedingFQDNLimit(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + // A long base domain so a 63-char label overflows the 255-char FQDN bound. + cfg.BaseDomain = strings.Repeat("b", 200) + ".example.com" + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("rename-user") + node := database.CreateRegisteredNodeForTest(user, "rename-node") + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // Valid 63-char DNS label, but the resulting FQDN exceeds 255 chars. + _, _, err = s.RenameNode(node.ID, strings.Repeat("a", 63)) + require.Error(t, err, "rename to a name whose FQDN exceeds the limit must be rejected") + + // A short, valid name is still accepted. + _, _, err = s.RenameNode(node.ID, "short") + require.NoError(t, err) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index edbed2e5d..549eb7433 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1031,7 +1031,10 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t // auto-sanitisation) and collisions error out rather than silently // bumping a user-facing label. See HOSTNAME.md for the CLI contract. func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, change.Change, error) { - err := dnsname.ValidLabel(newName) + // Validate the label AND that the resulting FQDN fits MaxHostnameLength: + // a valid 63-char label can still overflow under a long base_domain, and + // an unmappable name would break this node and its peers (issue #3346). + err := types.ValidateGivenName(newName, s.cfg.BaseDomain) if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("renaming node: %w", err) } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index cc0a51d0b..6b1f3bdda 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -21,6 +21,7 @@ import ( "tailscale.com/tailcfg" "tailscale.com/types/key" "tailscale.com/types/views" + "tailscale.com/util/dnsname" ) var ( @@ -568,6 +569,28 @@ func (node *Node) GetFQDN(baseDomain string) (string, error) { return hostname, nil } +// ValidateGivenName reports whether givenName is usable as a node's DNS label: +// a valid DNS label that, combined with baseDomain, yields an FQDN within +// MaxHostnameLength. Admin-facing write paths (e.g. node rename) reject names +// that fail this, since the mapper cannot build a map for a node — or any of +// its peers — whose GetFQDN fails. Derived paths sanitise/coerce instead. +func ValidateGivenName(givenName, baseDomain string) error { + err := dnsname.ValidLabel(givenName) + if err != nil { + return fmt.Errorf("%q is not a valid DNS label: %w", givenName, err) + } + + // Reuse GetFQDN so the length bound stays identical to what the mapper + // enforces; a valid 63-char label can still overflow under a long + // base_domain. + _, err = (&Node{GivenName: givenName}).GetFQDN(baseDomain) + if err != nil { + return err + } + + return nil +} + // AnnouncedRoutes returns the list of routes the node announces, as // reported by the client in [tailcfg.Hostinfo.RoutableIPs]. Announcement alone // does not grant visibility — see [Node.SubnetRoutes] for approval-gated diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 0bec4aeff..c2e0b45d1 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -418,6 +418,33 @@ func TestNodeFQDN(t *testing.T) { } } +func TestValidateGivenName(t *testing.T) { + tests := []struct { + name string + givenName string + baseDomain string + wantErr bool + }{ + {"valid", "test", "example.com", false}, + {"empty", "", "example.com", true}, + {"invalid label chars", "not valid", "example.com", true}, + {"label too long", strings.Repeat("a", 64), "example.com", true}, + // A valid 63-char label whose FQDN overflows only because the base + // domain is long: ValidLabel passes, the FQDN-length bound rejects it. + {"fqdn too long under long base domain", strings.Repeat("a", 63), strings.Repeat("b", 200) + ".example.com", true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := ValidateGivenName(tc.givenName, tc.baseDomain) + if (err != nil) != tc.wantErr { + t.Errorf("ValidateGivenName(%q, %q) error = %v, wantErr %v", + tc.givenName, tc.baseDomain, err, tc.wantErr) + } + }) + } +} + func TestPeerChangeFromMapRequest(t *testing.T) { nKeys := []key.NodePublic{ key.NewNode().Public(), From fd154fdb663d4992fb15cd95796c4791e48b096f Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:55 +0000 Subject: [PATCH 07/15] state: log nodes with map-breaking data at startup Scan a node-health check registry at boot and log each node whose name can't form a valid FQDN, with the rename fix. Log-only, no mutation. Updates #3346 (cherry picked from commit 4946d1c88d25aa9c3d426437aca4520b5376a86c) --- hscontrol/state/node_health.go | 92 +++++++++++++++++++++++++++++ hscontrol/state/node_health_test.go | 67 +++++++++++++++++++++ hscontrol/state/state.go | 11 +++- 3 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 hscontrol/state/node_health.go create mode 100644 hscontrol/state/node_health_test.go diff --git a/hscontrol/state/node_health.go b/hscontrol/state/node_health.go new file mode 100644 index 000000000..4edaf3986 --- /dev/null +++ b/hscontrol/state/node_health.go @@ -0,0 +1,92 @@ +package state + +import ( + "fmt" + + "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" + "github.com/rs/zerolog/log" +) + +// nodeHealthCheck names a class of stored-node-data defect that breaks normal +// operation and explains how to fix it. ok == true means the node passes the +// check. This is the extension point for node-data validation: add a check +// here as new corrupt-data classes surface (nil hostinfo, invalid IPs, +// tags-XOR-user violations, ...) and both the boot scan and any future caller +// run the whole set. +type nodeHealthCheck struct { + name string + check func(nv types.NodeView, cfg *types.Config) (problem, fixHint string, ok bool) +} + +// nodeHealthChecks is the registry of node-data health checks. Today it carries +// the one issue #3346 needs; append to it rather than reshaping callers. +var nodeHealthChecks = []nodeHealthCheck{givenNameMapsToValidFQDN} + +// givenNameMapsToValidFQDN flags a node whose stored GivenName cannot produce a +// valid FQDN (empty, or longer than MaxHostnameLength once base_domain is +// applied). Such a node cannot be rendered into a netmap — neither its own nor +// any peer's — so it must be renamed to recover. +var givenNameMapsToValidFQDN = nodeHealthCheck{ + name: "given-name-maps-to-valid-fqdn", + check: func(nv types.NodeView, cfg *types.Config) (string, string, bool) { + err := types.ValidateGivenName(nv.GivenName(), cfg.BaseDomain) + if err != nil { + return err.Error(), fmt.Sprintf("headscale nodes rename %d ", nv.ID()), false + } + + return "", "", true + }, +} + +// nodeHealthFinding is a single failed check for a single node. +type nodeHealthFinding struct { + nodeID types.NodeID + hostname string + check string + problem string + fixHint string +} + +// scanNodeHealth runs every registered check against every node in the store +// and returns one finding per failure. It only reports — it never mutates a +// node — so an operator can repair the underlying data without the server +// silently rewriting a user-visible name. +func (s *State) scanNodeHealth() []nodeHealthFinding { + var findings []nodeHealthFinding + + for _, nv := range s.nodeStore.ListNodes().All() { + for _, c := range nodeHealthChecks { + problem, fixHint, ok := c.check(nv, s.cfg) + if ok { + continue + } + + findings = append(findings, nodeHealthFinding{ + nodeID: nv.ID(), + hostname: nv.Hostname(), + check: c.name, + problem: problem, + fixHint: fixHint, + }) + } + } + + return findings +} + +// logNodeHealth scans the store once and logs an actionable warning per +// finding. Called at startup so an operator learns — by node id and fix +// command — about stored data that will break map generation, without the +// server changing anything itself. +func (s *State) logNodeHealth() { + for _, f := range s.scanNodeHealth() { + log.Warn(). + Uint64(zf.NodeID, f.nodeID.Uint64()). + Str(zf.NodeHostname, f.hostname). + Str("check", f.check). + Str("problem", f.problem). + Str("fix", f.fixHint). + Msg("node has invalid data that breaks map generation; rename it to restore connectivity") + } +} diff --git a/hscontrol/state/node_health_test.go b/hscontrol/state/node_health_test.go new file mode 100644 index 000000000..cd61a25b8 --- /dev/null +++ b/hscontrol/state/node_health_test.go @@ -0,0 +1,67 @@ +package state + +import ( + "testing" + + "github.com/juanfont/headscale/hscontrol/db" + "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/require" +) + +func TestGivenNameMapsToValidFQDNCheck(t *testing.T) { + cfg := &types.Config{BaseDomain: "example.com"} + + _, _, ok := givenNameMapsToValidFQDN.check((&types.Node{ID: 1, GivenName: "valid"}).View(), cfg) + require.True(t, ok, "a valid given name must pass the check") + + problem, fixHint, ok := givenNameMapsToValidFQDN.check((&types.Node{ID: 7, GivenName: ""}).View(), cfg) + require.False(t, ok, "an empty given name must fail the check") + require.NotEmpty(t, problem) + require.Contains(t, fixHint, "rename 7", "fix hint must name the offending node") +} + +// TestScanNodeHealthReportsInvalidNameWithoutMutating proves the boot scan +// reports a node whose stored name would break map generation (issue #3346) +// with an actionable fix, and that it never rewrites the stored name — the +// maintainer's decision is log-only, no silent mutation. +func TestScanNodeHealthReportsInvalidNameWithoutMutating(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("scan-user") + bad := database.CreateRegisteredNodeForTest(user, "scan-bad") + good := database.CreateRegisteredNodeForTest(user, "scan-good") + + require.NoError(t, database.DB. + Model(&types.Node{}). + Where("id = ?", bad.ID). + Update("given_name", "").Error) + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + findings := s.scanNodeHealth() + + var badFinding *nodeHealthFinding + + for i := range findings { + require.NotEqual(t, good.ID, findings[i].nodeID, "a valid node must not be reported") + + if findings[i].nodeID == bad.ID { + badFinding = &findings[i] + } + } + + require.NotNil(t, badFinding, "a node with an invalid name must be reported") + require.Contains(t, badFinding.fixHint, "rename", "finding must carry an actionable fix") + + // Log-only: neither the scan nor boot may rewrite the stored name. + nv, ok := s.GetNodeByID(bad.ID) + require.True(t, ok) + require.Empty(t, nv.GivenName(), "boot scan must not mutate the stored name") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 549eb7433..645c65e4d 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -286,7 +286,7 @@ func NewState(cfg *types.Config) (*State, error) { ) nodeStore.Start() - return &State{ + s := &State{ cfg: cfg, db: db, @@ -298,7 +298,14 @@ func NewState(cfg *types.Config) (*State, error) { sshCheckAuth: make(map[sshCheckPair]time.Time), registerLocks: xsync.NewMap[key.MachinePublic, *sync.Mutex](), - }, nil + } + + // Surface nodes whose stored data would break map generation (e.g. an + // invalid given name from a legacy row) so an operator can fix them. This + // only logs; it never mutates a node's stored name at boot. + s.logNodeHealth() + + return s, nil } // Close gracefully shuts down the [State] instance and releases all resources. From 735742e3eea9be93c42c2bd76c9641a74dc11e20 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:20:26 +0000 Subject: [PATCH 08/15] CHANGELOG: note 0.29.2 invalid-name map fix Updates #3346 (cherry picked from commit de9db9c811523a2667183d1e50a88329d5d2d013) --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 406f65d4a..cc8622c2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,19 @@ - Fix map generation serializing on the policy lock, so a mass reconnect on `autogroup:self`, via or relay policies no longer stalls clients into `unexpected EOF` retry loops [#3358](https://github.com/juanfont/headscale/pull/3358) +A node whose stored name could not be turned into a valid FQDN — empty, or long +enough that the full hostname exceeded 255 characters once the base domain was +applied — broke map delivery for itself and for every peer that could see it. +Affected clients looped on `PollNetMap: unexpected EOF` and reported being +unable to reach the coordination server, while other clients were unaffected. +The mapper now drops such a node from its peers' maps instead of failing the +whole response, the long-poll handler returns an explicit error rather than an +empty response, node renames that would exceed the hostname limit are rejected, +and startup logs each node whose stored name needs fixing together with the +command to fix it. + +[#3349](https://github.com/juanfont/headscale/pull/3349) + ## 0.29.1 (2026-06-18) **Minimum supported Tailscale client version: v1.80.0** From 1ec7b7fb726ea6270a1eb459534753ef27eda736 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 08:38:57 +0000 Subject: [PATCH 09/15] hscontrol: register /ts2021 for WebSocket GET The chi migration dropped GET; JS/WASM control clients open /ts2021 as a WebSocket GET and were rejected with 405 before reaching NoiseUpgradeHandler. Fixes #3357 (cherry picked from commit fc6f216b616778b6347a91148e9763cd22c6de94) --- hscontrol/app.go | 5 ++++ hscontrol/noise_test.go | 65 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/hscontrol/app.go b/hscontrol/app.go index c183c3bbe..fc0869b0e 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -544,6 +544,11 @@ func (h *Headscale) createRouter(grpcMux *grpcRuntime.ServeMux) *chi.Mux { r.Use(middleware.Recoverer) r.Use(securityHeaders) + // TS2021 accepts both the native client's HTTP POST upgrade and the + // browser/WASM client's WebSocket GET upgrade; NoiseUpgradeHandler + // dispatches on the Upgrade header, not the method. Registering GET as + // well keeps the router from rejecting the WebSocket handshake with 405. + r.Get(ts2021UpgradePath, h.NoiseUpgradeHandler) r.Post(ts2021UpgradePath, h.NoiseUpgradeHandler) r.Get("/robots.txt", h.RobotsHandler) diff --git a/hscontrol/noise_test.go b/hscontrol/noise_test.go index 922ef5051..99b3ec4bf 100644 --- a/hscontrol/noise_test.go +++ b/hscontrol/noise_test.go @@ -459,6 +459,71 @@ func TestSSHActionHandler_RejectsMissingSessionWithoutCheck(t *testing.T) { "a bogus auth_id with no active check must be rejected, body=%s", rec.Body.String()) } +// TestTS2021Route_AcceptsGETAndPOST reproduces a regression where the +// browser/WASM control client could not connect. Tailscale's JS/WASM control +// client opens /ts2021 as a WebSocket, which is an HTTP GET upgrade; the native +// Go client uses an HTTP POST upgrade. The gorilla->chi router migration +// registered /ts2021 for POST only, so the GET WebSocket handshake was rejected +// with 405 Method Not Allowed by the router before it could reach +// NoiseUpgradeHandler. Both methods must route to the handler. +// +// NoiseUpgradeHandler dispatches on the Upgrade header, not the HTTP method, so +// once the route is reachable the handler handles both upgrade styles. The +// httptest recorder is not an http.Hijacker, so the upgrade itself fails past +// the router (501 for the WebSocket path, 400 for the native path) — the point +// is only that neither is 405, i.e. the router no longer rejects GET early. +func TestTS2021Route_AcceptsGETAndPOST(t *testing.T) { + t.Parallel() + + handler := createTestApp(t).HTTPHandler() + + tests := []struct { + name string + method string + headers map[string]string + }{ + { + name: "websocket_get_from_wasm_client", + method: http.MethodGet, + headers: map[string]string{ + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": "dGhlIHNhbXBsZSBub25jZQ==", + "Sec-WebSocket-Protocol": "tailscale-control-protocol", + }, + }, + { + name: "native_post_upgrade", + method: http.MethodPost, + headers: map[string]string{ + "Connection": "upgrade", + "Upgrade": "tailscale-control-protocol", + "X-Tailscale-Handshake": "AAAA", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext(context.Background(), tt.method, + "/ts2021?X-Tailscale-Handshake=AAAA", nil) + for k, v := range tt.headers { + req.Header.Set(k, v) + } + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.NotEqual(t, http.StatusMethodNotAllowed, rec.Code, + "%s /ts2021 must reach NoiseUpgradeHandler, not be rejected by the router with 405", + tt.method) + }) + } +} + // newSSHActionFollowUpRequest is like newSSHActionRequest but carries the // auth_id query parameter that marks a follow-up poll. func newSSHActionFollowUpRequest(t *testing.T, src, dst types.NodeID, authID types.AuthID) *http.Request { From e7851ef8815248699833ac82fc7bd4d127a05713 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 08:39:12 +0000 Subject: [PATCH 10/15] integration: test /ts2021 WebSocket GET with a real WASM client A raw coder/websocket dial and the real tailscale.com js/wasm control client under Node, both against headscale alongside normal Tailscale clients. Updates #3357 (cherry picked from commit f4fba32dc6cf6ab83cb8a7b1c307973fa01f664c) --- Dockerfile.wasmclient | 30 +++ integration/ts2021_websocket_test.go | 315 ++++++++++++++++++++++++++ integration/wasmic/wasmclient/main.go | 103 +++++++++ integration/wasmic/wasmclient/stub.go | 8 + 4 files changed, 456 insertions(+) create mode 100644 Dockerfile.wasmclient create mode 100644 integration/ts2021_websocket_test.go create mode 100644 integration/wasmic/wasmclient/main.go create mode 100644 integration/wasmic/wasmclient/stub.go diff --git a/Dockerfile.wasmclient b/Dockerfile.wasmclient new file mode 100644 index 000000000..cc939a09f --- /dev/null +++ b/Dockerfile.wasmclient @@ -0,0 +1,30 @@ +# For integration testing only. +# +# Builds the Tailscale control client (integration/wasmic/wasmclient) for +# GOOS=js/GOARCH=wasm and packages it with Go's wasm_exec Node runner. The +# container idles; the integration test execs +# node /app/wasm_exec_node.js /app/client.wasm +# to drive a real browser-style WebSocket GET against headscale's /ts2021, +# guarding the regression in issue #3357. + +FROM golang:1.26.4-alpine AS build + +WORKDIR /src + +# Only the module metadata and the wasm client package are needed to build the +# js/wasm binary; its imports (tailscale.com/control/controlhttp, ...) resolve +# from the module proxy. +COPY go.mod go.sum ./ +COPY integration/wasmic/wasmclient ./integration/wasmic/wasmclient + +RUN GOOS=js GOARCH=wasm go build -o /out/client.wasm ./integration/wasmic/wasmclient \ + && cp "$(go env GOROOT)/lib/wasm/wasm_exec.js" /out/wasm_exec.js \ + && cp "$(go env GOROOT)/lib/wasm/wasm_exec_node.js" /out/wasm_exec_node.js + +FROM node:24-alpine + +WORKDIR /app +COPY --from=build /out/ /app/ + +# Idle; the test execs the client on demand with the headscale control URL. +ENTRYPOINT ["tail", "-f", "/dev/null"] diff --git a/integration/ts2021_websocket_test.go b/integration/ts2021_websocket_test.go new file mode 100644 index 000000000..fc7f425e3 --- /dev/null +++ b/integration/ts2021_websocket_test.go @@ -0,0 +1,315 @@ +package integration + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/netip" + "net/url" + "testing" + "time" + + "github.com/coder/websocket" + "github.com/juanfont/headscale/integration/dockertestutil" + "github.com/juanfont/headscale/integration/hsic" + "github.com/juanfont/headscale/integration/tsic" + "github.com/ory/dockertest/v3" + "github.com/samber/lo" + "github.com/stretchr/testify/require" + "tailscale.com/control/controlbase" + "tailscale.com/control/controlhttp/controlhttpcommon" + "tailscale.com/net/wsconn" + "tailscale.com/tailcfg" + "tailscale.com/types/key" + "tailscale.com/util/rands" +) + +// Tailscale's JS/WASM control client opens /ts2021 as a browser WebSocket — an +// HTTP GET upgrade — rather than the native client's HTTP POST upgrade. A router +// that registers /ts2021 for POST only rejects that GET with 405 before the +// Noise handshake starts, which breaks every WASM client (issue #3357). +// +// These two tests guard that path against real headscale: +// +// - TestTS2021WebSocketGET dials the WebSocket GET directly from the test +// process using the same coder/websocket + controlbase primitives the WASM +// client uses. It is fast and always on. +// - TestTS2021WASMClientUnderNode runs the *actual* tailscale.com js/wasm +// control dial (integration/wasmic/wasmclient, built for GOOS=js) inside a +// Node container, alongside normal Tailscale clients, and asserts it +// completes the Noise handshake with headscale over the WebSocket. +// +// The server cannot tell the two apart: both send GET /ts2021 with +// Sec-WebSocket-Protocol: tailscale-control-protocol. Before the fix both fail +// with 405; after it, both complete the handshake. + +// TestTS2021WebSocketGET connects to /ts2021 over a WebSocket GET from the test +// process and completes the Noise handshake, exactly as a browser/WASM client +// would. +func TestTS2021WebSocketGET(t *testing.T) { + IntegrationSkip(t) + t.Parallel() + + spec := ScenarioSpec{ + NodesPerUser: 1, + Users: []string{"user1"}, + } + + scenario, err := NewScenario(spec) + + require.NoErrorf(t, err, "failed to create scenario: %s", err) + defer scenario.ShutdownAssertNoPanics(t) + + err = scenario.CreateHeadscaleEnv([]tsic.Option{}, hsic.WithTestName("ts2021ws")) + requireNoErrHeadscaleEnv(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + conn, err := dialTS2021WebSocket(t, headscale.GetEndpoint(), headscale.GetCert()) + require.NoError(t, err, + "WebSocket GET to /ts2021 must reach NoiseUpgradeHandler, not be rejected by the router with 405") + require.NotNil(t, conn) + t.Cleanup(func() { _ = conn.Close() }) + + t.Logf("noise established over websocket, protocol version %d", conn.ProtocolVersion()) +} + +// TestTS2021WASMClientUnderNode runs the real tailscale.com js/wasm control dial +// inside a Node container against real headscale, next to normal Tailscale +// clients, and asserts the WASM client completes the /ts2021 handshake. +func TestTS2021WASMClientUnderNode(t *testing.T) { + IntegrationSkip(t) + t.Parallel() + + spec := ScenarioSpec{ + NodesPerUser: 2, + Users: []string{"user1"}, + Networks: map[string]NetworkSpec{ + "wasmnet": {Users: []string{"user1"}}, + }, + ExtraService: map[string][]extraServiceFunc{ + "wasmnet": {wasmClientService}, + }, + // The wasm client image builds from this module; pair it with the + // head Tailscale clients so the whole environment is current. + Versions: []string{"head"}, + } + + scenario, err := NewScenario(spec) + + require.NoErrorf(t, err, "failed to create scenario: %s", err) + defer scenario.ShutdownAssertNoPanics(t) + + // The Tailscale JS/WASM client dials the control server as a WebSocket. + // client_js.go only honours a custom port for ws:// (plain HTTP); over + // wss:// it always targets :443, so it cannot reach a TLS control server on + // :8080. Run headscale without TLS, matching the http:// setup in the issue. + err = scenario.CreateHeadscaleEnv( + []tsic.Option{}, + hsic.WithTestName("ts2021wasm"), + hsic.WithoutTLS(), + ) + requireNoErrHeadscaleEnv(t, err) + + allClients, err := scenario.ListTailscaleClients() + requireNoErrListClients(t, err) + + allIPs, err := scenario.ListTailscaleClientsIPs() + requireNoErrListClientIPs(t, err) + + // Normal Tailscale clients come up and form a working tailnet alongside the + // WASM control client. + err = scenario.WaitForTailscaleSync() + requireNoErrSync(t, err) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + // Sanity-check the tailnet the WASM client is joining: the normal clients + // must be able to reach each other. + allAddrs := lo.Map(allIPs, func(x netip.Addr, _ int) string { return x.String() }) + assertPingAll(t, allClients, allAddrs) + + services, err := scenario.Services("wasmnet") + require.NoError(t, err) + require.Len(t, services, 1, "expected the wasm client container") + + wasm := services[0] + controlURL := headscale.GetEndpoint() + + // Fetch the server's Noise key here and pass it to the WASM client: Go's + // net/http DNS resolver is unavailable under GOOS=js, so the client can only + // use the JS WebSocket transport, not an HTTP GET to /key. + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + controlKey, err := fetchServerNoiseKey(ctx, &http.Client{Timeout: 15 * time.Second}, controlURL) + require.NoError(t, err) + controlKeyText, err := controlKey.MarshalText() + require.NoError(t, err) + + // Run the real js/wasm control client under Node: it dials /ts2021 as a + // WebSocket GET; success means the Noise handshake completed. + stdout, stderr, err := dockertestutil.ExecuteCommand( + wasm, + []string{"node", "/app/wasm_exec_node.js", "/app/client.wasm", controlURL, string(controlKeyText)}, + []string{}, + dockertestutil.ExecuteCommandTimeout(60*time.Second), + ) + t.Logf("wasm client stdout:\n%s", stdout) + t.Logf("wasm client stderr:\n%s", stderr) + + require.NoError(t, err, + "wasm control client must connect to /ts2021 over websocket (405 means the router rejected the GET)") + require.Contains(t, stdout, "WASM_TS2021_OK", + "wasm control client should report a completed Noise handshake") +} + +// wasmClientService builds and starts the Node + js/wasm control-client +// container (Dockerfile.wasmclient) on the given network so it can reach +// headscale by hostname. It idles; the test execs the client on demand. +func wasmClientService(s *Scenario, networkName string) (*dockertest.Resource, error) { + hash := rands.HexString(hsicOIDCMockHashLength) + hostname := "hs-wasmclient-" + hash + + network, ok := s.networks[s.prefixedNetworkName(networkName)] + if !ok { + return nil, fmt.Errorf("network does not exist: %s", networkName) //nolint:err113 + } + + runOpts := &dockertest.RunOptions{ + Name: hostname, + Networks: []*dockertest.Network{network}, + Env: []string{}, + } + dockertestutil.DockerAddIntegrationLabels(runOpts, "wasmclient") + + buildOpts := &dockertest.BuildOptions{ + Dockerfile: "Dockerfile.wasmclient", + ContextDir: dockerContextPath, + } + + resource, err := s.pool.BuildAndRunWithBuildOptions( + buildOpts, + runOpts, + dockertestutil.DockerRestartPolicy, + ) + if err != nil { + return nil, fmt.Errorf("building wasm client container: %w", err) + } + + return resource, nil +} + +// dialTS2021WebSocket opens /ts2021 as a WebSocket GET (subprotocol +// tailscale-control-protocol) and completes the Noise handshake, mirroring what +// tailscale.com/control/controlhttp/client_js.go does in a browser. It returns +// the established Noise connection, or an error (a router that only allows POST +// returns 405 here). +func dialTS2021WebSocket(t *testing.T, endpoint string, caCert []byte) (*controlbase.Conn, error) { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + + u, err := url.Parse(endpoint) + require.NoError(t, err) + + httpClient := &http.Client{Timeout: 15 * time.Second} + + if u.Scheme == "https" { + pool := x509.NewCertPool() + pool.AppendCertsFromPEM(caCert) + httpClient.Transport = &http.Transport{ + TLSClientConfig: &tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12}, + } + } + + controlKey, err := fetchServerNoiseKey(ctx, httpClient, endpoint) + require.NoError(t, err) + + init, cont, err := controlbase.ClientDeferred( + key.NewMachine(), + controlKey, + uint16(tailcfg.CurrentCapabilityVersion), + ) + require.NoError(t, err) + + wsScheme := "ws" + if u.Scheme == "https" { + wsScheme = "wss" + } + + wsURL := &url.URL{ + Scheme: wsScheme, + Host: u.Host, + Path: "/ts2021", + RawQuery: url.Values{ + controlhttpcommon.HandshakeHeaderName: []string{base64.StdEncoding.EncodeToString(init)}, + }.Encode(), + } + + wsConn, resp, err := websocket.Dial(ctx, wsURL.String(), &websocket.DialOptions{ + Subprotocols: []string{controlhttpcommon.UpgradeHeaderValue}, + HTTPClient: httpClient, + }) + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + + if err != nil { + return nil, err + } + + netConn := wsconn.NetConn(ctx, wsConn, websocket.MessageBinary, wsURL.String()) + + cbConn, err := cont(ctx, netConn) + if err != nil { + _ = netConn.Close() + return nil, fmt.Errorf("noise handshake over websocket: %w", err) + } + + return cbConn, nil +} + +// fetchServerNoiseKey retrieves headscale's Noise public key from /key, the same +// endpoint a real client consults before dialing /ts2021. +func fetchServerNoiseKey( + ctx context.Context, + client *http.Client, + endpoint string, +) (key.MachinePublic, error) { + var zero key.MachinePublic + + keyURL := fmt.Sprintf("%s/key?v=%d", endpoint, tailcfg.CurrentCapabilityVersion) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, keyURL, nil) + if err != nil { + return zero, err + } + + resp, err := client.Do(req) + if err != nil { + return zero, err + } + defer resp.Body.Close() + + var k tailcfg.OverTLSPublicKeyResponse + + err = json.NewDecoder(resp.Body).Decode(&k) + if err != nil { + return zero, fmt.Errorf("decoding /key response: %w", err) + } + + if k.PublicKey.IsZero() { + return zero, errors.New("server returned zero Noise public key") //nolint:err113 + } + + return k.PublicKey, nil +} diff --git a/integration/wasmic/wasmclient/main.go b/integration/wasmic/wasmclient/main.go new file mode 100644 index 000000000..dfc099251 --- /dev/null +++ b/integration/wasmic/wasmclient/main.go @@ -0,0 +1,103 @@ +//go:build js + +// Command wasmclient is a minimal Tailscale control client compiled to +// GOOS=js/GOARCH=wasm and run under Node. It exercises the real +// tailscale.com/control/controlhttp js/wasm dial path +// (control/controlhttp/client_js.go), which opens /ts2021 as a browser-style +// WebSocket GET — the exact transport a Tailscale JS/WASM client uses. +// +// It is the container-side half of the integration test guarding issue #3357: +// headscale must register /ts2021 for GET, not POST only, or the WebSocket +// upgrade is rejected with 405 before the Noise handshake can start. +// +// It is intentionally not the full tsconnect IPN — the regression is entirely +// in the control-connection upgrade, and this drives the real upgrade code with +// the smallest possible harness. On success it prints wasmSuccessMarker and +// exits 0; on any failure it prints wasmFailureMarker and exits non-zero. +package main + +import ( + "context" + "fmt" + "net/url" + "os" + "time" + + "tailscale.com/control/controlhttp" + "tailscale.com/tailcfg" + "tailscale.com/types/key" +) + +// These markers are matched by the integration test on the client's stdout. +const ( + wasmSuccessMarker = "WASM_TS2021_OK" + wasmFailureMarker = "WASM_TS2021_FAIL" +) + +func main() { + if len(os.Args) < 3 { + fmt.Printf("%s: usage: wasmclient \n", wasmFailureMarker) + os.Exit(2) + } + + if err := run(os.Args[1], os.Args[2]); err != nil { + fmt.Printf("%s: %v\n", wasmFailureMarker, err) + os.Exit(1) + } +} + +// run dials /ts2021 exactly as tailscale.com/control/controlhttp/client_js.go +// does in a browser: a WebSocket GET via the JS/undici WebSocket. The server's +// Noise key is passed in (the test fetches /key) rather than fetched here, +// because Go's net/http DNS resolver is unavailable under GOOS=js — only the +// WebSocket transport, which runs through the JS host, works. +func run(controlURL, noiseKeyText string) error { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + u, err := url.Parse(controlURL) + if err != nil { + return fmt.Errorf("parse control url %q: %w", controlURL, err) + } + + var controlKey key.MachinePublic + if err := controlKey.UnmarshalText([]byte(noiseKeyText)); err != nil { + return fmt.Errorf("parse noise key %q: %w", noiseKeyText, err) + } + + port := u.Port() + if port == "" { + if u.Scheme == "https" { + port = "443" + } else { + port = "80" + } + } + + // client_js.go selects ws:// (and appends the port) only when HTTPPort is a + // custom non-80 port and HTTPS is 443 or disabled; otherwise it dials wss:// + // on the default port. Set the fields to match the server's actual scheme. + d := &controlhttp.Dialer{ + Hostname: u.Hostname(), + MachineKey: key.NewMachine(), + ControlKey: controlKey, + ProtocolVersion: uint16(tailcfg.CurrentCapabilityVersion), + } + if u.Scheme == "https" { + d.HTTPSPort = port + } else { + d.HTTPPort = port + d.HTTPSPort = controlhttp.NoPort + } + + conn, err := d.Dial(ctx) + if err != nil { + return fmt.Errorf("ts2021 websocket dial: %w", err) + } + defer conn.Close() + + fmt.Printf("%s: noise established over websocket, protocol version %d\n", + wasmSuccessMarker, conn.ProtocolVersion()) + + return nil +} diff --git a/integration/wasmic/wasmclient/stub.go b/integration/wasmic/wasmclient/stub.go new file mode 100644 index 000000000..10ee01de5 --- /dev/null +++ b/integration/wasmic/wasmclient/stub.go @@ -0,0 +1,8 @@ +//go:build !js + +// This package only does something when built for GOOS=js (see main.go). The +// stub exists so `go build ./...` and `go vet ./...` on the host don't fail with +// "build constraints exclude all Go files" for this directory. +package main + +func main() {} From 8f4e69d2a643f41378275c1b33395448ba8a596d Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 08:39:21 +0000 Subject: [PATCH 11/15] integration: add TS2021 WebSocket tests to CI matrix Generated by gh-action-integration-generator. Updates #3357 (cherry picked from commit 01ef350d5f969c5a434df26beab96d71564a234c) --- .github/workflows/test-integration.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index a5feddff0..984fb6655 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -367,6 +367,8 @@ jobs: - TestTagsAuthKeyWithoutUserInheritsTags - TestTagsAuthKeyWithoutUserRejectsAdvertisedTags - TestTagsAuthKeyConvertToUserViaCLIRegister + - TestTS2021WebSocketGET + - TestTS2021WASMClientUnderNode - TestTailscaleRustAxum uses: ./.github/workflows/integration-test-template.yml secrets: inherit From f708c5b0108ac2ba5ff384ae9b85bdef488e1ffc Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 08:40:47 +0000 Subject: [PATCH 12/15] CHANGELOG: note /ts2021 WebSocket GET fix Updates #3357 (cherry picked from commit fef80f3eb08236ccba5f8681a5d486741fda6706) --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc8622c2e..f855557b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ ### Changes - Fix map generation serializing on the policy lock, so a mass reconnect on `autogroup:self`, via or relay policies no longer stalls clients into `unexpected EOF` retry loops [#3358](https://github.com/juanfont/headscale/pull/3358) +- Fix `/ts2021` rejecting the WebSocket `GET` upgrade with 405, which prevented Tailscale JS/WASM control clients from connecting [#3359](https://github.com/juanfont/headscale/pull/3359) A node whose stored name could not be turned into a valid FQDN — empty, or long enough that the full hostname exceeded 255 characters once the base domain was From 3ac33cf1d5fa66bafcdf11fd0919d10f118ee8ea Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 13:35:10 +0000 Subject: [PATCH 13/15] CHANGELOG: shorten 0.29.2 invalid-name entry, set date Updates #3346 (cherry picked from commit a84945f134b5bebe850e7ac1f911d5f15da0a7ae) --- CHANGELOG.md | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f855557b8..fab173ba3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ **Minimum supported Tailscale client version: v1.xx.0** -## 0.29.2 (202x-xx-xx) +## 0.29.2 (2026-07-01) **Minimum supported Tailscale client version: v1.80.0** @@ -12,19 +12,7 @@ - Fix map generation serializing on the policy lock, so a mass reconnect on `autogroup:self`, via or relay policies no longer stalls clients into `unexpected EOF` retry loops [#3358](https://github.com/juanfont/headscale/pull/3358) - Fix `/ts2021` rejecting the WebSocket `GET` upgrade with 405, which prevented Tailscale JS/WASM control clients from connecting [#3359](https://github.com/juanfont/headscale/pull/3359) - -A node whose stored name could not be turned into a valid FQDN — empty, or long -enough that the full hostname exceeded 255 characters once the base domain was -applied — broke map delivery for itself and for every peer that could see it. -Affected clients looped on `PollNetMap: unexpected EOF` and reported being -unable to reach the coordination server, while other clients were unaffected. -The mapper now drops such a node from its peers' maps instead of failing the -whole response, the long-poll handler returns an explicit error rather than an -empty response, node renames that would exceed the hostname limit are rejected, -and startup logs each node whose stored name needs fixing together with the -command to fix it. - -[#3349](https://github.com/juanfont/headscale/pull/3349) +- Gracefully handle nodes with an invalid FQDN (empty or too long) instead of failing map delivery; offending names are logged at startup with the fix command [#3349](https://github.com/juanfont/headscale/pull/3349) ## 0.29.1 (2026-06-18) From f885d87827bcae30a07063f2723cd03458144a00 Mon Sep 17 00:00:00 2001 From: Florian Preinstorfer Date: Wed, 24 Jun 2026 08:00:57 +0200 Subject: [PATCH 14/15] Fix invalid ip syntax The "ip" field needs to be a list. (cherry picked from commit a066a06befc39d6409c82ec5c4cf4ef684dc37f7) --- docs/ref/policy.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/ref/policy.md b/docs/ref/policy.md index 3529a81ee..f326d7ecb 100644 --- a/docs/ref/policy.md +++ b/docs/ref/policy.md @@ -158,17 +158,17 @@ devices. Can only be used in policy destinations. { "src": ["boss@"], "dst": ["boss@"], - "ip": "*" + "ip": ["*"] }, { "src": ["dev1@"], "dst": ["dev1@"], - "ip": "*" + "ip": ["*"] }, { "src": ["intern1@"], "dst": ["intern1@"], - "ip": "*" + "ip": ["*"] } ] } From 8eea89488c642f3d5f617fab5493d5f51f6f4ad0 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 1 Jul 2026 14:41:40 +0000 Subject: [PATCH 15/15] CHANGELOG: drop unreleased 0.30.0 stub Release-branch only; the stub tracks unreleased main work. --- CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fab173ba3..baf66829c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,5 @@ # CHANGELOG -## 0.30.0 (202x-xx-xx) - -**Minimum supported Tailscale client version: v1.xx.0** - ## 0.29.2 (2026-07-01) **Minimum supported Tailscale client version: v1.80.0**