Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve renewal rate limiting #2832

Merged
merged 10 commits into from Jun 27, 2017
Merged

Improve renewal rate limiting #2832

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a79a73d
Add test for renewal exemption.
jsha Jun 9, 2017
b0d8345
Improve renewal rate limiting.
Jun 22, 2017
ed3ff70
Editorial fixes
Jun 26, 2017
369dfbc
Review feedback
Jun 26, 2017
76e30d1
Explicitly log getFQDNSetsBySerials consistency violation
Jun 26, 2017
4ec800d
Special case ErrNoRows check
Jun 26, 2017
712b4a6
Add AllowRenewalFirstRL feature flag
Jun 27, 2017
8314b9e
Control renewal first RL exemption by feature flag.
Jun 27, 2017
c32623c
Return 0 consistently
Jun 27, 2017
10c32a6
Merge remote-tracking branch 'le/master' into cpu-rate-limit-renewal-OOO
Jun 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 26 additions & 16 deletions sa/sa.go
View file
Open in desktop
Expand Up @@ -434,10 +434,10 @@ func (ssa *SQLStorageAuthority) countCertificatesByExactName(domain string, earl
}

// countCertificates returns, for a single domain, the count of
// non-renewal certificates issued in the given time range for that domain using the
// non-renewal certificate issuances in the given time range for that domain using the
// provided query, assumed to be either `countCertificatesExactSelect` or
// `countCertificatesSelect`. Renewals of certificates issued within the same
// window are considered "free" and not returned as part of this count.
// window are considered "free" and are not counted.
//
// 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
Expand Down Expand Up @@ -1046,10 +1046,14 @@ 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
// setHash is a []byte representing the hash of an FQDN Set
type setHash []byte

// getFQDNSetsBySerials finds the setHashes corresponding to a set of
// certificate serials. These serials can be used to check whether any
// certificates have been issued for the same set of names previously.
func (ssa *SQLStorageAuthority) getFQDNSetsBySerials(serials []string) ([]setHash, error) {
var fqdnSets []setHash

// It is unexpected that this function would be called with no serials
if len(serials) == 0 {
Expand All @@ -1065,32 +1069,38 @@ func (ssa *SQLStorageAuthority) getFQDNSetsBySerials(serials []string) ([][]byte
qmarks[i] = "?"
}
query := "SELECT setHash FROM fqdnSets " +
"WHERE serial IN (" + strings.Join(qmarks, ",") + ") "
"WHERE serial IN (" + strings.Join(qmarks, ",") + ")"
_, err := ssa.dbMap.Select(
&fqdnSets,
query,
params...)

if err != nil && err != sql.ErrNoRows {
// NOTE(@cpu): We don't explicitly check if `err != sql.ErrNoRows` here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but even better would be to use the same number of lines to explicitly check for ErrNoRows, and log an error saying something about "internal consistency violation" like above. Leans a little more in the "self-documenting code" direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved (I pushed the commit but it hasn't shown up yet. Maybe some GH lag?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There it is: Fixed in 76e30d1

// because we *want* ErrNoRows to be treated as an error since its an internal
// consistency violation. The serials existed when we found them in
// issuedNames, they should continue to exist here.
if err != nil {
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
// included) for a given slice of fqdnSets that occurred after the earliest
// parameter.
func (ssa *SQLStorageAuthority) getNewIssuancesByFQDNSet(fqdnSets [][]byte, earliest time.Time) (int, error) {
func (ssa *SQLStorageAuthority) getNewIssuancesByFQDNSet(fqdnSets []setHash, earliest time.Time) (int, error) {
var results []struct {
Serial string
SetHash []byte
SetHash setHash
Issued time.Time
}

qmarks := make([]string, len(fqdnSets))
params := make([]interface{}, len(fqdnSets))
for i, setHash := range fqdnSets {
params[i] = setHash
// We have to cast the setHash back to []byte here since the sql package
// isn't able to convert `sa.setHash` for the parameter value itself
params[i] = []byte(setHash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: probably doesn't make sense for this since it's a single use but we could add a converter case to sa/type-converter.go so that gorp knows how to natively use the setHash type without having to cast it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Agreed that its probably not worth it here, but good to know for the future.

qmarks[i] = "?"
}

Expand All @@ -1115,15 +1125,15 @@ func (ssa *SQLStorageAuthority) getNewIssuancesByFQDNSet(fqdnSets [][]byte, earl
return 0, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per above comment you're also not consistently returning -1 so I'm not sure I see the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - in that case I'll just update the whole thing to use 0. I agree that a -1 sentinel value is un-gophery.

}

processedSetHashes := make(map[string]struct{})
processedSetHashes := make(map[string]bool)
issuanceCount := 0
// Loop through each set hash result, counting issuance per unique set hash
// Loop through each set hash result, counting issuances 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 {
if processedSetHashes[key] {
continue
}

Expand All @@ -1134,7 +1144,7 @@ func (ssa *SQLStorageAuthority) getNewIssuancesByFQDNSet(fqdnSets [][]byte, earl

// Otherwise note the issuance and mark the set hash as processed
issuanceCount++
processedSetHashes[key] = struct{}{}
processedSetHashes[key] = true
}

// Return the count of how many non-renewal issuances there were
Expand Down
18 changes: 9 additions & 9 deletions sa/sa_test.go
View file
Open in desktop
Expand Up @@ -1041,7 +1041,7 @@ func TestReverseName(t *testing.T) {
type fqdnTestcase struct {
Serial string
Names []string
ExpectedHash []byte
ExpectedHash setHash
Issued time.Time
Expires time.Time
}
Expand All @@ -1050,9 +1050,9 @@ func setupFQDNSets(t *testing.T, db *gorp.DbMap, fc clock.FakeClock) map[string]
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}
expectedHashA := setHash{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 := setHash{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 := setHash{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()

Expand Down Expand Up @@ -1179,32 +1179,32 @@ func TestGetNewIssuancesByFQDNSet(t *testing.T) {

// 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)
count, err = sa.getNewIssuancesByFQDNSet([]setHash{setHash{0xC0, 0xFF, 0xEE}, setHash{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)
count, err = sa.getNewIssuancesByFQDNSet([]setHash{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)
count, err = sa.getNewIssuancesByFQDNSet([]setHash{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)
count, err = sa.getNewIssuancesByFQDNSet([]setHash{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))
count, err = sa.getNewIssuancesByFQDNSet([]setHash{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)
}