Merge pull request #1276 from letsencrypt/email_error

return ProblemDetails when validating emails in ra

probs/probs.go

@@ -15,6 +15,7 @@ const (
 	UnknownHostProblem    = ProblemType("urn:acme:error:unknownHost")
 	RateLimitedProblem    = ProblemType("urn:acme:error:rateLimited")
 	BadNonceProblem       = ProblemType("urn:acme:error:badNonce")
+	InvalidEmailProblem   = ProblemType("urn:acme:error:invalidEmail")
 )
 
 // ProblemType defines the error types in the ACME protocol

ra/registration-authority.go

@@ -20,6 +20,7 @@ import (
 	"github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/cactus/go-statsd-client/statsd"
 	"github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/jmhodges/clock"
 	"github.com/letsencrypt/boulder/Godeps/_workspace/src/golang.org/x/net/publicsuffix"
+	"github.com/letsencrypt/boulder/probs"
 
 	"github.com/letsencrypt/boulder/bdns"
 	"github.com/letsencrypt/boulder/cmd"
@@ -78,13 +79,18 @@ func NewRegistrationAuthorityImpl(clk clock.Clock, logger *blog.AuditLogger, sta
 	return ra
 }
 
-var errUnparseableEmail = errors.New("not a valid e-mail address")
+const (
-var errEmptyDNSResponse = errors.New("empty DNS response")
+	unparseableEmailDetail = "not a valid e-mail address"
+	emptyDNSResponseDetail = "empty DNS response"
+)
 
-func validateEmail(address string, resolver bdns.DNSResolver) (rtt time.Duration, count int64, err error) {
+func validateEmail(address string, resolver bdns.DNSResolver) (rtt time.Duration, count int64, prob *probs.ProblemDetails) {
-	_, err = mail.ParseAddress(address)
+	_, err := mail.ParseAddress(address)
 	if err != nil {
-		return time.Duration(0), 0, errUnparseableEmail
+		return 0, 0, &probs.ProblemDetails{
+			Type:   probs.InvalidEmailProblem,
+			Detail: unparseableEmailDetail,
+		}
 	}
 	splitEmail := strings.SplitN(address, "@", -1)
 	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
@@ -93,20 +99,27 @@ func validateEmail(address string, resolver bdns.DNSResolver) (rtt time.Duration
 	var resultA []net.IP
 	resultMX, rtt1, err = resolver.LookupMX(domain)
 	count++
+
 	if err == nil && len(resultMX) == 0 {
 		resultA, rtt2, err = resolver.LookupHost(domain)
 		count++
 		if err == nil && len(resultA) == 0 {
-			err = errEmptyDNSResponse
+			return rtt1 + rtt2, count, &probs.ProblemDetails{
+				Type:   probs.InvalidEmailProblem,
+				Detail: emptyDNSResponseDetail,
+			}
 		}
 	}
 	if err != nil {
-		problem := bdns.ProblemDetailsFromDNSError(err)
+		dnsProblem := bdns.ProblemDetailsFromDNSError(err)
-		err = core.MalformedRequestError(problem.Detail)
+		return rtt, count, &probs.ProblemDetails{
+			Type:   probs.InvalidEmailProblem,
+			Detail: dnsProblem.Detail,
+		}
 	}
 	rtt = rtt1 + rtt2
 
-	return
+	return rtt, count, nil
 }
 
 type certificateRequestEvent struct {
@@ -235,14 +248,13 @@ func (ra *RegistrationAuthorityImpl) validateContacts(contacts []*core.AcmeURL)
 			// Note: the stats handling here is a bit of a lie,
 			// since validateEmail() mainly does MX lookups and
 			// only does A lookups when the MX is missing.
-			rtt, count, err := validateEmail(contact.Opaque, ra.DNSResolver)
+			rtt, count, problem := validateEmail(contact.Opaque, ra.DNSResolver)
 			if count > 0 {
 				ra.stats.TimingDuration("RA.DNS.RTT.A", time.Duration(int64(rtt)/count), 1.0)
 				ra.stats.Inc("RA.DNS.Rate", count, 1.0)
 			}
-			if err != nil {
+			if problem != nil {
-				return core.MalformedRequestError(fmt.Sprintf(
+				return problem
-					"Validation of contact %s failed: %s", contact, err))
 			}
 		default:
 			err = core.MalformedRequestError(fmt.Sprintf("Contact method %s is not supported", contact.Scheme))

ra/registration-authority_test.go

@@ -28,6 +28,7 @@ import (
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/mocks"
 	"github.com/letsencrypt/boulder/policy"
+	"github.com/letsencrypt/boulder/probs"
 	"github.com/letsencrypt/boulder/sa"
 	"github.com/letsencrypt/boulder/test"
 	"github.com/letsencrypt/boulder/test/vars"
@@ -314,8 +315,8 @@ func TestValidateEmail(t *testing.T) {
 		input    string
 		expected string
 	}{
-		{"an email`", errUnparseableEmail.Error()},
+		{"an email`", unparseableEmailDetail},
-		{"a@always.invalid", "Server failure at resolver"},
+		{"a@always.invalid", emptyDNSResponseDetail},
 		{"a@always.timeout", "DNS query timed out"},
 		{"a@always.error", "DNS networking error"},
 	}
@@ -324,16 +325,20 @@ func TestValidateEmail(t *testing.T) {
 		"b@email.only",
 	}
 	for _, tc := range testFailures {
-		_, _, err := validateEmail(tc.input, &mocks.DNSResolver{})
+		_, _, problem := validateEmail(tc.input, &mocks.DNSResolver{})
-		if err.Error() != tc.expected {
+		if problem.Type != probs.InvalidEmailProblem {
+			t.Errorf("validateEmail(%q): got problem type %#v, expected %#v", tc.input, problem.Type, probs.InvalidEmailProblem)
+		}
+		if problem.Detail != tc.expected {
 			t.Errorf("validateEmail(%q): got %#v, expected %#v",
-				tc.input, err, tc.expected)
+				tc.input, problem.Detail, tc.expected)
 		}
 	}
+
 	for _, addr := range testSuccesses {
-		if _, _, err := validateEmail(addr, &mocks.DNSResolver{}); err != nil {
+		if _, _, prob := validateEmail(addr, &mocks.DNSResolver{}); prob != nil {
 			t.Errorf("validateEmail(%q): expected success, but it failed: %s",
-				addr, err)
+				addr, prob)
 		}
 	}
 }

test/test-tools.go

@@ -50,7 +50,7 @@ func AssertNotError(t *testing.T, err error, message string) {
 // AssertError checks that err is non-nil
 func AssertError(t *testing.T, err error, message string) {
 	if err == nil {
-		t.Fatalf("%s %s", caller(), message)
+		t.Fatalf("%s %s: expected error but received none", caller(), message)
 	}
 }