Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] testing for server_url containing base_domain is too restrictive #2210

Open
2 of 4 tasks
quite opened this issue Oct 24, 2024 · 7 comments · May be fixed by #2248
Open
2 of 4 tasks

[Bug] testing for server_url containing base_domain is too restrictive #2210

quite opened this issue Oct 24, 2024 · 7 comments · May be fixed by #2248
Labels
bug Something isn't working

Comments

@quite
Copy link

quite commented Oct 24, 2024

Is this a support request?

  • This is not a support request

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

I have server_url: https://homer.example.com and dns.base_domain: h. This does not work, headscale complains with server_url cannot contain the base_domain [..].

@mtoohey31 recently noted the same issue in a comment to #2034:

I could be wrong, but I wonder if the string.Contains check might be too strict of a test. I want to use (for example) https://bar.foo.com as my server_url, and just foo as my dns.base_domain, but this check rejects that. I've manually removed the check and this seems to work without issue.

Expected Behavior

I don't want to be overly and unnecessarily restricted about the choice of base_domain.
I think that what should not be allowed are the following:

  • base_domain that is a prefix of server_url's hostname
  • base_domain equal to server_url's hostname
  • maybe also server_url's hostname that is a prefix of base_domain?

Possible plain prefix comparison is not suitable, but dot-separated parts needs to be taken into consideration separately? All this needs some more thinking through. Any ideas?

Steps To Reproduce

I have server_url: https://homer.example.com and dns.base_domain: h. This does not work, headscale complains with server_url cannot contain the base_domain [..].

Environment

- OS: Debian 12
- Headscale version: 0.23.0
- Tailscale version: 1.76.1

Runtime environment

  • Headscale is behind a (reverse) proxy
  • Headscale runs in a container

Anything else?

No response

@quite quite added the bug Something isn't working label Oct 24, 2024
@quite quite mentioned this issue Oct 24, 2024
4 tasks
@hopleus
Copy link
Contributor

hopleus commented Oct 28, 2024

I assume that using a second-level domain in dns.base_domain, may cause conflicts, for example, when using *.base.domain at NPM or CloudFlare level (example 1), as well as possible conflicts with server_url (example 2).

Example 1:
There is a test-stage.base.domain that leads to a specific portal.
If a node with the same name test-stage is added, the portal will never open because the dns headscale will see this DNS record.

Example 2:

server_url: https://gateway.headscale.net
dns.base_domain: headscale.net

Add a node with the name gateway, that's it, the Headscale API working on gateway.headscale.net is no longer working, so the connection with the TailScale client is lost.

With the current implementation it seems logical to allocate a separate subdomain for the HeadScale network

@motiejus
Copy link
Contributor

I stumbled upon the same issue while testing an upgrade to NixOS 24.11. Here is my config:

          server_url = "https://vpn.jakstys.lt";
          dns_config.base_domain = "jakst";

I don't care about the base_domain part (it's vanity). Is it safe to just change it on a running headscale instance with clients connected?

@kradalby
Copy link
Collaborator

I don't care about the base_domain part (it's vanity). Is it safe to just change it on a running headscale instance with clients connected?

Yes, its only added at runtime, no database involved.

@kradalby
Copy link
Collaborator

kradalby commented Nov 17, 2024

I could be wrong, but I wonder if the string.Contains check might be too strict of a test. I want to use (for example) https://bar.foo.com as my server_url, and just foo as my dns.base_domain, but this check rejects that. I've manually removed the check and this seems to work without issue.

I agree that it is a bit conservative. I am happy for a contribution relaxing it to ContainsSuffix given that:

  • potential characters that breaks the suffix are stripped
  • all "imaginable" test cases are added to ensure we dont have a silent breaking scenario causing support load.

Edit: parsing the full URL and comparing only the domain should probably be a good place to start.

@motiejus
Copy link
Contributor

Started implementing this.

Do I understand correctly that the Host parts of server_url and base_domain must be nonequal?

motiejus added a commit to motiejus/config that referenced this issue Nov 20, 2024
did not test samba and headscale yet

juanfont/headscale#2210 (comment)
@kradalby
Copy link
Collaborator

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.

motiejus added a commit to motiejus/headscale that referenced this issue Nov 21, 2024
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.

[1]: juanfont#2210 (comment)
motiejus added a commit to motiejus/headscale that referenced this issue Nov 21, 2024
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 juanfont#2210

[1]: juanfont#2210 (comment)
motiejus added a commit to motiejus/headscale that referenced this issue Nov 21, 2024
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 juanfont#2210

[1]: juanfont#2210 (comment)
motiejus added a commit to motiejus/headscale that referenced this issue Nov 21, 2024
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 juanfont#2210

[1]: juanfont#2210 (comment)
motiejus added a commit to motiejus/headscale that referenced this issue Nov 21, 2024
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 juanfont#2210

[1]: juanfont#2210 (comment)
@motiejus motiejus linked a pull request Nov 21, 2024 that will close this issue
6 tasks
@motiejus
Copy link
Contributor

#2248

motiejus added a commit to motiejus/nixpkgs that referenced this issue Nov 21, 2024
Currently users upgrading from 24.05 to 24.11 may stumble across an
overly-restrictive BaseURL and ServerURL check in headscale[1].

A fix has been merged upstream[2], this is backport, so users can have
it easier upgrading from 24.05 to 24.11.

[1]: juanfont/headscale#2210
[2]: juanfont/headscale#2248
motiejus added a commit to motiejus/nixpkgs that referenced this issue Nov 21, 2024
Currently users upgrading from 24.05 to 24.11 may stumble across an
overly-restrictive BaseURL and ServerURL check in headscale[1].

A fix has been merged upstream[2], this is backport, so users can have
it easier upgrading from 24.05 to 24.11.

[1]: juanfont/headscale#2210
[2]: juanfont/headscale#2248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants