Merge pull request #1121 from letsencrypt/no-500-dns

Don't serve 500's on DNS timeout.

dns/problem.go

@@ -0,0 +1,29 @@
+package dns
+
+import (
+	"net"
+
+	"github.com/letsencrypt/boulder/core"
+)
+
+const detailDNSTimeout = "DNS query timed out"
+const detailDNSNetFailure = "DNS networking error"
+const detailServerFailure = "Server failure at resolver"
+
+// ProblemDetailsFromDNSError checks the error returned from Lookup...
+// methods and tests if the error was an underlying net.OpError or an error
+// caused by resolver returning SERVFAIL or other invalid Rcodes and returns
+// the relevant core.ProblemDetails.
+func ProblemDetailsFromDNSError(err error) *core.ProblemDetails {
+	problem := &core.ProblemDetails{Type: core.ConnectionProblem}
+	if netErr, ok := err.(*net.OpError); ok {
+		if netErr.Timeout() {
+			problem.Detail = detailDNSTimeout
+		} else {
+			problem.Detail = detailDNSNetFailure
+		}
+	} else {
+		problem.Detail = detailServerFailure
+	}
+	return problem
+}

dns/problem_test.go

@@ -0,0 +1,37 @@
+package dns
+
+import (
+	"errors"
+	"net"
+	"testing"
+
+	"github.com/letsencrypt/boulder/core"
+	"github.com/letsencrypt/boulder/mocks"
+)
+
+func TestProblemDetailsFromDNSError(t *testing.T) {
+	testCases := []struct {
+		err      error
+		expected string
+	}{
+		{
+			mocks.TimeoutError(),
+			detailDNSTimeout,
+		}, {
+			errors.New("other failure"),
+			detailServerFailure,
+		}, {
+			&net.OpError{Err: errors.New("some net error")},
+			detailDNSNetFailure,
+		},
+	}
+	for _, tc := range testCases {
+		err := ProblemDetailsFromDNSError(tc.err)
+		if err.Type != core.ConnectionProblem {
+			t.Errorf("ProblemDetailsFromDNSError(%q).Type = %q, expected %q", tc.err, err.Type, core.ConnectionProblem)
+		}
+		if err.Detail != tc.expected {
+			t.Errorf("ProblemDetailsFromDNSError(%q).Detail = %q, expected %q", tc.err, err.Detail, tc.expected)
+		}
+	}
+}

mocks/mocks.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net"
+	"os"
 	"strings"
 	"time"
 
@@ -44,11 +45,35 @@ func (mock *DNSResolver) LookupTXT(hostname string) ([]string, time.Duration, er
 	return []string{"hostname"}, 0, nil
 }
 
+// TimeoutError returns a a net.OpError for which Timeout() returns true.
+func TimeoutError() *net.OpError {
+	return &net.OpError{
+		Err: os.NewSyscallError("ugh timeout", timeoutError{}),
+	}
+}
+
+type timeoutError struct{}
+
+func (t timeoutError) Error() string {
+	return "so sloooow"
+}
+func (t timeoutError) Timeout() bool {
+	return true
+}
+
 // LookupHost is a mock
 func (mock *DNSResolver) LookupHost(hostname string) ([]net.IP, time.Duration, error) {
 	if hostname == "always.invalid" || hostname == "invalid.invalid" {
 		return []net.IP{}, 0, nil
 	}
+	if hostname == "always.timeout" {
+		return []net.IP{}, 0, TimeoutError()
+	}
+	if hostname == "always.error" {
+		return []net.IP{}, 0, &net.OpError{
+			Err: errors.New("some net error"),
+		}
+	}
 	ip := net.ParseIP("127.0.0.1")
 	return []net.IP{ip}, 0, nil
 }
@@ -58,6 +83,8 @@ func (mock *DNSResolver) LookupCAA(domain string) ([]*dns.CAA, time.Duration, er
 	var results []*dns.CAA
 	var record dns.CAA
 	switch strings.TrimRight(domain, ".") {
+	case "caa-timeout.com":
+		return nil, 0, TimeoutError()
 	case "reserved.com":
 		record.Tag = "issue"
 		record.Value = "symantec.com"

ra/registration-authority.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/letsencrypt/boulder/cmd"
 	"github.com/letsencrypt/boulder/core"
+	"github.com/letsencrypt/boulder/dns"
 	blog "github.com/letsencrypt/boulder/log"
 )
 
@@ -77,19 +78,23 @@ func NewRegistrationAuthorityImpl(clk clock.Clock, logger *blog.AuditLogger, sta
 	return ra
 }
 
+var errUnparseableEmail = errors.New("not a valid e-mail address")
+var errEmptyDNSResponse = errors.New("empty DNS response")
+
 func validateEmail(address string, resolver core.DNSResolver) (rtt time.Duration, err error) {
 	_, err = mail.ParseAddress(address)
 	if err != nil {
-		err = core.MalformedRequestError(fmt.Sprintf("%s is not a valid e-mail address", address))
+		return time.Duration(0), errUnparseableEmail
-		return
 	}
 	splitEmail := strings.SplitN(address, "@", -1)
 	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
-	var mx []string
+	result, rtt, err := resolver.LookupHost(domain)
-	mx, rtt, err = resolver.LookupMX(domain)
+	if err == nil && len(result) == 0 {
-	if err != nil || len(mx) == 0 {
+		err = errEmptyDNSResponse
-		err = core.MalformedRequestError(fmt.Sprintf("No MX record for domain %s", domain))
+	}
-		return
+	if err != nil {
+		problem := dns.ProblemDetailsFromDNSError(err)
+		err = core.MalformedRequestError(problem.Detail)
 	}
 
 	return
@@ -216,10 +221,11 @@ func (ra *RegistrationAuthorityImpl) validateContacts(contacts []*core.AcmeURL)
 			continue
 		case "mailto":
 			rtt, err := validateEmail(contact.Opaque, ra.DNSResolver)
-			ra.stats.TimingDuration("RA.DNS.RTT.MX", rtt, 1.0)
+			ra.stats.TimingDuration("RA.DNS.RTT.A", rtt, 1.0)
 			ra.stats.Inc("RA.DNS.Rate", 1, 1.0)
 			if err != nil {
-				return err
+				return core.MalformedRequestError(fmt.Sprintf(
+					"Validation of contact %s failed: %s", contact, err))
 			}
 		default:
 			err = core.MalformedRequestError(fmt.Sprintf("Contact method %s is not supported", contact.Scheme))
@@ -281,18 +287,6 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization
 		}
 	}
 
-	// Check CAA records for the requested identifier
-	present, valid, err := ra.VA.CheckCAARecords(identifier)
-	if err != nil {
-		return authz, err
-	}
-	// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
-	ra.log.Audit(fmt.Sprintf("Checked CAA records for %s, registration ID %d [Present: %t, Valid for issuance: %t]", identifier.Value, regID, present, valid))
-	if !valid {
-		err = errors.New("CAA check for identifier failed")
-		return authz, err
-	}
-
 	// Create validations. The WFE will  update them with URIs before sending them out.
 	challenges, combinations, err := ra.PA.ChallengesFor(identifier, &reg.Key)
 

ra/registration-authority_test.go

@@ -279,7 +279,6 @@ func TestValidateContacts(t *testing.T) {
 	tel, _ := core.ParseAcmeURL("tel:")
 	ansible, _ := core.ParseAcmeURL("ansible:earth.sol.milkyway.laniakea/letsencrypt")
 	validEmail, _ := core.ParseAcmeURL("mailto:admin@email.com")
-	invalidEmail, _ := core.ParseAcmeURL("mailto:admin@example.com")
 	malformedEmail, _ := core.ParseAcmeURL("mailto:admin.com")
 
 	err := ra.validateContacts([]*core.AcmeURL{})
@@ -294,30 +293,33 @@ func TestValidateContacts(t *testing.T) {
 	err = ra.validateContacts([]*core.AcmeURL{validEmail})
 	test.AssertNotError(t, err, "Valid Email")
 
-	err = ra.validateContacts([]*core.AcmeURL{invalidEmail})
-	test.AssertError(t, err, "Invalid Email")
-
 	err = ra.validateContacts([]*core.AcmeURL{malformedEmail})
 	test.AssertError(t, err, "Malformed Email")
 
 	err = ra.validateContacts([]*core.AcmeURL{ansible})
-	test.AssertError(t, err, "Unknown scehme")
+	test.AssertError(t, err, "Unknown scheme")
 }
 
 func TestValidateEmail(t *testing.T) {
-	_, err := validateEmail("an email`", &mocks.DNSResolver{})
+	testCases := []struct {
-	test.AssertError(t, err, "Malformed")
+		input    string
-
+		expected string
-	_, err = validateEmail("a@not.a.domain", &mocks.DNSResolver{})
+	}{
-	test.AssertError(t, err, "Cannot resolve")
+		{"an email`", errUnparseableEmail.Error()},
-	t.Logf("No Resolve: %s", err)
+		{"a@always.invalid", "Server failure at resolver"},
-
+		{"a@always.timeout", "DNS query timed out"},
-	_, err = validateEmail("a@example.com", &mocks.DNSResolver{})
+		{"a@always.error", "DNS networking error"},
-	test.AssertError(t, err, "No MX Record")
+	}
-	t.Logf("No MX: %s", err)
+	for _, tc := range testCases {
-
+		_, err := validateEmail(tc.input, &mocks.DNSResolver{})
-	_, err = validateEmail("a@email.com", &mocks.DNSResolver{})
+		if err.Error() != tc.expected {
-	test.AssertNotError(t, err, "Valid")
+			t.Errorf("validateEmail(%q): got %#v, expected %#v",
+				tc.input, err, tc.expected)
+		}
+	}
+	if _, err := validateEmail("a@email.com", &mocks.DNSResolver{}); err != nil {
+		t.Errorf("Expected a@email.com to validate, but it failed: %s", err)
+	}
 }
 
 func TestNewRegistration(t *testing.T) {

va/validation-authority.go

@@ -11,7 +11,6 @@ import (
 	"crypto/tls"
 	"encoding/hex"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -28,18 +27,14 @@ import (
 	"github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/miekg/dns"
 
 	"github.com/letsencrypt/boulder/core"
+	bdns "github.com/letsencrypt/boulder/dns"
 	blog "github.com/letsencrypt/boulder/log"
 )
 
-const maxCNAME = 16 // Prevents infinite loops. Same limit as BIND.
 const maxRedirect = 10
 
 var validationTimeout = time.Second * 5
 
-// ErrTooManyCNAME is returned by CheckCAARecords if it has to follow too many
-// consecutive CNAME lookups.
-var ErrTooManyCNAME = errors.New("too many CNAME/DNAME lookups")
-
 // ValidationAuthorityImpl represents a VA
 type ValidationAuthorityImpl struct {
 	RA           core.RegistrationAuthority
@@ -124,24 +119,6 @@ func verifyValidationJWS(validation *jose.JsonWebSignature, accountKey *jose.Jso
 	return nil
 }
 
-// problemDetailsFromDNSError checks the error returned from Lookup...
-// methods and tests if the error was an underlying net.OpError or an error
-// caused by resolver returning SERVFAIL or other invalid Rcodes and returns
-// the relevant core.ProblemDetails.
-func problemDetailsFromDNSError(err error) *core.ProblemDetails {
-	problem := &core.ProblemDetails{Type: core.ConnectionProblem}
-	if netErr, ok := err.(*net.OpError); ok {
-		if netErr.Timeout() {
-			problem.Detail = "DNS query timed out"
-		} else if netErr.Temporary() {
-			problem.Detail = "Temporary network connectivity error"
-		}
-	} else {
-		problem.Detail = "Server failure at resolver"
-	}
-	return problem
-}
-
 // getAddr will query for all A records associated with hostname and return the
 // prefered address, the first net.IP in the addrs slice, and all addresses resolved.
 // This is the same choice made by the Go internal resolution library used by
@@ -150,7 +127,7 @@ func problemDetailsFromDNSError(err error) *core.ProblemDetails {
 func (va ValidationAuthorityImpl) getAddr(hostname string) (addr net.IP, addrs []net.IP, problem *core.ProblemDetails) {
 	addrs, rtt, err := va.DNSResolver.LookupHost(hostname)
 	if err != nil {
-		problem = problemDetailsFromDNSError(err)
+		problem = bdns.ProblemDetailsFromDNSError(err)
 		va.log.Debug(fmt.Sprintf("%s DNS failure: %s", hostname, err))
 		return
 	}
@@ -634,7 +611,7 @@ func (va *ValidationAuthorityImpl) validateDNS01(identifier core.AcmeIdentifier,
 
 	if err != nil {
 		challenge.Status = core.StatusInvalid
-		challenge.Error = problemDetailsFromDNSError(err)
+		challenge.Error = bdns.ProblemDetailsFromDNSError(err)
 		va.log.Debug(fmt.Sprintf("%s [%s] DNS failure: %s", challenge.Type, identifier, err))
 		return challenge, challenge.Error
 	}
@@ -654,6 +631,24 @@ func (va *ValidationAuthorityImpl) validateDNS01(identifier core.AcmeIdentifier,
 	return challenge, challenge.Error
 }
 
+func (va *ValidationAuthorityImpl) checkCAA(identifier core.AcmeIdentifier, regID int64) *core.ProblemDetails {
+	// Check CAA records for the requested identifier
+	present, valid, err := va.CheckCAARecords(identifier)
+	if err != nil {
+		va.log.Warning(fmt.Sprintf("Problem checking CAA: %s", err))
+		return bdns.ProblemDetailsFromDNSError(err)
+	}
+	// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
+	va.log.Audit(fmt.Sprintf("Checked CAA records for %s, registration ID %d [Present: %t, Valid for issuance: %t]", identifier.Value, regID, present, valid))
+	if !valid {
+		return &core.ProblemDetails{
+			Type:   core.ConnectionProblem,
+			Detail: "CAA check for identifier failed",
+		}
+	}
+	return nil
+}
+
 // Overall validation process
 
 func (va *ValidationAuthorityImpl) validate(authz core.Authorization, challengeIndex int) {
@@ -673,20 +668,7 @@ func (va *ValidationAuthorityImpl) validate(authz core.Authorization, challengeI
 		var err error
 
 		vStart := va.clk.Now()
-		switch authz.Challenges[challengeIndex].Type {
+		authz.Challenges[challengeIndex], err = va.validateChallengeAndCAA(authz.Identifier, authz.Challenges[challengeIndex], authz.RegistrationID)
-		case core.ChallengeTypeSimpleHTTP:
-			// TODO(https://github.com/letsencrypt/boulder/issues/894): Delete this case
-			authz.Challenges[challengeIndex], err = va.validateSimpleHTTP(authz.Identifier, authz.Challenges[challengeIndex])
-		case core.ChallengeTypeDVSNI:
-			// TODO(https://github.com/letsencrypt/boulder/issues/894): Delete this case
-			authz.Challenges[challengeIndex], err = va.validateDvsni(authz.Identifier, authz.Challenges[challengeIndex])
-		case core.ChallengeTypeHTTP01:
-			authz.Challenges[challengeIndex], err = va.validateHTTP01(authz.Identifier, authz.Challenges[challengeIndex])
-		case core.ChallengeTypeTLSSNI01:
-			authz.Challenges[challengeIndex], err = va.validateTLSSNI01(authz.Identifier, authz.Challenges[challengeIndex])
-		case core.ChallengeTypeDNS01:
-			authz.Challenges[challengeIndex], err = va.validateDNS01(authz.Identifier, authz.Challenges[challengeIndex])
-		}
 		va.stats.TimingDuration(fmt.Sprintf("VA.Validations.%s.%s", authz.Challenges[challengeIndex].Type, authz.Challenges[challengeIndex].Status), time.Since(vStart), 1.0)
 
 		if err != nil {
@@ -709,6 +691,42 @@ func (va *ValidationAuthorityImpl) validate(authz core.Authorization, challengeI
 	va.RA.OnValidationUpdate(authz)
 }
 
+func (va *ValidationAuthorityImpl) validateChallengeAndCAA(identifier core.AcmeIdentifier, challenge core.Challenge, regID int64) (core.Challenge, error) {
+	result, err := va.validateChallenge(identifier, challenge)
+	if err != nil {
+		return result, err
+	}
+
+	// Checking CAA happens after challenge validation because DNS errors affect
+	// both, and giving a DNS error on validation makes more sense than a DNS
+	// error on CAA.
+	problemDetails := va.checkCAA(identifier, regID)
+	if problemDetails != nil {
+		challenge.Error = problemDetails
+		challenge.Status = core.StatusInvalid
+		return result, problemDetails
+	}
+	return result, nil
+}
+
+func (va *ValidationAuthorityImpl) validateChallenge(identifier core.AcmeIdentifier, challenge core.Challenge) (core.Challenge, error) {
+	switch challenge.Type {
+	case core.ChallengeTypeSimpleHTTP:
+		// TODO(https://github.com/letsencrypt/boulder/issues/894): Delete this case
+		return va.validateSimpleHTTP(identifier, challenge)
+	case core.ChallengeTypeDVSNI:
+		// TODO(https://github.com/letsencrypt/boulder/issues/894): Delete this case
+		return va.validateDvsni(identifier, challenge)
+	case core.ChallengeTypeHTTP01:
+		return va.validateHTTP01(identifier, challenge)
+	case core.ChallengeTypeTLSSNI01:
+		return va.validateTLSSNI01(identifier, challenge)
+	case core.ChallengeTypeDNS01:
+		return va.validateDNS01(identifier, challenge)
+	}
+	return core.Challenge{}, fmt.Errorf("invalid challenge type %s", challenge.Type)
+}
+
 // UpdateValidations runs the validate() method asynchronously using goroutines.
 func (va *ValidationAuthorityImpl) UpdateValidations(authz core.Authorization, challengeIndex int) error {
 	go va.validate(authz, challengeIndex)

va/validation-authority_test.go

@@ -1061,6 +1061,21 @@ func TestUpdateValidations(t *testing.T) {
 	test.Assert(t, (took < (time.Second * 3)), "UpdateValidations blocked")
 }
 
+func TestCAATimeout(t *testing.T) {
+	stats, _ := statsd.NewNoopClient()
+	va := NewValidationAuthorityImpl(&PortConfig{}, nil, stats, clock.Default())
+	va.DNSResolver = &mocks.DNSResolver{}
+	va.IssuerDomain = "letsencrypt.org"
+	err := va.checkCAA(core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "caa-timeout.com"}, 101)
+	if err.Type != core.ConnectionProblem {
+		t.Errorf("Expected timeout error type %s, got %s", core.ConnectionProblem, err.Type)
+	}
+	expected := "DNS query timed out"
+	if err.Detail != expected {
+		t.Errorf("checkCAA: got %s, expected %s", err.Detail, expected)
+	}
+}
+
 func TestCAAChecking(t *testing.T) {
 	type CAATest struct {
 		Domain  string