Repository navigation
Only accept revocation reasons 0, 1, 3, 4, 5 and 9 - #558
Preston12321 wants to merge 1 commit into
Conversation
Replace the check for reasons 7 and >10 with an allowlist. This also rejects cACompromise (2), which applies only to CA certificates; certificateHold (6), which the Baseline Requirements forbid; removeFromCRL (8), which is only allowed in delta CRLs; and aACompromise (10), which applies only to attribute authorities.
| // - 2 (cACompromise) applies only to CA certificates. | ||
| // - 6 (certificateHold) is forbidden by the Baseline Requirements. | ||
| // - 7 is unused, and codes above 10 are undefined. | ||
| // - 8 (removeFromCRL) is only allowed in delta CRLs (RFC 5280 Section 5.3.1). | ||
| // - 10 (aACompromise) applies only to attribute authorities. |
There was a problem hiding this comment.
This block is extra fluff, we already know what we accept so we shouldn't need to track what we don't accept. Delete this please.
There was a problem hiding this comment.
Agreed. Also potentially worth noting the ways in which this list (purposefully!) differ's from Boulder's allowlist. Boulder does not permit affiliationChanged (because it's irrelevant to DV certs) and privilegeWithdrawn (because only the CA can request that code). Having pebble accept those two is good (we like pebble behaving differently from Boulder), but it's nice to note the divergence so people don't think it's a bug.
|
|
||
| func TestProcessRevocationReasons(t *testing.T) { | ||
| badReasonType := acme.BadRevocationReasonProblem("").Type | ||
| for reason := range uint(12) { |
There was a problem hiding this comment.
I understand what this is doing, but this test feels weird to me and required a several doubletakes.
| // - 7 is unused, and codes above 10 are undefined. | ||
| // - 8 (removeFromCRL) is only allowed in delta CRLs (RFC 5280 Section 5.3.1). | ||
| // - 10 (aACompromise) applies only to attribute authorities. | ||
| var validRevocationReasons = map[uint]struct{}{ |
There was a problem hiding this comment.
nit: its slightly better to do this as a function with a switch/case inside it. The issue with the map approach is that maps aren't immutable; some code very far from here could accidentally edit the contents of this map. But for pebble it doesn't matter that much, so feel free to disregard this nit.
3 participants
Replace the check for reasons 7 and >10 with an allowlist. This also rejects cACompromise (2), which applies only to CA certificates; certificateHold (6), which the Baseline Requirements forbid; removeFromCRL (8), which is only allowed in delta CRLs; and aACompromise (10), which applies only to attribute authorities.
Note: This change is entirely generated by Claude, but I provided significant guidance and have manually reviewed the diff