add mx preference to smtpclient.GatherDestinations

mostly so moxtools can show the mx preferences in its output

apidiff/next.txt

@@ -0,0 +1,5 @@
+Below are the incompatible changes between v0.0.15 and next, per package.
+
+# smtpclient
+- GatherDestinations: changed from func(context.Context, *log/slog.Logger, github.com/mjl-/mox/dns.Resolver, github.com/mjl-/mox/dns.IPDomain) (bool, bool, bool, github.com/mjl-/mox/dns.Domain, []github.com/mjl-/mox/dns.IPDomain, bool, error) to func(context.Context, *log/slog.Logger, github.com/mjl-/mox/dns.Resolver, github.com/mjl-/mox/dns.IPDomain) (bool, bool, bool, github.com/mjl-/mox/dns.Domain, []HostPref, bool, error)
+

main.go

@@ -2204,12 +2204,12 @@ sharing most of its code.
 	var haveMX bool
 	var expandedNextHopAuthentic bool
 	var expandedNextHop dns.Domain
-	var hosts []dns.IPDomain
+	var hostPrefs []smtpclient.HostPref
 	if len(args) == 1 {
 		var permanent bool
 		var origNextHopAuthentic bool
 		var err error
-		haveMX, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hosts, permanent, err = smtpclient.GatherDestinations(ctxbg, c.log.Logger, resolver, dns.IPDomain{Domain: origNextHop})
+		haveMX, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hostPrefs, permanent, err = smtpclient.GatherDestinations(ctxbg, c.log.Logger, resolver, dns.IPDomain{Domain: origNextHop})
 		status := "temporary"
 		if permanent {
 			status = "permanent"
@@ -2233,8 +2233,12 @@ sharing most of its code.
 		}
 
 		l := []string{}
-		for _, h := range hosts {
+		for _, hp := range hostPrefs {
-			l = append(l, h.String())
+			s := hp.Host.String()
+			if hp.Pref >= 0 {
+				s += fmt.Sprintf(" (pref %d)", hp.Pref)
+			}
+			l = append(l, s)
 		}
 		log.Printf("destinations: %s", strings.Join(l, ", "))
 	} else {
@@ -2243,18 +2247,14 @@ sharing most of its code.
 
 		expandedNextHopAuthentic = true
 		expandedNextHop = d
-		hosts = []dns.IPDomain{{Domain: d}}
+		hostPrefs = []smtpclient.HostPref{{Host: dns.IPDomain{Domain: d}, Pref: -1}}
 	}
 
 	dialedIPs := map[string][]net.IP{}
-	for _, host := range hosts {
+	for _, hp := range hostPrefs {
-		// It should not be possible for hosts to have IP addresses: They are not
+		host := hp.Host
-		// allowed by dns.ParseDomain, and MX records cannot contain them.
-		if host.IsIP() {
-			log.Fatalf("unexpected IP address for destination host")
-		}
 
-		log.Printf("attempting to connect to %s", host)
+		log.Printf("attempting to connect to %s (pref %d)", host, hp.Pref)
 
 		authentic, expandedAuthentic, expandedHost, ips, _, err := smtpclient.GatherIPs(ctxbg, c.log.Logger, resolver, "ip", host, dialedIPs)
 		if err != nil {

queue/direct.go

@@ -139,7 +139,7 @@ func deliverDirect(qlog mlog.Log, resolver dns.Resolver, dialer smtpclient.Diale
 	// directly.
 	origNextHop := m0.RecipientDomain.Domain
 	ctx := mox.Shutdown
-	haveMX, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hosts, permanent, err := smtpclient.GatherDestinations(ctx, qlog.Logger, resolver, m0.RecipientDomain)
+	haveMX, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hostPrefs, permanent, err := smtpclient.GatherDestinations(ctx, qlog.Logger, resolver, m0.RecipientDomain)
 	if err != nil {
 		// If this is a DNSSEC authentication error, we'll collect it for TLS reporting.
 		// Hopefully it's a temporary misconfiguration that is solve before we try to send
@@ -196,7 +196,8 @@ func deliverDirect(qlog mlog.Log, resolver dns.Resolver, dialer smtpclient.Diale
 	var lastErr = errors.New("no error") // Can be smtpclient.Error.
 	nmissingRequireTLS := 0
 	// todo: should make distinction between host permanently not accepting the message, and the message not being deliverable permanently. e.g. a mx host may have a size limit, or not accept 8bitmime, while another host in the list does accept the message. same for smtputf8, ../rfc/6531:555
-	for _, h := range hosts {
+	for _, hp := range hostPrefs {
+		h := hp.Host
 		// ../rfc/8461:913
 		if policy != nil && policy.Mode != mtasts.ModeNone && !policy.Matches(h.Domain) {
 			// todo: perhaps only send tlsrpt failure if none of the mx hosts matched? reporting about each mismatch seems useful for domain owners, to discover mtasts policies they didn't update after changing mx. there is a risk a domain owner intentionally didn't put all mx'es in the mtasts policy, but they probably won't mind being reported about that.
@@ -348,7 +349,7 @@ func deliverDirect(qlog mlog.Log, resolver dns.Resolver, dialer smtpclient.Diale
 	// If we failed due to requiretls not being satisfied, make the delivery permanent.
 	// It is unlikely the recipient domain will implement requiretls during our retry
 	// period. Best to let the sender know immediately.
-	if len(hosts) > 0 && nmissingRequireTLS == len(hosts) {
+	if len(hostPrefs) > 0 && nmissingRequireTLS == len(hostPrefs) {
 		qlog.Info("marking delivery as permanently failed because recipient domain does not implement requiretls")
 		err := smtpclient.Error{
 			Permanent: true,

smtpclient/gather.go

@@ -26,6 +26,12 @@ var (
 	errNoMail     = errors.New("domain does not accept email as indicated with single dot for mx record")
 )
 
+// HostPref is a host for delivery, with preference for MX records.
+type HostPref struct {
+	Host dns.IPDomain
+	Pref int // -1 when not an MX record.
+}
+
 // GatherDestinations looks up the hosts to deliver email to a domain ("next-hop").
 // If it is an IP address, it is the only destination to try. Otherwise CNAMEs of
 // the domain are followed. Then MX records for the expanded CNAME are looked up.
@@ -46,14 +52,14 @@ var (
 // were found, both the original and expanded next-hops must be authentic for DANE
 // to be option. For a non-IP with no MX records found, the authentic result can
 // be used to decide which of the names to use as TLSA base domain.
-func GatherDestinations(ctx context.Context, elog *slog.Logger, resolver dns.Resolver, origNextHop dns.IPDomain) (haveMX, origNextHopAuthentic, expandedNextHopAuthentic bool, expandedNextHop dns.Domain, hosts []dns.IPDomain, permanent bool, err error) {
+func GatherDestinations(ctx context.Context, elog *slog.Logger, resolver dns.Resolver, origNextHop dns.IPDomain) (haveMX, origNextHopAuthentic, expandedNextHopAuthentic bool, expandedNextHop dns.Domain, hostPrefs []HostPref, permanent bool, err error) {
 	// ../rfc/5321:3824
 
 	log := mlog.New("smtpclient", elog)
 
 	// IP addresses are dialed directly, and don't have TLSA records.
 	if len(origNextHop.IP) > 0 {
-		return false, false, false, expandedNextHop, []dns.IPDomain{origNextHop}, false, nil
+		return false, false, false, expandedNextHop, []HostPref{{origNextHop, -1}}, false, nil
 	}
 
 	// We start out assuming the result is authentic. Updated with each lookup.
@@ -133,8 +139,8 @@ func GatherDestinations(ctx context.Context, elog *slog.Logger, resolver dns.Res
 			}
 
 			// No MX record, attempt delivery directly to host. ../rfc/5321:3842
-			hosts = []dns.IPDomain{{Domain: expandedNextHop}}
+			hostPrefs = []HostPref{{dns.IPDomain{Domain: expandedNextHop}, -1}}
-			return false, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hosts, false, nil
+			return false, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hostPrefs, false, nil
 		} else if err != nil {
 			log.Infox("mx record has some invalid records, keeping only the valid mx records", err)
 		}
@@ -158,12 +164,12 @@ func GatherDestinations(ctx context.Context, elog *slog.Logger, resolver dns.Res
 				err = fmt.Errorf("%w: invalid host name in mx record %q: %v", errDNS, mx.Host, err)
 				return true, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, nil, true, err
 			}
-			hosts = append(hosts, dns.IPDomain{Domain: host})
+			hostPrefs = append(hostPrefs, HostPref{dns.IPDomain{Domain: host}, int(mx.Pref)})
 		}
-		if len(hosts) > 0 {
+		if len(hostPrefs) > 0 {
 			err = nil
 		}
-		return true, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hosts, false, err
+		return true, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hostPrefs, false, err
 	}
 }
 

smtpclient/gather_test.go

@@ -35,11 +35,11 @@ func ipdomain(s string) dns.IPDomain {
 	return dns.IPDomain{Domain: d}
 }
 
-func ipdomains(s ...string) (l []dns.IPDomain) {
+func hostprefs(pref int, names ...string) (l []HostPref) {
-	for _, e := range s {
+	for _, s := range names {
-		l = append(l, ipdomain(e))
+		l = append(l, HostPref{Host: ipdomain(s), Pref: pref})
 	}
-	return
+	return l
 }
 
 // Test basic MX lookup case, but also following CNAME, detecting CNAME loops and
@@ -86,10 +86,10 @@ func TestGatherDestinations(t *testing.T) {
 		resolver.CNAME[s] = next
 	}
 
-	test := func(ipd dns.IPDomain, expHosts []dns.IPDomain, expDomain dns.Domain, expPerm, expAuthic, expExpAuthic bool, expErr error) {
+	test := func(ipd dns.IPDomain, expHostPrefs []HostPref, expDomain dns.Domain, expPerm, expAuthic, expExpAuthic bool, expErr error) {
 		t.Helper()
 
-		_, authic, authicExp, ed, hosts, perm, err := GatherDestinations(ctxbg, log.Logger, resolver, ipd)
+		_, authic, authicExp, ed, hostPrefs, perm, err := GatherDestinations(ctxbg, log.Logger, resolver, ipd)
 		if (err == nil) != (expErr == nil) || err != nil && !errors.Is(err, expErr) {
 			// todo: could also check the individual errors? code currently does not have structured errors.
 			t.Fatalf("gather hosts: %v, expected %v", err, expErr)
@@ -97,8 +97,8 @@ func TestGatherDestinations(t *testing.T) {
 		if err != nil {
 			return
 		}
-		if !reflect.DeepEqual(hosts, expHosts) || ed != expDomain || perm != expPerm || authic != expAuthic || authicExp != expExpAuthic {
+		if !reflect.DeepEqual(hostPrefs, expHostPrefs) || ed != expDomain || perm != expPerm || authic != expAuthic || authicExp != expExpAuthic {
-			t.Fatalf("got hosts %#v, effectiveDomain %#v, permanent %#v, authic %v %v, expected %#v %#v %#v %v %v", hosts, ed, perm, authic, authicExp, expHosts, expDomain, expPerm, expAuthic, expExpAuthic)
+			t.Fatalf("got hosts %#v, effectiveDomain %#v, permanent %#v, authic %v %v, expected %#v %#v %#v %v %v", hostPrefs, ed, perm, authic, authicExp, expHostPrefs, expDomain, expPerm, expAuthic, expExpAuthic)
 		}
 	}
 
@@ -108,18 +108,18 @@ func TestGatherDestinations(t *testing.T) {
 		authic := i == 1
 		resolver.AllAuthentic = authic
 		// Basic with simple MX.
-		test(ipdomain("basic.example"), ipdomains("mail.basic.example"), domain("basic.example"), false, authic, authic, nil)
+		test(ipdomain("basic.example"), hostprefs(10, "mail.basic.example"), domain("basic.example"), false, authic, authic, nil)
-		test(ipdomain("multimx.example"), ipdomains("mail1.multimx.example", "mail2.multimx.example"), domain("multimx.example"), false, authic, authic, nil)
+		test(ipdomain("multimx.example"), hostprefs(10, "mail1.multimx.example", "mail2.multimx.example"), domain("multimx.example"), false, authic, authic, nil)
 		// Only an A record.
-		test(ipdomain("justhost.example"), ipdomains("justhost.example"), domain("justhost.example"), false, authic, authic, nil)
+		test(ipdomain("justhost.example"), hostprefs(-1, "justhost.example"), domain("justhost.example"), false, authic, authic, nil)
 		// Only an AAAA record.
-		test(ipdomain("justhost6.example"), ipdomains("justhost6.example"), domain("justhost6.example"), false, authic, authic, nil)
+		test(ipdomain("justhost6.example"), hostprefs(-1, "justhost6.example"), domain("justhost6.example"), false, authic, authic, nil)
 		// Follow CNAME.
-		test(ipdomain("cname.example"), ipdomains("mail.basic.example"), domain("basic.example"), false, authic, authic, nil)
+		test(ipdomain("cname.example"), hostprefs(10, "mail.basic.example"), domain("basic.example"), false, authic, authic, nil)
 		// No MX/CNAME, non-existence of host will be found out later.
-		test(ipdomain("absent.example"), ipdomains("absent.example"), domain("absent.example"), false, authic, authic, nil)
+		test(ipdomain("absent.example"), hostprefs(-1, "absent.example"), domain("absent.example"), false, authic, authic, nil)
 		// Followed CNAME, has no MX, non-existence of host will be found out later.
-		test(ipdomain("danglingcname.example"), ipdomains("absent.example"), domain("absent.example"), false, authic, authic, nil)
+		test(ipdomain("danglingcname.example"), hostprefs(-1, "absent.example"), domain("absent.example"), false, authic, authic, nil)
 		test(ipdomain("cnamelimit1.example"), nil, zerodom, true, authic, authic, errCNAMELimit)
 		test(ipdomain("cnameloop.example"), nil, zerodom, true, authic, authic, errCNAMELoop)
 		test(ipdomain("nullmx.example"), nil, zerodom, true, authic, authic, errNoMail)
@@ -127,9 +127,9 @@ func TestGatherDestinations(t *testing.T) {
 		test(ipdomain("temperror-cname.example"), nil, zerodom, false, authic, authic, errDNS)
 	}
 
-	test(ipdomain("10.0.0.1"), ipdomains("10.0.0.1"), zerodom, false, false, false, nil)
+	test(ipdomain("10.0.0.1"), hostprefs(-1, "10.0.0.1"), zerodom, false, false, false, nil)
-	test(ipdomain("cnameinauthentic.example"), ipdomains("mail.basic.example"), domain("basic.example"), false, false, false, nil)
+	test(ipdomain("cnameinauthentic.example"), hostprefs(10, "mail.basic.example"), domain("basic.example"), false, false, false, nil)
-	test(ipdomain("cname-to-inauthentic.example"), ipdomains("mail.basic.example"), domain("basic.example"), false, true, false, nil)
+	test(ipdomain("cname-to-inauthentic.example"), hostprefs(10, "mail.basic.example"), domain("basic.example"), false, true, false, nil)
 }
 
 func TestGatherIPs(t *testing.T) {

webmail/api.go

@@ -1717,7 +1717,7 @@ func recipientSecurity(ctx context.Context, log mlog.Log, resolver dns.Resolver,
 		defer logPanic(ctx)
 		defer wg.Done()
 
-		_, origNextHopAuthentic, expandedNextHopAuthentic, _, hosts, _, err := smtpclient.GatherDestinations(ctx, log.Logger, resolver, dns.IPDomain{Domain: addr.Domain})
+		_, origNextHopAuthentic, expandedNextHopAuthentic, _, hostPrefs, _, err := smtpclient.GatherDestinations(ctx, log.Logger, resolver, dns.IPDomain{Domain: addr.Domain})
 		if err != nil {
 			rs.DNSSEC = SecurityResultError
 			return
@@ -1734,10 +1734,10 @@ func recipientSecurity(ctx context.Context, log mlog.Log, resolver dns.Resolver,
 		}
 
 		// We're only looking at the first host to deliver to (typically first mx destination).
-		if len(hosts) == 0 || hosts[0].Domain.IsZero() {
+		if len(hostPrefs) == 0 || hostPrefs[0].Host.Domain.IsZero() {
 			return // Should not happen.
 		}
-		host := hosts[0]
+		host := hostPrefs[0].Host
 
 		// Resolve the IPs. Required for DANE to prevent bad DNS servers from causing an
 		// error result instead of no-DANE result.