Validate CSR signatures and check signature type#386
Conversation
|
Boulder rejects SHA1 hashes in its |
d5bfbd2 to
f11eff4
Compare
With the deprecation of SHA1 signatures on CSRs, we should ensure Pebble rejects them as well. As well, ensure the signature itself is valid.
f11eff4 to
0dba3f8
Compare
|
I don't think where exactly the messages are rejected matters: Pebble is a much smaller codebase with less modules than boulder so it won't ever align the same exactly. Pebble only handles the CSRs within the WFE (web frontend) code here so it would require restructuring more code to have the check occur elsewhere. |
aarongable
left a comment
There was a problem hiding this comment.
Overall LGTM, just a few formatting comments
This removes all of the use of "msg"+err.Error() in this file. In some cases I've added a variable to avoid over-long lines without resorting to awkwardly line breaking in function calls
|
Design question: Should either of these checks be gated on Pebble's "strict mode"? |
aarongable
left a comment
There was a problem hiding this comment.
LGTM. I wouldn't gate rejecting SHA1 behind "strict mode": half the reason we want to remove it is that Go itself is going to remove support, so Pebble needs to deprecate it just as much as Boulder does. Validating the signature could be behind the strict-mode flag but I don't feel strongly either way.
|
I don't think it's likely to be helpful to anyone to allow invalid CSR signatures, especially since Let's Encrypt and presumably other CAs always require valid CSR signatures. So I think we can leave it as-is and merge this. |
With the deprecation of SHA1 signatures on CSRs, we should ensure Pebble rejects them as well. As well, ensure the signature itself is valid. Unlike Boulder, which currently allows SHA1 to be configurably accepted, Pebble will always reject SHA1.
There's no tests for the WFE code in this repository, so I didn't add a unit test ensuring SHA1 is rejected. However, I tested this by running an ACME client against Pebble that was patched to use SHA1 signatures and verifying they were successfully rejected, as well as ensuring SHA256 was still accepted
Fixes #384