net: LookupMX behaviour broken

[horkhe]

Change https://golang.org/cl/322230 introduced in release 1.16.5 and backported to 1.15.13, altered net.LookupMX behaviour that breaks the old contract in at least two ways:

• Previously it returned ALL MX records, so users could filter out bad ones and connect to those that are valid. However now if a domain has at least one "bad" MX record none is returned along with &net.DNSError{Err: "MX target is invalid", Name: name} error.
• If a domain has Null MX record (https://tools.ietf.org/html/rfc7505) configured then &net.DNSError{Err: "MX target is invalid", Name: name} is returned. So it is impossible to distinguish a domain explicitly forbidding SMTP connections from a domain with misconfigured MX records.

I believe users of net.LookupXXX family have come to expect that those methods return whatever DNS servers on the Internet have configured. We understand that those records can be incorrect and sometimes they are, but mostly due to human errors. Following the Robustness Principal we try to salvage useful pieces, ignore and/or auto-correct bad records. But this change robbed us of that option. So I think https://golang.org/cl/322230 is a regression and should be rolled back, alternatively you can provide net.RawLookupXXX methods, or add SkipVerify bool field to net.Resolver that would allow disabling verification of retrieved records.

[seankhliao]

cc @rolandshoemaker

[ianlancetaylor]

These changes (https://golang.org/cl/323131 and cherry picks) were made to address a security issue. See #46241 and https://www.whitesourcesoftware.com/vulnerability-database/CVE-2021-33195.

Marking as release blocker for 1.17 to decide whether there is anything to change here.

CC @golang/osp-team @golang/security

[rsc]

@horkhe do you have an example of why a server would legitimately return a set of MX records with some "invalid" ones, or do you have an example of any real-world servers doing that? That is, what breaks due to the filtering?

[gopherbot]

Change https://golang.org/cl/332094 mentions this issue: net: don't reject null mx records

[FiloSottile]

@horkhe do you have an example of why a server would legitimately return a set of MX records with some "invalid" ones, or do you have an example of any real-world servers doing that? That is, what breaks due to the filtering?

To be clear, invalid even after we fix validation to allow null records.

[FiloSottile]

@gopherbot please open a backport issue for Go 1.16, this is a regression in the last minor release.

[gopherbot]

Backport issue(s) opened: #46999 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[horkhe]

@rcs full disclosure, I am a developer from Mailgun by Pathwire. We send emails... quite a few of them. Obviously we query MX records a lot. Occasionally we see domains that have several MX records sprinkled with invalid ones. Those are obviously human errors. I cannot really give you a real world example of those records, but here is a domain that I created to run tests - test-unicode.definbox.com. It was inspired by a real configuration that we stumbled up on in our line of business. The original domain has already been fixed by the owner.

So the answer to your question is: there is no legitimate reason to configure a bad record, but one human error should not prevent users from seen all the correct MX records if any.

Thanks a lot for a quick turn around on this issue.

[dmitshur]

@FiloSottile If I understand correctly, Go 1.15 is also affected, and so it also needs a backport candidate. Opened #47012.

[gopherbot]

Change https://golang.org/cl/332371 mentions this issue: [release-branch.go1.16] net: don't reject null mx records

[gopherbot]

Change https://golang.org/cl/332372 mentions this issue: [release-branch.go1.15] net: don't reject null mx records

[rsc]

@horkhe Thanks for the extra information.

[gopherbot]

Change https://golang.org/cl/332549 mentions this issue: net: filter bad names from Lookup functions instead of hard failing

[gopherbot]

Change https://golang.org/cl/333330 mentions this issue: [release-branch.go1.16] net: filter bad names from Lookup functions instead of hard failing

[gopherbot]

Change https://golang.org/cl/333331 mentions this issue: [release-branch.go1.15] net: filter bad names from Lookup functions instead of hard failing

[horkhe]

Thank you, guys!