From 1474b7f21f7a45a10ad3b4656299de0b1a8803bc Mon Sep 17 00:00:00 2001 From: "J.C. Jones" Date: Thu, 11 Jun 2015 23:23:07 -0500 Subject: [PATCH 1/3] Resolves Issue #344: Only send InternalServerError when needed Basically, just send InternalServerError when it indicates an internal state was broken. --- core/util.go | 6 ++++++ ra/registration-authority.go | 29 ++++++++++++++++++++--------- wfe/web-front-end.go | 24 +++++++++++++++++++++--- wfe/web-front-end_test.go | 2 +- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/core/util.go b/core/util.go index cae357663..98929a163 100644 --- a/core/util.go +++ b/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 diff --git a/ra/registration-authority.go b/ra/registration-authority.go index 62b39703a..e2f0d8800 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -62,7 +62,7 @@ func validateEmail(address string) (err error) { var mx []*net.MX mx, err = net.LookupMX(domain) if err != nil { - err = core.InternalServerError(err.Error()) + err = core.MalformedRequestError(fmt.Sprintf("Supplied email domain %s could not be resolved.", domain)) return } if len(mx) == 0 { @@ -116,7 +116,7 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration) (re // Store the authorization object, then return it reg, err = ra.SA.NewRegistration(reg) if err != nil { - err = core.InternalServerError(err.Error()) + err = core.MalformedRequestError(err.Error()) } return @@ -124,7 +124,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 +162,7 @@ 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()) + err = core.MalformedRequestError(fmt.Sprintf("Invalid authorization request: %s", err)) return authz, err } @@ -173,6 +173,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 +186,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 +215,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 +316,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 +332,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 @@ -345,7 +354,7 @@ func (ra *RegistrationAuthorityImpl) UpdateRegistration(base core.Registration, reg = base err = ra.SA.UpdateRegistration(base) if err != nil { - err = core.InternalServerError(err.Error()) + err = core.MalformedRequestError(fmt.Sprintf("Could not update registration: %s", err)) } return } @@ -354,14 +363,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 } diff --git a/wfe/web-front-end.go b/wfe/web-front-end.go index cbdef9223..57afe5682 100644 --- a/wfe/web-front-end.go +++ b/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 } @@ -322,6 +332,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 } @@ -385,6 +396,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 } @@ -441,6 +453,7 @@ func (wfe *WebFrontEndImpl) RevokeCertificate(response http.ResponseWriter, requ } parsedCertificate, err := x509.ParseCertificate(certDER) if err != nil { + // InternalServerError because this is a failure to decode from our DB. wfe.sendError(response, "Invalid certificate", err, http.StatusInternalServerError) return } @@ -552,7 +565,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" { @@ -588,6 +601,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 } @@ -646,6 +661,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 } @@ -729,6 +745,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 } @@ -758,7 +775,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 } @@ -775,6 +792,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 } @@ -822,7 +840,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) } diff --git a/wfe/web-front-end_test.go b/wfe/web-front-end_test.go index 63e8afa9c..fcbf9d247 100644 --- a/wfe/web-front-end_test.go +++ b/wfe/web-front-end_test.go @@ -521,7 +521,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)), From 2d79af0c695c6c2b59e058989da6f6f7a4be9786 Mon Sep 17 00:00:00 2001 From: "J.C. Jones" Date: Sat, 13 Jun 2015 00:19:56 -0500 Subject: [PATCH 2/3] Issue 344 Rework: Revert Internal Errors in RA, add validation to UpdateReg --- ra/registration-authority.go | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/ra/registration-authority.go b/ra/registration-authority.go index e2f0d8800..dfdace860 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -116,7 +116,9 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration) (re // Store the authorization object, then return it reg, err = ra.SA.NewRegistration(reg) if err != nil { - err = core.MalformedRequestError(err.Error()) + // InternalServerError since the user-data was validated before being + // passed to the SA. + err = core.InternalServerError(err.Error()) } return @@ -162,7 +164,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.MalformedRequestError(fmt.Sprintf("Invalid authorization request: %s", err)) + // 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 } @@ -351,10 +355,28 @@ 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) + + for _, contact := range base.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 + } + } + reg = base err = ra.SA.UpdateRegistration(base) if err != nil { - err = core.MalformedRequestError(fmt.Sprintf("Could not update registration: %s", err)) + // 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 } From dbe88d5d8062678da5d274fb6c61c1b1a460cf72 Mon Sep 17 00:00:00 2001 From: "J.C. Jones" Date: Mon, 15 Jun 2015 16:42:37 -0500 Subject: [PATCH 3/3] Issue #344: RA: Pull out a `validateContacts` method - Created a `validateContacts` method to avoid duplicated code - Added tests for `validateContacts` and `validateEmail` - Fix error formatting in `validateEmail`: Discovered while testing `validateEmail` that, if no MX records are found, `err` is returned, not an empty array. As such, the error message was misleading, so I consoldated the conditions into one. --- ra/registration-authority.go | 57 ++++++++++++++----------------- ra/registration-authority_test.go | 42 +++++++++++++++++++++++ 2 files changed, 68 insertions(+), 31 deletions(-) diff --git a/ra/registration-authority.go b/ra/registration-authority.go index dfdace860..9122a2f8c 100644 --- a/ra/registration-authority.go +++ b/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 { - err = core.MalformedRequestError(fmt.Sprintf("Supplied email domain %s could not be resolved.", domain)) - return - } - if len(mx) == 0 { + if err != nil || 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,19 +113,9 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration) (re } reg.MergeUpdate(init) - for _, contact := range 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 - } + err = validateContacts(reg.Contact) + if err != nil { + return } // Store the authorization object, then return it @@ -356,19 +361,9 @@ 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) - for _, contact := range base.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 - } + err = validateContacts(base.Contact) + if err != nil { + return } reg = base diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index 44c82101f..02204259e 100644 --- a/ra/registration-authority_test.go +++ b/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")