ca: Pass OCSP Must-Staple from CSR into generated certificate #436
Conversation
|
I would prefer to look for just the Must Staple extension and set that, which is what Boulder does. It's quite dangerous to pass through arbitrary extensions from the CSR, because some extensions change the semantics of certificates in dramatic ways. Of course Pebble is a testing CA but we should still avoid creating dangerous code for someone to possibly emulate. :D |
ca/ca.go
Outdated
| var extensions []pkix.Extension | ||
| for _, ext := range order.ParsedCSR.Extensions { | ||
| if isOCSPMustStapleExtension(ext) { | ||
| extensions = append(extensions, ext) |
There was a problem hiding this comment.
Directly appending the extension from the CSR is still slightly dangerous, as we are directly carrying over the critical bit. Boulder has a pre-built OCSP must-staple extension, and it uses that both to detect must-staple in the CSR and include must-staple in the resulting cert. That way Boulder can guarantee that the extension is never critical.
What you have here is probably fine for Pebble, but if you want to be even safer that's a pattern you could follow.
There was a problem hiding this comment.
ah nice, I'll do that! I wasn't sure how risky the critical value was, but that makes sense.
ca/ca_test.go
Outdated
| } | ||
|
|
||
| ca.CompleteOrder(order) | ||
| log.Printf("cert: %+v", order.CertificateObject.Cert) |
There was a problem hiding this comment.
Seems like this log line should be replaced by parsing the resulting cert and inspecting its extensions to confirm the inclusion of must-staple.
There was a problem hiding this comment.
Oh shoot, I actually didn't mean to include this file, it was just for debugging. Though I could turn it into a full test, if that seems desirable
There was a problem hiding this comment.
I think it's great to have a test for this!
aarongable
changed the title
This will allow for extensions such as OCSP stapling.
Also adds unit tests.
|
Added some unit tests and modeled this after Boulder's approach. Ready for review again! |
|
Can you rebase or merge in main please? We've merged some fixes in the last few day for pebble's CI |
|
Should be all set |
|
thanks. A few things from the linter, otherwise lgtm |
This will allow for extensions such as OCSP stapling. Happy to add a test if desired, also I wasn't sure if it's expected to filter for specific extensions or to pass them wholesale onto the cert?
Fixes #433