|
| 1 | +From 6ba8990b0b982b261b0b549080a2f7f780cc70d6 Mon Sep 17 00:00:00 2001 |
| 2 | +From: =?UTF-8?q?Motiejus=20Jak=C5=A1tys?= < [email protected]> |
| 3 | +Date: Thu, 21 Nov 2024 06:28:45 +0200 |
| 4 | +Subject: [PATCH] config: loosen up BaseDomain and ServerURL checks |
| 5 | + |
| 6 | +Requirements [here][1]: |
| 7 | + |
| 8 | +> OK: |
| 9 | +> server_url: headscale.com, base: clients.headscale.com |
| 10 | +> server_url: headscale.com, base: headscale.net |
| 11 | +> |
| 12 | +> Not OK: |
| 13 | +> server_url: server.headscale.com, base: headscale.com |
| 14 | +> |
| 15 | +> Essentially we have to prevent the possibility where the headscale |
| 16 | +> server has a URL which can also be assigned to a node. |
| 17 | +> |
| 18 | +> So for the Not OK scenario: |
| 19 | +> |
| 20 | +> if the server is: server.headscale.com, and a node joins with the name |
| 21 | +> server, it will be assigned server.headscale.com and that will break |
| 22 | +> the connection for nodes which will now try to connect to that node |
| 23 | +> instead of the headscale server. |
| 24 | + |
| 25 | +Fixes #2210 |
| 26 | + |
| 27 | +[1]: https://github.com/juanfont/headscale/issues/2210#issuecomment-2488165187 |
| 28 | +--- |
| 29 | + hscontrol/types/config.go | 44 +++++++++++-- |
| 30 | + hscontrol/types/config_test.go | 64 ++++++++++++++++++- |
| 31 | + .../testdata/base-domain-in-server-url.yaml | 2 +- |
| 32 | + 3 files changed, 102 insertions(+), 8 deletions(-) |
| 33 | + |
| 34 | +diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go |
| 35 | +index 50ce2f075f4c..b10118aaeade 100644 |
| 36 | +--- a/hscontrol/types/config.go |
| 37 | ++++ b/hscontrol/types/config.go |
| 38 | +@@ -28,8 +28,9 @@ const ( |
| 39 | + maxDuration time.Duration = 1<<63 - 1 |
| 40 | + ) |
| 41 | + |
| 42 | +-var errOidcMutuallyExclusive = errors.New( |
| 43 | +- "oidc_client_secret and oidc_client_secret_path are mutually exclusive", |
| 44 | ++var ( |
| 45 | ++ errOidcMutuallyExclusive = errors.New("oidc_client_secret and oidc_client_secret_path are mutually exclusive") |
| 46 | ++ errServerURLSuffix = errors.New("server_url cannot be part of base_domain in a way that could make the DERP and headscale server unreachable") |
| 47 | + ) |
| 48 | + |
| 49 | + type IPAllocationStrategy string |
| 50 | +@@ -814,10 +815,10 @@ func LoadServerConfig() (*Config, error) { |
| 51 | + // - DERP run on their own domains |
| 52 | + // - Control plane runs on login.tailscale.com/controlplane.tailscale.com |
| 53 | + // - MagicDNS (BaseDomain) for users is on a *.ts.net domain per tailnet (e.g. tail-scale.ts.net) |
| 54 | +- // |
| 55 | +- // TODO(kradalby): remove dnsConfig.UserNameInMagicDNS check when removed. |
| 56 | +- if !dnsConfig.UserNameInMagicDNS && dnsConfig.BaseDomain != "" && strings.Contains(serverURL, dnsConfig.BaseDomain) { |
| 57 | +- 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.") |
| 58 | ++ if !dnsConfig.UserNameInMagicDNS && dnsConfig.BaseDomain != "" { |
| 59 | ++ if err := isSafeServerURL(serverURL, dnsConfig.BaseDomain); err != nil { |
| 60 | ++ return nil, err |
| 61 | ++ } |
| 62 | + } |
| 63 | + |
| 64 | + return &Config{ |
| 65 | +@@ -910,6 +911,37 @@ func LoadServerConfig() (*Config, error) { |
| 66 | + }, nil |
| 67 | + } |
| 68 | + |
| 69 | ++// BaseDomain cannot be a suffix of the server URL. |
| 70 | ++// This is because Tailscale takes over the domain in BaseDomain, |
| 71 | ++// causing the headscale server and DERP to be unreachable. |
| 72 | ++// For Tailscale upstream, the following is true: |
| 73 | ++// - DERP run on their own domains. |
| 74 | ++// - Control plane runs on login.tailscale.com/controlplane.tailscale.com. |
| 75 | ++// - MagicDNS (BaseDomain) for users is on a *.ts.net domain per tailnet (e.g. tail-scale.ts.net). |
| 76 | ++func isSafeServerURL(serverURL, baseDomain string) error { |
| 77 | ++ server, err := url.Parse(serverURL) |
| 78 | ++ if err != nil { |
| 79 | ++ return err |
| 80 | ++ } |
| 81 | ++ |
| 82 | ++ serverDomainParts := strings.Split(server.Host, ".") |
| 83 | ++ baseDomainParts := strings.Split(baseDomain, ".") |
| 84 | ++ |
| 85 | ++ if len(serverDomainParts) <= len(baseDomainParts) { |
| 86 | ++ return nil |
| 87 | ++ } |
| 88 | ++ |
| 89 | ++ s := len(serverDomainParts) |
| 90 | ++ b := len(baseDomainParts) |
| 91 | ++ for i := range len(baseDomainParts) { |
| 92 | ++ if serverDomainParts[s-i-1] != baseDomainParts[b-i-1] { |
| 93 | ++ return nil |
| 94 | ++ } |
| 95 | ++ } |
| 96 | ++ |
| 97 | ++ return errServerURLSuffix |
| 98 | ++} |
| 99 | ++ |
| 100 | + type deprecator struct { |
| 101 | + warns set.Set[string] |
| 102 | + fatals set.Set[string] |
| 103 | +diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go |
| 104 | +index e6e8d6c2e0b1..68a13f6c0f40 100644 |
| 105 | +--- a/hscontrol/types/config_test.go |
| 106 | ++++ b/hscontrol/types/config_test.go |
| 107 | +@@ -1,6 +1,7 @@ |
| 108 | + package types |
| 109 | + |
| 110 | + import ( |
| 111 | ++ "fmt" |
| 112 | + "os" |
| 113 | + "path/filepath" |
| 114 | + "testing" |
| 115 | +@@ -141,7 +142,7 @@ func TestReadConfig(t *testing.T) { |
| 116 | + return LoadServerConfig() |
| 117 | + }, |
| 118 | + want: nil, |
| 119 | +- wantErr: "server_url cannot contain the base_domain, this will cause the headscale server and embedded DERP to become unreachable from the Tailscale node.", |
| 120 | ++ wantErr: errServerURLSuffix.Error(), |
| 121 | + }, |
| 122 | + { |
| 123 | + name: "base-domain-not-in-server-url", |
| 124 | +@@ -337,3 +338,64 @@ tls_letsencrypt_challenge_type: TLS-ALPN-01 |
| 125 | + err = LoadConfig(tmpDir, false) |
| 126 | + assert.NoError(t, err) |
| 127 | + } |
| 128 | ++ |
| 129 | ++// OK |
| 130 | ++// server_url: headscale.com, base: clients.headscale.com |
| 131 | ++// server_url: headscale.com, base: headscale.net |
| 132 | ++// |
| 133 | ++// NOT OK |
| 134 | ++// server_url: server.headscale.com, base: headscale.com. |
| 135 | ++func TestSafeServerURL(t *testing.T) { |
| 136 | ++ tests := []struct { |
| 137 | ++ serverURL, baseDomain, |
| 138 | ++ wantErr string |
| 139 | ++ }{ |
| 140 | ++ { |
| 141 | ++ serverURL: "https://example.com", |
| 142 | ++ baseDomain: "example.org", |
| 143 | ++ }, |
| 144 | ++ { |
| 145 | ++ serverURL: "https://headscale.com", |
| 146 | ++ baseDomain: "headscale.com", |
| 147 | ++ }, |
| 148 | ++ { |
| 149 | ++ serverURL: "https://headscale.com", |
| 150 | ++ baseDomain: "clients.headscale.com", |
| 151 | ++ }, |
| 152 | ++ { |
| 153 | ++ serverURL: "https://headscale.com", |
| 154 | ++ baseDomain: "clients.subdomain.headscale.com", |
| 155 | ++ }, |
| 156 | ++ { |
| 157 | ++ serverURL: "https://headscale.kristoffer.com", |
| 158 | ++ baseDomain: "mybase", |
| 159 | ++ }, |
| 160 | ++ { |
| 161 | ++ serverURL: "https://server.headscale.com", |
| 162 | ++ baseDomain: "headscale.com", |
| 163 | ++ wantErr: errServerURLSuffix.Error(), |
| 164 | ++ }, |
| 165 | ++ { |
| 166 | ++ serverURL: "https://server.subdomain.headscale.com", |
| 167 | ++ baseDomain: "headscale.com", |
| 168 | ++ wantErr: errServerURLSuffix.Error(), |
| 169 | ++ }, |
| 170 | ++ { |
| 171 | ++ serverURL: "http://foo\x00", |
| 172 | ++ wantErr: `parse "http://foo\x00": net/url: invalid control character in URL`, |
| 173 | ++ }, |
| 174 | ++ } |
| 175 | ++ |
| 176 | ++ for _, tt := range tests { |
| 177 | ++ testName := fmt.Sprintf("server=%s domain=%s", tt.serverURL, tt.baseDomain) |
| 178 | ++ t.Run(testName, func(t *testing.T) { |
| 179 | ++ err := isSafeServerURL(tt.serverURL, tt.baseDomain) |
| 180 | ++ if tt.wantErr != "" { |
| 181 | ++ assert.EqualError(t, err, tt.wantErr) |
| 182 | ++ |
| 183 | ++ return |
| 184 | ++ } |
| 185 | ++ assert.NoError(t, err) |
| 186 | ++ }) |
| 187 | ++ } |
| 188 | ++} |
| 189 | +diff --git a/hscontrol/types/testdata/base-domain-in-server-url.yaml b/hscontrol/types/testdata/base-domain-in-server-url.yaml |
| 190 | +index 683e021837c9..2d6a4694a09a 100644 |
| 191 | +--- a/hscontrol/types/testdata/base-domain-in-server-url.yaml |
| 192 | ++++ b/hscontrol/types/testdata/base-domain-in-server-url.yaml |
| 193 | +@@ -8,7 +8,7 @@ prefixes: |
| 194 | + database: |
| 195 | + type: sqlite3 |
| 196 | + |
| 197 | +-server_url: "https://derp.no" |
| 198 | ++server_url: "https://server.derp.no" |
| 199 | + |
| 200 | + dns: |
| 201 | + magic_dns: true |
| 202 | +-- |
| 203 | +2.47.0 |
| 204 | + |
0 commit comments