Skip to content

Commit 3786401

Browse files
aarongableaarongable
authored
Improve redis rate limit construction errors (#7525)
Change ratelimits.validateIdForName to call the appropriate validate function based on the contents of the to-be-validated ID, rather than falling back and potentially performing multiple validations. Previously, if an ID like "12345:bad.domain" was passed into this function, it would fail the first validateRegIdDomain validation due to having a bad domain name (no TLD), fall back to the simpler validateRegId function, and then fail that because it contains a colon. This obscured the true reason for the failure. Changing this code to not fall back means that the true reason for the id validation failure will be exposed in the error message.
1 parent db2857b commit 3786401

File tree

3 files changed

+190
-89
lines changed

3 files changed

+190
-89
lines changed

‎ratelimits/limit_test.go‎

Lines changed: 0 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -56,83 +56,6 @@ func TestValidateLimit(t *testing.T) {
5656
}
5757
}
5858

59-
func TestValidateIdForName(t *testing.T) {
60-
// 'enum:ipAddress'
61-
// Valid IPv4 address.
62-
err := validateIdForName(NewRegistrationsPerIPAddress, "10.0.0.1")
63-
test.AssertNotError(t, err, "valid ipv4 address")
64-
65-
// 'enum:ipAddress'
66-
// Valid IPv6 address.
67-
err = validateIdForName(NewRegistrationsPerIPAddress, "2001:0db8:85a3:0000:0000:8a2e:0370:7334")
68-
test.AssertNotError(t, err, "valid ipv6 address")
69-
70-
// 'enum:ipv6rangeCIDR'
71-
// Valid IPv6 address range.
72-
err = validateIdForName(NewRegistrationsPerIPv6Range, "2001:0db8:0000::/48")
73-
test.AssertNotError(t, err, "should not error")
74-
75-
// 'enum:regId'
76-
// Valid regId.
77-
err = validateIdForName(NewOrdersPerAccount, "1234567890")
78-
test.AssertNotError(t, err, "valid regId")
79-
80-
// 'enum:domain'
81-
// Valid regId and domain.
82-
err = validateIdForName(CertificatesPerDomain, "example.com")
83-
test.AssertNotError(t, err, "valid regId and domain")
84-
85-
// 'enum:fqdnSet'
86-
// Valid fqdnSet containing a single domain.
87-
err = validateIdForName(CertificatesPerFQDNSet, "example.com")
88-
test.AssertNotError(t, err, "valid regId and FQDN set containing a single domain")
89-
90-
// 'enum:fqdnSet'
91-
// Valid fqdnSet containing multiple domains.
92-
err = validateIdForName(CertificatesPerFQDNSet, "example.com,example.org")
93-
test.AssertNotError(t, err, "valid regId and FQDN set containing multiple domains")
94-
95-
// Empty string.
96-
err = validateIdForName(NewRegistrationsPerIPAddress, "")
97-
test.AssertError(t, err, "Id is an empty string")
98-
99-
// One space.
100-
err = validateIdForName(NewRegistrationsPerIPAddress, " ")
101-
test.AssertError(t, err, "Id is a single space")
102-
103-
// Invalid IPv4 address.
104-
err = validateIdForName(NewRegistrationsPerIPAddress, "10.0.0.9000")
105-
test.AssertError(t, err, "invalid IPv4 address")
106-
107-
// Invalid IPv6 address.
108-
err = validateIdForName(NewRegistrationsPerIPAddress, "2001:0db8:85a3:0000:0000:8a2e:0370:7334:9000")
109-
test.AssertError(t, err, "invalid IPv6 address")
110-
111-
// Invalid IPv6 CIDR range.
112-
err = validateIdForName(NewRegistrationsPerIPv6Range, "2001:0db8:0000::/128")
113-
test.AssertError(t, err, "invalid IPv6 CIDR range")
114-
115-
// Invalid IPv6 CIDR.
116-
err = validateIdForName(NewRegistrationsPerIPv6Range, "2001:0db8:0000::/48/48")
117-
test.AssertError(t, err, "invalid IPv6 CIDR")
118-
119-
// IPv4 CIDR when we expect IPv6 CIDR range.
120-
err = validateIdForName(NewRegistrationsPerIPv6Range, "10.0.0.0/16")
121-
test.AssertError(t, err, "ipv4 cidr when we expect ipv6 cidr range")
122-
123-
// Invalid regId.
124-
err = validateIdForName(NewOrdersPerAccount, "lol")
125-
test.AssertError(t, err, "invalid regId")
126-
127-
// Invalid domain, malformed.
128-
err = validateIdForName(CertificatesPerDomain, "example:.com")
129-
test.AssertError(t, err, "valid regId with bad domain")
130-
131-
// Invalid domain, empty.
132-
err = validateIdForName(CertificatesPerDomain, "")
133-
test.AssertError(t, err, "valid regId with empty domain")
134-
}
135-
13659
func TestLoadAndParseOverrideLimits(t *testing.T) {
13760
// Load a single valid override limit with Id formatted as 'enum:RegId'.
13861
l, err := loadAndParseOverrideLimits("testdata/working_override.yml")

‎ratelimits/names.go‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -212,22 +212,22 @@ func validateIdForName(name Name, id string) error {
212212
return validateRegId(id)
213213

214214
case FailedAuthorizationsPerDomainPerAccount:
215-
// 'enum:regId:domain' for transaction
216-
err := validateRegIdDomain(id)
217-
if err == nil {
218-
return nil
215+
if strings.Contains(id, ":") {
216+
// 'enum:regId:domain' for transaction
217+
return validateRegIdDomain(id)
218+
} else {
219+
// 'enum:regId' for overrides
220+
return validateRegId(id)
219221
}
220-
// 'enum:regId' for overrides
221-
return validateRegId(id)
222222

223223
case CertificatesPerDomainPerAccount:
224-
// 'enum:regId:domain' for transaction
225-
err := validateRegIdDomain(id)
226-
if err == nil {
227-
return nil
224+
if strings.Contains(id, ":") {
225+
// 'enum:regId:domain' for transaction
226+
return validateRegIdDomain(id)
227+
} else {
228+
// 'enum:regId' for overrides
229+
return validateRegId(id)
228230
}
229-
// 'enum:regId' for overrides
230-
return validateRegId(id)
231231

232232
case CertificatesPerDomain:
233233
// 'enum:domain'

‎ratelimits/names_test.go‎

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package ratelimits
22

33
import (
4+
"fmt"
45
"testing"
56

67
"github.com/letsencrypt/boulder/test"
@@ -27,3 +28,180 @@ func TestNameIsValid(t *testing.T) {
2728
})
2829
}
2930
}
31+
32+
func TestValidateIdForName(t *testing.T) {
33+
t.Parallel()
34+
35+
testCases := []struct {
36+
limit Name
37+
desc string
38+
id string
39+
err string
40+
}{
41+
{
42+
limit: NewRegistrationsPerIPAddress,
43+
desc: "valid IPv4 address",
44+
id: "10.0.0.1",
45+
},
46+
{
47+
limit: NewRegistrationsPerIPAddress,
48+
desc: "valid IPv6 address",
49+
id: "2001:0db8:85a3:0000:0000:8a2e:0370:7334",
50+
},
51+
{
52+
limit: NewRegistrationsPerIPAddress,
53+
desc: "empty string",
54+
id: "",
55+
err: "must be an IP address",
56+
},
57+
{
58+
limit: NewRegistrationsPerIPAddress,
59+
desc: "one space",
60+
id: " ",
61+
err: "must be an IP address",
62+
},
63+
{
64+
limit: NewRegistrationsPerIPAddress,
65+
desc: "invalid IPv4 address",
66+
id: "10.0.0.9000",
67+
err: "must be an IP address",
68+
},
69+
{
70+
limit: NewRegistrationsPerIPAddress,
71+
desc: "invalid IPv6 address",
72+
id: "2001:0db8:85a3:0000:0000:8a2e:0370:7334:9000",
73+
err: "must be an IP address",
74+
},
75+
{
76+
limit: NewRegistrationsPerIPv6Range,
77+
desc: "valid IPv6 address range",
78+
id: "2001:0db8:0000::/48",
79+
},
80+
{
81+
limit: NewRegistrationsPerIPv6Range,
82+
desc: "invalid IPv6 CIDR range",
83+
id: "2001:0db8:0000::/128",
84+
err: "must be /48",
85+
},
86+
{
87+
limit: NewRegistrationsPerIPv6Range,
88+
desc: "invalid IPv6 CIDR",
89+
id: "2001:0db8:0000::/48/48",
90+
err: "must be an IPv6 CIDR range",
91+
},
92+
{
93+
limit: NewRegistrationsPerIPv6Range,
94+
desc: "IPv4 CIDR when we expect IPv6 CIDR range",
95+
id: "10.0.0.0/16",
96+
err: "must be /48",
97+
},
98+
{
99+
limit: NewOrdersPerAccount,
100+
desc: "valid regId",
101+
id: "1234567890",
102+
},
103+
{
104+
limit: NewOrdersPerAccount,
105+
desc: "invalid regId",
106+
id: "lol",
107+
err: "must be an ACME registration Id",
108+
},
109+
{
110+
limit: FailedAuthorizationsPerDomainPerAccount,
111+
desc: "transaction: valid regId and domain",
112+
id: "12345:example.com",
113+
},
114+
{
115+
limit: FailedAuthorizationsPerDomainPerAccount,
116+
desc: "transaction: invalid regId",
117+
id: "12ea5:example.com",
118+
err: "invalid regId",
119+
},
120+
{
121+
limit: FailedAuthorizationsPerDomainPerAccount,
122+
desc: "transaction: invalid domain",
123+
id: "12345:examplecom",
124+
err: "name needs at least one dot",
125+
},
126+
{
127+
limit: FailedAuthorizationsPerDomainPerAccount,
128+
desc: "override: valid regId",
129+
id: "12345",
130+
},
131+
{
132+
limit: FailedAuthorizationsPerDomainPerAccount,
133+
desc: "override: invalid regId",
134+
id: "12ea5",
135+
err: "invalid regId",
136+
},
137+
{
138+
limit: CertificatesPerDomainPerAccount,
139+
desc: "transaction: valid regId and domain",
140+
id: "12345:example.com",
141+
},
142+
{
143+
limit: CertificatesPerDomainPerAccount,
144+
desc: "transaction: invalid regId",
145+
id: "12ea5:example.com",
146+
err: "invalid regId",
147+
},
148+
{
149+
limit: CertificatesPerDomainPerAccount,
150+
desc: "transaction: invalid domain",
151+
id: "12345:examplecom",
152+
err: "name needs at least one dot",
153+
},
154+
{
155+
limit: CertificatesPerDomainPerAccount,
156+
desc: "override: valid regId",
157+
id: "12345",
158+
},
159+
{
160+
limit: CertificatesPerDomainPerAccount,
161+
desc: "override: invalid regId",
162+
id: "12ea5",
163+
err: "invalid regId",
164+
},
165+
{
166+
limit: CertificatesPerDomain,
167+
desc: "valid domain",
168+
id: "example.com",
169+
},
170+
{
171+
limit: CertificatesPerDomain,
172+
desc: "malformed domain",
173+
id: "example:.com",
174+
err: "name contains an invalid character",
175+
},
176+
{
177+
limit: CertificatesPerDomain,
178+
desc: "empty domain",
179+
id: "",
180+
err: "name is empty",
181+
},
182+
{
183+
limit: CertificatesPerFQDNSet,
184+
desc: "valid fqdnSet containing a single domain",
185+
id: "example.com",
186+
},
187+
{
188+
limit: CertificatesPerFQDNSet,
189+
desc: "valid fqdnSet containing multiple domains",
190+
id: "example.com,example.org",
191+
},
192+
}
193+
194+
for _, tc := range testCases {
195+
tc := tc
196+
t.Run(fmt.Sprintf("%s/%s", tc.limit, tc.desc), func(t *testing.T) {
197+
t.Parallel()
198+
err := validateIdForName(tc.limit, tc.id)
199+
if tc.err != "" {
200+
test.AssertError(t, err, "should have failed")
201+
test.AssertContains(t, err.Error(), tc.err)
202+
} else {
203+
test.AssertNotError(t, err, "should have succeeded")
204+
}
205+
})
206+
}
207+
}

0 commit comments

Comments
 (0)