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

net: LookupNS() cannot be used to discover root name servers #45715

Closed
markdingo opened this issue Apr 23, 2021 · 16 comments
Closed

net: LookupNS() cannot be used to discover root name servers #45715

markdingo opened this issue Apr 23, 2021 · 16 comments
Assignees
Milestone

Comments

@markdingo
Copy link

@markdingo markdingo commented Apr 23, 2021

What version of Go are you using (go version)?

go version go1.16.3 freebsd/arm64
go version go1.16.3 darwin/arm64
go version go1.16.3 freebsd/amd64
go version go1.11.6 linux/amd64

Does this issue reproduce with the latest release?

Yes, any recent release and on multiple platforms.

What did you do?

package main

import (
        "fmt"
        "net"
)

const hardCodedRiskyName = "root-servers.net."

func main() {
        for _, n := range []string{"", ".", hardCodedRiskyName} {
                results, err := net.LookupNS(n)
                if err != nil {
                        fmt.Printf("Failed using '%s' with error %s\n", n, err.Error())
                } else {
                        fmt.Printf("Works using '%s', giving %d\n", n, len(results))
                }
        }
}

What did you expect to see?

$ go run gr.go
Works using '', giving 13
Works using '.', giving 13
Works using 'root-servers.net.', giving 13

I would expect at least one of the empty string or '.' to return the list of root name servers, much as is returned by the command dig . ns. I think I'd prefer '.' but both should result in a zero-length qname NS query sent to the local resolver.

What did you see instead?

$ go run gr.go
Failed using '' with error lookup : no such host
Failed using '.' with error lookup .: no such host
Works using 'root-servers.net.', giving 13

Which means the only way to get the root name servers is to use a hard-coded name, which while probably safe for the foreseeable future, is not as future-proof as the moral equivalent of dig . ns.

The root cause (ahem) appears to be that src/net/dnsclient.go:isDomainName() disallows the singular "." and the empty string as valid domain names for any type of query, including NS.

Alternatives

I'm happy to use an alternative mechanism within the net package, but a fairly decent look at the source code suggests that there are no lower-level functions which bypass isDomainName().

@markdingo
Copy link
Author

@markdingo markdingo commented Apr 23, 2021

Ah. I see this issue has been prosecuted before in #12421 and #1167 and ultimately frozen due to age.

On further reflection, it's not clear that isDomainName() is adding any value to the lookup process.

First off, the test is at the wrong level as dnsclient_unix.go:Resolver.lookup() is not type-aware and thus can have no clue as to which types have what label constraints.

Second off, RFC1034 is quite explicit about allowing any characters as labels in future possible query types ("The rationale for this choice is that we may someday need to add full binary domain names for new services") and only recommends restricting labels as a transitionary guideline ("the prudent user will select a name which satisfies both the rules of the domain system and any existing rules for the object, whether these rules are published or implied by existing programs").

Even if one were to view this prudence as a "MUST" in modern RFC parlance - shouldn't that be constrained on the database entry side of the process, not the database query side?

Third, LookupNS() allows lookups for "com.", "net." and "org." but not ".". It's inconsistent that the function allows access to some NS RRs in the global DNS but not others.

Finally, as others have noted, there is also the presumption of this code running on the public internet when in fact it should work perfectly well in a closed system; mDNS being the most obvious example. It is quite embarrassing having to inform my colleague, Charles André de Gaulle, that they have to change their name because it doesn't fit with the world view of what an office speaker-phone can be called.

I suggest that isDomainName() be recast as canBeEncodedAsAQName() and only reject the qName if it cannot be encoded into the protocol. In short, let the local resolver decide whether the qName exists in its database and otherwise avoid getting involved in deciding what constitutes a valid database key for a remote, general-purpose database.

Loading

@cherrymui cherrymui added this to the Backlog milestone Apr 26, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 26, 2021

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 26, 2021

Loading

@iangudger
Copy link
Contributor

@iangudger iangudger commented Apr 29, 2021

Is there a reason not to allow "." for all lookup types?

Loading

@markdingo
Copy link
Author

@markdingo markdingo commented Apr 29, 2021

Is there a reason not to allow "." for all lookup types?

While that would solve my immediate problem, I think the bigger question is whether isDomainName() is the right test to make on entry to lookup(). The claim is that isDomainName() tests for a "presentation-format domain name" but it really doesn't. As, e.g. it doesn't deal with escape sequences as defined in RFC1035 "Quoting conventions allow arbitrary characters to be stored in domain names.".

Loading

@iangudger
Copy link
Contributor

@iangudger iangudger commented May 2, 2021

I tried to fix that, but it got stuck in review: golang.org/cl/99623

Loading

@markdingo
Copy link
Author

@markdingo markdingo commented May 2, 2021

Ok, that's quite a change, but if it does the job and also solves a few other issues, maybe that's the best approach. How do we push this code review forward?

Loading

@networkimprov
Copy link

@networkimprov networkimprov commented May 16, 2021

Mark, you may need to ping this again after 1.17 is released, as the tree is frozen until then.

Loading

@markdingo
Copy link
Author

@markdingo markdingo commented May 16, 2021

Mark, you may need to ping this again after 1.17 is released, as the tree is frozen until then.

Will do.

Loading

@iangudger
Copy link
Contributor

@iangudger iangudger commented May 16, 2021

It should be possible to update x/net now and integrate the changes into the standard library after the freeze is lifted.

Loading

@markdingo
Copy link
Author

@markdingo markdingo commented May 16, 2021

It should be possible to update x/net now and integrate the changes into the standard library after the freeze is lifted.

Which I presume still means you need completion of the review for golang.org/cl/99623?

Loading

@iangudger
Copy link
Contributor

@iangudger iangudger commented May 16, 2021

That or an alternate approach.

Loading

@pjediny
Copy link
Contributor

@pjediny pjediny commented Jun 4, 2021

So there are two issues:

  • Mixing host name, domain name and query name validation. This one needs quite an effort.
  • isDomainName() is filtering out valid ., looking at the validation logic, it considers it .., because of the init of last octet to .. I assume this was not intended and can be easily fixed.

isDomainName() is used for validating both requests and from 1.16.5(#46241) responses, so it now breaks for example the net.LookupMX("example.com"). Response having . is probably quite rare, so considering this and the special purpose of the example.com domain, this probably should not have a big impact.

$ dig +noall +answer -t mx -q example.com
example.com.		72473	IN	MX	0 .

Questions are:

  • Should these two issues be addressed separately? I would prefer to quick fix the isDomainName() to allow the ., back-port the patch to 1.17 and older versions if possible and fix the first issue separately.
  • Was the exclusion of the . intended?

What do you think?

Loading

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jul 8, 2021

We are fixing . responses for MX in #46979, but I would support allowing . entirely in isDomainName in Go 1.18. This has been the behavior since at least Go 1.11, so there aren't really grounds for a backport (we only backport critical fixes).

Allowing a wider set of characters than the ones used by the open Internet DNS is more complicated, because it turned out to be a footgun at least in responses (#46241).

Loading

@FiloSottile FiloSottile self-assigned this Jul 8, 2021
@FiloSottile FiloSottile removed this from the Backlog milestone Jul 8, 2021
@FiloSottile FiloSottile added this to the Go1.18 milestone Jul 8, 2021
@markdingo
Copy link
Author

@markdingo markdingo commented Jul 8, 2021

So there are two issues:

  • Mixing host name, domain name and query name validation. This one needs quite an effort.

This is probably the fundamental issue and, as you say, quite an effort. I think just accepting "." seems like the least surprising change.

isDomainName() is used for validating both requests and from 1.16.5(#46241) responses, so it now breaks for example the net.LookupMX("example.com"). Response having . is probably quite rare

As the co-author of rfc7505 we were hoping NULL MX would get reasonable deployment to avoid the fallback to A RR that is part of SMTP. But checking some of the larger domains I used to administer I see that maybe it has fallen into disuse?

So maybe "quite rare" is true today, but given that it's standardized behavior its deployment may ebb and flow.

I for one use NULL MX on all my personal non-mail domains, if that helps :-)

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 1, 2021

Change https://golang.org/cl/360314 mentions this issue: net: accept "." as a valid domain name

Loading

@gopherbot gopherbot closed this in 35a5881 Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants