-
Notifications
You must be signed in to change notification settings - Fork 440
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
enable SSL for curl in bazel build environment #2415
Conversation
deps = [ | ||
"@boringssl//:crypto", | ||
"@boringssl//:ssl", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CMake build has a dependency on OpenSSL. Can we have the same dependency to maintain consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into the boringssl documentation (https://github.com/google/boringssl) -
--
Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.
--
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @lalitb , I think we should use openssl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but gRPC also use boringssl, maybe we can use alias to switch the ssl library. (https://bazel.build/docs/configurable-attributes?#faq-select-bind)
bazel/curl.BUILD
Outdated
@@ -34,6 +34,8 @@ cc_library( | |||
]), | |||
copts = CURL_COPTS + [ | |||
"-DOS=\"os\"", | |||
"-DCURL_WITH_MULTI_SSL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the option CURL_WITH_MULTI_SSL
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI build now fails with:
external/curl/lib/vtls/vtls.c:1254:2: error: #error "Missing struct Curl_ssl for selected SSL backend"
It looks like curl needs to be built with USE_OPENSSL
and linked with openssl.
Relevant CURL code:
const struct Curl_ssl *Curl_ssl =
#if defined(CURL_WITH_MULTI_SSL)
&Curl_ssl_multi;
#elif defined(USE_WOLFSSL)
&Curl_ssl_wolfssl;
#elif defined(USE_SECTRANSP)
&Curl_ssl_sectransp;
#elif defined(USE_GNUTLS)
&Curl_ssl_gnutls;
#elif defined(USE_GSKIT)
&Curl_ssl_gskit;
#elif defined(USE_MBEDTLS)
&Curl_ssl_mbedtls;
#elif defined(USE_NSS)
&Curl_ssl_nss;
#elif defined(USE_RUSTLS)
&Curl_ssl_rustls;
#elif defined(USE_OPENSSL)
&Curl_ssl_openssl;
#elif defined(USE_SCHANNEL)
&Curl_ssl_schannel;
#elif defined(USE_BEARSSL)
&Curl_ssl_bearssl;
#else
#error "Missing struct Curl_ssl for selected SSL backend"
#endif
The discussion on this code review is inconclusive, as there is no agreement on which SSL library to use to build CURL, and the topic needs further investigation. Created issue #2489 to document the bug found, and closing this PR. |
No description provided.