diff --git a/apidiff/next.txt b/apidiff/next.txt index e69de29..cde7ac2 100644 --- a/apidiff/next.txt +++ b/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) + diff --git a/main.go b/main.go index 9cb8986..6bde8bb 100644 --- a/main.go +++ b/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 { - l = append(l, h.String()) + for _, hp := range hostPrefs { + 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 { - // It should not be possible for hosts to have IP addresses: They are not - // allowed by dns.ParseDomain, and MX records cannot contain them. - if host.IsIP() { - log.Fatalf("unexpected IP address for destination host") - } + for _, hp := range hostPrefs { + host := hp.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 { diff --git a/queue/direct.go b/queue/direct.go index bcb2d44..faf7ad5 100644 --- a/queue/direct.go +++ b/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, diff --git a/smtpclient/gather.go b/smtpclient/gather.go index d7cf923..6c3074f 100644 --- a/smtpclient/gather.go +++ b/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}} - return false, origNextHopAuthentic, expandedNextHopAuthentic, expandedNextHop, hosts, false, nil + hostPrefs = []HostPref{{dns.IPDomain{Domain: expandedNextHop}, -1}} + 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 } } diff --git a/smtpclient/gather_test.go b/smtpclient/gather_test.go index b1760a9..086ef6d 100644 --- a/smtpclient/gather_test.go +++ b/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) { - for _, e := range s { - l = append(l, ipdomain(e)) +func hostprefs(pref int, names ...string) (l []HostPref) { + for _, s := range names { + 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 { - 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) + 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", 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("multimx.example"), ipdomains("mail1.multimx.example", "mail2.multimx.example"), domain("multimx.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"), 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("cnameinauthentic.example"), ipdomains("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("10.0.0.1"), hostprefs(-1, "10.0.0.1"), zerodom, 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"), hostprefs(10, "mail.basic.example"), domain("basic.example"), false, true, false, nil) } func TestGatherIPs(t *testing.T) { diff --git a/webmail/api.go b/webmail/api.go index 71976b8..1ee1cdf 100644 --- a/webmail/api.go +++ b/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.