From ed38945fca3305544f3997cbbef81ee4450f8799 Mon Sep 17 00:00:00 2001 From: Alberto Bertogli Date: Fri, 8 Oct 2021 23:03:49 +0100 Subject: [PATCH] courier: Use DNSError.IsNotFound to identify NXDOMAIN When resolving MX records, we need to distinguish between "no such domain" and other kinds of errors. Before Go 1.13, this was not possible, so we had a workaround that assumed any permanent error was a "no such domain", which is not great, but functional. Now that our minimum supported version is Go 1.15, we can remove the workaround. This patch replaces the workaround with proper logic using DNSError.IsNotFound to identify NXDOMAIN results when resolving MX records. This requires to adjust a few tests, that used to work on environments where resolving unknown domains (used for testing) returned a permanent error, and now they no longer do so. Instead of relying on this environmental property, we make the affected tests use our own DNS server, which should make them more hermetic and reproducible. --- internal/courier/smtp.go | 19 ++++++++----------- internal/courier/smtp_test.go | 9 +++++---- test/t-02-exim/run.sh | 8 ++++++++ test/t-02-exim/zones | 6 ++++++ test/t-06-idna/hosts | 2 -- test/t-06-idna/run.sh | 9 +++++++++ test/t-06-idna/zones | 4 ++++ test/t-09-loop/run.sh | 9 +++++++++ test/t-09-loop/zones | 4 ++++ test/util/minidns.go | 5 +++++ 10 files changed, 58 insertions(+), 17 deletions(-) create mode 100644 test/t-02-exim/zones delete mode 100644 test/t-06-idna/hosts create mode 100644 test/t-06-idna/zones create mode 100644 test/t-09-loop/zones diff --git a/internal/courier/smtp.go b/internal/courier/smtp.go index 6a93c15..27b6c9b 100644 --- a/internal/courier/smtp.go +++ b/internal/courier/smtp.go @@ -249,22 +249,19 @@ func lookupMXs(tr *trace.Trace, domain string) ([]string, error, bool) { if err != nil { // There was an error. It could be that the domain has no MX, in which // case we have to fall back to A, or a bigger problem. - // Unfortunately, go's API doesn't let us easily distinguish between - // them. For now, if the error is permanent, we assume it's because - // there was no MX and fall back, otherwise we return. - // TODO: Use dnsErr.IsNotFound once we can use Go >= 1.13. dnsErr, ok := err.(*net.DNSError) if !ok { - tr.Debugf("MX lookup error: %v", err) - return nil, err, true - } else if dnsErr.Temporary() { - tr.Debugf("temporary DNS error: %v", dnsErr) + tr.Debugf("Error resolving MX on %q: %v", domain, err) return nil, err, false + } else if dnsErr.IsNotFound { + // MX not found, fall back to A. + tr.Debugf("MX for %s not found, falling back to A", domain) + mxs = []string{domain} + } else { + tr.Debugf("MX lookup error on %q: %v", domain, dnsErr) + return nil, err, !dnsErr.Temporary() } - // Permanent error, we assume MX does not exist and fall back to A. - tr.Debugf("failed to resolve MX for %s, falling back to A", domain) - mxs = []string{domain} } else { // Convert the DNS records to a plain string slice. They're already // sorted by priority. diff --git a/internal/courier/smtp_test.go b/internal/courier/smtp_test.go index 0455023..93cedbf 100644 --- a/internal/courier/smtp_test.go +++ b/internal/courier/smtp_test.go @@ -231,14 +231,15 @@ func TestFallbackToA(t *testing.T) { testMXErr["domain"] = &net.DNSError{ Err: "no such host (test)", IsTemporary: false, + IsNotFound: true, } mxs, err, perm := lookupMXs(tr, "domain") if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Errorf("unexpected error: %v", err) } if perm != true { - t.Fatalf("expected perm == true") + t.Errorf("expected perm == true") } if !(len(mxs) == 1 && mxs[0] == "domain") { t.Errorf("expected mxs == [domain], got: %v", mxs) @@ -271,8 +272,8 @@ func TestMXLookupError(t *testing.T) { if !(mxs == nil && err == testMXErr["domain"]) { t.Errorf("expected mxs == nil, err == test error, got: %v, %v", mxs, err) } - if perm != true { - t.Fatalf("expected perm == true") + if perm != false { + t.Errorf("expected perm == false") } } diff --git a/test/t-02-exim/run.sh b/test/t-02-exim/run.sh index 78b3006..69b6677 100755 --- a/test/t-02-exim/run.sh +++ b/test/t-02-exim/run.sh @@ -38,6 +38,12 @@ fi mkdir -p .exim4 EXIMDIR="$PWD/.exim4" envsubst < config/exim4.in > .exim4/config +# Build with the DNS override, so we can fake DNS records. +export GOTAGS="dnsoverride" + +# Launch minidns in the background using our configuration. +minidns_bg --addr=":9053" -zones=zones >> .minidns.log 2>&1 + generate_certs_for srv-chasquid add_user user@srv-chasquid secretpassword add_user someone@srv-chasquid secretpassword @@ -47,9 +53,11 @@ add_user someone@srv-chasquid secretpassword # Bypass MX lookup, so it can find srv-exim (via our host alias). mkdir -p .logs chasquid -v=2 --logfile=.logs/chasquid.log --config_dir=config \ + --testing__dns_addr=127.0.0.1:9053 \ --testing__outgoing_smtp_port=2025 & wait_until_ready 1025 +wait_until_ready 9053 # Launch exim at port 2025 .exim4/exim4 -bd -d -C "$PWD/.exim4/config" > .exim4/log 2>&1 & diff --git a/test/t-02-exim/zones b/test/t-02-exim/zones new file mode 100644 index 0000000..8f8b9bc --- /dev/null +++ b/test/t-02-exim/zones @@ -0,0 +1,6 @@ +# srv-chasquid and srv-exim to localhost. +# Neither have an MX record, but that's okay. +srv-chasquid A 127.0.0.1 +srv-chasquid AAAA ::1 +srv-exim A 127.0.0.1 +srv-exim AAAA ::1 diff --git a/test/t-06-idna/hosts b/test/t-06-idna/hosts deleted file mode 100644 index 05e33f4..0000000 --- a/test/t-06-idna/hosts +++ /dev/null @@ -1,2 +0,0 @@ -xn--srv--3ra localhost -xn--srv--jqa localhost diff --git a/test/t-06-idna/run.sh b/test/t-06-idna/run.sh index 427fef6..6c538e1 100755 --- a/test/t-06-idna/run.sh +++ b/test/t-06-idna/run.sh @@ -10,6 +10,12 @@ rm -rf .data-A .data-B .mail skip_if_python_is_too_old +# Build with the DNS override, so we can fake DNS records. +export GOTAGS="dnsoverride" + +# Launch minidns in the background using our configuration. +minidns_bg --addr=":9053" -zones=zones >> .minidns.log 2>&1 + # Two servers: # A - listens on :1025, hosts srv-ñ # B - listens on :2015, hosts srv-ü @@ -25,12 +31,15 @@ CONFDIR=B add_user nadaB@nadaB nadaB mkdir -p .logs-A .logs-B chasquid -v=2 --logfile=.logs-A/chasquid.log --config_dir=A \ + --testing__dns_addr=127.0.0.1:9053 \ --testing__outgoing_smtp_port=2025 & chasquid -v=2 --logfile=.logs-B/chasquid.log --config_dir=B \ + --testing__dns_addr=127.0.0.1:9053 \ --testing__outgoing_smtp_port=1025 & wait_until_ready 1025 wait_until_ready 2025 +wait_until_ready 9053 # Send from A to B. smtpc.py --server=localhost:1025 --user=nadaA@nadaA --password=nadaA \ diff --git a/test/t-06-idna/zones b/test/t-06-idna/zones new file mode 100644 index 0000000..a6f3826 --- /dev/null +++ b/test/t-06-idna/zones @@ -0,0 +1,4 @@ +xn--srv--3ra A 127.0.0.1 +xn--srv--3ra AAAA ::1 +xn--srv--jqa A 127.0.0.1 +xn--srv--jqa AAAA ::1 diff --git a/test/t-09-loop/run.sh b/test/t-09-loop/run.sh index 9411e26..d1cdcc7 100755 --- a/test/t-09-loop/run.sh +++ b/test/t-09-loop/run.sh @@ -8,6 +8,12 @@ check_hostaliases rm -rf .data-A .data-B .mail +# Build with the DNS override, so we can fake DNS records. +export GOTAGS="dnsoverride" + +# Launch minidns in the background using our configuration. +minidns_bg --addr=":9053" -zones=zones >> .minidns.log 2>&1 + # Two servers: # A - listens on :1025, hosts srv-A # B - listens on :2015, hosts srv-B @@ -23,13 +29,16 @@ CONFDIR=B generate_certs_for srv-B mkdir -p .logs-A .logs-B chasquid -v=2 --logfile=.logs-A/chasquid.log --config_dir=A \ + --testing__dns_addr=127.0.0.1:9053 \ --testing__max_received_headers=5 \ --testing__outgoing_smtp_port=2025 & chasquid -v=2 --logfile=.logs-B/chasquid.log --config_dir=B \ + --testing__dns_addr=127.0.0.1:9053 \ --testing__outgoing_smtp_port=1025 & wait_until_ready 1025 wait_until_ready 2025 +wait_until_ready 9053 run_msmtp aliasB@srv-B < content diff --git a/test/t-09-loop/zones b/test/t-09-loop/zones new file mode 100644 index 0000000..67154de --- /dev/null +++ b/test/t-09-loop/zones @@ -0,0 +1,4 @@ +srv-a A 127.0.0.1 +srv-a AAAA ::1 +srv-b A 127.0.0.1 +srv-b AAAA ::1 diff --git a/test/util/minidns.go b/test/util/minidns.go index 9c3aeb4..8189eef 100644 --- a/test/util/minidns.go +++ b/test/util/minidns.go @@ -209,6 +209,11 @@ func (m *miniDNS) handle(msg *dnsmessage.Message) *dnsmessage.Message { ID: msg.ID, Response: true, RCode: dnsmessage.RCodeSuccess, + + // We're authoritative for the zones we're serving. + // We should either set this, or RecursionAvailable, otherwise + // some client libraries will complain. + Authoritative: true, }, Questions: msg.Questions, }