RA: Remove email DNS validations. (#3851)

Performing DNS lookups to check the A/AAAA/MX records of a provided contact e-mail address adds variability to the RA's NewRegistration/UpdateRegistration functions and requires that the RA be able to reach out to the EFN. Since this is simply a convenience to prevent some classes of registration errors we can remove it to improve performance and to tighten up our security posture slightly.

Resolves https://github.com/letsencrypt/boulder/issues/3849

ra/ra.go

@@ -11,7 +11,6 @@ import (
 	"reflect"
 	"sort"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/jmhodges/clock"
@@ -161,80 +160,6 @@ func (ra *RegistrationAuthorityImpl) rateLimitPoliciesLoadError(err error) {
 	ra.log.Errf("error reloading rate limit policy: %s", err)
 }
 
-var (
-	unparseableEmailError = berrors.InvalidEmailError("not a valid e-mail address")
-	emptyDNSResponseError = berrors.InvalidEmailError(
-		"empty DNS response validating email domain - no MX/A records")
-	multipleAddressError = berrors.InvalidEmailError("more than one e-mail address")
-)
-
-func problemIsTimeout(err error) bool {
-	if dnsErr, ok := err.(*bdns.DNSError); ok && dnsErr.Timeout() {
-		return true
-	}
-
-	return false
-}
-
-// forbiddenMailDomains is a map of domain names we do not allow after the
-// @ symbol in contact mailto addresses. These are frequently used when
-// copy-pasting example configurations and would not result in expiration
-// messages and subscriber communications reaching the user that created the
-// registration if allowed.
-var forbiddenMailDomains = map[string]bool{
-	// https://tools.ietf.org/html/rfc2606#section-3
-	"example.com": true,
-	"example.net": true,
-	"example.org": true,
-}
-
-func validateEmail(ctx context.Context, address string, resolver bdns.DNSClient) error {
-	email, err := mail.ParseAddress(address)
-	if err != nil {
-		return unparseableEmailError
-	}
-	splitEmail := strings.SplitN(email.Address, "@", -1)
-	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
-	if forbiddenMailDomains[domain] {
-		return berrors.InvalidEmailError(
-			"invalid contact domain. Contact emails @%s are forbidden",
-			domain)
-	}
-	var resultMX []string
-	var resultA []net.IP
-	var errMX, errA error
-	var wg sync.WaitGroup
-	wg.Add(2)
-	go func() {
-		resultMX, errMX = resolver.LookupMX(ctx, domain)
-		wg.Done()
-	}()
-	go func() {
-		resultA, errA = resolver.LookupHost(ctx, domain)
-		wg.Done()
-	}()
-	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 {
-		return berrors.InvalidEmailError(errMX.Error())
-	} else if len(resultMX) > 0 {
-		return nil
-	}
-	if errA != nil {
-		return berrors.InvalidEmailError(errA.Error())
-	} else if len(resultA) > 0 {
-		return nil
-	}
-
-	return emptyDNSResponseError
-}
-
 // certificateRequestAuthz is a struct for holding information about a valid
 // authz referenced during a certificateRequestEvent. It holds both the
 // authorization ID and the challenge type that made the authorization valid. We
@@ -384,6 +309,15 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, init c
 	return reg, nil
 }
 
+// validateContacts checks the provided list of contacts, returning an error if
+// any are not acceptable. Unacceptable contacts lists include:
+// * An empty list
+// * A list has more than maxContactsPerReg contacts
+// * A list containing an empty contact
+// * A list containing a contact that does not parse as a URL
+// * A list containing a contact that has a URL scheme other than mailto
+// * A list containing a contact that has non-ascii characters
+// * A list containing a contact that doesn't pass `validateEmail`
 func (ra *RegistrationAuthorityImpl) validateContacts(ctx context.Context, contacts *[]string) error {
 	if contacts == nil || len(*contacts) == 0 {
 		return nil // Nothing to validate
@@ -413,21 +347,50 @@ func (ra *RegistrationAuthorityImpl) validateContacts(ctx context.Context, conta
 				contact,
 			)
 		}
-
+		if err := validateEmail(parsed.Opaque); err != nil {
-		start := ra.clk.Now()
-		ra.stats.Inc("ValidateEmail.Calls", 1)
-		err = validateEmail(ctx, parsed.Opaque, ra.DNSClient)
-		ra.stats.TimingDuration("ValidateEmail.Latency", ra.clk.Now().Sub(start))
-		if err != nil {
-			ra.stats.Inc("ValidateEmail.Errors", 1)
 			return err
 		}
-		ra.stats.Inc("ValidateEmail.Successes", 1)
 	}
 
 	return nil
 }
 
+var (
+	// unparseableEmailError is returned by validateEmail when the given address
+	// is not parseable.
+	unparseableEmailError = berrors.InvalidEmailError("not a valid e-mail address")
+)
+
+// forbiddenMailDomains is a map of domain names we do not allow after the
+// @ symbol in contact mailto addresses. These are frequently used when
+// copy-pasting example configurations and would not result in expiration
+// messages and subscriber communications reaching the user that created the
+// registration if allowed.
+var forbiddenMailDomains = map[string]bool{
+	// https://tools.ietf.org/html/rfc2606#section-3
+	"example.com": true,
+	"example.net": true,
+	"example.org": true,
+}
+
+// validateEmail returns an error if the given address is not parseable as an
+// email address or if the domain portion of the email address is a member of
+// the forbiddenMailDomains map.
+func validateEmail(address string) error {
+	email, err := mail.ParseAddress(address)
+	if err != nil {
+		return unparseableEmailError
+	}
+	splitEmail := strings.SplitN(email.Address, "@", -1)
+	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
+	if forbiddenMailDomains[domain] {
+		return berrors.InvalidEmailError(
+			"invalid contact domain. Contact emails @%s are forbidden",
+			domain)
+	}
+	return nil
+}
+
 func (ra *RegistrationAuthorityImpl) checkPendingAuthorizationLimit(ctx context.Context, regID int64) error {
 	limit := ra.rlPolicies.PendingAuthorizationsPerAccount()
 	if limit.Enabled() {

ra/ra_test.go

@@ -330,6 +330,8 @@ func TestValidateContacts(t *testing.T) {
 	otherValidEmail := "mailto:other-admin@email.com"
 	malformedEmail := "mailto:admin.com"
 	nonASCII := "mailto:señor@email.com"
+	unparseable := "mailto:a@email.com, b@email.com"
+	forbidden := "mailto:a@example.org"
 
 	err := ra.validateContacts(context.Background(), &[]string{})
 	test.AssertNotError(t, err, "No Contacts")
@@ -351,49 +353,12 @@ func TestValidateContacts(t *testing.T) {
 
 	err = ra.validateContacts(context.Background(), &[]string{nonASCII})
 	test.AssertError(t, err, "Non ASCII email")
-}
 
-func TestValidateEmail(t *testing.T) {
+	err = ra.validateContacts(context.Background(), &[]string{unparseable})
-	testFailures := []struct {
+	test.AssertError(t, err, "Unparseable email")
-		input    string
-		expected string
-	}{
-		{"an email`", unparseableEmailError.Error()},
-		{"a@always.invalid", emptyDNSResponseError.Error()},
-		{"a@email.com, b@email.com", unparseableEmailError.Error()},
-		{"a@always.error", "DNS problem: networking error looking up A for always.error"},
-		{"a@example.com", "invalid contact domain. Contact emails @example.com are forbidden"},
-		{"a@example.net", "invalid contact domain. Contact emails @example.net are forbidden"},
-		{"a@example.org", "invalid contact domain. Contact emails @example.org are forbidden"},
-	}
 
-	testSuccesses := []string{
+	err = ra.validateContacts(context.Background(), &[]string{forbidden})
-		"a@email.com",
+	test.AssertError(t, err, "Forbidden email")
-		"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 {
-		err := validateEmail(context.Background(), tc.input, &bdns.MockDNSClient{})
-		if !berrors.Is(err, berrors.InvalidEmail) {
-			t.Errorf("validateEmail(%q): got error %#v, expected type berrors.InvalidEmail", tc.input, err)
-		}
-
-		if err.Error() != tc.expected {
-			t.Errorf("validateEmail(%q): got %#v, expected %#v",
-				tc.input, err.Error(), tc.expected)
-		}
-	}
-
-	for _, addr := range testSuccesses {
-		if err := validateEmail(context.Background(), addr, &bdns.MockDNSClient{}); err != nil {
-			t.Errorf("validateEmail(%q): expected success, but it failed: %#v",
-				addr, err)
-		}
-	}
 }
 
 func TestNewRegistration(t *testing.T) {

test/challtestsrv/dnsone.go

@@ -63,18 +63,6 @@ func (s *ChallSrv) dnsHandler(w dns.ResponseWriter, r *dns.Msg) {
 			}
 			record.A = net.ParseIP(fakeDNS)
 
-			m.Answer = append(m.Answer, record)
-		case dns.TypeMX:
-			record := new(dns.MX)
-			record.Hdr = dns.RR_Header{
-				Name:   q.Name,
-				Rrtype: dns.TypeMX,
-				Class:  dns.ClassINET,
-				Ttl:    0,
-			}
-			record.Mx = "mail." + q.Name
-			record.Preference = 10
-
 			m.Answer = append(m.Answer, record)
 		case dns.TypeTXT:
 			values := s.GetDNSOneChallenge(q.Name)