Don't panic on empty cert list or unsupported format. #231
Conversation
roblabla
force-pushed
the
dont-panic-weird-cert
branch
from
8830ed5 to
8787549
Compare
|
Fixed the clippy error. |
| Some(cert) | ||
| } else { | ||
| panic!() | ||
| None |
There was a problem hiding this comment.
I think perhaps we should perhaps change the iterator item type to something like Result<&Certificate, CertificateIterError>. Silently skipping over certificates matching CertificateChoices::Other seems like it could lead to incorrect validation against the certificate chain.
There was a problem hiding this comment.
Well if we're going to be changing the signature of the function, we should probably just be returning the CertificateChoice directly in the iterator?
Silently skipping over certificates matching CertificateChoices::Other seems like it could lead to incorrect validation against the certificate chain.
I mean, the worse that can happen is that we fail to validate the chain due to a missing certificate. Which feels fine? And anyways, I'm pretty sure windows skips over certificates it doesn't know how to handle either - though I'll need to double check that (should be as simple as adding an unused Other certificate in the certificate set).
There was a problem hiding this comment.
Good point about returning CertificateChoice directly, let's do that.
I could be convinced that skipping is the right thing to do if we confirm that Windows does that (and add a note about that behavior in the function's docstring).
2 participants
Currently, if the certificate list is missing or contains unsupported cert format, authenticode-rs will panic when calling the certificates function, which is not ideal.