ratelimits: API improvements necessary for batches and limit fixes (#…

…7117)

The `Limiter` API has been adjusted significantly to both improve both
safety and ergonomics and two `Limit` types have been corrected to match
the legacy implementations.

**Safety**
Previously, the key used for looking up limit overrides and for fetching
individual buckets from the key-value store was constructed within the
WFE. This posed a risk: if the key was malformed, the default limit
would still be enforced, but individual overrides would fail to function
properly. This has been addressed by the introduction of a new
`BucketId` type along with a `BucketId` constructor for each `Limit`
type. Each constructor is responsible for producing a well-formed bucket
key which undergoes the very same validation as any potentially matching
override key.

**Ergonomics**
Previously, each of the `Limiter` methods took a `Limit` name, a bucket
identifier, and a cost to be spent/ refunded. To simplify this, each
method now accepts a new `Transaction` type which provides a cost, and
wraps a `BucketId` identifying the specific bucket.

The two changes above, when taken together, make the implementation of
batched rate limit transactions considerably easier, as a batch method
can accept a slice of `Transaction`.

**Limit Corrections**
PR #6947 added all of the existing rate limits which could be made
compatible with the key-value approach. Two of these were improperly
implemented;
- `CertificatesPerDomain` and `CertificatesPerFQDNSet`, were implemented
as
- `CertificatesPerDomainPerAccount` and
`CertificatesPerFQDNSetPerAccount`.

Since we do not actually associate these limits with a particular ACME
account, the `regID` portion of each of their bucket keys has been
removed.

ratelimits/README.md

@@ -79,22 +79,21 @@ Example: `NewRegistrationsPerIPv6Range:2001:0db8:0000::/48`
 
 #### regId
 
-The registration ID of the account.
+An ACME account registration ID.
 
 Example: `NewOrdersPerAccount:12345678`
 
-#### regId:domain
+#### domain
 
-A combination of registration ID and domain, formatted 'regId:domain'.
+A valid eTLD+1 domain name.
 
-Example: `CertificatesPerDomainPerAccount:12345678:example.com`
+Example: `CertificatesPerDomain:example.com`
 
-#### regId:fqdnSet
+#### fqdnSet
 
-A combination of registration ID and a comma-separated list of domain names,
-formatted 'regId:fqdnSet'.
+A comma-separated list of domain names.
 
-Example: `CertificatesPerFQDNSetPerAccount:12345678:example.com,example.org`
+Example: `CertificatesPerFQDNSet:example.com,example.org`
 
 ## Bucket Key Definitions
 

ratelimits/bucket.go

@@ -0,0 +1,52 @@
+package ratelimits
+
+import (
+	"fmt"
+	"net"
+)
+
+// BucketId should only be created using the New*BucketId functions. It is used
+// by the Limiter to look up the bucket and limit overrides for a specific
+// subscriber and limit.
+type BucketId struct {
+	// limit is the name of the associated rate limit. It is used for looking up
+	// default limits.
+	limit Name
+
+	// bucketKey is the limit Name enum (e.g. "1") concatenated with the
+	// subscriber identifier specific to the associate limit Name type.
+	bucketKey string
+}
+
+// NewRegistrationsPerIPAddressBucketId returns a BucketId for the provided IP
+// address.
+func NewRegistrationsPerIPAddressBucketId(ip net.IP) (BucketId, error) {
+	id := ip.String()
+	err := validateIdForName(NewRegistrationsPerIPAddress, id)
+	if err != nil {
+		return BucketId{}, err
+	}
+	return BucketId{
+		limit:     NewRegistrationsPerIPAddress,
+		bucketKey: joinWithColon(NewRegistrationsPerIPAddress.EnumString(), id),
+	}, nil
+}
+
+// NewRegistrationsPerIPv6RangeBucketId returns a BucketId for the /48 IPv6
+// range containing the provided IPv6 address.
+func NewRegistrationsPerIPv6RangeBucketId(ip net.IP) (BucketId, error) {
+	if ip.To4() != nil {
+		return BucketId{}, fmt.Errorf("invalid IPv6 address, %q must be an IPv6 address", ip.String())
+	}
+	ipMask := net.CIDRMask(48, 128)
+	ipNet := &net.IPNet{IP: ip.Mask(ipMask), Mask: ipMask}
+	id := ipNet.String()
+	err := validateIdForName(NewRegistrationsPerIPv6Range, id)
+	if err != nil {
+		return BucketId{}, err
+	}
+	return BucketId{
+		limit:     NewRegistrationsPerIPv6Range,
+		bucketKey: joinWithColon(NewRegistrationsPerIPv6Range.EnumString(), id),
+	}, nil
+}

ratelimits/limit.go

@@ -121,22 +121,14 @@ func loadAndParseOverrideLimits(path string) (limits, error) {
 			return nil, fmt.Errorf(
 				"validating name %s and id %q for override limit %q: %w", name, id, k, err)
 		}
-		if name == CertificatesPerFQDNSetPerAccount {
+		if name == CertificatesPerFQDNSet {
 			// FQDNSet hashes are not a nice thing to ask for in a config file,
 			// so we allow the user to specify a comma-separated list of FQDNs
 			// and compute the hash here.
-			regIdDomains := strings.SplitN(id, ":", 2)
-			if len(regIdDomains) != 2 {
-				// Should never happen, the Id format was validated above.
-				return nil, fmt.Errorf("invalid override limit %q, must be formatted 'name:id'", k)
-			}
-			regId := regIdDomains[0]
-			domains := strings.Split(regIdDomains[1], ",")
-			fqdnSet := core.HashNames(domains)
-			id = fmt.Sprintf("%s:%s", regId, fqdnSet)
+			id = string(core.HashNames(strings.Split(id, ",")))
 		}
 		v.isOverride = true
-		parsed[bucketKey(name, id)] = precomputeLimit(v)
+		parsed[joinWithColon(name.EnumString(), id)] = precomputeLimit(v)
 	}
 	return parsed, nil
 }
@@ -159,7 +151,7 @@ func loadAndParseDefaultLimits(path string) (limits, error) {
 		if !ok {
 			return nil, fmt.Errorf("unrecognized name %q in default limit, must be one of %v", k, limitNames)
 		}
-		parsed[nameToEnumString(name)] = precomputeLimit(v)
+		parsed[name.EnumString()] = precomputeLimit(v)
 	}
 	return parsed, nil
 }

ratelimits/limit_test.go

@@ -78,19 +78,19 @@ func Test_validateIdForName(t *testing.T) {
 	err = validateIdForName(NewOrdersPerAccount, "1234567890")
 	test.AssertNotError(t, err, "valid regId")
 
-	// 'enum:regId:domain'
+	// 'enum:domain'
 	// Valid regId and domain.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "1234567890:example.com")
+	err = validateIdForName(CertificatesPerDomain, "example.com")
 	test.AssertNotError(t, err, "valid regId and domain")
 
-	// 'enum:regId:fqdnSet'
-	// Valid regId and FQDN set containing a single domain.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "1234567890:example.com")
+	// 'enum:fqdnSet'
+	// Valid fqdnSet containing a single domain.
+	err = validateIdForName(CertificatesPerFQDNSet, "example.com")
 	test.AssertNotError(t, err, "valid regId and FQDN set containing a single domain")
 
-	// 'enum:regId:fqdnSet'
-	// Valid regId and FQDN set containing multiple domains.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "1234567890:example.com,example.org")
+	// 'enum:fqdnSet'
+	// Valid fqdnSet containing multiple domains.
+	err = validateIdForName(CertificatesPerFQDNSet, "example.com,example.org")
 	test.AssertNotError(t, err, "valid regId and FQDN set containing multiple domains")
 
 	// Empty string.
@@ -125,107 +125,56 @@ func Test_validateIdForName(t *testing.T) {
 	err = validateIdForName(NewOrdersPerAccount, "lol")
 	test.AssertError(t, err, "invalid regId")
 
-	// Invalid regId with good domain.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "lol:example.com")
-	test.AssertError(t, err, "invalid regId with good domain")
-
-	// Valid regId with bad domain.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "1234567890:lol")
-	test.AssertError(t, err, "valid regId with bad domain")
-
-	// Empty regId with good domain.
-	err = validateIdForName(CertificatesPerDomainPerAccount, ":lol")
+	// Invalid domain, malformed.
+	err = validateIdForName(CertificatesPerDomain, "example:.com")
 	test.AssertError(t, err, "valid regId with bad domain")
 
-	// Valid regId with empty domain.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "1234567890:")
+	// Invalid domain, empty.
+	err = validateIdForName(CertificatesPerDomain, "")
 	test.AssertError(t, err, "valid regId with empty domain")
-
-	// Empty regId with empty domain, no separator.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "")
-	test.AssertError(t, err, "empty regId with empty domain, no separator")
-
-	// Instead of anything we would expect, we get lol.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "lol")
-	test.AssertError(t, err, "instead of anything we would expect, just lol")
-
-	// Valid regId with good domain and a secret third separator.
-	err = validateIdForName(CertificatesPerDomainPerAccount, "1234567890:example.com:lol")
-	test.AssertError(t, err, "valid regId with good domain and a secret third separator")
-
-	// Valid regId with bad FQDN set.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "1234567890:lol..99")
-	test.AssertError(t, err, "valid regId with bad FQDN set")
-
-	// Bad regId with good FQDN set.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "lol:example.com,example.org")
-	test.AssertError(t, err, "bad regId with good FQDN set")
-
-	// Empty regId with good FQDN set.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, ":example.com,example.org")
-	test.AssertError(t, err, "empty regId with good FQDN set")
-
-	// Good regId with empty FQDN set.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "1234567890:")
-	test.AssertError(t, err, "good regId with empty FQDN set")
-
-	// Empty regId with empty FQDN set, no separator.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "")
-	test.AssertError(t, err, "empty regId with empty FQDN set, no separator")
-
-	// Instead of anything we would expect, just lol.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "lol")
-	test.AssertError(t, err, "instead of anything we would expect, just lol")
-
-	// Valid regId with good FQDN set and a secret third separator.
-	err = validateIdForName(CertificatesPerFQDNSetPerAccount, "1234567890:example.com,example.org:lol")
-	test.AssertError(t, err, "valid regId with good FQDN set and a secret third separator")
 }
 
 func Test_loadAndParseOverrideLimits(t *testing.T) {
-	newRegistrationsPerIPAddressEnumStr := nameToEnumString(NewRegistrationsPerIPAddress)
-	newRegistrationsPerIPv6RangeEnumStr := nameToEnumString(NewRegistrationsPerIPv6Range)
-
 	// Load a single valid override limit with Id formatted as 'enum:RegId'.
 	l, err := loadAndParseOverrideLimits("testdata/working_override.yml")
 	test.AssertNotError(t, err, "valid single override limit")
-	expectKey := newRegistrationsPerIPAddressEnumStr + ":" + "10.0.0.2"
+	expectKey := joinWithColon(NewRegistrationsPerIPAddress.EnumString(), "10.0.0.2")
 	test.AssertEquals(t, l[expectKey].Burst, int64(40))
 	test.AssertEquals(t, l[expectKey].Count, int64(40))
 	test.AssertEquals(t, l[expectKey].Period.Duration, time.Second)
 
 	// Load single valid override limit with Id formatted as 'regId:domain'.
 	l, err = loadAndParseOverrideLimits("testdata/working_override_regid_domain.yml")
 	test.AssertNotError(t, err, "valid single override limit with Id of regId:domain")
-	expectKey = nameToEnumString(CertificatesPerDomainPerAccount) + ":" + "12345678:example.com"
+	expectKey = joinWithColon(CertificatesPerDomain.EnumString(), "example.com")
 	test.AssertEquals(t, l[expectKey].Burst, int64(40))
 	test.AssertEquals(t, l[expectKey].Count, int64(40))
 	test.AssertEquals(t, l[expectKey].Period.Duration, time.Second)
 
 	// Load multiple valid override limits with 'enum:RegId' Ids.
 	l, err = loadAndParseOverrideLimits("testdata/working_overrides.yml")
-	expectKey1 := newRegistrationsPerIPAddressEnumStr + ":" + "10.0.0.2"
+	expectKey1 := joinWithColon(NewRegistrationsPerIPAddress.EnumString(), "10.0.0.2")
 	test.AssertNotError(t, err, "multiple valid override limits")
 	test.AssertEquals(t, l[expectKey1].Burst, int64(40))
 	test.AssertEquals(t, l[expectKey1].Count, int64(40))
 	test.AssertEquals(t, l[expectKey1].Period.Duration, time.Second)
-	expectKey2 := newRegistrationsPerIPv6RangeEnumStr + ":" + "2001:0db8:0000::/48"
+	expectKey2 := joinWithColon(NewRegistrationsPerIPv6Range.EnumString(), "2001:0db8:0000::/48")
 	test.AssertEquals(t, l[expectKey2].Burst, int64(50))
 	test.AssertEquals(t, l[expectKey2].Count, int64(50))
 	test.AssertEquals(t, l[expectKey2].Period.Duration, time.Second*2)
 
-	// Load multiple valid override limits with 'regID:fqdnSet' Ids as follows:
-	//   - CertificatesPerFQDNSetPerAccount:12345678:example.com
-	//   - CertificatesPerFQDNSetPerAccount:12345678:example.com,example.net
-	//   - CertificatesPerFQDNSetPerAccount:12345678:example.com,example.net,example.org
+	// Load multiple valid override limits with 'fqdnSet' Ids, as follows:
+	//   - CertificatesPerFQDNSet:example.com
+	//   - CertificatesPerFQDNSet:example.com,example.net
+	//   - CertificatesPerFQDNSet:example.com,example.net,example.org
 	firstEntryFQDNSetHash := string(core.HashNames([]string{"example.com"}))
 	secondEntryFQDNSetHash := string(core.HashNames([]string{"example.com", "example.net"}))
 	thirdEntryFQDNSetHash := string(core.HashNames([]string{"example.com", "example.net", "example.org"}))
-	firstEntryKey := nameToEnumString(CertificatesPerFQDNSetPerAccount) + ":" + "12345678:" + firstEntryFQDNSetHash
-	secondEntryKey := nameToEnumString(CertificatesPerFQDNSetPerAccount) + ":" + "12345678:" + secondEntryFQDNSetHash
-	thirdEntryKey := nameToEnumString(CertificatesPerFQDNSetPerAccount) + ":" + "12345678:" + thirdEntryFQDNSetHash
+	firstEntryKey := joinWithColon(CertificatesPerFQDNSet.EnumString(), firstEntryFQDNSetHash)
+	secondEntryKey := joinWithColon(CertificatesPerFQDNSet.EnumString(), secondEntryFQDNSetHash)
+	thirdEntryKey := joinWithColon(CertificatesPerFQDNSet.EnumString(), thirdEntryFQDNSetHash)
 	l, err = loadAndParseOverrideLimits("testdata/working_overrides_regid_fqdnset.yml")
-	test.AssertNotError(t, err, "multiple valid override limits with Id of regId:fqdnSets")
+	test.AssertNotError(t, err, "multiple valid override limits with 'fqdnSet' Ids")
 	test.AssertEquals(t, l[firstEntryKey].Burst, int64(40))
 	test.AssertEquals(t, l[firstEntryKey].Count, int64(40))
 	test.AssertEquals(t, l[firstEntryKey].Period.Duration, time.Second)
@@ -278,25 +227,22 @@ func Test_loadAndParseOverrideLimits(t *testing.T) {
 }
 
 func Test_loadAndParseDefaultLimits(t *testing.T) {
-	newRestistrationsPerIPv4AddressEnumStr := nameToEnumString(NewRegistrationsPerIPAddress)
-	newRegistrationsPerIPv6RangeEnumStr := nameToEnumString(NewRegistrationsPerIPv6Range)
-
 	// Load a single valid default limit.
 	l, err := loadAndParseDefaultLimits("testdata/working_default.yml")
 	test.AssertNotError(t, err, "valid single default limit")
-	test.AssertEquals(t, l[newRestistrationsPerIPv4AddressEnumStr].Burst, int64(20))
-	test.AssertEquals(t, l[newRestistrationsPerIPv4AddressEnumStr].Count, int64(20))
-	test.AssertEquals(t, l[newRestistrationsPerIPv4AddressEnumStr].Period.Duration, time.Second)
+	test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Burst, int64(20))
+	test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Count, int64(20))
+	test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Period.Duration, time.Second)
 
 	// Load multiple valid default limits.
 	l, err = loadAndParseDefaultLimits("testdata/working_defaults.yml")
 	test.AssertNotError(t, err, "multiple valid default limits")
-	test.AssertEquals(t, l[newRestistrationsPerIPv4AddressEnumStr].Burst, int64(20))
-	test.AssertEquals(t, l[newRestistrationsPerIPv4AddressEnumStr].Count, int64(20))
-	test.AssertEquals(t, l[newRestistrationsPerIPv4AddressEnumStr].Period.Duration, time.Second)
-	test.AssertEquals(t, l[newRegistrationsPerIPv6RangeEnumStr].Burst, int64(30))
-	test.AssertEquals(t, l[newRegistrationsPerIPv6RangeEnumStr].Count, int64(30))
-	test.AssertEquals(t, l[newRegistrationsPerIPv6RangeEnumStr].Period.Duration, time.Second*2)
+	test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Burst, int64(20))
+	test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Count, int64(20))
+	test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Period.Duration, time.Second)
+	test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].Burst, int64(30))
+	test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].Count, int64(30))
+	test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].Period.Duration, time.Second*2)
 
 	// Path is empty string.
 	_, err = loadAndParseDefaultLimits("")