Fix FQDNSet rate limit exemption. (#1935)

As reported in #1925 the Certificates per Domain rate limit was being
incorrectly enforced on certificate renewals for FQDN sets that have
been previously issued. This is counter to the described rate
limit policies[0] that detail a separate rate limit for certificates
issued for the "exact same set of Fully Qualified Domain Names".

The bug was caused by the result of `domainsForRateLimiting` overwriting
the original `names []string` provided to the RA's
`checkCertificatesPerNameLimit` function. This meant instead of looking
for an existing FQDN set for the full set of domain names being
requested we checked for an FQDN set for just the eTLD+1's of the
domains. (e.g "www.example.com, foo.example.com, bar.example.com" vs
"example.com").

This commit preserves the original `names` values for doing an FQDN set
lookup and uses the `tldNames` from `domainsForRateLimiting` elsewhere.
This fixes #1925.

A test is added to ensure that `checkCertificatesPerNameLimit` does the
correct thing both with and without an existing FQDN set.

[0] https://community.letsencrypt.org/t/rate-limits-for-lets-encrypt/6769

ra/ra.go

@@ -677,18 +677,18 @@ func domainsForRateLimiting(names []string) ([]string, error) {
 }
 
 func (ra *RegistrationAuthorityImpl) checkCertificatesPerNameLimit(ctx context.Context, names []string, limit ratelimit.RateLimitPolicy, regID int64) error {
-	names, err := domainsForRateLimiting(names)
+	tldNames, err := domainsForRateLimiting(names)
 	if err != nil {
 		return err
 	}
 	now := ra.clk.Now()
 	windowBegin := limit.WindowBegin(now)
-	counts, err := ra.SA.CountCertificatesByNames(ctx, names, windowBegin, now)
+	counts, err := ra.SA.CountCertificatesByNames(ctx, tldNames, windowBegin, now)
 	if err != nil {
 		return err
 	}
 	var badNames []string
-	for _, name := range names {
+	for _, name := range tldNames {
 		count, ok := counts[name]
 		if !ok {
 			// Shouldn't happen, but let's be careful anyhow.

ra/ra_test.go

@@ -2,6 +2,7 @@ package ra
 
 import (
 	"crypto/rand"
+	"crypto/sha256"
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
@@ -10,6 +11,7 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -1033,6 +1035,90 @@ func TestCheckCertificatesPerNameLimit(t *testing.T) {
 	}
 }
 
+// A mockSAWithFQDNSet is a mock StorageAuthority that supports
+// CountCertificatesByName as well as FQDNSetExists. This allows testing
+// checkCertificatesPerNameRateLimit's FQDN exemption logic.
+type mockSAWithFQDNSet struct {
+	mocks.StorageAuthority
+	fqdnSet    map[string]bool
+	nameCounts map[string]int
+	t          *testing.T
+}
+
+// Construct the FQDN Set key the same way as the SA - by using
+// `core.UniqueLowerNames`, joining the names with a `,` and hashing them.
+func (m mockSAWithFQDNSet) hashNames(names []string) string {
+	names = core.UniqueLowerNames(names)
+	hash := sha256.Sum256([]byte(strings.Join(names, ",")))
+	return string(hash[:])
+}
+
+// Add a set of domain names to the FQDN set
+func (m mockSAWithFQDNSet) addFQDNSet(names []string) {
+	hash := m.hashNames(names)
+	m.fqdnSet[hash] = true
+}
+
+// Search for a set of domain names in the FQDN set map
+func (m mockSAWithFQDNSet) FQDNSetExists(_ context.Context, names []string) (bool, error) {
+	hash := m.hashNames(names)
+	if _, exists := m.fqdnSet[hash]; exists {
+		return true, nil
+	}
+	return false, nil
+}
+
+// Return a map of domain -> certificate count. Note: This naive implementation
+// ignores names, earliest and latest paremeters and always returns the same
+// nameCount map.
+func (m mockSAWithFQDNSet) CountCertificatesByNames(ctx context.Context, names []string, earliest, latest time.Time) (ret map[string]int, err error) {
+	return m.nameCounts, nil
+}
+
+// Tests for boulder issue 1925[0] - that the `checkCertificatesPerNameLimit`
+// properly honours the FQDNSet exemption. E.g. that if a set of domains has
+// reached the certificates per name rate limit policy threshold but the exact
+// same set of FQDN's was previously issued, then it should not be considered
+// over the certificates per name limit.
+//
+// [0] https://github.com/letsencrypt/boulder/issues/1925
+func TestCheckFQDNSetRateLimitOverride(t *testing.T) {
+	_, _, ra, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+
+	// Simple policy that only allows 1 certificate per name.
+	certsPerNamePolicy := ratelimit.RateLimitPolicy{
+		Threshold: 1,
+		Window:    cmd.ConfigDuration{Duration: 24 * time.Hour},
+	}
+
+	// Create a mock SA that has both name counts and an FQDN set
+	mockSA := &mockSAWithFQDNSet{
+		nameCounts: map[string]int{
+			"example.com": 100,
+			"zombo.com":   100,
+		},
+		fqdnSet: map[string]bool{},
+		t:       t,
+	}
+	ra.SA = mockSA
+
+	// First check that without a pre-existing FQDN set that the provided set of
+	// names is rate limited due to being over the certificates per name limit for
+	// "example.com" and "zombo.com"
+	err := ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com", "www.zombo.com"}, certsPerNamePolicy, 99)
+	test.AssertError(t, err, "certificate per name rate limit not applied correctly")
+
+	// Now add a FQDN set entry for these domains
+	mockSA.addFQDNSet([]string{"www.example.com", "example.com", "www.zombo.com"})
+
+	// A subsequent check against the certificates per name limit should now be OK
+	// - there exists a FQDN set and so the exemption to this particular limit
+	// comes into effect.
+	err = ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com", "www.zombo.com"}, certsPerNamePolicy, 99)
+	test.AssertNotError(t, err, "FQDN set certificate per name exemption not applied correctly")
+}
+
 var CAkeyPEM = `
 -----BEGIN RSA PRIVATE KEY-----
 MIIJKQIBAAKCAgEAqmM0dEf/J9MCk2ItzevL0dKJ84lVUtf/vQ7AXFi492vFXc3b