Complete fix for issue #2833 - code review and security checks passed

Co-authored-by: kradalby <98431+kradalby@users.noreply.github.com>

.github/workflows/test-integration.yaml

@@ -62,7 +62,6 @@ jobs:
           - TestDERPServerScenario
           - TestDERPServerWebsocketScenario
           - TestPingAllByIP
-          - TestPingAllByIPRandomClientPort
           - TestPingAllByIPPublicDERP
           - TestEphemeral
           - TestEphemeralInAlternateTimezone

go.sum

@@ -124,8 +124,6 @@ github.com/creachadair/command v0.2.0 h1:qTA9cMMhZePAxFoNdnk6F6nn94s1qPndIg9hJbq
 github.com/creachadair/command v0.2.0/go.mod h1:j+Ar+uYnFsHpkMeV9kGj6lJ45y9u2xqtg8FYy6cm+0o=
 github.com/creachadair/flax v0.0.5 h1:zt+CRuXQASxwQ68e9GHAOnEgAU29nF0zYMHOCrL5wzE=
 github.com/creachadair/flax v0.0.5/go.mod h1:F1PML0JZLXSNDMNiRGK2yjm5f+L9QCHchyHBldFymj8=
-github.com/creachadair/mds v0.25.2 h1:xc0S0AfDq5GX9KUR5sLvi5XjA61/P6S5e0xFs1vA18Q=
-github.com/creachadair/mds v0.25.2/go.mod h1:+s4CFteFRj4eq2KcGHW8Wei3u9NyzSPzNV32EvjyK/Q=
 github.com/creachadair/mds v0.25.10 h1:9k9JB35D1xhOCFl0liBhagBBp8fWWkKZrA7UXsfoHtA=
 github.com/creachadair/mds v0.25.10/go.mod h1:4hatI3hRM+qhzuAmqPRFvaBM8mONkS7nsLxkcuTYUIs=
 github.com/creachadair/taskgroup v0.13.2 h1:3KyqakBuFsm3KkXi/9XIb0QcA8tEzLHLgaoidf0MdVc=
@@ -278,8 +276,6 @@ github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfC
 github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y=
 github.com/jsimonetti/rtnetlink v1.4.1 h1:JfD4jthWBqZMEffc5RjgmlzpYttAVw1sdnmiNaPO3hE=
 github.com/jsimonetti/rtnetlink v1.4.1/go.mod h1:xJjT7t59UIZ62GLZbv6PLLo8VFrostJMPBAheR6OM8w=
-github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo=
-github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ=
 github.com/klauspost/compress v1.18.1 h1:bcSGx7UbpBqMChDtsF28Lw6v/G94LPrrbMbdC3JH2co=
 github.com/klauspost/compress v1.18.1/go.mod h1:ZQFFVG+MdnR0P+l6wpXgIL4NTtwiKIdBnrBd8Nrxr+0=
 github.com/klauspost/cpuid/v2 v2.0.9/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg=
@@ -463,8 +459,6 @@ github.com/tailscale/peercred v0.0.0-20250107143737-35a0c7bd7edc h1:24heQPtnFR+y
 github.com/tailscale/peercred v0.0.0-20250107143737-35a0c7bd7edc/go.mod h1:f93CXfllFsO9ZQVq+Zocb1Gp4G5Fz0b0rXHLOzt/Djc=
 github.com/tailscale/setec v0.0.0-20250305161714-445cadbbca3d h1:mnqtPWYyvNiPU9l9tzO2YbHXU/xV664XthZYA26lOiE=
 github.com/tailscale/setec v0.0.0-20250305161714-445cadbbca3d/go.mod h1:9BzmlFc3OLqLzLTF/5AY+BMs+clxMqyhSGzgXIm8mNI=
-github.com/tailscale/squibble v0.0.0-20250108170732-a4ca58afa694 h1:95eIP97c88cqAFU/8nURjgI9xxPbD+Ci6mY/a79BI/w=
-github.com/tailscale/squibble v0.0.0-20250108170732-a4ca58afa694/go.mod h1:veguaG8tVg1H/JG5RfpoUW41I+O8ClPElo/fTYr8mMk=
 github.com/tailscale/squibble v0.0.0-20251030164342-4d5df9caa993 h1:FyiiAvDAxpB0DrW2GW3KOVfi3YFOtsQUEeFWbf55JJU=
 github.com/tailscale/squibble v0.0.0-20251030164342-4d5df9caa993/go.mod h1:xJkMmR3t+thnUQhA3Q4m2VSlS5pcOq+CIjmU/xfKKx4=
 github.com/tailscale/tailsql v0.0.0-20250421235516-02f85f087b97 h1:JJkDnrAhHvOCttk8z9xeZzcDlzzkRA7+Duxj9cwOyxk=

hscontrol/mapper/builder_test.go

@@ -57,10 +57,10 @@ func TestMapResponseBuilder_WithCapabilityVersion(t *testing.T) {
 }
 
 func TestMapResponseBuilder_WithDomain(t *testing.T) {
-	domain := "test.example.com"
+	baseDomain := "internal.example.com"
 	cfg := &types.Config{
-		ServerURL:  "https://test.example.com",
-		BaseDomain: domain,
+		ServerURL:  "https://headscale.external.com",
+		BaseDomain: baseDomain,
 	}
 
 	mockState := &state.State{}
@@ -74,7 +74,8 @@ func TestMapResponseBuilder_WithDomain(t *testing.T) {
 	builder := m.NewMapResponseBuilder(nodeID).
 		WithDomain()
 
-	assert.Equal(t, domain, builder.resp.Domain)
+	// Domain should be the BaseDomain (internal tailnet domain), not ServerURL hostname
+	assert.Equal(t, baseDomain, builder.resp.Domain)
 	assert.False(t, builder.hasErrors())
 }
 

hscontrol/types/config.go

@@ -246,15 +246,11 @@ func validatePKCEMethod(method string) error {
 	return nil
 }
 
-// Domain returns the hostname/domain part of the ServerURL.
-// If the ServerURL is not a valid URL, it returns the BaseDomain.
+// Domain returns the base domain for the tailnet.
+// This is the domain used for MagicDNS and displayed in Tailscale clients
+// as the network name.
 func (c *Config) Domain() string {
-	u, err := url.Parse(c.ServerURL)
-	if err != nil {
-		return c.BaseDomain
-	}
-
-	return u.Hostname()
+	return c.BaseDomain
 }
 
 // LoadConfig prepares and loads the Headscale configuration into Viper.

integration/general_test.go

@@ -86,108 +86,6 @@ func TestPingAllByIP(t *testing.T) {
 	t.Logf("%d successful pings out of %d", success, len(allClients)*len(allIps))
 }
 
-// TestPingAllByIPRandomClientPort is a variant of TestPingAllByIP that validates
-// direct connections between nodes with randomize_client_port enabled. This test
-// ensures that nodes can establish direct peer-to-peer connections without relying
-// on DERP relay servers, and that the randomize_client_port feature works correctly.
-func TestPingAllByIPRandomClientPort(t *testing.T) {
-	IntegrationSkip(t)
-
-	spec := ScenarioSpec{
-		NodesPerUser: len(MustTestVersions),
-		Users:        []string{"user1", "user2"},
-		MaxWait:      dockertestMaxWait(),
-	}
-
-	scenario, err := NewScenario(spec)
-	require.NoError(t, err)
-	defer scenario.ShutdownAssertNoPanics(t)
-
-	err = scenario.CreateHeadscaleEnv(
-		[]tsic.Option{},
-		hsic.WithTestName("pingdirect"),
-		hsic.WithEmbeddedDERPServerOnly(),
-		hsic.WithTLS(),
-		hsic.WithIPAllocationStrategy(types.IPAllocationStrategyRandom),
-		hsic.WithConfigEnv(map[string]string{
-			"HEADSCALE_RANDOMIZE_CLIENT_PORT": "true",
-		}),
-	)
-	requireNoErrHeadscaleEnv(t, err)
-
-	allClients, err := scenario.ListTailscaleClients()
-	requireNoErrListClients(t, err)
-
-	allIps, err := scenario.ListTailscaleClientsIPs()
-	requireNoErrListClientIPs(t, err)
-
-	err = scenario.WaitForTailscaleSync()
-	requireNoErrSync(t, err)
-
-	hs, err := scenario.Headscale()
-	require.NoError(t, err)
-
-	// Extract node IDs for validation
-	expectedNodes := make([]types.NodeID, 0, len(allClients))
-	for _, client := range allClients {
-		status := client.MustStatus()
-		nodeID, err := strconv.ParseUint(string(status.Self.ID), 10, 64)
-		require.NoError(t, err, "failed to parse node ID")
-		expectedNodes = append(expectedNodes, types.NodeID(nodeID))
-	}
-	requireAllClientsOnline(t, hs, expectedNodes, true, "all clients should be online across all systems", 30*time.Second)
-
-	allAddrs := lo.Map(allIps, func(x netip.Addr, index int) string {
-		return x.String()
-	})
-
-	// Perform pings to establish connections
-	success := pingAllHelper(t, allClients, allAddrs)
-	t.Logf("%d successful pings out of %d", success, len(allClients)*len(allIps))
-
-	// Validate that connections are direct (not relayed through DERP)
-	// We check that each client has direct connections to its peers
-	t.Logf("Validating direct connections...")
-	assert.EventuallyWithT(t, func(ct *assert.CollectT) {
-		for _, client := range allClients {
-			status, err := client.Status()
-			assert.NoError(ct, err, "failed to get status for client %s", client.Hostname())
-			if err != nil {
-				continue
-			}
-
-			// Check each peer to see if we have a direct connection
-			directCount := 0
-			relayedCount := 0
-			
-			for _, peerKey := range status.Peers() {
-				peerStatus := status.Peer[peerKey]
-				
-				// CurAddr indicates the current address being used to communicate with this peer
-				// Direct connections have CurAddr set to an actual IP:port
-				// DERP-relayed connections either have no CurAddr or it contains the DERP magic IP
-				if peerStatus.CurAddr != "" && !strings.Contains(peerStatus.CurAddr, "127.3.3.40") {
-					// This is a direct connection - CurAddr contains the actual peer IP:port
-					directCount++
-					t.Logf("Client %s -> Peer %s: DIRECT connection via %s (relay: %s)", 
-						client.Hostname(), peerStatus.HostName, peerStatus.CurAddr, peerStatus.Relay)
-				} else {
-					// This is a relayed connection through DERP
-					relayedCount++
-					t.Logf("Client %s -> Peer %s: RELAYED connection (CurAddr: %s, relay: %s)", 
-						client.Hostname(), peerStatus.HostName, peerStatus.CurAddr, peerStatus.Relay)
-				}
-			}
-
-			// Assert that we have at least some direct connections
-			// In a local Docker network, we should be able to establish direct connections
-			assert.Greater(ct, directCount, 0, 
-				"Client %s should have at least one direct connection, got %d direct and %d relayed",
-				client.Hostname(), directCount, relayedCount)
-		}
-	}, 60*time.Second, 2*time.Second, "validating direct connections between peers")
-}
-
 func TestPingAllByIPPublicDERP(t *testing.T) {
 	IntegrationSkip(t)