Fix assert in MapVerifyErrorToChainStatus from DigiCert CRLs - #39430
Conversation
* Define all of the X509_V_ERR* values form OpenSSL 1.0/1.1 * Exempt X509_V_ERR_DIFFERENT_CRL_SCOPE from being reported, as it is redundant to X509_V_ERR_UNABLE_TO_GET_CRL The remaining new X509_V_ERR* codes will still trip the assert, largely because they should be behind option flags that we don't enable.
|
The test failure is #39381. |
| // which will trigger OCSP, so is ignorable. | ||
| if (errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_OK && | ||
| errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_CRL_NOT_YET_VALID) | ||
| errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_CRL_NOT_YET_VALID && |
There was a problem hiding this comment.
X509_V_ERR_PERMITTED_VIOLATION also seems like something which could be ignored
There was a problem hiding this comment.
That's "A name constraint violation occurred in the permitted subtrees." which might mean "this certificate has name constraints that exceed the bounds of the parent"... which probably isn't something we'd want to ignore.
I don't really want to ignore things until we can see it, and understand that we're mapping it the same as on Windows 😄.
krwq
left a comment
krwq
left a comment
There was a problem hiding this comment.
Change makes sense to me although I have very little context on this subject
DigiCert currently has at least two distinct CRLs running for the same CA (C = US, O = DigiCert Inc, CN = DigiCert SHA2 Secure Server CA) * URI:http://crl3.digicert.com/ssca-sha2-g5.crl * URI:http://crl3.digicert.com/ssca-sha2-g6.crl Using the current cache name strategy, just a hash of the CA name, some of the certs get meaningful revocation, and others do not. Now the file name will be {ca name hash}.{url hash}.crl, enabling the DigiCert scenario. Upgrading across this change will cause CRLs to be re-downloaded, but that's a thing that happens over time anyways.
|
Huh. I thought I had made a new branch for this last commit, and also that I had merged this change already. But, neither of these things is actually true. @stephentoub / @krwq wanna look at the "fix the problem" commit that goes with the earlier "fix the symptom" commit? |
Now both DigitCert CRLs get used (as appropriate). |
GrabYourPitchforks
left a comment
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
I reviewed specifically the commit at 4f3b75e and it LGTM.
5 participants
The remaining new X509_V_ERR* codes will still trip the assert, largely because
they should be behind option flags that we don't enable.
Fixes #39372.