Remember make checksrc locally, as then you might pick up some warnings before the CI finds them:

/usr/bin/perl ./checksrc.pl -D. -W./curl_config.h      \
	./*.[ch] ./vauth/*.[ch] ./vtls/*.[ch]
./vtls/openssl.c:2344:81: warning: Longer than 79 columns (LONGLINE)
       if(!SSL_CTX_load_verify_locations(BACKEND->ctx, ssl_cafile, ssl_capath)) {
./vtls/openssl.c:2361:96: warning: Longer than 79 columns (LONGLINE)
       infof(data, "ignoring certificate verify locations due to disabled peer verification\n");

Thanks!

I'm not sure if commenting on this closed pull request is the right way to raise this point.

This commit has resulted in a regression in our application that uses libcurl. The application code assumes that libcurl (with the OpenSSL backend) will perform the server certificate verification even if verifypeer == 0 and that the verification result will be available using CURLINFO_SSL_VERIFYRESULT. The libcurl code (in vtls/openssl.c) appears to be written to support this use case. Here are a couple of code snippets from vtls/openssl.c in support of that statement

Code snippet 1:

  /* SSL always tries to verify the peer, this only says whether it should
   * fail to connect if the verification fails, or if it should continue
   * anyway. In the latter case the result of the verification is checked with
   * SSL_get_verify_result() below. */
  SSL_CTX_set_verify(BACKEND->ctx,
                     verifypeer ? SSL_VERIFY_PEER : SSL_VERIFY_NONE, NULL);

Code snippet 2:

    lerr = *certverifyresult = SSL_get_verify_result(BACKEND->handle);

    if(*certverifyresult != X509_V_OK) {
      if(SSL_CONN_CONFIG(verifypeer)) {
        /* We probably never reach this, because SSL_connect() will fail
           and we return earlier if verifypeer is set? */
        if(strict)
          failf(data, "SSL certificate verify result: %s (%ld)",
                X509_verify_cert_error_string(lerr), lerr);
        result = CURLE_PEER_FAILED_VERIFICATION;
      }
      else
        infof(data, " SSL certificate verify result: %s (%ld),"
              " continuing anyway.\n",
              X509_verify_cert_error_string(lerr), lerr);
    }
    else
      infof(data, " SSL certificate verify ok.\n");
  }

With the change in this commit (to not set the verify location when verifypeer == 0), the CURLINFO_SSL_VERIFYRESULT does not have useful information for the verifypeer == 0 use case. It will always fail with X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY.

My suggestion is to revert this commit. The performance optimization to skip the SSL_CTX_load_verify_locations call can still be achieved if the caller does not set ssl_cafile and ssl_capath.

Would you like me to create a pull request?
Is there a test suite that can be extended to verify this behaviour? I will try to create a test for this scenario if that is desirable and possible.

Ouch. It would be great with a new issue or PR for this, yes!

I'll create a pull request shortly.

Thanks for pointing this out. I agree that it should be reverted.
Can we maybe extend the documentation of CURLOPT_SSL_VERIFYPEER with a note about this potential performance issue and how to work around this on user side? If this is desired, I could prepare a patch for it.

Can we maybe extend the documentation of CURLOPT_SSL_VERIFYPEER with a note about this potential performance issue and how to work around this on user side? If this is desired, I could prepare a patch for it.

Please do @pschlan!