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

MagicDNS doesn't register hostnames with upper-case letters #363

Closed
skoobasteeve opened this issue Feb 27, 2022 · 8 comments · Fixed by #387
Closed

MagicDNS doesn't register hostnames with upper-case letters #363

skoobasteeve opened this issue Feb 27, 2022 · 8 comments · Fixed by #387
Labels
bug Something isn't working

Comments

@skoobasteeve
Copy link

skoobasteeve commented Feb 27, 2022

Bug description

Machines are not added to MagicDNS if their hostname contains capital letters.

To Reproduce

  1. Set hostname on client system to something with capital letters e.g. Foo-Bar.
  2. Enroll with Headscale server using tailscale up --login-server <URL> --auth-key <AUTH_KEY>
  3. Try to ping the client system from another client using Foo-Bar or Foo-Bar..
    OR run nslookup Foo-Bar
  4. Receive failures on both commands
  5. Re-connect to the Headscale server and manually set hostname to lower-case letters using tailscale up --hostname=foo-bar --login-server <URL>
  6. Ping and nslookup are both able to find the system at foo-bar.

Context info

  • Headscale version: 14.0
  • Tailscale client: 1.22
  • Server OS: Ubuntu 20.04
  • Client OS: Ubuntu 20.04

Let me know if this is a known issue or if anyone else can reproduce it. I have limited systems to test on.

Thank you!

@skoobasteeve skoobasteeve added the bug Something isn't working label Feb 27, 2022
@kyhwana
Copy link
Contributor

kyhwana commented Feb 28, 2022

Hmm, appears to work with tailscale ping, but not nslookup or ping in linux..

@kyhwana
Copy link
Contributor

kyhwana commented Feb 28, 2022

There's something funky somewhere.. Adding hostname = strings.ToLower(hostname) at line 616 in machine.go fixes this.
(As in)

} else {
		hostname = machine.Name
	}
	hostname = strings.ToLower(hostname)

	node := tailcfg.Node{

@restanrm
Copy link
Contributor

In your opinion, should we normalize the machine.Name with

headscale/namespaces.go

Lines 242 to 264 in b1bd17f

func NormalizeNamespaceName(name string, stripEmailDomain bool) (string, error) {
name = strings.ToLower(name)
name = strings.ReplaceAll(name, "'", "")
atIdx := strings.Index(name, "@")
if stripEmailDomain && atIdx > 0 {
name = name[:atIdx]
} else {
name = strings.ReplaceAll(name, "@", ".")
}
name = invalidCharsInNamespaceRegex.ReplaceAllString(name, "-")
for _, elt := range strings.Split(name, ".") {
if len(elt) > labelHostnameLength {
return "", fmt.Errorf(
"label %v is more than 63 chars: %w",
elt,
errInvalidNamespaceName,
)
}
}
return name, nil
}
or accept Uppercase letters ?

@kyhwana
Copy link
Contributor

kyhwana commented Feb 28, 2022

@restanrm I think it depends where the actual issue with upper case letters affecting magicDNS is? It just needs to be done before wherever they get added to magicdns dns entries. (From what I can tell)

@e-zk
Copy link
Contributor

e-zk commented Mar 2, 2022

Let me know if this is a known issue or if anyone else can reproduce it.

Not sure if it's directly related to this issue, but my Android phone with hostname "Pixel 4a" (shown via tailscale status), doesn't resolve with nslookup pixel-4a nor, nslookup Pixel-4a.

@ohdearaugustin
Copy link
Collaborator

I guess the magicdns should be conform to the RFC https://tools.ietf.org/html/rfc4343. Therefore we should accept uppercase letters in the dns name.

@kradalby
Copy link
Collaborator

kradalby commented Mar 3, 2022

If the issue is that uppercase is not accepted, then I think we should just lowercase it at that point, and let users have uppercase if they join with that.

I think it would be too much a nightmare to deny uppercase.

I would agree that all names should be unique in lowercase and therefor a normalise to lowercase before storing could make sense regardless. We need them to be unique

@restanrm
Copy link
Contributor

restanrm commented Mar 3, 2022

@ohdearaugustin this RFC seems to have a lot of uncertainty about the behavior that we should expect (§4.2). Also if we apply this we should escape all special chars instead of replacing them (if I understand that RFC properly).

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.

6 participants