courier: DNS temporary errors should cause temporary delivery failures

In the SMTP courier, when the MX lookup fails with a DNS temporary
error, we should propagate that up and cause delivery to fail with a
temporary error too.

That way the delivery can be attempted later by the queue.

However, the code today doesn't make the distinction and will treat any
temporary DNS error as a permanent delivery failure, which is a bug.

The bug was found by ludusrusso and reported in
https://github.com/albertito/chasquid/issues/4

This patch fixes the bug by propagating the error properly.

internal/courier/smtp.go

@@ -68,13 +68,13 @@ func (s *SMTP) Deliver(from string, to string, data []byte) (error, bool) {
 		a.from = ""
 	}
 
-	mxs, err := lookupMXs(a.tr, a.toDomain)
+	mxs, err, perm := lookupMXs(a.tr, a.toDomain)
 	if err != nil || len(mxs) == 0 {
 		// Note this is considered a permanent error.
 		// This is in line with what other servers (Exim) do. However, the
 		// downside is that temporary DNS issues can affect delivery, so we
 		// have to make sure we try hard enough on the lookup above.
-		return a.tr.Errorf("Could not find mail server: %v", err), true
+		return a.tr.Errorf("Could not find mail server: %v", err), perm
 	}
 
 	// Issue an EHLO with a valid domain; otherwise, some servers like postfix
@@ -245,10 +245,10 @@ func (s *SMTP) fetchSTSPolicy(tr *trace.Trace, domain string) *sts.Policy {
 	return policy
 }
 
-func lookupMXs(tr *trace.Trace, domain string) ([]string, error) {
+func lookupMXs(tr *trace.Trace, domain string) ([]string, error, bool) {
 	domain, err := idna.ToASCII(domain)
 	if err != nil {
-		return nil, err
+		return nil, err, true
 	}
 
 	mxs := []string{}
@@ -264,10 +264,10 @@ func lookupMXs(tr *trace.Trace, domain string) ([]string, error) {
 		dnsErr, ok := err.(*net.DNSError)
 		if !ok {
 			tr.Debugf("MX lookup error: %v", err)
-			return nil, err
+			return nil, err, true
 		} else if dnsErr.Temporary() {
 			tr.Debugf("temporary DNS error: %v", dnsErr)
-			return nil, err
+			return nil, err, false
 		}
 
 		// Permanent error, we assume MX does not exist and fall back to A.
@@ -292,5 +292,5 @@ func lookupMXs(tr *trace.Trace, domain string) ([]string, error) {
 	}
 
 	tr.Debugf("MXs: %v", mxs)
-	return mxs, nil
+	return mxs, nil, true
 }

internal/courier/smtp_test.go

@@ -213,10 +213,13 @@ func TestTooManyMX(t *testing.T) {
 		{Host: "h3", Pref: 30}, {Host: "h4", Pref: 40},
 		{Host: "h5", Pref: 50}, {Host: "h5", Pref: 60},
 	}
-	mxs, err := lookupMXs(tr, "domain")
+	mxs, err, perm := lookupMXs(tr, "domain")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	if perm != true {
+		t.Fatalf("expected perm == true")
+	}
 	if len(mxs) != 5 {
 		t.Errorf("expected len(mxs) == 5, got: %v", mxs)
 	}
@@ -230,10 +233,13 @@ func TestFallbackToA(t *testing.T) {
 		IsTemporary: false,
 	}
 
-	mxs, err := lookupMXs(tr, "domain")
+	mxs, err, perm := lookupMXs(tr, "domain")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	if perm != true {
+		t.Fatalf("expected perm == true")
+	}
 	if !(len(mxs) == 1 && mxs[0] == "domain") {
 		t.Errorf("expected mxs == [domain], got: %v", mxs)
 	}
@@ -247,10 +253,13 @@ func TestTemporaryDNSerror(t *testing.T) {
 		IsTemporary: true,
 	}
 
-	mxs, err := lookupMXs(tr, "domain")
+	mxs, err, perm := lookupMXs(tr, "domain")
 	if !(mxs == nil && err == testMXErr["domain"]) {
 		t.Errorf("expected mxs == nil, err == test error, got: %v, %v", mxs, err)
 	}
+	if perm != false {
+		t.Errorf("expected perm == false")
+	}
 }
 
 func TestMXLookupError(t *testing.T) {
@@ -258,19 +267,25 @@ func TestMXLookupError(t *testing.T) {
 	testMX["domain"] = nil
 	testMXErr["domain"] = fmt.Errorf("test error")
 
-	mxs, err := lookupMXs(tr, "domain")
+	mxs, err, perm := lookupMXs(tr, "domain")
 	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")
+	}
 }
 
 func TestLookupInvalidDomain(t *testing.T) {
 	tr := trace.New("test", "test")
 
-	mxs, err := lookupMXs(tr, invalidDomain)
+	mxs, err, perm := lookupMXs(tr, invalidDomain)
 	if !(mxs == nil && err != nil) {
 		t.Errorf("expected err != nil, got: %v, %v", mxs, err)
 	}
+	if perm != true {
+		t.Fatalf("expected perm == true")
+	}
 }
 
 // TODO: Test STARTTLS negotiation.