On 2023/10/18 08:40:14 +0100, Stuart Henderson <s...@spacehopper.org> wrote: > On 2023/10/17 22:27, Philipp wrote: > > [2023-10-17 17:32] Omar Polo <o...@omarpolo.com> > > > There is one part of the RFC7505 that I'd like to quote and discuss > > > with you however. The last paragraph of the section 3 says: > > > > > > : A domain that advertises a null MX MUST NOT advertise any other MX > > > : RR. > > > > > > Your implementation handles the case where there are more than one MX > > > by setting the `nullmx' field in the shared struct when we encounter > > > one, and then don't process the other MXs when that field is set. > > > This is fine. > > > > > > However, I think we can simplify here by not honouring the Null MX > > > when there are other MX records. We're not violating the RFC. See > > > diff below to see what I mean. The only difference is in dns.c, the > > > rest is the same with yours. > > > > My reasaning for that was: When you set Null MX you explicitly opt out > > from reciving mail even when you violate the spec. But if you want > > it diffrent I can change it. > > I think in that situation, the recipient's DNS admin has *not* set a > valid nullmx, and delivery to other MXes should still be attempted.
That's what I though too. > > But I don't think your proposed patch is a good solution, because the > > result depend on the order of the RR in the repsonse. The problem is > > when the first entry in the response is a Null MX your patch truncate > > the other results and a bounce is generated. But when the Null MX is > > later in the response the other entries are used to deliver the mail. that's true; I fell for a shorter diff. I've attached below a version that doesn't depend on the order. > > > So far I've only compile-tested the code. I don't have a spare domain > > > to test and don't know of anyone using a Null MX. > > > > To test Null MX you can use example.com. To test the localhost patch a > > friend of me has set up mxtest.yannikenss.de. thanks! > [...] > > [1] I still belive OpenSMTPD should only connect to public routable > > addresses for relay. So don't connect to something like 127.0.0.1/8, > > ::1/128, RFC1918 addresses, ULA, ... > > But this is independent of this patch. > > Support for nullmx makes sense to me. Perhaps also the localhost > handling. But some intranet setups may require connections to > RFC1918/ULA addresses - I don't think an MTA should prevent these. Completely agree. diff 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0 02bb94351d3865e61483023cab9fa02bcac2970d commit - 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0 commit + 02bb94351d3865e61483023cab9fa02bcac2970d blob - 4cf5d23d1d14b5400c6f4429dae0a4f6490073d4 blob + 552a5cf9115401b4fac3d131d6e5c022ebb8b7fd --- usr.sbin/smtpd/dns.c +++ usr.sbin/smtpd/dns.c @@ -232,10 +232,10 @@ dns_dispatch_mx(struct asr_result *ar, void *arg) struct dns_rr rr; char buf[512]; size_t found; + int nullmx = 0; if (ar->ar_h_errno && ar->ar_h_errno != NO_DATA && ar->ar_h_errno != NOTIMP) { - m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1); m_add_id(s->p, s->reqid); if (ar->ar_rcode == NXDOMAIN) @@ -259,13 +259,29 @@ dns_dispatch_mx(struct asr_result *ar, void *arg) unpack_rr(&pack, &rr); if (rr.rr_type != T_MX) continue; + print_dname(rr.rr.mx.exchange, buf, sizeof(buf)); buf[strlen(buf) - 1] = '\0'; + + if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) { + nullmx = 1; + continue; + } + dns_lookup_host(s, buf, rr.rr.mx.preference); found++; } free(ar->ar_data); + if (nullmx && found == 0) { + m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1); + m_add_id(s->p, s->reqid); + m_add_int(s->p, DNS_NULLMX); + m_close(s->p); + free(s); + return; + } + /* fallback to host if no MX is found. */ if (found == 0) dns_lookup_host(s, s->name, 0); blob - c0d0fc02931b90409441035d2744923af9e42df1 blob + 25e158b68a88c8485428a2476c1c557f8c60404d --- usr.sbin/smtpd/mta.c +++ usr.sbin/smtpd/mta.c @@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data) else relay->failstr = "No MX found for domain"; break; + case DNS_NULLMX: + relay->fail = IMSG_MTA_DELIVERY_PERMFAIL; + relay->failstr = "Domain does not accept mail"; + break; default: fatalx("bad DNS lookup error code"); break; blob - 6781286928da45e6159bce81ff2437011ebdef72 blob + d5cbe88feeaf9e91eca119b31dda957aca68c031 --- usr.sbin/smtpd/smtpd.h +++ usr.sbin/smtpd/smtpd.h @@ -1026,6 +1026,8 @@ enum dns_error { DNS_EINVAL, DNS_ENONAME, DNS_ENOTFOUND, + /* RFC 7505 */ + DNS_NULLMX, }; enum lka_resp_status {