Repository navigation
THRIFT-6174: Make it compatible with OpenSSL 4.0 - #3761
sebastianas wants to merge 1 commit into
Conversation
|
Really nice we get so maoy patches for the same problem. Just wonder why we need so many. |
Code reviewFound 2 issues, plus 3 suggestions:
thrift/lib/cpp/test/SecurityTest.cpp Lines 359 to 367 in 38b6e3c thrift/lib/cpp/test/SecurityFromBufferTest.cpp Lines 231 to 238 in 38b6e3c The guard that makes those protocols unreachable: thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp Lines 184 to 197 in 38b6e3c
Expected Lines 33 to 38 in 38b6e3c Suggestions
thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp Lines 804 to 819 in 38b6e3c The same function already handles this exact class of divergence with a version guard, so there is a local precedent to follow: thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp Lines 774 to 780 in 38b6e3c
thrift/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c Lines 283 to 289 in 38b6e3c thrift/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c Lines 708 to 712 in 38b6e3c Retained, version-gated, in the C++ binding: thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp Lines 159 to 172 in 38b6e3c
thrift/lib/cpp/src/thrift/transport/TSSLSocket.h Lines 39 to 47 in 38b6e3c thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp Lines 195 to 200 in 38b6e3c thrift/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c Lines 850 to 857 in 38b6e3c 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Make it compatible with OpenSSL 4.0: - Don't use ERR_remove_state(). It has been an empty stub since OpenSSL 1.1.0 and got finally removed in 4.0- - SSLv3_method(), TLSv1_method(), TLSv1_1_method() and TLSv1_2_method() have been removed. The recommendation is to use TLS_method() instead. LATEST points to TLSv1_2_method which is not going to work. Make SSLTLS and LATEST point to TLS_method() for newer OpenSSL which uses the highest supported protocol version. Adjust the testsuite to only test those two. - The return value of a few functions such as X509_get_subject_name() has been made const. Use the const pointer only for newer OpenSSL. - While at it, pull the libressl related change from apache#3736 Client: cpp,c_glib. Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
I updated my commit based on your suggestions (unless I missed something) and pulled in the change from the reference PR so you have one patch. The testsuite passes now, too. It is not part of the Debian package so I wasn't aware of the fallout. |
Code review (round 2)Re-reviewed at
I re-swept 1. On OpenSSL >= 4.0,
|
| requested | context | options applied |
|---|---|---|
SSLTLS |
TLS_method() |
NO_SSLv2 | NO_SSLv3 | NO_TLSv1 | NO_TLSv1_1 |
LATEST (== TLSv1_2) |
TLS_method() |
none |
So explicitly asking for TLSv1_2 now yields a laxer context than taking the default, and the minimum version falls back to the OpenSSL build/openssl.cnf instead of being set by Thrift. Before this patch that same value meant TLSv1_2_method() — TLS 1.2 exactly. That undoes THRIFT-3165 for that enum value.
c_glib has the identical shape, and there it skips the function's only SSL_CTX_set_options call entirely:
thrift/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c
Lines 877 to 882 in 829c7e5
One line either way — widen the condition, or SSL_CTX_set_min_proto_version(ctx_, TLS1_2_VERSION) in the new arm.
2. The trailing . in Client: cpp,c_glib. silently drops c_glib from the release notes
build/generate-changes.py:303-304 lowercases and strips each token but does not strip punctuation, and unknown tokens are dropped without warning. Run against this exact commit:
'cpp,c_glib.' -> ['C++']
'cpp,c_glib' -> ['C++', 'C glib']
So the c_glib half of the patch never appears under "C glib". Dropping the period fixes it.
Suggestions
3. TLSv1_0/TLSv1_1/TLSv1_2 don't have to be dropped — and #3752 already keeps them. TLS_method() + SSL_CTX_set_min_proto_version()/set_max_proto_version() reproduce the old per-version methods exactly, which is also OpenSSL's own documented migration. That's the substantive difference between the two PRs and would settle the "one patch" question — it also fixes issue 1 structurally rather than by widening a predicate.
4. The SSLTLS arm didn't need duplicating. SSLv23_method is not removed in 4.0 — it is an unconditional #define SSLv23_method TLS_method with no deprecation guard, as the PR description itself notes. Gating only the four legacy else if branches would leave one shared if (protocol == SSLTLS) head, halve the diff, keep each #if region brace-balanced, and remove the second copy of the predicate that drifted from line 214 in the first place. Same for thrift_ssl_socket.c:845.
5. The behaviour change is untested. The new guard only disables tests: the four surviving cells are {SSLTLS, LATEST}², all expected true, so nothing asserts that SSLv3/TLSv1_0/TLSv1_1 now raise TSSLException("SSL_CTX_new: Unknown protocol"), and the floor regression in issue 1 is invisible to it. SecurityTest.cpp already has the right home next to default_ssl_context_options / custom_ssl_context_options. Worth noting no CI leg compiles any >= 0x40000000L branch, so this all rests on your local build.
6. Enum docs still advertise the dropped protocols — TSSLSocket.h:39-46 and thrift_ssl_socket.h:98-106. TLSv1_2 = 5, // Supports TLSv1_2 or later. also becomes actively wrong per issue 1.
7. Attribution for the #3736 hunk. The two && !defined(LIBRESSL_VERSION_NUMBER) changes are byte-identical to @brad0's still-open #3736. Merging this makes that PR unmergeable while his name never enters history — CONTRIBUTING.md endorses Co-Authored-By: for exactly this. (The fix itself is correct.) Note the retained comment "Do nothing unless an openssl derivative is detected" now sits under a condition that excludes LibreSSL, itself a derivative.
8. Whitespace. Both new test blocks use hard tabs in files that are otherwise entirely space-indented, and in SecurityTest.cpp the if is tab-indented while its own braces and body are space-indented. .clang-format sets UseTab: Never; make style runs it over lib/cpp/test/*.cpp, but no CI job does, so it will land as committed.
9. Out of scope, worth a follow-up ticket: the D binding still calls ERR_remove_state(0) ungated at lib/d/src/thrift/transport/ssl.d:139 and lib/d/src/thrift/async/ssl.d:216, plus ASN1_STRING_data at lib/d/src/thrift/internal/ssl.d:113 — both removed in 4.0. (ssl.d:487 is correctly guarded.) Correctly outside Client: cpp,c_glib, but it means lib/d won't link against libcrypto 4.0.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
Builds fine on OpenBSD (LibreSSL). |
|
Any news here? |
|
For reference (I don't want to open a third PR for this topic), here is the patch I've developed and will propose for Ubuntu (this is against 0.23): --- a/lib/cpp/src/thrift/transport/TSSLSocket.cpp
+++ b/lib/cpp/src/thrift/transport/TSSLSocket.cpp
@@ -157,7 +157,7 @@
#endif
EVP_cleanup();
CRYPTO_cleanup_all_ex_data();
-#if OPENSSL_VERSION_NUMBER >= 0x10100000
+#if OPENSSL_VERSION_NUMBER >= 0x10100000 && !defined(LIBRESSL_VERSION_NUMBER)
// Do nothing unless an openssl derivative is detected
# if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC)
// https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_thread_stop.html
@@ -180,6 +180,37 @@
// SSLContext implementation
SSLContext::SSLContext(const SSLProtocol& protocol) {
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
+ int version = 0;
+ switch (protocol) {
+ case SSLTLS:
+ break;
+#if !defined(OPENSSL_NO_SSL3) && OPENSSL_VERSION_NUMBER < 0x40000000L
+ case SSLv3:
+ version = SSL3_VERSION;
+ break;
+#endif
+ case TLSv1_0:
+ version = TLS1_VERSION;
+ break;
+ case TLSv1_1:
+ version = TLS1_1_VERSION;
+ break;
+ case TLSv1_2:
+ version = TLS1_2_VERSION;
+ break;
+ default:
+ throw TSSLException("SSL_CTX_new: Unknown protocol");
+ }
+
+ ctx_ = SSL_CTX_new(TLS_method());
+ if (ctx_ != nullptr && version != 0
+ && (SSL_CTX_set_min_proto_version(ctx_, version) != 1
+ || SSL_CTX_set_max_proto_version(ctx_, version) != 1)) {
+ SSL_CTX_free(ctx_);
+ ctx_ = nullptr;
+ }
+#else
if (protocol == SSLTLS) {
ctx_ = SSL_CTX_new(SSLv23_method());
#ifndef OPENSSL_NO_SSL3
@@ -196,6 +227,7 @@
/// UNKNOWN PROTOCOL!
throw TSSLException("SSL_CTX_new: Unknown protocol");
}
+#endif
if (ctx_ == nullptr) {
string errors;
@@ -398,7 +430,7 @@
SSL_free(ssl_);
ssl_ = nullptr;
handshakeCompleted_ = false;
-#if OPENSSL_VERSION_NUMBER >= 0x10100000
+#if OPENSSL_VERSION_NUMBER >= 0x10100000 && !defined(LIBRESSL_VERSION_NUMBER)
// Do nothing unless an openssl derivative is detected
# if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC)
// https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_thread_stop.html
@@ -763,7 +795,11 @@
if (name == nullptr) {
continue;
}
- char* data = (char*)ASN1_STRING_data(name->d.ia5);
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
+ const char* data = reinterpret_cast<const char*>(ASN1_STRING_get0_data(name->d.ia5));
+#else
+ const char* data = reinterpret_cast<const char*>(ASN1_STRING_data(name->d.ia5));
+#endif
int length = ASN1_STRING_length(name->d.ia5);
switch (name->type) {
case GEN_DNS:
@@ -789,9 +825,17 @@
}
// extract commonName
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
+ const X509_NAME* name = X509_get_subject_name(cert);
+#else
X509_NAME* name = X509_get_subject_name(cert);
+#endif
if (name != nullptr) {
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
+ const X509_NAME_ENTRY* entry;
+#else
X509_NAME_ENTRY* entry;
+#endif
unsigned char* utf8;
int last = -1;
while (decision == AccessManager::SKIP) {
@@ -801,7 +845,11 @@
entry = X509_NAME_get_entry(name, last);
if (entry == nullptr)
continue;
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L
+ const ASN1_STRING* common = X509_NAME_ENTRY_get_data(entry);
+#else
ASN1_STRING* common = X509_NAME_ENTRY_get_data(entry);
+#endif
int size = ASN1_STRING_to_UTF8(&utf8, common);
if (host.empty()) {
host = (server() ? getPeerHost() : getHost());
--- a/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c
+++ b/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c
@@ -285,7 +285,9 @@
SSL_shutdown(ssl_socket->ssl);
SSL_free(ssl_socket->ssl);
ssl_socket->ssl = NULL;
+#if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
ERR_remove_state(0);
+#endif
}
return thrift_socket_close(transport, error);
}
@@ -705,7 +707,9 @@
ERR_free_strings();
EVP_cleanup();
CRYPTO_cleanup_all_ex_data();
+#if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
ERR_remove_state(0);
+#endif
}
@@ -826,6 +830,40 @@
thrift_ssl_socket_context_initialize(ThriftSSLSocketProtocol ssl_protocol, GError **error)
{
SSL_CTX* context = NULL;
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
+ int version = 0;
+ switch(ssl_protocol){
+ case SSLTLS:
+ break;
+#if !defined(OPENSSL_NO_SSL3) && OPENSSL_VERSION_NUMBER < 0x40000000L
+ case SSLv3:
+ version = SSL3_VERSION;
+ break;
+#endif
+ case TLSv1_0:
+ version = TLS1_VERSION;
+ break;
+ case TLSv1_1:
+ version = TLS1_1_VERSION;
+ break;
+ case TLSv1_2:
+ version = TLS1_2_VERSION;
+ break;
+ default:
+ g_set_error (error, THRIFT_TRANSPORT_ERROR,
+ THRIFT_SSL_SOCKET_ERROR_CIPHER_NOT_AVAILABLE,
+ "The SSL protocol is unknown for %d", ssl_protocol);
+ return NULL;
+ }
+
+ context = SSL_CTX_new(TLS_method());
+ if (context != NULL && version != 0
+ && (SSL_CTX_set_min_proto_version(context, version) != 1
+ || SSL_CTX_set_max_proto_version(context, version) != 1)) {
+ SSL_CTX_free(context);
+ context = NULL;
+ }
+#else
switch(ssl_protocol){
case SSLTLS:
context = SSL_CTX_new(SSLv23_method());
@@ -851,6 +889,7 @@
return NULL;
break;
}
+#endif
if (context == NULL) {
thrift_ssl_socket_get_error((const guchar*)"No cipher overlay", THRIFT_SSL_SOCKET_ERROR_CIPHER_NOT_AVAILABLE, error);
--- a/lib/cpp/test/SecurityFromBufferTest.cpp
+++ b/lib/cpp/test/SecurityFromBufferTest.cpp
@@ -229,7 +229,7 @@
continue;
}
-#ifdef OPENSSL_NO_SSL3
+#if defined(OPENSSL_NO_SSL3) || OPENSSL_VERSION_NUMBER >= 0x40000000L
if (si == 2 || ci == 2) {
// Skip all SSLv3 cases - protocol not supported
continue;
--- a/lib/cpp/test/SecurityTest.cpp
+++ b/lib/cpp/test/SecurityTest.cpp
@@ -254,7 +254,7 @@
continue;
}
-#ifdef OPENSSL_NO_SSL3
+#if defined(OPENSSL_NO_SSL3) || OPENSSL_VERSION_NUMBER >= 0x40000000L
if (si == 2 || ci == 2)
{
// Skip all SSLv3 cases - protocol not supportedI believe it incorporates the feedback on this PR. |
Nice, but a patch in a comment is worse than another PR. It has no CI and it is entirely unclear,. who is going to extract it from there into sth useful. So bottom line right now: We have 3 (or 4) patches from different people but neither one can be merged. |
|
Superseded by #3850, credits retained. |
Make it compatible with OpenSSL 4.0:
Remove ERR_remove_state(). It has been an empty stub since OpenSSL 1.1.0. Given that it was doing nothingfor so long, there is no need to preserve anything for the earlier versions.
SSLv3_method(), TLSv1_method(), TLSv1_1_method() and TLSv1_2_method() have been removed. The recommendation is to use TLS_method() instead. SSLv23_method() is an alias for TLS_method(). It is possible to allow a different TLS version by using SSL_CTX_set_min_proto_version()/ SSL_CTX_set_max_proto_version() but be aware that TLSv1.2 is usually the lower supported version and TLSv1.3 should be the default.
The return value of a few functions such as X509_get_subject_name() has been made const. The return value should not be modified before that change.
Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
Now, I guess this is trivial.
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
Yes
If your change does not involve any code, include
[skip ci]anywhere in the commit message to free up build resources.