Merge pull request #354 from letsencrypt/344-internal_server_errors

Resolves Issue #344: Only send InternalServerError when needed

core/util.go

@@ -42,6 +42,12 @@ var BuildTime string
 
 // Errors
 
+// InternalServerError indicates that something has gone wrong unrelated to the
+// user's input, and will be considered by the Load Balancer as an indication
+// that this Boulder instance may be malfunctioning. Minimally, returning this
+// will cause an error page to be generated at the CDN/LB for the client.
+// Consequently, you should only use this error when Boulder's internal
+// constraints have been violated.
 type InternalServerError string
 type NotSupportedError string
 type MalformedRequestError string

ra/registration-authority.go

@@ -61,17 +61,32 @@ func validateEmail(address string) (err error) {
 	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
 	var mx []*net.MX
 	mx, err = net.LookupMX(domain)
-	if err != nil {
+	if err != nil || len(mx) == 0 {
-		err = core.InternalServerError(err.Error())
-		return
-	}
-	if len(mx) == 0 {
 		err = core.MalformedRequestError(fmt.Sprintf("No MX record for domain %s", domain))
 		return
 	}
 	return
 }
 
+func validateContacts(contacts []core.AcmeURL) (err error) {
+	for _, contact := range contacts {
+		switch contact.Scheme {
+		case "tel":
+			continue
+		case "mailto":
+			err = validateEmail(contact.Opaque)
+			if err != nil {
+				return
+			}
+		default:
+			err = core.MalformedRequestError(fmt.Sprintf("Contact method %s is not supported", contact.Scheme))
+			return
+		}
+	}
+
+	return
+}
+
 type certificateRequestEvent struct {
 	ID                  string    `json:",omitempty"`
 	Requester           int64     `json:",omitempty"`
@@ -98,24 +113,16 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration) (re
 	}
 	reg.MergeUpdate(init)
 
-	for _, contact := range reg.Contact {
+	err = validateContacts(reg.Contact)
-		switch contact.Scheme {
-		case "tel":
-			continue
-		case "mailto":
-			err = validateEmail(contact.Opaque)
 	if err != nil {
 		return
 	}
-		default:
-			err = core.MalformedRequestError(fmt.Sprintf("Contact method %s is not supported", contact.Scheme))
-			return
-		}
-	}
 
 	// Store the authorization object, then return it
 	reg, err = ra.SA.NewRegistration(reg)
 	if err != nil {
+		// InternalServerError since the user-data was validated before being
+		// passed to the SA.
 		err = core.InternalServerError(err.Error())
 	}
 
@@ -124,7 +131,7 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration) (re
 
 func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization, regID int64) (authz core.Authorization, err error) {
 	if regID <= 0 {
-		err = core.InternalServerError("Invalid registration ID")
+		err = core.MalformedRequestError(fmt.Sprintf("Invalid registration ID: %d", regID))
 		return authz, err
 	}
 
@@ -162,7 +169,9 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization
 	// Get a pending Auth first so we can get our ID back, then update with challenges
 	authz, err = ra.SA.NewPendingAuthorization(authz)
 	if err != nil {
-		err = core.InternalServerError(err.Error())
+		// InternalServerError since the user-data was validated before being
+		// passed to the SA.
+		err = core.InternalServerError(fmt.Sprintf("Invalid authorization request: %s", err))
 		return authz, err
 	}
 
@@ -173,6 +182,8 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization
 		challenges[i].URI = core.AcmeURL(*challengeURI)
 
 		if !challenges[i].IsSane(false) {
+			// InternalServerError because we generated these challenges, they should
+			// be OK.
 			err = core.InternalServerError(fmt.Sprintf("Challenge didn't pass sanity check: %+v", challenges[i]))
 			return authz, err
 		}
@@ -184,6 +195,8 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization
 	// Store the authorization object, then return it
 	err = ra.SA.UpdatePendingAuthorization(authz)
 	if err != nil {
+		// InternalServerError because we created the authorization just above,
+		// and adding Sane challenges should not break it.
 		err = core.InternalServerError(err.Error())
 	}
 	return authz, err
@@ -211,7 +224,7 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(req core.CertificateRequest,
 	}()
 
 	if regID <= 0 {
-		err = core.InternalServerError("Invalid registration ID")
+		err = core.MalformedRequestError(fmt.Sprintf("Invalid registration ID: %d", regID))
 		return emptyCert, err
 	}
 
@@ -312,7 +325,10 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(req core.CertificateRequest,
 
 	// Create the certificate and log the result
 	if cert, err = ra.CA.IssueCertificate(*csr, regID, earliestExpiry); err != nil {
-		err = core.InternalServerError(err.Error())
+		// While this could be InternalServerError for certain conditions, most
+		// of the failure reasons (such as GoodKey failing) are caused by malformed
+		// requests.
+		err = core.MalformedRequestError(err.Error())
 		logEvent.Error = err.Error()
 		return emptyCert, err
 	}
@@ -325,6 +341,8 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(req core.CertificateRequest,
 
 	parsedCertificate, err := x509.ParseCertificate([]byte(cert.DER))
 	if err != nil {
+		// InternalServerError because the certificate from the CA should be
+		// parseable.
 		err = core.InternalServerError(err.Error())
 		logEvent.Error = err.Error()
 		return emptyCert, err
@@ -342,10 +360,18 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(req core.CertificateRequest,
 
 func (ra *RegistrationAuthorityImpl) UpdateRegistration(base core.Registration, update core.Registration) (reg core.Registration, err error) {
 	base.MergeUpdate(update)
+
+	err = validateContacts(base.Contact)
+	if err != nil {
+		return
+	}
+
 	reg = base
 	err = ra.SA.UpdateRegistration(base)
 	if err != nil {
-		err = core.InternalServerError(err.Error())
+		// InternalServerError since the user-data was validated before being
+		// passed to the SA.
+		err = core.InternalServerError(fmt.Sprintf("Could not update registration: %s", err))
 	}
 	return
 }
@@ -354,14 +380,16 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(base core.Authorization
 	// Copy information over that the client is allowed to supply
 	authz = base
 	if challengeIndex >= len(authz.Challenges) {
-		err = core.MalformedRequestError("Invalid challenge index")
+		err = core.MalformedRequestError(fmt.Sprintf("Invalid challenge index: %d", challengeIndex))
 		return
 	}
 	authz.Challenges[challengeIndex] = authz.Challenges[challengeIndex].MergeResponse(response)
 
 	// Store the updated version
 	if err = ra.SA.UpdatePendingAuthorization(authz); err != nil {
-		err = core.InternalServerError(err.Error())
+		// This can pretty much only happen when the client corrupts the Challenge
+		// data.
+		err = core.MalformedRequestError(err.Error())
 		return
 	}
 

ra/registration-authority_test.go

@@ -219,6 +219,48 @@ func assertAuthzEqual(t *testing.T, a1, a2 core.Authorization) {
 	// Not testing: Challenges
 }
 
+func TestValidateContacts(t *testing.T) {
+	tel, _ := url.Parse("tel:")
+	ansible, _ := url.Parse("ansible:earth.sol.milkyway.laniakea/letsencrypt")
+	validEmail, _ := url.Parse("mailto:admin@email.com")
+	invalidEmail, _ := url.Parse("mailto:admin@example.com")
+	malformedEmail, _ := url.Parse("mailto:admin.com")
+
+	err := validateContacts([]core.AcmeURL{})
+	test.AssertNotError(t, err, "No Contacts")
+
+	err = validateContacts([]core.AcmeURL{core.AcmeURL(*tel)})
+	test.AssertNotError(t, err, "Simple Telephone")
+
+	err = validateContacts([]core.AcmeURL{core.AcmeURL(*validEmail)})
+	test.AssertNotError(t, err, "Valid Email")
+
+	err = validateContacts([]core.AcmeURL{core.AcmeURL(*invalidEmail)})
+	test.AssertError(t, err, "Invalid Email")
+
+	err = validateContacts([]core.AcmeURL{core.AcmeURL(*malformedEmail)})
+	test.AssertError(t, err, "Malformed Email")
+
+	err = validateContacts([]core.AcmeURL{core.AcmeURL(*ansible)})
+	test.AssertError(t, err, "Unknown scehme")
+}
+
+func TestValidateEmail(t *testing.T) {
+	err := validateEmail("an email`")
+	test.AssertError(t, err, "Malformed")
+
+	err = validateEmail("a@not.a.domain")
+	test.AssertError(t, err, "Cannot resolve")
+	t.Logf("No Resolve: %s", err)
+
+	err = validateEmail("a@example.com")
+	test.AssertError(t, err, "No MX Record")
+	t.Logf("No MX: %s", err)
+
+	err = validateEmail("a@email.com")
+	test.AssertNotError(t, err, "Valid")
+}
+
 func TestNewRegistration(t *testing.T) {
 	_, _, sa, ra := initAuthorities(t)
 	mailto, _ := url.Parse("mailto:foo@letsencrypt.org")

wfe/web-front-end.go

@@ -71,8 +71,18 @@ func statusCodeFromError(err interface{}) int {
 	switch err.(type) {
 	case core.MalformedRequestError:
 		return http.StatusBadRequest
+	case core.NotSupportedError:
+		return http.StatusNotImplemented
+	case core.SyntaxError:
+		return http.StatusBadRequest
 	case core.UnauthorizedError:
 		return http.StatusForbidden
+	case core.NotFoundError:
+		return http.StatusNotFound
+	case core.SignatureValidationError:
+		return http.StatusPreconditionFailed
+	case core.InternalServerError:
+		return http.StatusInternalServerError
 	default:
 		return http.StatusInternalServerError
 	}
@@ -327,6 +337,7 @@ func (wfe *WebFrontEndImpl) NewRegistration(response http.ResponseWriter, reques
 	regURL := fmt.Sprintf("%s%d", wfe.RegBase, id)
 	responseBody, err := json.Marshal(reg)
 	if err != nil {
+		// StatusInternalServerError because we just created this registration, it should be OK.
 		wfe.sendError(response, "Error marshaling registration", err, http.StatusInternalServerError)
 		return
 	}
@@ -390,6 +401,7 @@ func (wfe *WebFrontEndImpl) NewAuthorization(response http.ResponseWriter, reque
 	authz.RegistrationID = 0
 	responseBody, err := json.Marshal(authz)
 	if err != nil {
+		// StatusInternalServerError because we generated the authz, it should be OK
 		wfe.sendError(response, "Error marshaling authz", err, http.StatusInternalServerError)
 		return
 	}
@@ -446,6 +458,7 @@ func (wfe *WebFrontEndImpl) RevokeCertificate(response http.ResponseWriter, requ
 	}
 	parsedCertificate, err := x509.ParseCertificate(cert.DER)
 	if err != nil {
+		// InternalServerError because this is a failure to decode from our DB.
 		wfe.sendError(response, "Invalid certificate", err, http.StatusInternalServerError)
 		return
 	}
@@ -557,7 +570,7 @@ func (wfe *WebFrontEndImpl) NewCertificate(response http.ResponseWriter, request
 	wfe.Stats.Inc("Certificates", 1, 1.0)
 }
 
-func (wfe *WebFrontEndImpl) Challenge(authz core.Authorization, response http.ResponseWriter, request *http.Request) {
+func (wfe *WebFrontEndImpl) challenge(authz core.Authorization, response http.ResponseWriter, request *http.Request) {
 	wfe.sendStandardHeaders(response)
 
 	if request.Method != "GET" && request.Method != "POST" {
@@ -593,6 +606,8 @@ func (wfe *WebFrontEndImpl) Challenge(authz core.Authorization, response http.Re
 		challenge := authz.Challenges[challengeIndex]
 		jsonReply, err := json.Marshal(challenge)
 		if err != nil {
+			// InternalServerError because this is a failure to decode data passed in
+			// by the caller, which got it from the DB.
 			wfe.sendError(response, "Failed to marshal challenge", err, http.StatusInternalServerError)
 			return
 		}
@@ -651,6 +666,7 @@ func (wfe *WebFrontEndImpl) Challenge(authz core.Authorization, response http.Re
 		// assumption: UpdateAuthorization does not modify order of challenges
 		jsonReply, err := json.Marshal(challenge)
 		if err != nil {
+			// StatusInternalServerError because we made the challenges, they should be OK
 			wfe.sendError(response, "Failed to marshal challenge", err, http.StatusInternalServerError)
 			return
 		}
@@ -734,6 +750,7 @@ func (wfe *WebFrontEndImpl) Registration(response http.ResponseWriter, request *
 
 	jsonReply, err := json.Marshal(updatedReg)
 	if err != nil {
+		// StatusInternalServerError because we just generated the reg, it should be OK
 		wfe.sendError(response, "Failed to marshal registration", err, http.StatusInternalServerError)
 		return
 	}
@@ -763,7 +780,7 @@ func (wfe *WebFrontEndImpl) Authorization(response http.ResponseWriter, request
 
 	// If there is a fragment, then this is actually a request to a challenge URI
 	if len(request.URL.RawQuery) != 0 {
-		wfe.Challenge(authz, response, request)
+		wfe.challenge(authz, response, request)
 		return
 	}
 
@@ -780,6 +797,7 @@ func (wfe *WebFrontEndImpl) Authorization(response http.ResponseWriter, request
 
 		jsonReply, err := json.Marshal(authz)
 		if err != nil {
+			// InternalServerError because this is a failure to decode from our DB.
 			wfe.sendError(response, "Failed to marshal authz", err, http.StatusInternalServerError)
 			return
 		}
@@ -827,7 +845,7 @@ func (wfe *WebFrontEndImpl) Certificate(response http.ResponseWriter, request *h
 		cert, err := wfe.SA.GetCertificateByShortSerial(serial)
 		if err != nil {
 			if strings.HasPrefix(err.Error(), "gorp: multiple rows returned") {
-				wfe.sendError(response, "Multiple certificates with same short serial", err, http.StatusInternalServerError)
+				wfe.sendError(response, "Multiple certificates with same short serial", err, http.StatusConflict)
 			} else {
 				wfe.sendError(response, "Not found", err, http.StatusNotFound)
 			}

wfe/web-front-end_test.go

@@ -550,7 +550,7 @@ func TestChallenge(t *testing.T) {
 		RegistrationID: 1,
 	}
 
-	wfe.Challenge(authz, responseWriter, &http.Request{
+	wfe.challenge(authz, responseWriter, &http.Request{
 		Method: "POST",
 		URL:    challengeURL,
 		Body:   makeBody(signRequest(t, "{}", &wfe.nonceService)),