From c4ac626298a2503ec4ec5dfbf465c2004dad5a8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Motiejus=20Jak=C5=A1tys?= Date: Thu, 21 Nov 2024 06:28:45 +0200 Subject: [PATCH] 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]: https://github.com/juanfont/headscale/issues/2210#issuecomment-2488165187 --- CHANGELOG.md | 1 + hscontrol/types/config.go | 45 +++++++++++--- hscontrol/types/config_test.go | 59 ++++++++++++++++++- .../testdata/base-domain-in-server-url.yaml | 2 +- 4 files changed, 98 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d435d04..9ca0ed05 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index ec963793..114ee015 100644 --- a/hscontrol/types/config.go +++ b/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 a suffix of the base_domain, this will cause the headscale server and embedded DERP to become unreachable from the Tailscale node.") ) 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 := 1; i < len(baseDomainParts)-1; i++ { + if serverDomainParts[s-i] != baseDomainParts[b-i] { + return nil + } + } + + return errServerURLSuffix +} + type deprecator struct { warns set.Set[string] fatals set.Set[string] diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index 70c0ce7a..f4ed3501 100644 --- a/hscontrol/types/config_test.go +++ b/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: "server_url cannot be a suffix of the base_domain, this will cause the headscale server and embedded DERP to become unreachable from the Tailscale node.", }, { name: "base-domain-not-in-server-url", @@ -333,3 +334,59 @@ 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://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) + }) + } +} diff --git a/hscontrol/types/testdata/base-domain-in-server-url.yaml b/hscontrol/types/testdata/base-domain-in-server-url.yaml index 683e0218..2d6a4694 100644 --- a/hscontrol/types/testdata/base-domain-in-server-url.yaml +++ b/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