From e49ffaf94c87c0bbac0357d2d5ebab1729a1da91 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 26 Apr 2019 10:53:47 -0700 Subject: [PATCH] sa: use a faster query for certificates per name rate limit (#4179) Right now we run a `SELECT COUNT` query on issuedNames to calculate this rate limit. Unfortunately, counting large numbers of rows is very slow. This change introduces a dedicated table for keeping track of that rate limit. Instead of being keyed by FQDN, this new `certificatesPerName` table is keyed by the same field as rate limits are calculated on: the base domain. Each row has a base domain, a time (chunked by hour), and a count. This means calculating the rate limit status for each domain reads at most 7 * 24 rows. This should particularly speed up cases when a single domain has lots of subdomains. Fixes #4152 --- features/featureflag_string.go | 7 +- features/features.go | 4 + .../20190416000000_AddRateLimitTable.sql | 15 +++ sa/rate_limits.go | 102 ++++++++++++++++++ sa/rate_limits_test.go | 94 ++++++++++++++++ sa/sa.go | 23 +++- test/config-next/sa.json | 1 + test/sa_db_users.sql | 1 + 8 files changed, 241 insertions(+), 6 deletions(-) create mode 100644 sa/_db-next/migrations/20190416000000_AddRateLimitTable.sql create mode 100644 sa/rate_limits.go create mode 100644 sa/rate_limits_test.go diff --git a/features/featureflag_string.go b/features/featureflag_string.go index 2013ace56..ff4059dd1 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -25,12 +25,13 @@ func _() { _ = x[EnforceMultiVA-14] _ = x[MultiVAFullResults-15] _ = x[RemoveWFE2AccountID-16] - _ = x[CheckRenewalFirst-17] + _ = x[FasterRateLimit-17] + _ = x[CheckRenewalFirst-18] } -const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitCAAValidationMethodsCAAAccountURIProbeCTLogsHeadNonceStatusOKNewAuthorizationSchemaRevokeAtRAEarlyOrderRateLimitEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirst" +const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitCAAValidationMethodsCAAAccountURIProbeCTLogsHeadNonceStatusOKNewAuthorizationSchemaRevokeAtRAEarlyOrderRateLimitEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDFasterRateLimitCheckRenewalFirst" -var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 140, 153, 164, 181, 203, 213, 232, 246, 264, 283, 300} +var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 140, 153, 164, 181, 203, 213, 232, 246, 264, 283, 298, 315} func (i FeatureFlag) String() string { if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) { diff --git a/features/features.go b/features/features.go index 97ad6263b..d6cc7407d 100644 --- a/features/features.go +++ b/features/features.go @@ -45,6 +45,9 @@ const ( // RemoveWFE2AccountID will remove the account ID from account objects returned // from the new-account endpoint if enabled. RemoveWFE2AccountID + // FasterRateLimit enables use of a separate table for counting the + // Certificates Per Name rate limit. + FasterRateLimit // CheckRenewalFirst will check whether an issuance is a renewal before // checking the "certificates per name" rate limit. CheckRenewalFirst @@ -69,6 +72,7 @@ var features = map[FeatureFlag]bool{ EnforceMultiVA: false, MultiVAFullResults: false, RemoveWFE2AccountID: false, + FasterRateLimit: false, CheckRenewalFirst: false, } diff --git a/sa/_db-next/migrations/20190416000000_AddRateLimitTable.sql b/sa/_db-next/migrations/20190416000000_AddRateLimitTable.sql new file mode 100644 index 000000000..78dc64bcb --- /dev/null +++ b/sa/_db-next/migrations/20190416000000_AddRateLimitTable.sql @@ -0,0 +1,15 @@ +-- +goose Up +-- SQL in section 'Up' is executed when this migration is applied + +CREATE TABLE `certificatesPerName` ( + `id` BIGINT(20) PRIMARY KEY AUTO_INCREMENT, + `eTLDPlusOne` VARCHAR(255) NOT NULL, + `time` DATETIME NOT NULL, + `count` INTEGER NOT NULL, + UNIQUE KEY `eTLDPlusOne_time_idx` (`eTLDPlusOne`, `time`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +-- +goose Down +-- SQL section 'Down' is executed when this migration is rolled back + +DROP TABLE `certificatesPerName`; diff --git a/sa/rate_limits.go b/sa/rate_limits.go new file mode 100644 index 000000000..b5c1a8ccc --- /dev/null +++ b/sa/rate_limits.go @@ -0,0 +1,102 @@ +package sa + +import ( + "database/sql" + "strings" + "time" + + "github.com/letsencrypt/boulder/features" + "github.com/weppos/publicsuffix-go/publicsuffix" + "golang.org/x/net/context" +) + +// baseDomain returns the eTLD+1 of a domain name for the purpose of rate +// limiting. For a domain name that is itself an eTLD, it returns its input. +func baseDomain(name string) string { + eTLDPlusOne, err := publicsuffix.Domain(name) + if err != nil { + // publicsuffix.Domain will return an error if the input name is itself a + // public suffix. In that case we use the input name as the key for rate + // limiting. Since all of its subdomains will have separate keys for rate + // limiting (e.g. "foo.bar.publicsuffix.com" will have + // "bar.publicsuffix.com", this means that domains exactly equal to a + // public suffix get their own rate limit bucket. This is important + // because otherwise they might be perpetually unable to issue, assuming + // the rate of issuance from their subdomains was high enough. + return name + } + return eTLDPlusOne +} + +// addCertificatesPerName adds 1 to the rate limit count for the provided domains, +// in a specific time bucket. It must be executed in a transaction, and the +// input timeToTheHour must be a time rounded to an hour. +func (ssa *SQLStorageAuthority) addCertificatesPerName( + ctx context.Context, + db dbSelectExecer, + names []string, + timeToTheHour time.Time, +) error { + if !features.Enabled(features.FasterRateLimit) { + return nil + } + // De-duplicate the base domains. + baseDomainsMap := make(map[string]bool) + var qmarks []string + var values []interface{} + for _, name := range names { + base := baseDomain(name) + if !baseDomainsMap[base] { + baseDomainsMap[base] = true + values = append(values, base, timeToTheHour, 1) + qmarks = append(qmarks, "(?, ?, ?)") + } + } + + _, err := db.Exec(`INSERT INTO certificatesPerName (eTLDPlusOne, time, count) VALUES `+ + strings.Join(qmarks, ", ")+` ON DUPLICATE KEY UPDATE count=count+1;`, + values...) + if err != nil { + return err + } + + return nil +} + +// countCertificatesFaster returns, for a single domain, the count of +// certificates issued in the given time range for that domain's eTLD+1 (aka +// base domain). It uses the certificatesPerName table to make this lookup fast. +// This function can replace both countCertificatesByName and +// countCertificatesByExactName because domains that are exactly equal to an +// public suffix have their issuances counted under a separate bucket from their +// subdomains. +func (ssa *SQLStorageAuthority) countCertificatesFaster( + db dbSelector, + domain string, + earliest, + latest time.Time, +) (int, error) { + base := baseDomain(domain) + var counts []int + _, err := db.Select( + &counts, + `SELECT count FROM certificatesPerName + WHERE eTLDPlusOne = :baseDomain AND + time > :earliest AND + time <= :latest`, + map[string]interface{}{ + "baseDomain": base, + "earliest": earliest, + "latest": latest, + }) + if err == sql.ErrNoRows { + return 0, nil + } else if err != nil { + return 0, err + } + var total int + for _, count := range counts { + total += count + } + return total, nil +} diff --git a/sa/rate_limits_test.go b/sa/rate_limits_test.go new file mode 100644 index 000000000..12d1ddb71 --- /dev/null +++ b/sa/rate_limits_test.go @@ -0,0 +1,94 @@ +package sa + +import ( + "fmt" + "os" + "strings" + "testing" + "time" + + "github.com/letsencrypt/boulder/features" + + "golang.org/x/net/context" +) + +func TestFasterRateLimit(t *testing.T) { + sa, _, cleanUp := initSA(t) + defer cleanUp() + + if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") { + return + } + features.Set(map[string]bool{"FasterRateLimit": true}) + + aprilFirst, err := time.Parse(time.RFC3339, "2019-04-01T00:00:00Z") + if err != nil { + t.Fatal(err) + } + + type inputCase struct { + time time.Time + names []string + } + inputs := []inputCase{ + {aprilFirst, []string{"example.com"}}, + {aprilFirst, []string{"example.com", "www.example.com"}}, + {aprilFirst, []string{"example.com", "other.example.com"}}, + {aprilFirst, []string{"dyndns.org"}}, + {aprilFirst, []string{"mydomain.dyndns.org"}}, + {aprilFirst, []string{"mydomain.dyndns.org"}}, + {aprilFirst, []string{"otherdomain.dyndns.org"}}, + } + + // For each hour in a week, add an enry for a certificate that has + // progressively more names. + var manyNames []string + for i := 0; i < 7*24; i++ { + manyNames = append(manyNames, fmt.Sprintf("%d.manynames.example.net", i)) + inputs = append(inputs, inputCase{aprilFirst.Add(time.Duration(i) * time.Hour), manyNames}) + } + + for _, input := range inputs { + tx, err := sa.dbMap.Begin() + if err != nil { + t.Fatal(err) + } + err = sa.addCertificatesPerName(context.Background(), tx, input.names, input.time) + if err != nil { + t.Fatal(err) + } + err = tx.Commit() + if err != nil { + t.Fatal(err) + } + } + + const aWeek = time.Duration(7*24) * time.Hour + + testCases := []struct { + caseName string + domainName string + expected int + }{ + {"name doesn't exist", "non.example.org", 0}, + {"base name gets dinged for all certs including it", "example.com", 3}, + {"subdomain gets dinged for neighbors", "www.example.com", 3}, + {"other subdomain", "other.example.com", 3}, + {"many subdomains", "1.manynames.example.net", 168}, + {"public suffix gets its own bucket", "dyndns.org", 1}, + {"subdomain of public suffix gets its own bucket", "mydomain.dyndns.org", 2}, + {"subdomain of public suffix gets its own bucket 2", "otherdomain.dyndns.org", 1}, + } + + for _, tc := range testCases { + t.Run(tc.caseName, func(t *testing.T) { + count, err := sa.countCertificatesFaster(sa.dbMap, tc.domainName, aprilFirst.Add(-1*time.Second), aprilFirst.Add(aWeek)) + if err != nil { + t.Fatal(err) + } + if count != tc.expected { + t.Errorf("Expected count of %d for %q, got %d", tc.expected, tc.domainName, count) + } + }) + } +} diff --git a/sa/sa.go b/sa/sa.go index 9c4909db7..3f49c8f60 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -20,6 +20,7 @@ import ( "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" bgrpc "github.com/letsencrypt/boulder/grpc" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/metrics" @@ -43,8 +44,9 @@ type SQLStorageAuthority struct { // We use function types here so we can mock out this internal function in // unittests. - countCertificatesByName certCountFunc - getChallenges getChallengesFunc + countCertificatesByName certCountFunc + countCertificatesByExactName certCountFunc + getChallenges getChallengesFunc } func digest256(data []byte) []byte { @@ -105,6 +107,11 @@ func NewSQLStorageAuthority( } ssa.countCertificatesByName = ssa.countCertificatesByNameImpl + ssa.countCertificatesByExactName = ssa.countCertificatesByExactNameImpl + if features.Enabled(features.FasterRateLimit) { + ssa.countCertificatesByName = ssa.countCertificatesFaster + ssa.countCertificatesByExactName = ssa.countCertificatesFaster + } ssa.getChallenges = ssa.getChallengesImpl return ssa, nil @@ -476,7 +483,7 @@ func (ssa *SQLStorageAuthority) countCertificatesByNameImpl( // countCertificatesByExactNames returns, for a single domain, the count of // certificates issued in the given time range for that domain. In contrast to // countCertificatesByNames subdomains are NOT considered. -func (ssa *SQLStorageAuthority) countCertificatesByExactName( +func (ssa *SQLStorageAuthority) countCertificatesByExactNameImpl( db dbSelector, domain string, earliest, @@ -985,6 +992,16 @@ func (ssa *SQLStorageAuthority) AddCertificate( return "", Rollback(tx, err) } + // Add to the rate limit table, but only for new certificates. Renewals + // don't count against the certificatesPerName limit. + if !isRenewal { + timeToTheHour := parsedCertificate.NotBefore.Round(time.Hour) + err = ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour) + if err != nil { + return "", Rollback(tx, err) + } + } + err = addFQDNSet( txWithCtx, parsedCertificate.DNSNames, diff --git a/test/config-next/sa.json b/test/config-next/sa.json index 7abf258f0..68fd8406c 100644 --- a/test/config-next/sa.json +++ b/test/config-next/sa.json @@ -25,6 +25,7 @@ ] }, "features": { + "FasterRateLimit": true } }, diff --git a/test/sa_db_users.sql b/test/sa_db_users.sql index 467ec8fee..8b0052e89 100644 --- a/test/sa_db_users.sql +++ b/test/sa_db_users.sql @@ -34,6 +34,7 @@ GRANT SELECT(id,Lockcol) ON pendingAuthorizations TO 'sa'@'localhost'; GRANT SELECT,INSERT ON certificates TO 'sa'@'localhost'; GRANT SELECT,INSERT,UPDATE ON certificateStatus TO 'sa'@'localhost'; GRANT SELECT,INSERT ON issuedNames TO 'sa'@'localhost'; +GRANT SELECT,INSERT,UPDATE ON certificatesPerName TO 'sa'@'localhost'; GRANT SELECT,INSERT ON sctReceipts TO 'sa'@'localhost'; GRANT SELECT,INSERT,UPDATE ON registrations TO 'sa'@'localhost'; GRANT SELECT,INSERT,UPDATE ON challenges TO 'sa'@'localhost';