Use OpenSSL default when certificate location not set

[jp39]

Also, do not disable certificate verification when using default OpenSSL
certificate location. This is a pretty reasonable default setting for
security.

[yhirose]

@jp39, thanks for the pull request, but there are a number of unit test cases failed. (Some tests are failing right now due to the httpbin.org issue, but it seems like this pull request breaks other unit tests...)

[yhirose]

@jp39, any update about the build errors? Thanks!

[jp39]

@yhirose, there are a few tests failing already for me even on the master branch:

[  FAILED  ] 3 tests, listed below:
[  FAILED  ] ChunkedEncodingTest.WithResponseHandlerAndContentReceiver
[  FAILED  ] BaseAuthTest.FromHTTPWatch
[  FAILED  ] DigestAuthTest.FromHTTPWatch

 3 FAILED TESTS

Now, with my patch, I see these failing:

[  FAILED  ] 8 tests, listed below:
[  FAILED  ] ServerTest.GetMethod302Redirect
[  FAILED  ] ServerTest.PostMethod1
[  FAILED  ] ServerTest.PostMethod2
[  FAILED  ] ServerTest.PutMethod3
[  FAILED  ] ServerTest.PostMethod303Redirect
[  FAILED  ] ServerTest.KeepAlive
[  FAILED  ] PayloadMaxLengthTest.ExceedLimit
[  FAILED  ] SSLClientTest.ServerCertificateVerification2

 8 FAILED TESTS

This is very curious because my change is only located in the SSLClient class. So I can't see why it would make the Server fail.

The SSLClientTest.ServerCertificateVerification2 test should probably be removed though because it's asserting that the certificate verfication fails when doing a GET on a google URL.

[yhirose]

@jp39, thanks for the report. But all unit tests in the master branch on CIs (Github actions and AppVeyor) pass successfully. It seems further careful investigation is needed to include the pull request. Thanks for your efforts!

[jp39]

Yeah, I'll take a deeper look into it when I get the chance. Also, I intend to update my PR a little if you agree:

1. What do you think about enabling server certificate verification by default (server_certificate_verification_ to true) ? I suppose we would need to update some of the test case, and some of the examples in the README too.

2. The way I understand what SSL_CTX_set_verify does (but I may be wrong), is that it forces the TLS handshake to fail if the server certificate verification fails, when set to SSL_VERIFY_PEER. But this is redundant with verifying the server certificate manually, as it's currently done in SSLClient::initialize_ssl when we call SSL_get_verify_result. What do you think of using SSL_VERIFY_NONE everywhere in SSLClient::initialize_ssl (unless my understanding of SSL_CTX_set_verify is not correct) ?

[yhirose]

@jp39, thanks for your attention to this issue.

1. What do you think about enabling server certificate verification by default (server_certificate_verification_ to true) ? I suppose we would need to update some of the test case, and some of the examples in the README too.

I basically agree with you. Only thing (but a big thing) is that users now have to call either set_ca_cert_path or `set_ca_cert_store', and they have to modify their existing code to match this new behavior.

To mitigate the migration pain, we may need to use the system default ca-bundle if a user doesn't call set_ca_cert_path or `set_ca_cert_store'. This is the curl's behavior.

2. The way I understand what SSL_CTX_set_verify does (but I may be wrong), is that it forces the TLS handshake to fail if the server certificate verification fails, when set to SSL_VERIFY_PEER. But this is redundant with verifying the server certificate manually, as it's currently done in SSLClient::initialize_ssl when we call SSL_get_verify_result. What do you think of using SSL_VERIFY_NONE everywhere in SSLClient::initialize_ssl (unless my understanding of SSL_CTX_set_verify is not correct) ?

You may be correct. I'll try to test if it works on my Mac as well.

[yhirose]

@jp39, I just tried to implement both you suggested. I confirmed that it works on Mac OS
and Ubuntu in GitHub Actions CI. But it doesn't work on my Windows 10 machine. (I probably need to write some code to get the system ca-bundle properly on Windows. https://stackoverflow.com/questions/9507184/can-openssl-on-windows-use-the-system-certificate-store)

[jp39]

@yhirose Thanks for looking into it.