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.

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

Improve renewal rate limiting #2832

Merged
merged 10 commits into from Jun 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions features/featureflag_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions features/features.go
Expand Up @@ -27,6 +27,7 @@ const (
RandomDirectoryEntry
IPv6First
DirectoryMeta
AllowRenewalFirstRL
)

// List of features and their default value, protected by fMu
Expand All @@ -45,6 +46,7 @@ var features = map[FeatureFlag]bool{
RandomDirectoryEntry: false,
IPv6First: false,
DirectoryMeta: false,
AllowRenewalFirstRL: false,
}

var fMu = new(sync.RWMutex)
Expand Down
161 changes: 150 additions & 11 deletions sa/sa.go
Expand Up @@ -442,19 +442,19 @@ 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 certificate issuances in the given time range for that domain using the
// provided query, assumed to be either `countCertificatesExactSelect` or
// `countCertificatesSelect`.
// `countCertificatesSelect`. If the `AllowRenewalFirstRL` feature flag is set,
// renewals of certificates issued within the same 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
// 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,
Expand All @@ -467,16 +467,45 @@ func (ssa *SQLStorageAuthority) countCertificates(domain string, earliest, lates
if err == sql.ErrNoRows {
return 0, nil
} else if err != nil {
return -1, err
return 0, err
} 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{}{}
}

return len(serialMap), nil
// If the `AllowRenewalFirstRL` feature flag is enabled then do the work
// required to discount renewals
if features.Enabled(features.AllowRenewalFirstRL) {
// 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 0, err
}

// 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 0, err
}
return nonRenewalIssuances, nil
} else {
// Otherwise, use the preexisting behaviour and deduplicate by serials
// returning a count of unique serials qignoring any potential renewals
serialMap := make(map[string]struct{}, len(serials))
for _, s := range serials {
serialMap[s] = struct{}{}
}

return len(serialMap), nil
}
}

// GetCertificate takes a serial number and returns the corresponding
Expand Down Expand Up @@ -1062,6 +1091,116 @@ func (ssa *SQLStorageAuthority) CountFQDNSets(ctx context.Context, window time.D
return count, err
}

// 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 {
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 {
return nil, err
}

// The serials existed when we found them in issuedNames, they should continue
// to exist here. Otherwise an internal consistency violation occured and
// needs to be audit logged
if err == sql.ErrNoRows {
err := fmt.Errorf("getFQDNSetsBySerials returned no rows - internal consistency violation")
ssa.log.AuditErr(err.Error())
return nil, err
}
return fqdnSets, nil
}

// getNewIssuancesByFQDNSet returns a count of new issuances (renewals are not
// included) for a given slice of fqdnSets that occurred after the earliest
// parameter.
func (ssa *SQLStorageAuthority) getNewIssuancesByFQDNSet(fqdnSets []setHash, earliest time.Time) (int, error) {
var results []struct {
Serial string
SetHash setHash
Issued time.Time
}

qmarks := make([]string, len(fqdnSets))
params := make([]interface{}, len(fqdnSets))
for i, setHash := range fqdnSets {
// 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] = "?"
}

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
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]bool)
issuanceCount := 0
// 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 processedSetHashes[key] {
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] = true
}

// 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) {
Expand Down