config: loosen up BaseDomain and ServerURL checks (#2248)

* config: loosen up BaseDomain and ServerURL checks

Requirements [here][1]:

> OK:
> server_url: headscale.com, base: clients.headscale.com
> server_url: headscale.com, base: headscale.net
>
> Not OK:
> server_url: server.headscale.com, base: headscale.com
>
> Essentially we have to prevent the possibility where the headscale
> server has a URL which can also be assigned to a node.
>
> So for the Not OK scenario:
>
> if the server is: server.headscale.com, and a node joins with the name
> server, it will be assigned server.headscale.com and that will break
> the connection for nodes which will now try to connect to that node
> instead of the headscale server.

Fixes #2210

[1]: #2210 (comment)

* server_url and base_domain: re-word error message, fix a one-off bug and add a test case for the bug.

* lint

* lint again

CHANGELOG.md

@@ -23,6 +23,7 @@
 - Added conversion of 'Hostname' to 'givenName' in a node with FQDN rules applied [#2198](https://github.com/juanfont/headscale/pull/2198)
 - Fixed updating of hostname and givenName when it is updated in HostInfo [#2199](https://github.com/juanfont/headscale/pull/2199)
 - Fixed missing `stable-debug` container tag [#2232](https://github.com/juanfont/headscale/pr/2232)
+- Loosened up `server_url` and `base_domain` check. It was overly strict in some cases.
 
 ## 0.23.0 (2024-09-18)
 

hscontrol/types/config.go

@@ -28,8 +28,9 @@ const (
 	maxDuration           time.Duration = 1<<63 - 1
 )
 
-var errOidcMutuallyExclusive = errors.New(
-	"oidc_client_secret and oidc_client_secret_path are mutually exclusive",
+var (
+	errOidcMutuallyExclusive = errors.New("oidc_client_secret and oidc_client_secret_path are mutually exclusive")
+	errServerURLSuffix       = errors.New("server_url cannot be part of base_domain in a way that could make the DERP and headscale server unreachable")
 )
 
 type IPAllocationStrategy string
@@ -827,11 +828,10 @@ func LoadServerConfig() (*Config, error) {
 	// - DERP run on their own domains
 	// - Control plane runs on login.tailscale.com/controlplane.tailscale.com
 	// - MagicDNS (BaseDomain) for users is on a *.ts.net domain per tailnet (e.g. tail-scale.ts.net)
-	if dnsConfig.BaseDomain != "" &&
-		strings.Contains(serverURL, dnsConfig.BaseDomain) {
-		return nil, errors.New(
-			"server_url cannot contain the base_domain, this will cause the headscale server and embedded DERP to become unreachable from the Tailscale node.",
-		)
+	if dnsConfig.BaseDomain != "" {
+		if err := isSafeServerURL(serverURL, dnsConfig.BaseDomain); err != nil {
+			return nil, err
+		}
 	}
 
 	return &Config{
@@ -924,6 +924,37 @@ func LoadServerConfig() (*Config, error) {
 	}, nil
 }
 
+// BaseDomain cannot be a suffix of the server URL.
+// This is because Tailscale takes over the domain in BaseDomain,
+// causing the headscale server and DERP to be unreachable.
+// For Tailscale upstream, the following is true:
+// - DERP run on their own domains.
+// - Control plane runs on login.tailscale.com/controlplane.tailscale.com.
+// - MagicDNS (BaseDomain) for users is on a *.ts.net domain per tailnet (e.g. tail-scale.ts.net).
+func isSafeServerURL(serverURL, baseDomain string) error {
+	server, err := url.Parse(serverURL)
+	if err != nil {
+		return err
+	}
+
+	serverDomainParts := strings.Split(server.Host, ".")
+	baseDomainParts := strings.Split(baseDomain, ".")
+
+	if len(serverDomainParts) <= len(baseDomainParts) {
+		return nil
+	}
+
+	s := len(serverDomainParts)
+	b := len(baseDomainParts)
+	for i := range len(baseDomainParts) {
+		if serverDomainParts[s-i-1] != baseDomainParts[b-i-1] {
+			return nil
+		}
+	}
+
+	return errServerURLSuffix
+}
+
 type deprecator struct {
 	warns  set.Set[string]
 	fatals set.Set[string]

hscontrol/types/config_test.go

@@ -1,6 +1,7 @@
 package types
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -139,7 +140,7 @@ func TestReadConfig(t *testing.T) {
 				return LoadServerConfig()
 			},
 			want:    nil,
-			wantErr: "server_url cannot contain the base_domain, this will cause the headscale server and embedded DERP to become unreachable from the Tailscale node.",
+			wantErr: errServerURLSuffix.Error(),
 		},
 		{
 			name:       "base-domain-not-in-server-url",
@@ -333,3 +334,64 @@ tls_letsencrypt_challenge_type: TLS-ALPN-01
 	err = LoadConfig(tmpDir, false)
 	assert.NoError(t, err)
 }
+
+// OK
+// server_url: headscale.com, base: clients.headscale.com
+// server_url: headscale.com, base: headscale.net
+//
+// NOT OK
+// server_url: server.headscale.com, base: headscale.com.
+func TestSafeServerURL(t *testing.T) {
+	tests := []struct {
+		serverURL, baseDomain,
+		wantErr string
+	}{
+		{
+			serverURL:  "https://example.com",
+			baseDomain: "example.org",
+		},
+		{
+			serverURL:  "https://headscale.com",
+			baseDomain: "headscale.com",
+		},
+		{
+			serverURL:  "https://headscale.com",
+			baseDomain: "clients.headscale.com",
+		},
+		{
+			serverURL:  "https://headscale.com",
+			baseDomain: "clients.subdomain.headscale.com",
+		},
+		{
+			serverURL:  "https://headscale.kristoffer.com",
+			baseDomain: "mybase",
+		},
+		{
+			serverURL:  "https://server.headscale.com",
+			baseDomain: "headscale.com",
+			wantErr:    errServerURLSuffix.Error(),
+		},
+		{
+			serverURL:  "https://server.subdomain.headscale.com",
+			baseDomain: "headscale.com",
+			wantErr:    errServerURLSuffix.Error(),
+		},
+		{
+			serverURL: "http://foo\x00",
+			wantErr:   `parse "http://foo\x00": net/url: invalid control character in URL`,
+		},
+	}
+
+	for _, tt := range tests {
+		testName := fmt.Sprintf("server=%s domain=%s", tt.serverURL, tt.baseDomain)
+		t.Run(testName, func(t *testing.T) {
+			err := isSafeServerURL(tt.serverURL, tt.baseDomain)
+			if tt.wantErr != "" {
+				assert.EqualError(t, err, tt.wantErr)
+
+				return
+			}
+			assert.NoError(t, err)
+		})
+	}
+}

hscontrol/types/testdata/base-domain-in-server-url.yaml

@@ -8,7 +8,7 @@ prefixes:
 database:
   type: sqlite3
 
-server_url: "https://derp.no"
+server_url: "https://server.derp.no"
 
 dns:
   magic_dns: true