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 1/4] 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 From e83401731429975b1f32066b9047f919c1a8ff66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Motiejus=20Jak=C5=A1tys?= Date: Thu, 21 Nov 2024 17:00:26 +0200 Subject: [PATCH 2/4] server_url and base_domain: re-word error message, fix a one-off bug and add a test case for the bug. --- hscontrol/types/config.go | 6 +++--- hscontrol/types/config_test.go | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 114ee015..21bade73 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -30,7 +30,7 @@ const ( 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.") + 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 @@ -946,8 +946,8 @@ func isSafeServerURL(serverURL, baseDomain string) error { s := len(serverDomainParts) b := len(baseDomainParts) - for i := 1; i < len(baseDomainParts)-1; i++ { - if serverDomainParts[s-i] != baseDomainParts[b-i] { + for i := 0; i < len(baseDomainParts); i++ { + if serverDomainParts[s-i-1] != baseDomainParts[b-i-1] { return nil } } diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index f4ed3501..54de13d1 100644 --- a/hscontrol/types/config_test.go +++ b/hscontrol/types/config_test.go @@ -140,7 +140,7 @@ func TestReadConfig(t *testing.T) { return LoadServerConfig() }, want: nil, - 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.", + wantErr: errServerURLSuffix.Error(), }, { name: "base-domain-not-in-server-url", @@ -362,6 +362,10 @@ func TestSafeServerURL(t *testing.T) { serverURL: "https://headscale.com", baseDomain: "clients.subdomain.headscale.com", }, + { + serverURL: "https://headscale.kristoffer.com", + baseDomain: "mybase", + }, { serverURL: "https://server.headscale.com", baseDomain: "headscale.com", From ddb1370c734629bae3b55f652f5aadf0a0581308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Motiejus=20Jak=C5=A1tys?= Date: Thu, 21 Nov 2024 20:04:48 +0200 Subject: [PATCH 3/4] lint --- hscontrol/types/config.go | 10 +++++----- hscontrol/types/config_test.go | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 21bade73..afa94e83 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -30,7 +30,7 @@ const ( 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.") + 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 @@ -928,9 +928,9 @@ func LoadServerConfig() (*Config, error) { // 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) +// - 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 { @@ -946,7 +946,7 @@ func isSafeServerURL(serverURL, baseDomain string) error { s := len(serverDomainParts) b := len(baseDomainParts) - for i := 0; i < len(baseDomainParts); i++ { + for i := range 0..len(baseDomainParts) { if serverDomainParts[s-i-1] != baseDomainParts[b-i-1] { return nil } diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index 54de13d1..b36b376e 100644 --- a/hscontrol/types/config_test.go +++ b/hscontrol/types/config_test.go @@ -340,7 +340,7 @@ tls_letsencrypt_challenge_type: TLS-ALPN-01 // server_url: headscale.com, base: headscale.net // // NOT OK -// server_url: server.headscale.com, base: headscale.com +// server_url: server.headscale.com, base: headscale.com. func TestSafeServerURL(t *testing.T) { tests := []struct { serverURL, baseDomain, @@ -388,6 +388,7 @@ func TestSafeServerURL(t *testing.T) { err := isSafeServerURL(tt.serverURL, tt.baseDomain) if tt.wantErr != "" { assert.EqualError(t, err, tt.wantErr) + return } assert.NoError(t, err) From c0c383359fa255030c609a857e307c5c30f875a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Motiejus=20Jak=C5=A1tys?= Date: Thu, 21 Nov 2024 20:15:07 +0200 Subject: [PATCH 4/4] lint again --- hscontrol/types/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index afa94e83..5895ebc9 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -946,7 +946,7 @@ func isSafeServerURL(serverURL, baseDomain string) error { s := len(serverDomainParts) b := len(baseDomainParts) - for i := range 0..len(baseDomainParts) { + for i := range len(baseDomainParts) { if serverDomainParts[s-i-1] != baseDomainParts[b-i-1] { return nil }