Improve renewal rate limiting

sa/sa.go

@@ -434,19 +434,18 @@ func (ssa *SQLStorageAuthority) countCertificatesByExactName(domain string, earl
 }
 
 // countCertificates returns, for a single domain, the count of
-// certificates issued in the given time range for that domain using the
+// non-renewal certificates issued in the given time range for that domain using the
 // provided query, assumed to be either `countCertificatesExactSelect` or
-// `countCertificatesSelect`.
+// `countCertificatesSelect`. Renewals of certificates issued within the same
+// window are considered "free" and not returned as part of this count.
 //
 // The highest count this function can return is 10,000. If there are more
 // certificates than that matching one of the provided domain names, it will return
 // TooManyCertificatesError.
 func (ssa *SQLStorageAuthority) countCertificates(domain string, earliest, latest time.Time, query string) (int, error) {
 	var count int64
 	const max = 10000
-	var serials []struct {
-		Serial string
-	}
+	var serials []string
 	_, err := ssa.dbMap.Select(
 		&serials,
 		query,
@@ -463,12 +462,28 @@ func (ssa *SQLStorageAuthority) countCertificates(domain string, earliest, lates
 	} else if count > max {
 		return max, TooManyCertificatesError(fmt.Sprintf("More than %d issuedName entries for %s.", max, domain))
 	}
-	serialMap := make(map[string]struct{}, len(serials))
-	for _, s := range serials {
-		serialMap[s.Serial] = struct{}{}
+
+	// If there are no serials found, short circuit since there isn't subsequent
+	// work to do
+	if len(serials) == 0 {
+		return 0, nil
+	}
+
+	// Find all FQDN Set Hashes with the serials from the issuedNames table that
+	// were visible within our search window
+	fqdnSets, err := ssa.getFQDNSetsBySerials(serials)
+	if err != nil {
+		return -1, err
 	}
 
-	return len(serialMap), nil
+	// Using those FQDN Set Hashes, we can then find all of the non-renewal
+	// issuances with a second query against the fqdnSets table using the set
+	// hashes we know about
+	nonRenewalIssuances, err := ssa.getNewIssuancesByFQDNSet(fqdnSets, earliest)
+	if err != nil {
+		return -1, err
+	}
+	return nonRenewalIssuances, nil
 }
 
 // GetCertificate takes a serial number and returns the corresponding
@@ -1031,6 +1046,101 @@ func (ssa *SQLStorageAuthority) CountFQDNSets(ctx context.Context, window time.D
 	return count, err
 }
 
+// getFQDNSetsBySerials returns a slice of []byte `setHash` entries from the
+// fqdnSets table for the certificate serials provided.
+func (ssa *SQLStorageAuthority) getFQDNSetsBySerials(serials []string) ([][]byte, error) {
+	var fqdnSets [][]byte
+
+	// It is unexpected that this function would be called with no serials
+	if len(serials) == 0 {
+		err := fmt.Errorf("getFQDNSetsBySerials called with no serials")
+		ssa.log.AuditErr(err.Error())
+		return nil, err
+	}
+
+	qmarks := make([]string, len(serials))
+	params := make([]interface{}, len(serials))
+	for i, serial := range serials {
+		params[i] = serial
+		qmarks[i] = "?"
+	}
+	query := "SELECT setHash FROM fqdnSets " +
+		"WHERE serial IN (" + strings.Join(qmarks, ",") + ") "
+	_, err := ssa.dbMap.Select(
+		&fqdnSets,
+		query,
+		params...)
+
+	if err != nil && err != sql.ErrNoRows {
+		return nil, err
+	}
+	return fqdnSets, nil
+}
+
+// getNewIssuancesByFQDNSet returns a count of new issuances (renewals are not
+// included) for a given slice of fqdnSets that were issued after the earliest
+// parameter.
+func (ssa *SQLStorageAuthority) getNewIssuancesByFQDNSet(fqdnSets [][]byte, earliest time.Time) (int, error) {
+	var results []struct {
+		Serial  string
+		SetHash []byte
+		Issued  time.Time
+	}
+
+	qmarks := make([]string, len(fqdnSets))
+	params := make([]interface{}, len(fqdnSets))
+	for i, setHash := range fqdnSets {
+		params[i] = setHash
+		qmarks[i] = "?"
+	}
+
+	query := "SELECT serial, setHash, issued FROM fqdnSets " +
+		"WHERE setHash IN (" + strings.Join(qmarks, ",") + ") " +
+		"ORDER BY setHash, issued"
+
+	// First, find the serial, sethash and issued date from the fqdnSets table for
+	// the given fqdn set hashes
+	_, err := ssa.dbMap.Select(
+		&results,
+		query,
+		params...)
+	if err != nil && err != sql.ErrNoRows {
+		return -1, err
+	}
+
+	// If there are no results we have encountered a major error and
+	// should loudly complain
+	if err == sql.ErrNoRows || len(results) == 0 {
+		ssa.log.AuditErr(fmt.Sprintf("Found no results from fqdnSets for setHashes known to exist: %#v", fqdnSets))
+		return 0, err
+	}
+
+	processedSetHashes := make(map[string]struct{})
+	issuanceCount := 0
+	// Loop through each set hash result, counting issuance per unique set hash
+	// that are within the window specified by the earliest parameter
+	for _, result := range results {
+		key := string(result.SetHash)
+		// Skip set hashes that we have already processed - we only care about the
+		// first issuance
+		if _, exists := processedSetHashes[key]; exists {
+			continue
+		}
+
+		// If the issued date is before our earliest cutoff then skip it
+		if result.Issued.Before(earliest) {
+			continue
+		}
+
+		// Otherwise note the issuance and mark the set hash as processed
+		issuanceCount++
+		processedSetHashes[key] = struct{}{}
+	}
+
+	// Return the count of how many non-renewal issuances there were
+	return issuanceCount, nil
+}
+
 // FQDNSetExists returns a bool indicating if one or more FQDN sets |names|
 // exists in the database
 func (ssa *SQLStorageAuthority) FQDNSetExists(ctx context.Context, names []string) (bool, error) {

sa/sa_test.go

@@ -18,6 +18,7 @@ import (
 	"golang.org/x/net/context"
 
 	"github.com/jmhodges/clock"
+	gorp "gopkg.in/go-gorp/gorp.v2"
 	jose "gopkg.in/square/go-jose.v1"
 
 	"github.com/letsencrypt/boulder/core"
@@ -1036,3 +1037,174 @@ func TestReverseName(t *testing.T) {
 		test.AssertEquals(t, output, tc.inputReversed)
 	}
 }
+
+type fqdnTestcase struct {
+	Serial       string
+	Names        []string
+	ExpectedHash []byte
+	Issued       time.Time
+	Expires      time.Time
+}
+
+func setupFQDNSets(t *testing.T, db *gorp.DbMap, fc clock.FakeClock) map[string]fqdnTestcase {
+	namesA := []string{"a.example.com", "B.example.com"}
+	namesB := []string{"example.org"}
+	namesC := []string{"letsencrypt.org"}
+	expectedHashA := []byte{0x92, 0xc7, 0xf2, 0x47, 0xbd, 0x1e, 0xea, 0x8d, 0x52, 0x7f, 0xb0, 0x59, 0x19, 0xe9, 0xbe, 0x81, 0x78, 0x88, 0xe6, 0xf7, 0x55, 0xf0, 0x1c, 0xc9, 0x63, 0x15, 0x5f, 0x8e, 0x52, 0xae, 0x95, 0xc1}
+	expectedHashB := []byte{0xbf, 0xab, 0xc3, 0x74, 0x32, 0x95, 0x8b, 0x6, 0x33, 0x60, 0xd3, 0xad, 0x64, 0x61, 0xc9, 0xc4, 0x73, 0x5a, 0xe7, 0xf8, 0xed, 0xd4, 0x65, 0x92, 0xa5, 0xe0, 0xf0, 0x14, 0x52, 0xb2, 0xe4, 0xb5}
+	expectedHashC := []byte{0xf2, 0xbb, 0x7b, 0xab, 0x8, 0x2c, 0x18, 0xee, 0x8, 0x97, 0x17, 0xbe, 0x67, 0xd7, 0x12, 0x14, 0xaa, 0x4, 0xac, 0xe2, 0x29, 0x2a, 0x67, 0x2c, 0x37, 0x2c, 0xf3, 0x33, 0xe1, 0xb0, 0xd8, 0xe7}
+
+	now := fc.Now()
+
+	testcases := map[string]fqdnTestcase{
+		// One test case with serial "a" issued now and expiring in two hours for
+		// namesA
+		"a": fqdnTestcase{
+			Serial:       "a",
+			Names:        namesA,
+			ExpectedHash: expectedHashA,
+			Issued:       now,
+			Expires:      now.Add(time.Hour * 2).UTC(),
+		},
+		// One test case with serial "b", issued one hour from now and expiring in
+		// two hours, also for namesA
+		"b": fqdnTestcase{
+			Serial:       "b",
+			Names:        namesA,
+			ExpectedHash: expectedHashA,
+			Issued:       now.Add(time.Hour),
+			Expires:      now.Add(time.Hour * 2).UTC(),
+		},
+		// One test case with serial "c", issued one hour from now and expiring in
+		// two hours, for namesB
+		"c": fqdnTestcase{
+			Serial:       "c",
+			Names:        namesB,
+			ExpectedHash: expectedHashB,
+			Issued:       now.Add(time.Hour),
+			Expires:      now.Add(time.Hour * 2).UTC(),
+		},
+		// One test case with serial "d", issued five hours in the past and expiring
+		// in two hours from now, with namesC
+		"d": fqdnTestcase{
+			Serial:       "d",
+			Names:        namesC,
+			ExpectedHash: expectedHashC,
+			Issued:       now.Add(-5 * time.Hour),
+			Expires:      now.Add(time.Hour * 2).UTC(),
+		},
+	}
+
+	for _, tc := range testcases {
+		tx, err := db.Begin()
+		test.AssertNotError(t, err, "Failed to open transaction")
+		err = addFQDNSet(tx, tc.Names, tc.Serial, tc.Issued, tc.Expires)
+		test.AssertNotError(t, err, fmt.Sprintf("Failed to add fqdnSet for %#v", tc))
+		test.AssertNotError(t, tx.Commit(), "Failed to commit transaction")
+	}
+
+	return testcases
+}
+
+func TestGetFQDNSetsBySerials(t *testing.T) {
+	sa, fc, cleanUp := initSA(t)
+	defer cleanUp()
+
+	// Add the test fqdn sets
+	testcases := setupFQDNSets(t, sa.dbMap, fc)
+
+	// Asking for the fqdnSets for no serials should produce an error since this
+	// is not expected in normal conditions
+	fqdnSets, err := sa.getFQDNSetsBySerials([]string{})
+	test.AssertError(t, err, "No error calling getFQDNSetsBySerials for empty serials")
+	test.AssertEquals(t, len(fqdnSets), 0)
+
+	// Asking for the fqdnSets for serials that don't exist should return nothing
+	fqdnSets, err = sa.getFQDNSetsBySerials([]string{"this", "doesn't", "exist"})
+	test.AssertNotError(t, err, "Error calling getFQDNSetsBySerials for non-existent serials")
+	test.AssertEquals(t, len(fqdnSets), 0)
+
+	// Asking for the fqdnSets for serial "a" should return the expectedHashA hash
+	fqdnSets, err = sa.getFQDNSetsBySerials([]string{"a"})
+	test.AssertNotError(t, err, "Error calling getFQDNSetsBySerials for serial \"a\"")
+	test.AssertEquals(t, len(fqdnSets), 1)
+	test.AssertEquals(t, string(fqdnSets[0]), string(testcases["a"].ExpectedHash))
+
+	// Asking for the fqdnSets for serial "b" should return the expectedHashA hash
+	// because cert "b" has namesA subjects
+	fqdnSets, err = sa.getFQDNSetsBySerials([]string{"b"})
+	test.AssertNotError(t, err, "Error calling getFQDNSetsBySerials for serial \"b\"")
+	test.AssertEquals(t, len(fqdnSets), 1)
+	test.AssertEquals(t, string(fqdnSets[0]), string(testcases["b"].ExpectedHash))
+
+	// Asking for the fqdnSets for serial "d" should return the expectedHashC hash
+	// because cert "d" has namesC subjects
+	fqdnSets, err = sa.getFQDNSetsBySerials([]string{"d"})
+	test.AssertNotError(t, err, "Error calling getFQDNSetsBySerials for serial \"d\"")
+	test.AssertEquals(t, len(fqdnSets), 1)
+	test.AssertEquals(t, string(fqdnSets[0]), string(testcases["d"].ExpectedHash))
+
+	// Asking for the fqdnSets for serial "c" should return the expectedHashB hash
+	// because cert "c" has namesB subjects
+	fqdnSets, err = sa.getFQDNSetsBySerials([]string{"c"})
+	test.AssertNotError(t, err, "Error calling getFQDNSetsBySerials for serial \"c\"")
+	test.AssertEquals(t, len(fqdnSets), 1)
+	test.AssertEquals(t, string(fqdnSets[0]), string(testcases["c"].ExpectedHash))
+
+	// Asking for the fqdnSets for serial "a", "b", "c" and "made up" should return
+	// the three expected hashes - two expectedHashA (for "a" and "b"), one
+	// expectedHashB (for "c")
+	fqdnSets, err = sa.getFQDNSetsBySerials([]string{"a", "b", "c", "made up"})
+	test.AssertNotError(t, err, "Error calling getFQDNSetsBySerials for serial \"a\", \"b\", \"c\", \"made up\"")
+	test.AssertEquals(t, len(fqdnSets), 3)
+	test.AssertEquals(t, string(fqdnSets[0]), string(testcases["a"].ExpectedHash))
+	test.AssertEquals(t, string(fqdnSets[1]), string(testcases["b"].ExpectedHash))
+	test.AssertEquals(t, string(fqdnSets[2]), string(testcases["c"].ExpectedHash))
+}
+
+func TestGetNewIssuancesByFQDNSet(t *testing.T) {
+	sa, fc, cleanUp := initSA(t)
+	defer cleanUp()
+
+	// Add the test fqdn sets
+	testcases := setupFQDNSets(t, sa.dbMap, fc)
+
+	// Use one hour ago as the earliest cut off
+	earliest := fc.Now().Add(-time.Hour)
+
+	// Calling getNewIssuancesByFQDNSet with an empty FQDNSet should error
+	count, err := sa.getNewIssuancesByFQDNSet(nil, earliest)
+	test.AssertError(t, err, "No error calling getNewIssuancesByFQDNSet for empty fqdn set")
+	test.AssertEquals(t, count, -1)
+
+	// Calling getNewIssuancesByFQDNSet with FQDNSet hashes that don't exist
+	// should return 0
+	count, err = sa.getNewIssuancesByFQDNSet([][]byte{[]byte{0xC0, 0xFF, 0xEE}, []byte{0x13, 0x37}}, earliest)
+	test.AssertNotError(t, err, "Error calling getNewIssuancesByFQDNSet for non-existent set hashes")
+	test.AssertEquals(t, count, 0)
+
+	// Calling getNewIssuancesByFQDNSet with the "a" expected hash should return
+	// 1, since both testcase "b" was a renewal of testcase "a"
+	count, err = sa.getNewIssuancesByFQDNSet([][]byte{testcases["a"].ExpectedHash}, earliest)
+	test.AssertNotError(t, err, "Error calling getNewIssuancesByFQDNSet for testcase a")
+	test.AssertEquals(t, count, 1)
+
+	// Calling getNewIssuancesByFQDNSet with the "c" expected hash should return
+	// 1, since there is only one issuance for this sethash
+	count, err = sa.getNewIssuancesByFQDNSet([][]byte{testcases["c"].ExpectedHash}, earliest)
+	test.AssertNotError(t, err, "Error calling getNewIssuancesByFQDNSet for testcase c")
+	test.AssertEquals(t, count, 1)
+
+	// Calling getNewIssuancesByFQDNSet with the "c" and "d" expected hashes should return
+	// only 1, since there is only one issuance for the provided set hashes that
+	// is within the earliest window. The issuance for "d" was too far in the past
+	// to be counted
+	count, err = sa.getNewIssuancesByFQDNSet([][]byte{testcases["c"].ExpectedHash, testcases["d"].ExpectedHash}, earliest)
+	test.AssertNotError(t, err, "Error calling getNewIssuancesByFQDNSet for testcase c and d")
+	test.AssertEquals(t, count, 1)
+
+	// But by moving the earliest point behind the "d" issuance, we should now get a count of 2
+	count, err = sa.getNewIssuancesByFQDNSet([][]byte{testcases["c"].ExpectedHash, testcases["d"].ExpectedHash}, earliest.Add(-6*time.Hour))
+	test.AssertNotError(t, err, "Error calling getNewIssuancesByFQDNSet for testcase c and d with adjusted earliest")
+	test.AssertEquals(t, count, 2)
+}

test/integration-test.py

@@ -311,6 +311,30 @@ def expect(target_time, num, table):
     expect(now, 0, "authz")
     expect(after_grace_period, 1, "authz")
 
+def test_renewal_exemption():
+    """
+    Under a single domain, issue one certificate, then two renewals of that
+    certificate, then one more different certificate (with a different
+    subdomain). Since the certificatesPerName rate limit in testing is 2 per 90
+    days, and the renewals should be discounted under the renewal exemption,
+    each of these issuances should succeed. Then do one last issuance that we
+    expect to be rate limited, just to check that the rate limit is actually 2,
+    and we are testing what we think we are testing. See
+    https://letsencrypt.org/docs/rate-limits/ for more details.
+    """
+    base_domain = random_domain()
+    # First issuance
+    auth_and_issue(["www." + base_domain])
+    # First Renewal
+    auth_and_issue(["www." + base_domain])
+    # Second Renewal
+    auth_and_issue(["www." + base_domain])
+    # Issuance of a different cert
+    auth_and_issue(["blog." + base_domain])
+    # Final, failed issuance, for another different cert
+    chisel.expect_problem("urn:acme:error:rateLimited",
+        lambda: auth_and_issue(["mail." + base_domain]))
+
 def test_certificates_per_name():
     chisel.expect_problem("urn:acme:error:rateLimited",
         lambda: auth_and_issue(["lim.it"]))
@@ -410,6 +434,7 @@ def run_chisel():
     test_ocsp()
     test_single_ocsp()
     test_dns_challenge()
+    test_renewal_exemption()
 
 if __name__ == "__main__":
     try: