HTHou commented on code in PR #3600:
URL: https://github.com/apache/thrift/pull/3600#discussion_r3427460771
##########
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:
Thanks. I kept the `SSL_OP_NO_*` path here for compatibility with the
OpenSSL/LibreSSL versions this code still needs to build against;
`SSL_CTX_set_min_proto_version`/`TLS1_2_VERSION` are clearer where available,
but adding that path would need extra version/provider guards. This change
stays aligned with the existing option-based code and does not set a maximum
protocol version, so TLS 1.3 remains negotiable when the underlying TLS library
supports it.
##########
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:
Updated. The comment now says `Constructor` and clarifies that the factory
is invoked during construction to obtain the custom OpenSSL context.
##########
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:
Updated. Added `custom_ssl_context_factory_validation` coverage for both an
empty `SSLContextFactory` and a factory that returns `nullptr`, including the
expected `TSSLException` messages.
##########
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:
Updated. Added an `SSL_OP_NO_SSLv2` check when that option is observable
(`SSL_OP_NO_SSLv2 != 0`). On newer OpenSSL versions the macro is `0`, so there
is no option bit to assert there.
--
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]
