Copilot commented on code in PR #3600:
URL: https://github.com/apache/thrift/pull/3600#discussion_r3427423843
##########
lib/cpp/src/thrift/transport/TSSLSocket.cpp:
##########
@@ -204,11 +204,12 @@ SSLContext::SSLContext(const SSLProtocol& protocol) {
}
SSL_CTX_set_mode(ctx_, SSL_MODE_AUTO_RETRY);
- // Disable horribly insecure SSLv2 and SSLv3 protocols but allow a handshake
- // with older clients so they get a graceful denial.
+ // Keep version-flexible negotiation for current protocol versions while
setting
+ // the default protocol floor at TLSv1.2.
if (protocol == SSLTLS) {
- SSL_CTX_set_options(ctx_, SSL_OP_NO_SSLv2);
- SSL_CTX_set_options(ctx_, SSL_OP_NO_SSLv3); // THRIFT-3164
+ SSL_CTX_set_options(ctx_,
+ SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1
+ | SSL_OP_NO_TLSv1_1);
}
Review Comment:
Disabling TLS versions via `SSL_OP_NO_*` works, but OpenSSL provides a more
direct and future-proof way to enforce a minimum protocol version (e.g.,
`SSL_CTX_set_min_proto_version(..., TLS1_2_VERSION)` when available). Consider
setting the min protocol version in addition to (or instead of)
`SSL_OP_NO_TLSv1`/`SSL_OP_NO_TLSv1_1`; this also makes the intended 'floor'
behavior clearer and less dependent on option semantics across OpenSSL variants.
##########
lib/cpp/src/thrift/transport/TSSLSocket.h:
##########
@@ -210,6 +212,12 @@ class TSSLSocketFactory {
* @param protocol The SSL/TLS protocol to use.
*/
TSSLSocketFactory(SSLProtocol protocol = SSLTLS);
+ /**
+ * Constructor/Destructor
+ *
+ * @param contextFactory Function returning a custom OpenSSL context to use.
+ */
+ TSSLSocketFactory(const SSLContextFactory& contextFactory);
Review Comment:
The docstring section title is inaccurate: this block documents a
constructor only (not a destructor). Updating it to \"Constructor\" (and
optionally clarifying that the factory is invoked during construction to obtain
the context) would make the API contract clearer.
##########
lib/cpp/src/thrift/transport/TSSLSocket.cpp:
##########
@@ -884,6 +885,33 @@ bool TSSLSocketFactory::manualOpenSSLInitialization_ =
false;
bool TSSLSocketFactory::didWeInitializeOpenSSL_ = false;
TSSLSocketFactory::TSSLSocketFactory(SSLProtocol protocol) : server_(false) {
+ initializeOpenSSLState();
+ try {
+ ctx_ = std::make_shared<SSLContext>(protocol);
+ } catch (...) {
+ cleanupOpenSSLState();
+ throw;
+ }
+}
+
+TSSLSocketFactory::TSSLSocketFactory(const SSLContextFactory& contextFactory)
: server_(false) {
+ if (!contextFactory) {
+ throw TSSLException("SSLContextFactory must not be empty");
+ }
+ initializeOpenSSLState();
+ try {
+ std::shared_ptr<SSLContext> ctx = contextFactory();
+ if (ctx == nullptr) {
+ throw TSSLException("SSLContextFactory must not return null");
+ }
+ ctx_ = ctx;
+ } catch (...) {
+ cleanupOpenSSLState();
+ throw;
+ }
+}
Review Comment:
The new validation branches (`contextFactory` empty and factory returning
null) introduce observable behavior (specific `TSSLException` messages) but
aren't covered by tests in the updated suite. Add unit tests that assert the
constructor throws for an empty `SSLContextFactory` and for a factory that
returns `nullptr` to prevent regressions.
##########
lib/cpp/test/SecurityTest.cpp:
##########
@@ -226,6 +226,38 @@ struct SecurityFixture
BOOST_FIXTURE_TEST_SUITE(BOOST_TEST_MODULE, SecurityFixture)
+BOOST_AUTO_TEST_CASE(default_ssl_context_options)
+{
+ apache::thrift::transport::SSLContext context;
+ const auto options = SSL_CTX_get_options(context.get());
+
+ BOOST_CHECK((options & SSL_OP_NO_SSLv3) != 0);
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1) != 0);
+ BOOST_CHECK((options & SSL_OP_NO_TLSv1_1) != 0);
+}
Review Comment:
This test validates several default-disabled protocols, but it doesn't
validate `SSL_OP_NO_SSLv2`, which is also set for `SSLTLS` in `SSLContext`.
Adding an assertion for `SSL_OP_NO_SSLv2` would more fully lock down the
intended default options.
--
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]
