-
Notifications
You must be signed in to change notification settings - Fork 811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow overriding CRL chain errors early so CRL chain processing will continue. #7501
base: master
Are you sure you want to change the base?
Conversation
src/crl.c
Outdated
@@ -512,9 +512,11 @@ int CheckCertCRL_ex(WOLFSSL_CRL* crl, byte* issuerHash, byte* serial, | |||
#endif | |||
if (foundEntry == 0) { | |||
WOLFSSL_MSG("Couldn't find CRL for status check"); | |||
#ifndef WOLFSSL_ALLOW_MISSING_CRL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this only be an option for root/intermediates but not the leaf cert? What will ret
be in the missing case? Please add inline code comment for purpose of this new macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the opposite, the customer wants it on the leaf and intermediates, not the root. The intention of the workaround is to continue parsing and verifying certs and CRLs up the chain even if a CRL is missing.
In the missing case, ret will be 0 as CheckCertCRLList would not have found any CRLs, so it wouldn't modify ret from the default value of 0. But depending on the build flags to wolfSSL, ret may be overwritten by crlIoCb.
I've added an inline comment, please let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to limit this to just the root? I am concerned there is not differentiation between the type of cert. I want to make sure the leaf cert still has the CRL missing check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again the customer wants to skip the check on the leaf and intermediates. They want to continue parsing even after a CRL_MISSING error so they can catch invalid certs or CRLs further up the chain. So if anything we should limit it to the leaf and intermediates.
This function does not have context on what point in the chain we are at, the workaround would need to be moved to a higher level function which is calling this ie. ProcessPeerCertsChainCRLCheck. I can move this, just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please review and see if it's possible to move it up the stack and do it where the type of cert is known.... I.E. root ca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the test_wolfSSL_X509_STORE_set_get_crl
test at #7445 I ignore CRL missing errors with the verification callback. It continues parsing the cert chain so the customer should be able to use the verify callback. Have they tried that?
src/crl.c
Outdated
@@ -512,9 +512,11 @@ int CheckCertCRL_ex(WOLFSSL_CRL* crl, byte* issuerHash, byte* serial, | |||
#endif | |||
if (foundEntry == 0) { | |||
WOLFSSL_MSG("Couldn't find CRL for status check"); | |||
#ifndef WOLFSSL_ALLOW_MISSING_CRL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to limit this to just the root? I am concerned there is not differentiation between the type of cert. I want to make sure the leaf cert still has the CRL missing check.
Retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to find a way to not call the verify callback twice for the same failure. If ProcessPeerCertsChainCRLCheck
fails and DoVerifyCallback
doesn't override the error the verify callback will be called twice. The easiest way would be to perform the ProcessPeerCertsChainCRLCheck
call after DoVerifyCallback
in ProcessPeerCerts
.
I tried moving the check to the end of the function, but this lead to test failures, and doesn't seem scalable if we decide to call the verify callback in more spots in the future. |
Description
Fixes zd#17844
Testing
Customer confirmed fix
Checklist