From 778c9bba862f1c15b176a7d5177544200e14b4e1 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 16 Jun 2016 16:50:39 -0400 Subject: [PATCH] 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 | 6 ++-- ra/ra_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index a3178ede1..b8d6de3bc 100644 --- a/ra/ra.go +++ b/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. diff --git a/ra/ra_test.go b/ra/ra_test.go index 8be376830..718eefa8d 100644 --- a/ra/ra_test.go +++ b/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