give delivering to mx targets with underscores in name a chance of succeeding

the underscores aren't valid, but have been seen in the wild, so we have a
workaround for them. there are limitations, it won't work with idna domains.
and if the domain has other policies, like mta-sts, the mx host won't pass
either.

after report from richard g about delivery issue, thanks!

dns/dns.go

@@ -10,9 +10,15 @@ import (
 	"golang.org/x/net/idna"
 
 	"github.com/mjl-/adns"
+
+	"github.com/mjl-/mox/moxvar"
 )
 
-var errTrailingDot = errors.New("dns name has trailing dot")
+var (
+	errTrailingDot = errors.New("dns name has trailing dot")
+	errUnderscore  = errors.New("domain name with underscore")
+	errIDNA        = errors.New("idna")
+)
 
 // Domain is a domain name, with one or more labels, with at least an ASCII
 // representation, and for IDNA non-ASCII domains a unicode representation.
@@ -83,13 +89,14 @@ func ParseDomain(s string) (Domain, error) {
 	if strings.HasSuffix(s, ".") {
 		return Domain{}, errTrailingDot
 	}
+
 	ascii, err := idna.Lookup.ToASCII(s)
 	if err != nil {
-		return Domain{}, fmt.Errorf("to ascii: %w", err)
+		return Domain{}, fmt.Errorf("%w: to ascii: %v", errIDNA, err)
 	}
 	unicode, err := idna.Lookup.ToUnicode(s)
 	if err != nil {
-		return Domain{}, fmt.Errorf("to unicode: %w", err)
+		return Domain{}, fmt.Errorf("%w: to unicode: %w", errIDNA, err)
 	}
 	// todo: should we cause errors for unicode domains that were not in
 	// canonical form? we are now accepting all kinds of obscure spellings
@@ -101,6 +108,41 @@ func ParseDomain(s string) (Domain, error) {
 	return Domain{ascii, unicode}, nil
 }
 
+// ParseDomainLax parses a domain like ParseDomain, but allows labels with
+// underscores if the entire domain name is ASCII-only non-IDNA and Pedantic mode
+// is not enabled. Used for interoperability, e.g. domains may specify MX
+// targets with underscores.
+func ParseDomainLax(s string) (Domain, error) {
+	if moxvar.Pedantic || !strings.Contains(s, "_") {
+		return ParseDomain(s)
+	}
+
+	// If there is any non-ASCII, this is certainly not an A-label-only domain.
+	s = strings.ToLower(s)
+	for _, c := range s {
+		if c >= 0x80 {
+			return Domain{}, fmt.Errorf("%w: underscore and non-ascii not allowed", errUnderscore)
+		}
+	}
+
+	// Try parsing with underscores replaced with allowed ASCII character.
+	// If that's not valid, the version with underscore isn't either.
+	repl := strings.ReplaceAll(s, "_", "a")
+	d, err := ParseDomain(repl)
+	if err != nil {
+		return Domain{}, fmt.Errorf("%w: %v", errUnderscore, err)
+	}
+	// If we found an IDNA domain, we're not going to allow it.
+	if d.Unicode != "" {
+		return Domain{}, fmt.Errorf("%w: idna domain with underscores not allowed", errUnderscore)
+	}
+	// Just to be safe, ensure no unexpected conversions happened.
+	if d.ASCII != repl {
+		return Domain{}, fmt.Errorf("%w: underscores and non-canonical names not allowed", errUnderscore)
+	}
+	return Domain{ASCII: s}, nil
+}
+
 // IsNotFound returns whether an error is an adns.DNSError with IsNotFound set.
 // IsNotFound means the requested type does not exist for the given domain (a
 // nodata or nxdomain response). It doesn't not necessarily mean no other types for

dns/dns_test.go

@@ -6,9 +6,15 @@ import (
 )
 
 func TestParseDomain(t *testing.T) {
-	test := func(s string, exp Domain, expErr error) {
+	test := func(lax bool, s string, exp Domain, expErr error) {
 		t.Helper()
-		dom, err := ParseDomain(s)
+		var dom Domain
+		var err error
+		if lax {
+			dom, err = ParseDomainLax(s)
+		} else {
+			dom, err = ParseDomain(s)
+		}
 		if (err == nil) != (expErr == nil) || expErr != nil && !errors.Is(err, expErr) {
 			t.Fatalf("parse domain %q: err %v, expected %v", s, err, expErr)
 		}
@@ -18,10 +24,15 @@ func TestParseDomain(t *testing.T) {
 	}
 
 	// We rely on normalization of names throughout the code base.
-	test("xmox.nl", Domain{"xmox.nl", ""}, nil)
-	test("XMOX.NL", Domain{"xmox.nl", ""}, nil)
-	test("TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil)
-	test("TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil)
-	test("ℂᵤⓇℒ。𝐒🄴", Domain{"curl.se", ""}, nil) // https://daniel.haxx.se/blog/2022/12/14/idn-is-crazy/
-	test("xmox.nl.", Domain{}, errTrailingDot)
+	test(false, "xmox.nl", Domain{"xmox.nl", ""}, nil)
+	test(false, "XMOX.NL", Domain{"xmox.nl", ""}, nil)
+	test(false, "TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil)
+	test(false, "TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil)
+	test(false, "ℂᵤⓇℒ。𝐒🄴", Domain{"curl.se", ""}, nil) // https://daniel.haxx.se/blog/2022/12/14/idn-is-crazy/
+	test(false, "xmox.nl.", Domain{}, errTrailingDot)
+
+	test(false, "_underscore.xmox.nl", Domain{}, errIDNA)
+	test(true, "_underscore.xmox.NL", Domain{ASCII: "_underscore.xmox.nl"}, nil)
+	test(true, "_underscore.☺.xmox.nl", Domain{}, errUnderscore)
+	test(true, "_underscore.xn--test-3o3b.xmox.nl", Domain{}, errUnderscore)
 }

smtpclient/gather.go

@@ -148,7 +148,8 @@ func GatherDestinations(ctx context.Context, log *mlog.Log, resolver dns.Resolve
 		// The Go resolver already sorts by preference, randomizing records of same
 		// preference. ../rfc/5321:3885
 		for _, mx := range mxl {
-			host, err := dns.ParseDomain(strings.TrimSuffix(mx.Host, "."))
+			// Parsing lax (unless pedantic mode) for MX targets with underscores as seen in the wild.
+			host, err := dns.ParseDomainLax(strings.TrimSuffix(mx.Host, "."))
 			if err != nil {
 				// note: should not happen because Go resolver already filters these out.
 				err = fmt.Errorf("%w: invalid host name in mx record %q: %v", errDNS, mx.Host, err)

spf/spf.go

@@ -444,7 +444,8 @@ func evaluate(ctx context.Context, record *Record, resolver dns.Resolver, args A
 				if i >= 10 {
 					return StatusPermerror, d.MechanismString(), "", rauthentic, ErrTooManyDNSRequests
 				}
-				mxd, err := dns.ParseDomain(strings.TrimSuffix(mx.Host, "."))
+				// Parsing lax (unless in pedantic mode) for MX targets with underscores as seen in the wild.
+				mxd, err := dns.ParseDomainLax(strings.TrimSuffix(mx.Host, "."))
 				if err != nil {
 					return StatusPermerror, d.MechanismString(), "", rauthentic, err
 				}