bakaid commented on a change in pull request #610: MINIFICPP-814 - Fixed
ListenHTTP and HTTPClient bugs, created tests f…
URL: https://github.com/apache/nifi-minifi-cpp/pull/610#discussion_r302964889
##########
File path: bootstrap.sh
##########
@@ -468,13 +468,6 @@ build_cmake_command(){
add_os_flags
- curl -V | grep OpenSSL &> /dev/null
Review comment:
This was a fundamentally flawed way to try to determine the SSL backend used
by curl and make decisions based on this information for the following reasons:
* What SSL backend the command line curl tool on the build machine is built
with, bears practically no relevance to what the curl library that ends up
invoked in the code is built with.
* First, while unlikely, the command line tool on the system might not use
the libcurl on the system that we will link with.
* Second, it is not at all guaranteed that we will link with the system
libcurl - we might build our own with USE_SYSTEM_CURL=OFF.
* Thirdly, even if we do dynamically link the system libcurl, the libcurl
on the target system, where the depencency will be loaded might be built with
an another SSL backend.
Therefore, if we want to determine what SSL backend the libcurl we end up
using has, it must be done at runtime.
While this would be possible - even if not elegant - to do by invoking
`curl_version_info`, I propose that we should not even try to do this at
runtime.
The only purpose we used USE_CURL_NSS for, was to determine whether we
should set the SSL information through curl settings or do it directly by
interacting with the SSL_CTX.
Even if we somehow determined that our curl used OpenSSL, so that we can use
CURLOPT_SSL_CTX_FUNCTION, it is not a good idea to do it.
It is not at all guaranteed that the OpenSSL we are linked with is the same
one curl is built with. And if that's the case, then curl will create the
SSL_CTX with its own OpenSSL version that we will try to manipulate with an
another OpenSSL version, possibly crashing the whole thing.
The only configuration this could work with, is if we are linked with the
system libcurl, system OpenSSL, and the system libcurl is linked with the same
system OpenSSL. While this is a common configuration, it only represents one
configuration out of the many we can be built with.
Additionally, because I see no benefit (in functionality or ease of use), or
certainly not one that would outweigh the risks, to use the SSLContextService's
`configure_ssl_context` to set up curl's SSL instead of extracting the
information from the SSLContextService and passing it properly to curl, and
there are many ways in which we can fail if we try to do it, I have removed
that path from the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
