Add Option to enable Revocation Checking

[jens-diewald]

I have added an option to enable checking for revoked certificates. Some Remarks:

1. asio::ssl does not provide such functionality. (The asio::ssl::stream does, however, expose the openssl "native handle" and one may set corresponding options in openssl directly. I do not know if this works well though, i have never tried to do it.)
2. The new option is a member function of stream and not of context_certificates as i originally envisioned it. This is only so we can request OCSP stapling during the handshake if revocation checking is enabled. It would, of course, be possible to enable this request via a separate member function if there are concerns against combining all this in the stream member function.
3. The new unit test does not cover OCSP as that would require some server providing the OCSP responses during the test. This may be doable, but would be way more work. I have done some tests on actual websites and everything seems to be working as i expect it to.
4. I have restored part of a mechanism to load certificates from files, that was used in the past and then removed in favour of simply hardcoding the test_certificate in a header file. As i have used several more certificates and CRLs now, i felt keeping them in separate files would be better.
5. As mentioned above, i have generated some more certificates and also CRLs for testing purposes and saved them to tests/test_certificates. I generated these files with a small bash script that i also put in this folder (generate_certs.sh). Bash is maybe not a good choice for a windows specific library, but was most natural to me. I suppose powershell would also be an option if bash is a problem..? If i did everything correct, the Files should all be valid for 100 Years, however. So the script is only helpful as documentation and should only be needed again when the tests are changed.

Some Testcases:

• https://revoked.badssl.com fails with revocation check enabled
• https://www.microsoft.com appears to use OCSP stapling:
  • It still works when we set CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT | CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY for CertGetCertificateChain
  • It does not work anymore if we do the above and do not request OCSP stapling via SCH_CRED_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT during the handshake.

[laudrup]

First of all this looks really, really good. Thanks a lot.

Considering some of your points:

The new option is a member function of stream and not of context_certificates as i originally envisioned it. This is only so we can request OCSP stapling during the handshake if revocation checking is enabled. It would, of course, be possible to enable this request via a separate member function if there are concerns against combining all this in the stream member function.

I might misunderstand but didn't you add a separate member function for the revocation check on the stream class?

I think that is just fine. If you're talking about the implementation detail then I don't care too much for now, but we probably shouldn't make the function do much more in case more functionality is added later on.

The new unit test does not cover OCSP as that would require some server providing the OCSP responses during the test. This may be doable, but would be way more work. I have done some tests on actual websites and everything seems to be working as i expect it to.

Unit tests would indeed be great but I understand that it requires quite a lot of work to implement (which is why I haven't done that yet). If it should be done it should probably be some kind of general way of adding tests similar to what badssl.com is doing in addition to the OCSP tests. That's not needed for this PR but would of course be very much appreciated.

I have restored part of a mechanism to load certificates from files, that was used in the past and then removed in favour of simply hardcoding the test_certificate in a header file. As i have used several more certificates and CRLs now, i felt keeping them in separate files would be better.

More certificates is definitely needed to test this. I think it might make more sense to hardcode them in header files instead of loading them from PEM files at runtime but I definitely appreciate what you have done here so it's not all that important.

As mentioned above, i have generated some more certificates and also CRLs for testing purposes and saved them to tests/test_certificates. I generated these files with a small bash script that i also put in this folder (generate_certs.sh). Bash is maybe not a good choice for a windows specific library, but was most natural to me. I suppose powershell would also be an option if bash is a problem..? If i did everything correct, the Files should all be valid for 100 Years, however. So the script is only helpful as documentation and should only be needed again when the tests are changed.

I definitely don't mind using bash for this. I actually rarely use Window myself and when I do the first thing I do is install a "real" shell anyway. More importantly bash is supported by the pipelines used for the CI tests so it's perfectly fine.

All in all I'll be happy to merge this but I'll give you a chance to address my comments first.

Once more. Great job. I really appreciate it.

[jens-diewald]

I might misunderstand but didn't you add a separate member function for the revocation check on the stream class?

I meant, alternatively we could have one member function of context to en/disable the actual revocation check and a second member function of stream to en/disable the OCSP stapling request. In use cases where you know ahead of time, that the server will not support OCSP stapling, you would not need to request it that way. But the overhead of sending the request is probably not too relevant?

[jens-diewald]

More certificates is definitely needed to test this. I think it might make more sense to hardcode them in header files instead of loading them from PEM files at runtime but I definitely appreciate what you have done here so it's not all that important.

Why do you prefer header files, if i may ask? Separate files have the advantage that they can be inspected by some other program, to check their content. Also it seemed like the easier-to-maintain choice to me, as it skips the step of copying the string from into a header file.

[laudrup]

I might misunderstand but didn't you add a separate member function for the revocation check on the stream class?

I meant, alternatively we could have one member function of context to en/disable the actual revocation check and a second member function of stream to en/disable the OCSP stapling request. In use cases where you know ahead of time, that the server will not support OCSP stapling, you would not need to request it that way. But the overhead of sending the request is probably not too relevant?

It doesn't seem to relevant currently. I assume that can always be added later. Let's keep it like this for now.

[laudrup]

More certificates is definitely needed to test this. I think it might make more sense to hardcode them in header files instead of loading them from PEM files at runtime but I definitely appreciate what you have done here so it's not all that important.

Why do you prefer header files, if i may ask? Separate files have the advantage that they can be inspected by some other program, to check their content. Also it seemed like the easier-to-maintain choice to me, as it skips the step of copying the string from into a header file.

Being able to inspect/use the PEM files is a good point. I just though about the extra coded needed to read the files in the tests instead of just using hardcoded strings. Arguments can be made both ways and the way you have done it is just fine so let's keep it like that.

[laudrup]

Seems like the tests are currently waiting for a windows-2016 image. As far as I can tell, that image doesn't exist anymore.

I've tried to update the CI pipeline to use the newer 2022 image as well as some other improvements including testing more boost versions.

I'll try to get that finished ASAP, merge it to master and then hopefully we can merge your PR as well.

Thanks once again. I really appreciate your help and interest.