diff --git a/errors/errors.go b/errors/errors.go index 280ae39eb..50be1087a 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -12,6 +12,7 @@ package errors import ( "fmt" + "time" "github.com/letsencrypt/boulder/identifier" ) @@ -56,6 +57,10 @@ type BoulderError struct { Type ErrorType Detail string SubErrors []SubBoulderError + + // RetryAfter the duration a client should wait before retrying the request + // which resulted in this error. + RetryAfter time.Duration } // SubBoulderError represents sub-errors specific to an identifier that are @@ -77,9 +82,10 @@ func (be *BoulderError) Unwrap() error { // provided subErrs to the existing BoulderError. func (be *BoulderError) WithSubErrors(subErrs []SubBoulderError) *BoulderError { return &BoulderError{ - Type: be.Type, - Detail: be.Detail, - SubErrors: append(be.SubErrors, subErrs...), + Type: be.Type, + Detail: be.Detail, + SubErrors: append(be.SubErrors, subErrs...), + RetryAfter: be.RetryAfter, } } @@ -107,31 +113,35 @@ func NotFoundError(msg string, args ...interface{}) error { return New(NotFound, msg, args...) } -func RateLimitError(msg string, args ...interface{}) error { +func RateLimitError(retryAfter time.Duration, msg string, args ...interface{}) error { return &BoulderError{ - Type: RateLimit, - Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/", args...), + Type: RateLimit, + Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/", args...), + RetryAfter: retryAfter, } } -func DuplicateCertificateError(msg string, args ...interface{}) error { +func DuplicateCertificateError(retryAfter time.Duration, msg string, args ...interface{}) error { return &BoulderError{ - Type: RateLimit, - Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/duplicate-certificate-limit/", args...), + Type: RateLimit, + Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/duplicate-certificate-limit/", args...), + RetryAfter: retryAfter, } } -func FailedValidationError(msg string, args ...interface{}) error { +func FailedValidationError(retryAfter time.Duration, msg string, args ...interface{}) error { return &BoulderError{ - Type: RateLimit, - Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/failed-validation-limit/", args...), + Type: RateLimit, + Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/failed-validation-limit/", args...), + RetryAfter: retryAfter, } } -func RegistrationsPerIPError(msg string, args ...interface{}) error { +func RegistrationsPerIPError(retryAfter time.Duration, msg string, args ...interface{}) error { return &BoulderError{ - Type: RateLimit, - Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/too-many-registrations-for-this-ip/", args...), + Type: RateLimit, + Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/too-many-registrations-for-this-ip/", args...), + RetryAfter: retryAfter, } } diff --git a/grpc/errors.go b/grpc/errors.go index 3d3e83836..9515d58f3 100644 --- a/grpc/errors.go +++ b/grpc/errors.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "strconv" + "time" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -41,8 +42,13 @@ func wrapError(ctx context.Context, err error) error { "error marshaling json SubErrors, orig error %q", err) } - pairs = append(pairs, "suberrors") - pairs = append(pairs, string(jsonSubErrs)) + pairs = append(pairs, "suberrors", string(jsonSubErrs)) + } + + // If there is a RetryAfter value then extend the metadata pairs to + // include the value. + if berr.RetryAfter != 0 { + pairs = append(pairs, "retryafter", berr.RetryAfter.String()) } // Ignoring the error return here is safe because if setting the metadata @@ -63,59 +69,75 @@ func unwrapError(err error, md metadata.MD) error { return nil } - unwrappedErr := status.Convert(err).Message() - errTypeStrs, ok := md["errortype"] if !ok { return err } + + inErrMsg := status.Convert(err).Message() if len(errTypeStrs) != 1 { return berrors.InternalServerError( - "multiple errorType metadata, wrapped error %q", - unwrappedErr, + "multiple 'errortype' metadata, wrapped error %q", + inErrMsg, ) } - errType, decErr := strconv.Atoi(errTypeStrs[0]) + inErrType, decErr := strconv.Atoi(errTypeStrs[0]) if decErr != nil { return berrors.InternalServerError( "failed to decode error type, decoding error %q, wrapped error %q", decErr, - unwrappedErr, + inErrMsg, ) } - outErr := berrors.New(berrors.ErrorType(errType), unwrappedErr) - - subErrsJSON, ok := md["suberrors"] - if !ok { - return outErr - } - if len(subErrsJSON) != 1 { - return berrors.InternalServerError( - "multiple suberrors metadata, wrapped error %q", - unwrappedErr, - ) - } - - var suberrs []berrors.SubBoulderError - err2 := json.Unmarshal([]byte(subErrsJSON[0]), &suberrs) - if err2 != nil { - return berrors.InternalServerError( - "error unmarshaling suberrs JSON %q, wrapped error %q", - subErrsJSON[0], - unwrappedErr, - ) - } - - var berr *berrors.BoulderError - if errors.As(outErr, &berr) { - outErr = berr.WithSubErrors(suberrs) - } else { + inErr := berrors.New(berrors.ErrorType(inErrType), inErrMsg) + var outErr *berrors.BoulderError + if !errors.As(inErr, &outErr) { return fmt.Errorf( - "expected type of outErr to be %T got %T: %q", - berr, outErr, - outErr.Error(), + "expected type of inErr to be %T got %T: %q", + outErr, + inErr, + inErr.Error(), ) } + + subErrorsVal, ok := md["suberrors"] + if ok { + if len(subErrorsVal) != 1 { + return berrors.InternalServerError( + "multiple 'suberrors' in metadata, wrapped error %q", + inErrMsg, + ) + } + + unmarshalErr := json.Unmarshal([]byte(subErrorsVal[0]), &outErr.SubErrors) + if unmarshalErr != nil { + return berrors.InternalServerError( + "JSON unmarshaling 'suberrors' %q, wrapped error %q: %s", + subErrorsVal[0], + inErrMsg, + unmarshalErr, + ) + } + } + + retryAfterVal, ok := md["retryafter"] + if ok { + if len(retryAfterVal) != 1 { + return berrors.InternalServerError( + "multiple 'retryafter' in metadata, wrapped error %q", + inErrMsg, + ) + } + var parseErr error + outErr.RetryAfter, parseErr = time.ParseDuration(retryAfterVal[0]) + if parseErr != nil { + return berrors.InternalServerError( + "parsing 'retryafter' as int64, wrapped error %q, parsing error: %s", + inErrMsg, + parseErr, + ) + } + } return outErr } diff --git a/grpc/errors_test.go b/grpc/errors_test.go index 935c07aba..a98ce87ce 100644 --- a/grpc/errors_test.go +++ b/grpc/errors_test.go @@ -2,6 +2,7 @@ package grpc import ( "context" + "errors" "fmt" "net" "testing" @@ -47,10 +48,19 @@ func TestErrorWrapping(t *testing.T) { test.AssertNotError(t, err, "Failed to dial grpc test server") client := test_proto.NewChillerClient(conn) - es.err = berrors.MalformedError("yup") + // RateLimitError with a RetryAfter of 500ms. + expectRetryAfter := time.Millisecond * 500 + es.err = berrors.RateLimitError(expectRetryAfter, "yup") _, err = client.Chill(context.Background(), &test_proto.Time{}) test.Assert(t, err != nil, fmt.Sprintf("nil error returned, expected: %s", err)) test.AssertDeepEquals(t, err, es.err) + var bErr *berrors.BoulderError + ok := errors.As(err, &bErr) + test.Assert(t, ok, "asserting error as boulder error") + // Ensure we got a RateLimitError + test.AssertErrorIs(t, bErr, berrors.RateLimit) + // Ensure our RetryAfter is still 500ms. + test.AssertEquals(t, bErr.RetryAfter, expectRetryAfter) test.AssertNil(t, wrapError(context.Background(), nil), "Wrapping nil should still be nil") test.AssertNil(t, unwrapError(nil, nil), "Unwrapping nil should still be nil") diff --git a/ra/ra.go b/ra/ra.go index 88703406c..5edbd8d5b 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -328,7 +328,7 @@ func (ra *RegistrationAuthorityImpl) checkRegistrationIPLimit(ctx context.Contex } if count.Count >= limit.GetThreshold(ip.String(), noRegistrationID) { - return berrors.RegistrationsPerIPError("too many registrations for this IP") + return berrors.RegistrationsPerIPError(0, "too many registrations for this IP") } return nil @@ -365,7 +365,7 @@ func (ra *RegistrationAuthorityImpl) checkRegistrationLimits(ctx context.Context ra.log.Infof("Rate limit exceeded, RegistrationsByIPRange, IP: %s", ip) // For the fuzzyRegLimit we use a new error message that specifically // mentions that the limit being exceeded is applied to a *range* of IPs - return berrors.RateLimitError("too many registrations for this IP range") + return berrors.RateLimitError(0, "too many registrations for this IP range") } ra.rateLimitCounter.WithLabelValues("registrations_by_ip_range", "pass").Inc() @@ -515,7 +515,7 @@ func (ra *RegistrationAuthorityImpl) checkPendingAuthorizationLimit(ctx context. if countPB.Count >= limit.GetThreshold(noKey, regID) { ra.rateLimitCounter.WithLabelValues("pending_authorizations_by_registration_id", "exceeded").Inc() ra.log.Infof("Rate limit exceeded, PendingAuthorizationsByRegID, regID: %d", regID) - return berrors.RateLimitError(fmt.Sprintf("too many currently pending authorizations: %d", countPB.Count)) + return berrors.RateLimitError(0, "too many currently pending authorizations: %d", countPB.Count) } ra.rateLimitCounter.WithLabelValues("pending_authorizations_by_registration_id", "pass").Inc() } @@ -567,7 +567,7 @@ func (ra *RegistrationAuthorityImpl) checkInvalidAuthorizationLimit(ctx context. noKey := "" if count.Count >= limit.GetThreshold(noKey, regID) { ra.log.Infof("Rate limit exceeded, InvalidAuthorizationsByRegID, regID: %d", regID) - return berrors.FailedValidationError("too many failed authorizations recently") + return berrors.FailedValidationError(0, "too many failed authorizations recently") } return nil } @@ -595,7 +595,7 @@ func (ra *RegistrationAuthorityImpl) checkNewOrdersPerAccountLimit(ctx context.C noKey := "" if count.Count >= limit.GetThreshold(noKey, acctID) { ra.rateLimitCounter.WithLabelValues("new_order_by_registration_id", "exceeded").Inc() - return berrors.RateLimitError("too many new orders recently") + return berrors.RateLimitError(0, "too many new orders recently") } ra.rateLimitCounter.WithLabelValues("new_order_by_registration_id", "pass").Inc() return nil @@ -1347,12 +1347,12 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerNameLimit(ctx context.C for _, name := range namesOutOfLimit { subErrors = append(subErrors, berrors.SubBoulderError{ Identifier: identifier.DNSIdentifier(name), - BoulderError: berrors.RateLimitError("too many certificates already issued").(*berrors.BoulderError), + BoulderError: berrors.RateLimitError(0, "too many certificates already issued").(*berrors.BoulderError), }) } - return berrors.RateLimitError("too many certificates already issued for multiple names (%s and %d others)", namesOutOfLimit[0], len(namesOutOfLimit)).(*berrors.BoulderError).WithSubErrors(subErrors) + return berrors.RateLimitError(0, "too many certificates already issued for multiple names (%s and %d others)", namesOutOfLimit[0], len(namesOutOfLimit)).(*berrors.BoulderError).WithSubErrors(subErrors) } - return berrors.RateLimitError("too many certificates already issued for: %s", namesOutOfLimit[0]) + return berrors.RateLimitError(0, "too many certificates already issued for: %s", namesOutOfLimit[0]) } ra.rateLimitCounter.WithLabelValues("certificates_for_domain", "pass").Inc() @@ -1394,7 +1394,9 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerFQDNSetLimit(ctx contex } } retryTime := time.Unix(0, prevIssuances.Timestamps[0]).Add(time.Duration(nsPerToken)) + retryAfter := retryTime.Sub(now) return berrors.DuplicateCertificateError( + retryAfter, "too many certificates (%d) already issued for this exact set of domains in the last %.0f hours: %s, retry after %s", threshold, limit.Window.Duration.Hours(), strings.Join(names, ","), retryTime.Format(time.RFC3339), ) diff --git a/va/http.go b/va/http.go index 269c3a49a..3f50d9569 100644 --- a/va/http.go +++ b/va/http.go @@ -310,8 +310,7 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (stri // Check that the request host isn't a bare IP address. We only follow // redirects to hostnames. if net.ParseIP(reqHost) != nil { - return "", 0, berrors.ConnectionFailureError( - "Invalid host in redirect target %q. Only domain names are supported, not IP addresses", reqHost) + return "", 0, berrors.ConnectionFailureError("Invalid host in redirect target %q. Only domain names are supported, not IP addresses", reqHost) } // Often folks will misconfigure their webserver to send an HTTP redirect @@ -331,8 +330,7 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (stri } if _, err := iana.ExtractSuffix(reqHost); err != nil { - return "", 0, berrors.ConnectionFailureError( - "Invalid hostname in redirect target, must end in IANA registered TLD") + return "", 0, berrors.ConnectionFailureError("Invalid hostname in redirect target, must end in IANA registered TLD") } return reqHost, reqPort, nil diff --git a/web/probs_test.go b/web/probs_test.go index b839ef4b8..dc56aa1e2 100644 --- a/web/probs_test.go +++ b/web/probs_test.go @@ -34,7 +34,7 @@ func TestProblemDetailsFromError(t *testing.T) { {berrors.MalformedError(detailMsg), 400, probs.MalformedProblem, fullDetail}, {berrors.UnauthorizedError(detailMsg), 403, probs.UnauthorizedProblem, fullDetail}, {berrors.NotFoundError(detailMsg), 404, probs.MalformedProblem, fullDetail}, - {berrors.RateLimitError(detailMsg), 429, probs.RateLimitedProblem, fullDetail + ": see https://letsencrypt.org/docs/rate-limits/"}, + {berrors.RateLimitError(0, detailMsg), 429, probs.RateLimitedProblem, fullDetail + ": see https://letsencrypt.org/docs/rate-limits/"}, {berrors.InvalidEmailError(detailMsg), 400, probs.InvalidEmailProblem, fullDetail}, {berrors.RejectedIdentifierError(detailMsg), 400, probs.RejectedIdentifierProblem, fullDetail}, } diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 317365bfb..af29d9af8 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -74,6 +74,8 @@ const ( // Non-ACME paths aiaIssuerPath = "/aia/issuer/" + + headerRetryAfter = "Retry-After" ) var errIncompleteGRPCResponse = errors.New("incomplete gRPC response message") @@ -584,6 +586,16 @@ func (wfe *WebFrontEndImpl) Nonce( // sendError wraps web.SendError func (wfe *WebFrontEndImpl) sendError(response http.ResponseWriter, logEvent *web.RequestEvent, prob *probs.ProblemDetails, ierr error) { + var bErr *berrors.BoulderError + if errors.As(ierr, &bErr) { + retryAfterSeconds := int(bErr.RetryAfter.Round(time.Second).Seconds()) + if retryAfterSeconds > 0 { + response.Header().Add(headerRetryAfter, strconv.Itoa(retryAfterSeconds)) + if bErr.Type == berrors.RateLimit { + response.Header().Add("Link", link("https://letsencrypt.org/docs/rate-limits", "help")) + } + } + } wfe.stats.httpErrorCount.With(prometheus.Labels{"type": string(prob.Type)}).Inc() web.SendError(wfe.log, probs.V2ErrorNS, response, logEvent, prob, ierr) } @@ -2304,7 +2316,7 @@ func (wfe *WebFrontEndImpl) RenewalInfo(ctx context.Context, logEvent *web.Reque beeline.AddFieldToTrace(ctx, "request.serial", serial) setDefaultRetryAfterHeader := func(response http.ResponseWriter) { - response.Header().Set("Retry-After", fmt.Sprintf("%d", int(6*time.Hour/time.Second))) + response.Header().Set(headerRetryAfter, fmt.Sprintf("%d", int(6*time.Hour/time.Second))) } // Check if the serial is part of an ongoing incident. diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 9e31decf3..f5c5eb8a6 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -3701,3 +3701,34 @@ func TestOldTLSInbound(t *testing.T) { wfe.Handler(metrics.NoopRegisterer).ServeHTTP(responseWriter, req) test.AssertEquals(t, responseWriter.Code, http.StatusBadRequest) } + +func Test_sendError(t *testing.T) { + features.Reset() + wfe, _ := setupWFE(t) + testResponse := httptest.NewRecorder() + + testErr := berrors.RateLimitError(0, "test") + wfe.sendError(testResponse, &web.RequestEvent{Endpoint: "test"}, probs.RateLimited("test"), testErr) + // Ensure a 0 value RetryAfter results in no Retry-After header. + test.AssertEquals(t, testResponse.Header().Get("Retry-After"), "") + // Ensure the Link header isn't populatsed. + test.AssertEquals(t, testResponse.Header().Get("Link"), "") + + testErr = berrors.RateLimitError(time.Millisecond*500, "test") + wfe.sendError(testResponse, &web.RequestEvent{Endpoint: "test"}, probs.RateLimited("test"), testErr) + // Ensure a 500ms RetryAfter is rounded up to a 1s Retry-After header. + test.AssertEquals(t, testResponse.Header().Get("Retry-After"), "1") + // Ensure the Link header is populated. + test.AssertEquals(t, testResponse.Header().Get("Link"), ";rel=\"help\"") + + // Clear headers for the next test. + testResponse.Header().Del("Retry-After") + testResponse.Header().Del("Link") + + testErr = berrors.RateLimitError(time.Millisecond*499, "test") + wfe.sendError(testResponse, &web.RequestEvent{Endpoint: "test"}, probs.RateLimited("test"), testErr) + // Ensure a 499ms RetryAfter results in no Retry-After header. + test.AssertEquals(t, testResponse.Header().Get("Retry-After"), "") + // Ensure the Link header isn't populatsed. + test.AssertEquals(t, testResponse.Header().Get("Link"), "") +}