Jens-G commented on PR #3606: URL: https://github.com/apache/thrift/pull/3606#issuecomment-4845276283
### Code review Found 6 issues (adversarially verified): 1. `SSLContext(SSL_CTX*, false)` stores the raw pointer without calling `SSL_CTX_up_ref`. If the external owner frees the context while this `SSLContext` is alive, the subsequent `SSL_new(ctx_)` call in `createSSL()` uses freed memory. https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L216-L221 Verified: no `SSL_CTX_up_ref` call present in PR code; constructor absent in base. 2. Adding `bool takeOwnership_` to `SSLContext` changes `sizeof(SSLContext)` from 8 to 16 bytes. `SSLContext` is a concrete public class in an installed header; no soname bump or ABI versioning mechanism exists in the project. https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L365-L374 Verified: member absent in base, no SOVERSION in CMakeLists.txt, no `.map`/`.sym` files under `lib/cpp/`. 3. Both new tests exercise `takeOwnership=true` only (`wrapped_ssl_context` passes `true` explicitly; `wrapped_ssl_context_null` throws before the ownership branch). The destructor branch where `SSL_CTX_free` is skipped (`takeOwnership_=false`) has no test. https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/test/SecurityTest.cpp#L286-L306 Verified: `grep -n "takeOwnership\|, false" SecurityTest.cpp` returns no output on PR code; branch absent in base. 4. The new constructor's Doxygen does not warn that callers are responsible for enforcing a minimum TLS version. `SSLContext(SSLProtocol)` applies `SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1` unconditionally; the new constructor wraps the context as-is and the test creates it with `TLS_method()` and no version restrictions. https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L367-L374 Verified: no `SSL_OP_NO_TLSv1*` in new constructor; existing floor confirmed at [`TSSLSocket.cpp:210-212`](https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L210-L212); constructor absent in base. 5. When `takeOwnership=false`, the caller must keep the `SSL_CTX*` alive at least as long as the `SSLContext` and all sockets created from it. This is not documented in the new constructor's Doxygen. The factory class has an explicit parallel warning ([`TSSLSocket.h:200-206`](https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L200-L206): *"It is the responsibility of the code using TSSLSocketFactory to ensure that the factory lifetime exceeds the lifetime of any sockets it might create"*). https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L367-L374 Verified: `@param takeOwnership` documents only the `true` case; factory warning confirmed in base and PR; constructor absent in base. 6. `AGENTS.md` ยง6 and its Quick Reference checklist require TLS configuration changes to be cross-checked against `doc/thrift-threat-model.md` before merging. The PR body contains no mention of this. `doc/thrift-threat-model.md` is present in the repo. (Note: per ยง6, any details about trust-boundary implications belong in the JIRA ticket THRIFT-6073, not in the PR description or commit messages.) Verified: AGENTS.md requirement confirmed; PR body grep for "threat" returns no output; `doc/thrift-threat-model.md` exists at PR head. ๐ค 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]
