Skip to content

THRIFT-3165: Disable TLSv1.0 and TLSv1.1 by default #3600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HTHou wants to merge 1 commit into
base: master
Choose a base branch
from
+135 −14
Open

THRIFT-3165: Disable TLSv1.0 and TLSv1.1 by default #3600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/cpp/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ TSimpleServer, TThreadedServer, and TThreadPoolServer.
There are two main classes TSSLSocketFactory and TSSLSocket. Instances of
TSSLSocket are always created from TSSLSocketFactory.

The default TSSLSocketFactory context uses OpenSSL's version-flexible TLS
method and sets TLS 1.2 as the minimum negotiated protocol version. Applications
that need a different protocol range can provide a custom SSLContext factory and
adjust the OpenSSL context options before creating sockets.

## How to use SSL APIs

See the TestClient.cpp and TestServer.cpp files for examples.
Expand Down Expand Up @@ -319,4 +324,3 @@ assertion or a core instead of undefined behavior. The lifetime of a TSSLSocket
up too early. If the static boolean is set to disable openssl initialization and
cleanup and leave it up to the consuming application, this requirement is not needed.
(THRIFT-4164)

45 changes: 39 additions & 6 deletions lib/cpp/src/thrift/transport/TSSLSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment thread
HTHou marked this conversation as resolved.
}

Expand Down Expand Up @@ -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;
}
}
Comment thread
HTHou marked this conversation as resolved.

void TSSLSocketFactory::initializeOpenSSLState() {
Guard guard(mutex_);
if (count_ == 0) {
if (!manualOpenSSLInitialization_) {
Expand All @@ -893,13 +921,18 @@ TSSLSocketFactory::TSSLSocketFactory(SSLProtocol protocol) : server_(false) {
randomize();
}
count_++;
ctx_ = std::make_shared<SSLContext>(protocol);
}

TSSLSocketFactory::~TSSLSocketFactory() {
cleanupOpenSSLState();
}

void TSSLSocketFactory::cleanupOpenSSLState() {
Guard guard(mutex_);
ctx_.reset();
count_--;
if (count_ > 0) {
count_--;
}
if (count_ == 0 && didWeInitializeOpenSSL_) {
cleanupOpenSSL();
didWeInitializeOpenSSL_ = false;
Expand Down
12 changes: 11 additions & 1 deletion lib/cpp/src/thrift/transport/TSSLSocket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
// Put this first to avoid WIN32 build failure
#include <thrift/transport/TSocket.h>

#include <functional>
#include <openssl/ssl.h>
#include <string>
#include <thrift/concurrency/Mutex.h>
Expand All @@ -33,9 +34,10 @@ namespace transport {

class AccessManager;
class SSLContext;
typedef std::function<std::shared_ptr<SSLContext>()> SSLContextFactory;

enum SSLProtocol {
SSLTLS = 0, // Supports SSLv2 and SSLv3 handshake but only negotiates at TLSv1_0 or later.
SSLTLS = 0, // Supports version-flexible TLS negotiation with TLSv1_2 as the default floor.
//SSLv2 = 1, // HORRIBLY INSECURE!
SSLv3 = 2, // Supports SSLv3 only - also horribly insecure!
TLSv1_0 = 3, // Supports TLSv1_0 or later.
Expand Down Expand Up @@ -210,6 +212,12 @@ class TSSLSocketFactory {
* @param protocol The SSL/TLS protocol to use.
*/
TSSLSocketFactory(SSLProtocol protocol = SSLTLS);
/**
* Constructor
*
* @param contextFactory Function invoked during construction to return a custom OpenSSL context.
*/
TSSLSocketFactory(const SSLContextFactory& contextFactory);
Comment thread
HTHou marked this conversation as resolved.
virtual ~TSSLSocketFactory();
/**
* Create an instance of TSSLSocket with a fresh new socket.
Expand Down Expand Up @@ -328,6 +336,8 @@ class TSSLSocketFactory {
static uint64_t count_;
static bool manualOpenSSLInitialization_;
static bool didWeInitializeOpenSSL_; // in that case we also perform de-init
void initializeOpenSSLState();
void cleanupOpenSSLState();
void setup(std::shared_ptr<TSSLSocket> ssl);
static int passwordCallback(char* password, int size, int, void* data);
};
Expand Down
6 changes: 3 additions & 3 deletions lib/cpp/test/SecurityFromBufferTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,11 @@ BOOST_AUTO_TEST_CASE(ssl_security_matrix) {
{
// server = SSLTLS SSLv2 SSLv3 TLSv1_0 TLSv1_1 TLSv1_2
// client
/* SSLTLS */ { true, false, false, true, true, true },
/* SSLTLS */ { true, false, false, false, false, true },
/* SSLv2 */ { false, false, false, false, false, false },
/* SSLv3 */ { false, false, true, false, false, false },
/* TLSv1_0 */ { true, false, false, true, false, false },
/* TLSv1_1 */ { true, false, false, false, true, false },
/* TLSv1_0 */ { false, false, false, true, false, false },
/* TLSv1_1 */ { false, false, false, false, true, false },
/* TLSv1_2 */ { true, false, false, false, false, true }
};

Expand Down
80 changes: 77 additions & 3 deletions lib/cpp/test/SecurityTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
#endif

using apache::thrift::transport::TSSLServerSocket;
using apache::thrift::transport::SSLContextFactory;
using apache::thrift::transport::TSSLException;
using apache::thrift::transport::TServerTransport;
using apache::thrift::transport::TSSLSocket;
using apache::thrift::transport::TSSLSocketFactory;
Expand Down Expand Up @@ -226,6 +228,78 @@ 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());

if (SSL_OP_NO_SSLv2 != 0) {
BOOST_CHECK((options & SSL_OP_NO_SSLv2) != 0);
}
if (SSL_OP_NO_SSLv3 != 0) {
BOOST_CHECK((options & SSL_OP_NO_SSLv3) != 0);
}
if (SSL_OP_NO_TLSv1 != 0) {
BOOST_CHECK((options & SSL_OP_NO_TLSv1) != 0);
}
if (SSL_OP_NO_TLSv1_1 != 0) {
BOOST_CHECK((options & SSL_OP_NO_TLSv1_1) != 0);
}
}
Comment thread
HTHou marked this conversation as resolved.

BOOST_AUTO_TEST_CASE(custom_ssl_context_options)
{
class CustomSSLContext : public apache::thrift::transport::SSLContext
{
public:
CustomSSLContext() : SSLContext()
{
SSL_CTX_clear_options(get(), SSL_OP_NO_TLSv1_1);
}
};

std::shared_ptr<apache::thrift::transport::SSLContext> context;
TSSLSocketFactory factory([&context]() {
context = std::make_shared<CustomSSLContext>();
return context;
});
const auto options = SSL_CTX_get_options(context->get());
Comment on lines +261 to +266

@HTHou HTHou Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in the latest push. custom_ssl_context_options now explicitly resets the captured SSLContext before the test exits, so the extra shared reference is released before TSSLSocketFactory reaches its destructor/cleanup path.


if (SSL_OP_NO_TLSv1 != 0) {
BOOST_CHECK((options & SSL_OP_NO_TLSv1) != 0);
}
if (SSL_OP_NO_TLSv1_1 != 0) {
BOOST_CHECK((options & SSL_OP_NO_TLSv1_1) == 0);
}
context.reset();
}

BOOST_AUTO_TEST_CASE(custom_ssl_context_factory_validation)
{
try
{
SSLContextFactory contextFactory;
TSSLSocketFactory factory(contextFactory);
BOOST_FAIL("Expected empty SSLContextFactory to throw");
}
catch (const TSSLException& ex)
{
BOOST_CHECK_EQUAL("SSLContextFactory must not be empty", std::string(ex.what()));
}

try
{
TSSLSocketFactory factory([]() {
return std::shared_ptr<apache::thrift::transport::SSLContext>();
});
BOOST_FAIL("Expected null SSLContextFactory result to throw");
}
catch (const TSSLException& ex)
{
BOOST_CHECK_EQUAL("SSLContextFactory must not return null", std::string(ex.what()));
}
Comment on lines +285 to +300

@HTHou HTHou Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in the latest push. The TSSLException message checks now compare against std::string(ex.what()), making the assertions explicit content comparisons.

}

BOOST_AUTO_TEST_CASE(ssl_security_matrix)
{
try
Expand All @@ -236,11 +310,11 @@ BOOST_AUTO_TEST_CASE(ssl_security_matrix)
{
// server = SSLTLS SSLv2 SSLv3 TLSv1_0 TLSv1_1 TLSv1_2
// client
/* SSLTLS */ { true, false, false, true, true, true },
/* SSLTLS */ { true, false, false, false, false, true },
/* SSLv2 */ { false, false, false, false, false, false },
/* SSLv3 */ { false, false, true, false, false, false },
/* TLSv1_0 */ { true, false, false, true, false, false },
/* TLSv1_1 */ { true, false, false, false, true, false },
/* TLSv1_0 */ { false, false, false, true, false, false },
/* TLSv1_1 */ { false, false, false, false, true, false },
/* TLSv1_2 */ { true, false, false, false, false, true }
};

Expand Down
Loading