From 1464c349387d087053caa2b90855c269e5a48fc0 Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 29 Jul 2022 17:39:31 -0700 Subject: [PATCH] RA: Implement leaky bucket for duplicate certificate limit (#6262) - Modify `ra.checkCertificatesPerFQDNSetLimit()` to use a leaky bucket algorithm - Return issuance timestamps from `sa.FQDNSetTimestampsForWindow()` in descending order Resolves #6154 --- ra/ra.go | 28 +++++++++++++++++++++------- ra/ra_test.go | 34 +++++++++++++++++----------------- sa/sa.go | 6 +++--- sa/sa_test.go | 36 +++++++++++++++--------------------- test/rate-limit-policies.yml | 2 +- 5 files changed, 57 insertions(+), 49 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 90b38b185..3fe519dc0 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1376,7 +1376,7 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerFQDNSetLimit(ctx contex return nil } - issuanceTimestampsInWindow, err := ra.SA.FQDNSetTimestampsForWindow(ctx, &sapb.CountFQDNSetsRequest{ + prevIssuances, err := ra.SA.FQDNSetTimestampsForWindow(ctx, &sapb.CountFQDNSetsRequest{ Domains: names, Window: limit.Window.Duration.Nanoseconds(), }) @@ -1384,16 +1384,30 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerFQDNSetLimit(ctx contex return fmt.Errorf("checking duplicate certificate limit for %q: %s", names, err) } - if int64(len(issuanceTimestampsInWindow.Timestamps)) >= threshold { - // FQDNSetTimestampsForWindow returns the issuance timestamps in - // ascending order so we can assume that the first one is the oldest. - retryAfter := time.Unix(0, issuanceTimestampsInWindow.Timestamps[0]).Add(limit.Window.Duration) + if int64(len(prevIssuances.Timestamps)) < threshold { + // Issuance in window is below the threshold, no need to limit. + return nil + } else { + // Evaluate the rate limit using a leaky bucket algorithm. The bucket + // has a capacity of threshold and is refilled at a rate of 1 token per + // limit.Window/threshold from the time of each issuance timestamp. + now := ra.clk.Now() + nsPerToken := limit.Window.Nanoseconds() / threshold + for i, timestamp := range prevIssuances.Timestamps { + tokensGeneratedSince := now.Add(-time.Duration(int64(i+1) * nsPerToken)) + if time.Unix(0, timestamp).Before(tokensGeneratedSince) { + // We know `i+1` tokens were generated since `tokenGeneratedSince`, + // and only `i` certificates were issued, so there's room to allow + // for an additional issuance. + return nil + } + } + retryTime := time.Unix(0, prevIssuances.Timestamps[0]).Add(time.Duration(nsPerToken)) return berrors.DuplicateCertificateError( "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, ","), retryAfter.Format(time.RFC3339), + threshold, limit.Window.Duration.Hours(), strings.Join(names, ","), retryTime.Format(time.RFC3339), ) } - return nil } func (ra *RegistrationAuthorityImpl) checkLimits(ctx context.Context, names []string, regID int64) error { diff --git a/ra/ra_test.go b/ra/ra_test.go index dc2e5130a..1a21a9791 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -1223,25 +1223,28 @@ func TestCheckExactCertificateLimit(t *testing.T) { const dupeCertLimit = 3 rlp := ratelimit.RateLimitPolicy{ Threshold: dupeCertLimit, - Window: cmd.ConfigDuration{Duration: 23 * time.Hour}, + Window: cmd.ConfigDuration{Duration: 24 * time.Hour}, } // Create a mock SA that has a count of already issued certificates for some // test names - firstIssuanceTimestamp := ra.clk.Now().Add(-23 * time.Hour) + firstIssuanceTimestamp := ra.clk.Now().Add(-rlp.Window.Duration) issuanceTimestamps := []int64{ + firstIssuanceTimestamp.Add(time.Hour * 23).UnixNano(), + firstIssuanceTimestamp.Add(time.Hour * 16).UnixNano(), + firstIssuanceTimestamp.Add(time.Hour * 8).UnixNano(), firstIssuanceTimestamp.UnixNano(), - firstIssuanceTimestamp.Add(time.Hour).UnixNano(), - firstIssuanceTimestamp.Add(time.Hour * 2).UnixNano(), - firstIssuanceTimestamp.Add(time.Hour * 3).UnixNano(), } - expectRetryAfter := time.Unix(0, issuanceTimestamps[0]).Add(23 * time.Hour).Format(time.RFC3339) + // Our window is 24 hours and our threshold is 3 issuance. If our most + // recent issuance was 1 hour ago, we expect the next token to be available + // 8 hours from issuance time or 7 hours from now. + expectRetryAfter := time.Unix(0, issuanceTimestamps[0]).Add(time.Hour * 8).Format(time.RFC3339) ra.SA = &mockSAWithFQDNSet{ issuanceTimestamps: map[string]*sapb.Timestamps{ - "none.example.com": {Timestamps: []int64{}}, - "under.example.com": {Timestamps: issuanceTimestamps[0 : dupeCertLimit-1]}, - "equal.example.com": {Timestamps: issuanceTimestamps[0:dupeCertLimit]}, - "over.example.com": {Timestamps: issuanceTimestamps[0 : dupeCertLimit+1]}, + "none.example.com": {Timestamps: []int64{}}, + "under.example.com": {Timestamps: issuanceTimestamps[3:3]}, + "equalbutvalid.example.com": {Timestamps: issuanceTimestamps[1:3]}, + "over.example.com": {Timestamps: issuanceTimestamps[0:3]}, }, t: t, } @@ -1262,18 +1265,15 @@ func TestCheckExactCertificateLimit(t *testing.T) { ExpectedErr: nil, }, { - Name: "FQDN set issuances equal to limit", - Domain: "equal.example.com", - ExpectedErr: fmt.Errorf( - "too many certificates (3) already issued for this exact set of domains in the last 23 hours: equal.example.com, retry after %s: see https://letsencrypt.org/docs/duplicate-certificate-limit/", - expectRetryAfter, - ), + Name: "FQDN set issuances equal to limit", + Domain: "equalbutvalid.example.com", + ExpectedErr: nil, }, { Name: "FQDN set issuances above limit", Domain: "over.example.com", ExpectedErr: fmt.Errorf( - "too many certificates (3) already issued for this exact set of domains in the last 23 hours: over.example.com, retry after %s: see https://letsencrypt.org/docs/duplicate-certificate-limit/", + "too many certificates (3) already issued for this exact set of domains in the last 24 hours: over.example.com, retry after %s: see https://letsencrypt.org/docs/duplicate-certificate-limit/", expectRetryAfter, ), }, diff --git a/sa/sa.go b/sa/sa.go index aec628bf5..a84f97daf 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -697,8 +697,8 @@ func (ssa *SQLStorageAuthority) CountFQDNSets(ctx context.Context, req *sapb.Cou } // FQDNSetTimestampsForWindow returns the issuance timestamps for each -// certificate, issued for a set of domains, during a given window of time, in -// ascending order. +// certificate, issued for a set of domains, during a given window of time, +// starting from the most recent issuance. func (ssa *SQLStorageAuthority) FQDNSetTimestampsForWindow(ctx context.Context, req *sapb.CountFQDNSetsRequest) (*sapb.Timestamps, error) { if req.Window == 0 || len(req.Domains) == 0 { return nil, errIncompleteRequest @@ -712,7 +712,7 @@ func (ssa *SQLStorageAuthority) FQDNSetTimestampsForWindow(ctx context.Context, `SELECT issued FROM fqdnSets WHERE setHash = ? AND issued > ? - ORDER BY issued ASC`, + ORDER BY issued DESC`, HashNames(req.Domains), ssa.clk.Now().Add(-time.Duration(req.Window)), ) diff --git a/sa/sa_test.go b/sa/sa_test.go index 75bf8f3fe..d64264335 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -637,69 +637,63 @@ func TestFQDNSetTimestampsForWindow(t *testing.T) { test.AssertNotError(t, err, "Failed to open transaction") names := []string{"a.example.com", "B.example.com"} - threeHours := time.Hour * 3 + window := time.Hour * 3 req := &sapb.CountFQDNSetsRequest{ Domains: names, - Window: threeHours.Nanoseconds(), + Window: window.Nanoseconds(), } - // zero issuance timestamps for names + // Ensure zero issuance has occurred for names. resp, err := sa.FQDNSetTimestampsForWindow(ctx, req) test.AssertNotError(t, err, "Failed to count name sets") test.AssertEquals(t, len(resp.Timestamps), 0) - // add a valid set inside the window + // Add an issuance for names inside the window. expires := fc.Now().Add(time.Hour * 2).UTC() firstIssued := fc.Now() err = addFQDNSet(tx, names, "serial", firstIssued, expires) test.AssertNotError(t, err, "Failed to add name set") test.AssertNotError(t, tx.Commit(), "Failed to commit transaction") - // only one valid + // Ensure there's 1 issuance timestamp for names inside the window. resp, err = sa.FQDNSetTimestampsForWindow(ctx, req) test.AssertNotError(t, err, "Failed to count name sets") test.AssertEquals(t, len(resp.Timestamps), 1) - test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[0]).UTC()) + test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[len(resp.Timestamps)-1]).UTC()) - // check hash isn't affected by changing name order/casing + // Ensure that the hash isn't affected by changing name order/casing. req.Domains = []string{"b.example.com", "A.example.COM"} resp, err = sa.FQDNSetTimestampsForWindow(ctx, req) test.AssertNotError(t, err, "Failed to count name sets") test.AssertEquals(t, len(resp.Timestamps), 1) - test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[0]).UTC()) + test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[len(resp.Timestamps)-1]).UTC()) - // add another valid set + // Add another issuance for names inside the window. tx, err = sa.dbMap.Begin() test.AssertNotError(t, err, "Failed to open transaction") err = addFQDNSet(tx, names, "anotherSerial", firstIssued, expires) test.AssertNotError(t, err, "Failed to add name set") test.AssertNotError(t, tx.Commit(), "Failed to commit transaction") - // only two valid + // Ensure there are two issuance timestamps for names inside the window. req.Domains = names resp, err = sa.FQDNSetTimestampsForWindow(ctx, req) test.AssertNotError(t, err, "Failed to count name sets") test.AssertEquals(t, len(resp.Timestamps), 2) - test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[0]).UTC()) + test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[len(resp.Timestamps)-1]).UTC()) - // add an expired set + // Add another issuance for names but just outside the window. tx, err = sa.dbMap.Begin() test.AssertNotError(t, err, "Failed to open transaction") - err = addFQDNSet( - tx, - names, - "yetAnotherSerial", - firstIssued.Add(-threeHours), - expires.Add(-threeHours), - ) + err = addFQDNSet(tx, names, "yetAnotherSerial", firstIssued.Add(-window), expires) test.AssertNotError(t, err, "Failed to add name set") test.AssertNotError(t, tx.Commit(), "Failed to commit transaction") - // only two valid + // Ensure there are still only two issuance timestamps in the window. resp, err = sa.FQDNSetTimestampsForWindow(ctx, req) test.AssertNotError(t, err, "Failed to count name sets") test.AssertEquals(t, len(resp.Timestamps), 2) - test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[0]).UTC()) + test.AssertEquals(t, firstIssued, time.Unix(0, resp.Timestamps[len(resp.Timestamps)-1]).UTC()) } func TestFQDNSetsExists(t *testing.T) { diff --git a/test/rate-limit-policies.yml b/test/rate-limit-policies.yml index 7a45a1f1d..fc63b5657 100644 --- a/test/rate-limit-policies.yml +++ b/test/rate-limit-policies.yml @@ -50,7 +50,7 @@ certificatesPerFQDNSet: ecdsa.le.wtf: 10000 must-staple.le.wtf: 10000 certificatesPerFQDNSetFast: - window: 2h + window: 3h threshold: 2 overrides: le.wtf: 100