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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
