Skip to content
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

src/openssl.c: enable check for revocation if CRL is added to store #180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fffonion
Copy link
Contributor

No description provided.

@fffonion fffonion changed the title src/openssl.c: enable check for revocation if CRL is added store src/openssl.c: enable check for revocation if CRL is added to store Feb 26, 2020
@daurnimator
Copy link
Collaborator

Why wouldn't we turn this on all the time?
Or expose it so a user can turn the flag on/off as they want?

@fffonion
Copy link
Contributor Author

fffonion commented Jul 6, 2020

@daurnimator If we set the X509_V_FLAG_CRL_CHECK flag but no CRL is added to the store, the verification will always fail with unable to get certificate CRL and this might break existing application. But it's also true that if some applications relies on the current behaviour that CRL never got checked even if it's added, they will also break, although they shouldn't in the first place.
I agree exposing it will be a sane approach regarding breaking changes, though there'll be more diff. I can make that change it it's okay.

@daurnimator
Copy link
Collaborator

I agree exposing it will be a sane approach regarding breaking changes, though there'll be more diff. I can make that change it it's okay.

would it make sense to expose all the verification flags? X509_V_FLAG_CRL_CHECK/X509_V_FLAG_CRL_CHECK_ALL/X509_V_FLAG_EXTENDED_CRL_SUPPORT/X509_V_FLAG_EXTENDED_CRL_SUPPORT/etc.

@fffonion
Copy link
Contributor Author

fffonion commented Jul 6, 2020

Yeah good point to expose all CRL verification flags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants