Allow `validateEmail` to timeout w/o error. (#2288)

This PR reworks the validateEmail() function from the RA to allow timeouts during DNS validation of MX/A/AAAA records for an email to be non-fatal and match our intention to verify emails best-effort.

Notes:

bdns/problem.go - DNSError.Timeout() was changed to also include context cancellation and timeout as DNS timeouts. This matches what DNSError.Error() was doing to set the error message and supports external callers to Timeout not duplicating the work.

bdns/mocks.go - the LookupMX mock was changed to support always.error and always.timeout in a manner similar to the LookupHost mock. Otherwise the TestValidateEmail unit test for the RA would fail when the MX lookup completed before the Host lookup because the error wouldn't be correct (empty DNS records vs a timeout or network error).

test/config/ra.json, test/config-next/ra.json - the dnsTries and dnsTimeout values were updated such that dnsTries * dnsTimeout was <= the WFE->RA RPC timeout (currently 15s in the test configs). This allows the dns lookups to all timeout without the overall RPC timing out.

Resolves #2260.

bdns/mocks.go

@@ -150,6 +150,11 @@ func (mock *MockDNSResolver) LookupMX(_ context.Context, domain string) ([]strin
 		fallthrough
 	case "email.com":
 		return []string{"mail.email.com"}, nil
+	case "always.error":
+		return []string{}, &DNSError{dns.TypeA, "always.error",
+			&net.OpError{Err: errors.New("always.error always errors")}, -1}
+	case "always.timeout":
+		return []string{}, &DNSError{dns.TypeA, "always.timeout", MockTimeoutError(), -1}
 	}
 	return nil, nil
 }

bdns/problem.go

@@ -27,6 +27,8 @@ func (d DNSError) Error() string {
 			} else {
 				detail = detailDNSNetFailure
 			}
+			// Note: we check d.underlying here even though `Timeout()` does this because the call to `netErr.Timeout()` above only
+			// happens for `*net.OpError` underlying types!
 		} else if d.underlying == context.Canceled || d.underlying == context.DeadlineExceeded {
 			detail = detailDNSTimeout
 		} else {
@@ -45,6 +47,8 @@ func (d DNSError) Error() string {
 func (d DNSError) Timeout() bool {
 	if netErr, ok := d.underlying.(*net.OpError); ok {
 		return netErr.Timeout()
+	} else if d.underlying == context.Canceled || d.underlying == context.DeadlineExceeded {
+		return true
 	}
 	return false
 }

ra/ra.go

@@ -164,6 +164,14 @@ const (
 	multipleAddressDetail  = "more than one e-mail address"
 )
 
+func problemIsTimeout(err error) bool {
+	if dnsErr, ok := err.(*bdns.DNSError); ok && dnsErr.Timeout() {
+		return true
+	}
+
+	return false
+}
+
 func validateEmail(ctx context.Context, address string, resolver bdns.DNSResolver) (prob *probs.ProblemDetails) {
 	emails, err := mail.ParseAddressList(address)
 	if err != nil {
@@ -189,6 +197,12 @@ func validateEmail(ctx context.Context, address string, resolver bdns.DNSResolve
 	}()
 	wg.Wait()
 
+	// We treat timeouts as non-failures for best-effort email validation
+	// See: https://github.com/letsencrypt/boulder/issues/2260
+	if problemIsTimeout(errMX) || problemIsTimeout(errA) {
+		return nil
+	}
+
 	if errMX != nil {
 		prob := bdns.ProblemDetailsFromDNSError(errMX)
 		prob.Type = probs.InvalidEmailProblem

ra/ra_test.go

@@ -321,12 +321,15 @@ func TestValidateEmail(t *testing.T) {
 		{"an email`", unparseableEmailDetail},
 		{"a@always.invalid", emptyDNSResponseDetail},
 		{"a@email.com, b@email.com", multipleAddressDetail},
-		{"a@always.timeout", "DNS problem: query timed out looking up A for always.timeout"},
 		{"a@always.error", "DNS problem: networking error looking up A for always.error"},
 	}
 	testSuccesses := []string{
 		"a@email.com",
 		"b@email.only",
+		// A timeout during email validation is treated as a success. We treat email
+		// validation during registration as a best-effort. See
+		// https://github.com/letsencrypt/boulder/issues/2260 for more
+		"a@always.timeout",
 	}
 
 	for _, tc := range testFailures {

test/config-next/ra.json

@@ -3,7 +3,7 @@
     "rateLimitPoliciesFilename": "test/rate-limit-policies.yml",
     "maxConcurrentRPCServerRequests": 16,
     "maxContactsPerRegistration": 100,
-    "dnsTries": 3,
+    "dnsTries": 2,
     "debugAddr": "localhost:8002",
     "hostnamePolicyFile": "test/hostname-policy.json",
     "maxNames": 1000,
@@ -68,7 +68,7 @@
 
   "common": {
     "dnsResolver": "127.0.0.1:8053",
-    "dnsTimeout": "10s",
+    "dnsTimeout": "5s",
     "dnsAllowLoopbackAddresses": true
   }
 }

test/config/ra.json

@@ -3,7 +3,7 @@
     "rateLimitPoliciesFilename": "test/rate-limit-policies.yml",
     "maxConcurrentRPCServerRequests": 16,
     "maxContactsPerRegistration": 100,
-    "dnsTries": 3,
+    "dnsTries": 2,
     "debugAddr": "localhost:8002",
     "hostnamePolicyFile": "test/hostname-policy.json",
     "maxNames": 1000,
@@ -56,7 +56,7 @@
 
   "common": {
     "dnsResolver": "127.0.0.1:8053",
-    "dnsTimeout": "10s",
+    "dnsTimeout": "5s",
     "dnsAllowLoopbackAddresses": true
   }
 }