You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "sebastianas (via GitHub)" <gi...@apache.org> on 2026/08/30 08:23:18 UTC
[PR] transport: Make it compatible with OpenSSL 4.0 [thrift]
sebastianas opened a new pull request, #3761:
URL: https://github.com/apache/thrift/pull/3761
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.
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
Now, I guess this is trivial.
- [X] Did you squash your changes to a single commit? (not required, but preferred)
- [X] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
Yes
- [X] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6170: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5468794154
I want to add that there is an upcoming conflict between #3736 and #3761.
Therefore I would like to suggest that you both discuss this and come up with **one** patch.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6170: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "sebastianas (via GitHub)" <gi...@apache.org>.
sebastianas commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5469516908
> I want to add that there is an upcoming conflict between #3736 and #3761. Therefore I would like to suggest that you both discuss this and come up with **one** patch.
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "loqs (via GitHub)" <gi...@apache.org>.
loqs commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5575971637
@FinnRG this patch can be applied to either 0.23 after cherry-picking d586077a37e14446cf385893608ea9e60a59d083 and e94992274272defbd1990037f3510dbbeefd279f and or the current HEAD with this PR (it should be equivalent to yours with tests added for the C code to match the C++ code and matching the C code to match this PR):
```patch
diff --git 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
index b734ec914..4987018b2 100644
--- 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
@@ -833,12 +833,25 @@ thrift_ssl_socket_context_initialize(ThriftSSLSocketProtocol ssl_protocol, GErro
case SSLTLS:
context = SSL_CTX_new(SSLv23_method());
break;
-#if OPENSSL_VERSION_NUMBER < 0x40000000L
-#ifndef OPENSSL_NO_SSL3
+#if !defined(OPENSSL_NO_SSL3) && OPENSSL_VERSION_NUMBER < 0x40000000L
case SSLv3:
context = SSL_CTX_new(SSLv3_method());
break;
#endif
+#if OPENSSL_VERSION_NUMBER >= 0x40000000L
+ case TLSv1_0:
+ case TLSv1_1:
+ case TLSv1_2:
+ context = SSL_CTX_new(TLS_method());
+ if (context != NULL) {
+ int ver = (ssl_protocol == TLSv1_0) ? TLS1_VERSION
+ : (ssl_protocol == TLSv1_1) ? TLS1_1_VERSION
+ : TLS1_2_VERSION;
+ SSL_CTX_set_min_proto_version(context, ver);
+ SSL_CTX_set_max_proto_version(context, ver);
+ }
+ break;
+#else
case TLSv1_0:
context = SSL_CTX_new(TLSv1_method());
break;
@@ -848,7 +861,7 @@ thrift_ssl_socket_context_initialize(ThriftSSLSocketProtocol ssl_protocol, GErro
case TLSv1_2:
context = SSL_CTX_new(TLSv1_2_method());
break;
-#endif /* OPENSSL_VERSION_NUMBER < 0x40000000L */
+#endif
default:
g_set_error (error, THRIFT_TRANSPORT_ERROR,
THRIFT_SSL_SOCKET_ERROR_CIPHER_NOT_AVAILABLE,
diff --git a/lib/c_glib/test/CMakeLists.txt b/lib/c_glib/test/CMakeLists.txt
index 4f60473b2..205390389 100644
--- a/lib/c_glib/test/CMakeLists.txt
+++ b/lib/c_glib/test/CMakeLists.txt
@@ -68,6 +68,12 @@ add_executable(testtransportsocket testtransportsocket.c)
target_link_libraries(testtransportsocket thrift_c_glib)
add_test(NAME testtransportsocket COMMAND testtransportsocket)
+if(OPENSSL_FOUND AND WITH_OPENSSL)
+ add_executable(testtransportsslsocket testtransportsslsocket.c)
+ target_link_libraries(testtransportsslsocket thrift_c_glib)
+ add_test(NAME testtransportsslsocket COMMAND testtransportsslsocket)
+endif()
+
add_executable(testbinaryprotocol testbinaryprotocol.c)
target_link_libraries(testbinaryprotocol thrift_c_glib)
add_test(NAME testbinaryprotocol COMMAND testbinaryprotocol)
diff --git a/lib/c_glib/test/testtransportsslsocket.c b/lib/c_glib/test/testtransportsslsocket.c
index ba9ffdcae..ec0b86e2a 100644
--- a/lib/c_glib/test/testtransportsslsocket.c
+++ b/lib/c_glib/test/testtransportsslsocket.c
@@ -514,6 +514,65 @@ thrift_socket_server (const int port)
g_object_unref (client);
}
+static void
+test_ssl_context_for_ssl_tls(void)
+{
+ GError *error = NULL;
+ SSL_CTX *ctx = thrift_ssl_socket_context_initialize(SSLTLS, &error);
+ g_assert (ctx != NULL);
+ g_assert_no_error (error);
+ SSL_CTX_free(ctx);
+}
+
+static void
+test_ssl_context_for_sslv3(void)
+{
+ GError *error = NULL;
+ SSL_CTX *ctx = thrift_ssl_socket_context_initialize(SSLv3, &error);
+#if defined(OPENSSL_NO_SSL3) || OPENSSL_VERSION_NUMBER >= 0x40000000L
+ if (ctx == NULL) {
+ g_assert (error != NULL);
+ g_clear_error (&error);
+ } else {
+ SSL_CTX_free(ctx);
+ }
+#else
+ g_assert (ctx != NULL);
+ g_assert_no_error (error);
+ SSL_CTX_free(ctx);
+#endif
+}
+
+static void
+test_ssl_context_for_tlsv1_0(void)
+{
+ GError *error = NULL;
+ SSL_CTX *ctx = thrift_ssl_socket_context_initialize(TLSv1_0, &error);
+ g_assert (ctx != NULL);
+ g_assert_no_error (error);
+ SSL_CTX_free(ctx);
+}
+
+static void
+test_ssl_context_for_tlsv1_1(void)
+{
+ GError *error = NULL;
+ SSL_CTX *ctx = thrift_ssl_socket_context_initialize(TLSv1_1, &error);
+ g_assert (ctx != NULL);
+ g_assert_no_error (error);
+ SSL_CTX_free(ctx);
+}
+
+static void
+test_ssl_context_for_tlsv1_2(void)
+{
+ GError *error = NULL;
+ SSL_CTX *ctx = thrift_ssl_socket_context_initialize(TLSv1_2, &error);
+ g_assert (ctx != NULL);
+ g_assert_no_error (error);
+ SSL_CTX_free(ctx);
+}
+
int
main(int argc, char *argv[])
{
@@ -530,6 +591,11 @@ main(int argc, char *argv[])
g_test_add_func ("/testtransportsslsocket/CreateAndSetProperties", test_ssl_create_and_set_properties);
g_test_add_func ("/testtransportsslsocket/OpenAndCloseNonSSLServer", test_ssl_open_and_close_non_ssl_server);
g_test_add_func ("/testtransportsslsocket/OpenAndWriteInvalidSocket", test_ssl_write_invalid_socket);
+ g_test_add_func ("/testtransportsslsocket/ContextForSSLTLS", test_ssl_context_for_ssl_tls);
+ g_test_add_func ("/testtransportsslsocket/ContextForSSLv3", test_ssl_context_for_sslv3);
+ g_test_add_func ("/testtransportsslsocket/ContextForTLSv1_0", test_ssl_context_for_tlsv1_0);
+ g_test_add_func ("/testtransportsslsocket/ContextForTLSv1_1", test_ssl_context_for_tlsv1_1);
+ g_test_add_func ("/testtransportsslsocket/ContextForTLSv1_2", test_ssl_context_for_tlsv1_2);
diff --git a/lib/cpp/test/SecurityFromBufferTest.cpp b/lib/cpp/test/SecurityFromBufferTest.cpp
index 08f76b3f2..3df15106e 100644
--- a/lib/cpp/test/SecurityFromBufferTest.cpp
+++ b/lib/cpp/test/SecurityFromBufferTest.cpp
@@ -229,7 +229,7 @@ BOOST_AUTO_TEST_CASE(ssl_security_matrix) {
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;
diff --git a/lib/cpp/test/SecurityTest.cpp b/lib/cpp/test/SecurityTest.cpp
index 86640bd68..b133ef0f8 100644
--- a/lib/cpp/test/SecurityTest.cpp
+++ b/lib/cpp/test/SecurityTest.cpp
@@ -357,7 +357,7 @@ BOOST_AUTO_TEST_CASE(ssl_security_matrix)
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
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6170: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5468683255
### Code review
Found 2 issues, plus 3 suggestions:
1. The C++ SSL test suite is not updated, so it breaks on OpenSSL 4.0 — the very target of this PR ([AGENTS.md](https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/AGENTS.md#L49-L51) says "Provide tests for every submitted change")
`SecurityTest.cpp` and `SecurityFromBufferTest.cpp` iterate the whole `SSLTLS..LATEST` protocol matrix and skip SSLv3 only under `#ifdef OPENSSL_NO_SSL3`, which is not defined on OpenSSL 4.0; the `TLSv1_0`/`TLSv1_1`/`TLSv1_2` rows have no guard at all. Once `SSLContext::SSLContext` compiles those branches out, every one of those rows reaches `throw TSSLException("SSL_CTX_new: Unknown protocol")`. loqs reproduced exactly this on the parallel #3752 and posted a fix there (`#if defined(OPENSSL_NO_SSL3) || OPENSSL_VERSION_NUMBER >= 0x40000000L`): https://github.com/apache/thrift/pull/3752#issuecomment-5445880876
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/test/SecurityTest.cpp#L359-L367
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/test/SecurityFromBufferTest.cpp#L231-L238
The guard that makes those protocols unreachable:
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L184-L197
2. Commit message is missing the `Client:` trailer ([AGENTS.md](https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/AGENTS.md#L33-L38) says "Commit message includes affected `Client:` languages")
Expected `Client: cpp,c_glib`. The PR title carries `THRIFT-6170:` correctly, but a squash-merge takes the body from the commit message, which ends at `Signed-off-by:` with no `Client:` line.
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/AGENTS.md#L33-L38
#### Suggestions
3. The new `const` qualifiers break the build against OpenSSL < 1.1.0. `name`, `entry` and `common` are passed to `X509_NAME_get_index_by_NID()` (L811), `X509_NAME_get_entry()` (L814) and `ASN1_STRING_to_UTF8()` (L818), all of which took non-const parameters before 1.1.0 — a hard compile error there. CI only builds against OpenSSL 3.x, so this would not show up in the checks.
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L804-L819
The same function already handles this exact class of divergence with a version guard, so there is a local precedent to follow:
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L774-L780
4. `ERR_remove_state(0)` is removed unconditionally in c_glib. "Empty stub since OpenSSL 1.1.0" holds for mainline >= 1.1.0, but not for 1.0.x, and not for LibreSSL, which reports `OPENSSL_VERSION_NUMBER` as `0x20000000L` yet ships a working implementation. c_glib has no `OPENSSL_thread_stop()` anywhere, so both bindings end up inconsistent: the C++ side of this same PR keeps the call under a version guard.
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c#L283-L289
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c#L708-L712
Retained, version-gated, in the C++ binding:
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L159-L172
5. On OpenSSL >= 4.0, explicitly requesting `SSLv3`/`TLSv1_0`/`TLSv1_1`/`TLSv1_2` — and therefore `LATEST`, which is `TLSv1_2` — falls through to the catch-all and reports "Unknown protocol", with no remap to `TLS_method()` + `SSL_CTX_set_min_proto_version()`/`SSL_CTX_set_max_proto_version()` as the commit message itself suggests. The default `SSLTLS` is unaffected, but the enum values stay declared and documented as supported.
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/src/thrift/transport/TSSLSocket.h#L39-L47
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L195-L200
https://github.com/apache/thrift/blob/38b6e3c066d7de057ab9e0448921d688b20db326/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c#L850-L857
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G closed pull request #3761: THRIFT-6174: Make it compatible with OpenSSL 4.0
URL: https://github.com/apache/thrift/pull/3761
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "FinnRG (via GitHub)" <gi...@apache.org>.
FinnRG commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5575383850
For reference (I don't want to open a third PR for this topic), here is the patch I've developed and proposing for Ubuntu (this is against 0.23):
```diff
--- 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 supported
```
I believe it incorporates the feedback on this PR.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5649525334
Superseded by #3850, credits retained.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5648898052
> 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):
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "brad0 (via GitHub)" <gi...@apache.org>.
brad0 commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5502085640
Builds fine on OpenBSD.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6174: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5566590382
Any news here?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] transport: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5468152957
Really nice we get so maoy patches for the same problem. Just wonder why we need so many.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6170: Make it compatible with OpenSSL 4.0 [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3761:
URL: https://github.com/apache/thrift/pull/3761#issuecomment-5500048576
### Code review (round 2)
Re-reviewed at `829c7e5`. Thanks for turning this around so quickly — four of the five points from the last round are addressed:
- ✅ **Test suite** — both matrix tests now skip non-`SSLTLS`/`LATEST` rows under OpenSSL ≥ 4.0.
- ✅ **`Client:` trailer** — present (but see issue 2).
- ✅ **`const` on old OpenSSL** — gated at `>= 0x40000000L`, so < 1.1.0 keeps the non-const types.
- ✅ **c_glib `ERR_remove_state`** — gated `< 0x40000000L`; LibreSSL (`0x20000000L`) keeps it.
- ⚠️ **Dropped protocols** — `SSLTLS`/`LATEST` work; `SSLv3`/`TLSv1_0`/`TLSv1_1` still throw. See suggestion 3.
I re-swept `lib/`, `test/`, `tutorial/` and `contrib/` for every symbol 4.0 removes — all remaining occurrences sit inside one of the new gated `#else` arms, nothing was missed. c_glib's own SSL test only ever passes `SSLTLS`, so it needed no change.
#### 1. On OpenSSL >= 4.0, `LATEST`/`TLSv1_2` ends up with no protocol floor — looser than the default `SSLTLS`
`LATEST` is an alias for `TLSv1_2` (`TSSLSocket.h:46`), so the new arm routes value `5` to `TLS_method()`. But the floor block below is still gated on `protocol == SSLTLS` alone:
https://github.com/apache/thrift/blob/829c7e59001247e5e60d82316f56e81f6ca606ff/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L212-L218
| 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:
https://github.com/apache/thrift/blob/829c7e59001247e5e60d82316f56e81f6ca606ff/lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c#L877-L882
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](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org