search

letsencrypt / boulder

An ACME-based certificate authority, written in Go.
Mozilla Public License 2.0
5.06k stars 593 forks source link

Improve redis rate limit construction errors #7525

Closed aarongable closed 3 weeks ago

aarongable commented 4 weeks ago

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.

aarongable commented 4 weeks ago

I've rewritten the test to be more modular, and added new test cases to it. Running the new test against the old code results in these failures, as expected:

--- FAIL: TestValidateIdForName (0.00s)
    --- FAIL: TestValidateIdForName/FailedAuthorizationsPerDomainPerAccount/transaction:_invalid_domain (0.00s)
        names_test.go:201: String [invalid regId, "12345:examplecom" must be an ACME registration Id] does not contain [name needs at least one dot]
    --- FAIL: TestValidateIdForName/CertificatesPerDomainPerAccount/transaction:_invalid_domain (0.00s)
        names_test.go:201: String [invalid regId, "12345:examplecom" must be an ACME registration Id] does not contain [name needs at least one dot]