From 9df9c21ddc50151d5cb0273cd1521c019217dbf9 Mon Sep 17 00:00:00 2001 From: Roland Bracewell Shoemaker Date: Mon, 9 Sep 2019 09:20:05 -0700 Subject: [PATCH] Use sub-problems for the certificates per name rate limit (#4416) Fixes #4360. --- ra/ra.go | 19 +++++++++++++------ ra/ra_test.go | 21 +++++++++++++++++++-- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 1f4609615..bf950b0ed 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1427,13 +1427,20 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerNameLimit(ctx context.C return nil } } - domains := strings.Join(namesOutOfLimit, ", ") + + ra.log.Infof("Rate limit exceeded, CertificatesForDomain, regID: %d, domains: %s", regID, strings.Join(namesOutOfLimit, ", ")) ra.certsForDomainStats.Inc("Exceeded", 1) - ra.log.Infof("Rate limit exceeded, CertificatesForDomain, regID: %d, domains: %s", regID, domains) - return berrors.RateLimitError( - "too many certificates already issued for: %s", - domains, - ) + if len(namesOutOfLimit) > 1 { + var subErrors []berrors.SubBoulderError + for _, name := range namesOutOfLimit { + subErrors = append(subErrors, berrors.SubBoulderError{ + Identifier: identifier.DNSIdentifier(name), + BoulderError: berrors.RateLimitError("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("too many certificates already issued for: %s", namesOutOfLimit[0]) } ra.certsForDomainStats.Inc("Pass", 1) diff --git a/ra/ra_test.go b/ra/ra_test.go index e3e127d89..fad58f5bd 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -1400,13 +1400,30 @@ func TestCheckCertificatesPerNameLimit(t *testing.T) { err := ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com"}, rlp, 99) test.AssertNotError(t, err, "rate limited example.com incorrectly") - // One base domain, above threshold + // Two base domains, one above threshold, one below mockSA.nameCounts["example.com"] = nameCount("example.com", 10) - err = ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com"}, rlp, 99) + mockSA.nameCounts["good-example.com"] = nameCount("good-example.com", 1) + err = ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com", "good-example.com"}, rlp, 99) test.AssertError(t, err, "incorrectly failed to rate limit example.com") if !berrors.Is(err, berrors.RateLimit) { t.Errorf("Incorrect error type %#v", err) } + // Verify it has no sub errors as there is only one bad name + test.AssertEquals(t, err.Error(), "too many certificates already issued for: example.com: see https://letsencrypt.org/docs/rate-limits/") + test.AssertEquals(t, len(err.(*berrors.BoulderError).SubErrors), 0) + + // Three base domains, two above threshold, one below + mockSA.nameCounts["example.com"] = nameCount("example.com", 10) + mockSA.nameCounts["other-example.com"] = nameCount("other-example.com", 10) + mockSA.nameCounts["good-example.com"] = nameCount("good-example.com", 1) + err = ra.checkCertificatesPerNameLimit(ctx, []string{"example.com", "other-example.com", "good-example.com"}, rlp, 99) + test.AssertError(t, err, "incorrectly failed to rate limit example.com, other-example.com") + if !berrors.Is(err, berrors.RateLimit) { + t.Errorf("Incorrect error type %#v", err) + } + // Verify it has two sub errors as there are two bad names + test.AssertEquals(t, err.Error(), "too many certificates already issued for multiple names (example.com and 2 others): see https://letsencrypt.org/docs/rate-limits/") + test.AssertEquals(t, len(err.(*berrors.BoulderError).SubErrors), 2) // SA misbehaved and didn't send back a count for every input name err = ra.checkCertificatesPerNameLimit(ctx, []string{"zombo.com", "www.example.com", "example.com"}, rlp, 99)