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