THRIFT-6073: Allow injecting external SSL_CTX into C++ SSLContext

[hongzhi-gao]

Summary

Add a backend-neutral extension point to the C++ TSSLSocket stack: applications can inject a pre-configured OpenSSL SSL_CTX through the existing SSLContextFactory / TSSLSocketFactory hook.

This makes it easier for applications to supply a fully configured TLS context—protocol options, cipher suites, certificate loading, and other OpenSSL settings—in application code, without patching libthrift or replacing the transport layer.

Background

Thrift C++ already provides SSL/TLS through TSSLSocket / TSSLSocketFactory, backed by OpenSSL. The default factory covers the common case: a single certificate/key pair via loadCertificate() / loadPrivateKey(), and protocol selection via SSLProtocol.

TSSLSocketFactory already accepts a custom SSLContextFactory, but SSLContext could previously only be created from SSLProtocol. Applications that need a context configured outside Thrift's helpers—multi-step cert setup, non-default methods, or options not exposed by TSSLSocketFactory—had to patch Thrift locally.

This PR adds a small wrapper constructor on SSLContext while keeping the default OpenSSL build unchanged.

What changed

• SSLContext: explicit SSLContext(SSL_CTX* ctx, bool takeOwnership = true) — wrap an application-owned context; destructor respects ownership.
• lib/cpp/README.md: document the injection pattern.
• SecurityTest: wrapped-context and null-validation coverage.

Removed from the earlier draft: WITH_TONGSUO, SSLProtocol::NTLS, loadSign* / loadEnc*, vendored test keys, and the optional local NTLS integration test.

Usage

Standard TLS (unchanged):

TSSLSocketFactory factory;  // default SSLTLS
factory.loadCertificate("server.crt");
factory.loadPrivateKey("server.key");

Injected SSL_CTX (application owns OpenSSL configuration):

#include <thrift/transport/TSSLSocket.h>

auto factory = std::make_shared<TSSLSocketFactory>([]() {
  SSL_CTX* ctx = SSL_CTX_new(TLS_method());
  SSL_CTX_set_mode(ctx, SSL_MODE_AUTO_RETRY);
  // Configure ctx as needed: ciphers, certificates, verify mode, etc.
  return std::make_shared<apache::thrift::transport::SSLContext>(ctx, true);
});
factory->loadTrustedCertificates("ca-bundle.crt");
factory->authenticate(true);
factory->server(true);
auto socket = factory->createSocket("127.0.0.1", port);

Link your application against whichever OpenSSL-compatible TLS library you choose; libthrift continues to default to system OpenSSL at build time.

Licensing

Thrift introduces no new build-time dependency. Nothing is vendored; no LICENSE / NOTICE update is required.

Test plan

• Standard OpenSSL build: ctest -R SecurityTest (wrapped_ssl_context, wrapped_ssl_context_null)

[Jens-G]

Code review

Found 2 issues:

1. All 8 new load* methods call requireNtlsSupport() before the null-pointer argument check, reversing the established contract of every existing load* method in the file (loadCertificate, loadPrivateKey, etc. all validate arguments first). On non-NTLS builds, callers that pass null arguments receive TSSLException("NTLS is not available") instead of TTransportException(BAD_ARGS), making errors undiagnosable.

thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp

Lines 1110 to 1115 in 0d5e69a

	void TSSLSocketFactory::loadSignCertificate(const char* path, const char* format) {
	requireNtlsSupport();
	if (path == nullptr || format == nullptr) {
	throw TTransportException(TTransportException::BAD_ARGS,
	"loadSignCertificate: either <path> or <format> is nullptr");
	}

2. Tongsuo is introduced as a new optional build dependency but the PR description contains no mention of license verification against the ASF Category A/X list, as required by AGENTS.md §1: "Before introducing any dependency … verify its license is compatible with Apache 2.0 … If in doubt … add it and flag it in the PR description for committer review."

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[HTHou]

Should 6 be TLSv1_3?

[hongzhi-gao]

NTLS = 6 was removed with the provider-specific APIs.

[HTHou]

Thanks for putting this together. Could we consider narrowing the upstream change to a generic TLS-provider compatibility layer, rather than making Tongsuo/NTLS a first-class Thrift backend?

The current patch adds Tongsuo-specific build plumbing, SSLProtocol::NTLS, and loadSign* / loadEnc* APIs to TSSLSocketFactory. That makes the Tongsuo/TLCP surface part of Thrift's public C++ API/ABI and ongoing build/test matrix.

An alternative might be to expose a small backend-neutral extension point around SSLContext / SSL_CTX creation and configuration, for example allowing applications to provide an already-created SSLContext/SSL_CTX, or a protected/public hook that creates and configures the context. Then downstream applications that need Tongsuo can link against it themselves and apply the NTLS/TLCP dual-certificate setup in their own factory/subclass, while Thrift only maintains the generic OpenSSL-compatible transport layer.

That would still let users avoid carrying a private transport fork, but keeps the provider-specific dependency and policy choices outside the core Thrift tree. Would you be open to reshaping the patch in that direction?

[hongzhi-gao]

Thanks for putting this together. Could we consider narrowing the upstream change to a generic TLS-provider compatibility layer, rather than making Tongsuo/NTLS a first-class Thrift backend?

The current patch adds Tongsuo-specific build plumbing, SSLProtocol::NTLS, and loadSign* / loadEnc* APIs to TSSLSocketFactory. That makes the Tongsuo/TLCP surface part of Thrift's public C++ API/ABI and ongoing build/test matrix.

An alternative might be to expose a small backend-neutral extension point around SSLContext / SSL_CTX creation and configuration, for example allowing applications to provide an already-created SSLContext/SSL_CTX, or a protected/public hook that creates and configures the context. Then downstream applications that need Tongsuo can link against it themselves and apply the NTLS/TLCP dual-certificate setup in their own factory/subclass, while Thrift only maintains the generic OpenSSL-compatible transport layer.

That would still let users avoid carrying a private transport fork, but keeps the provider-specific dependency and policy choices outside the core Thrift tree. Would you be open to reshaping the patch in that direction?

1. Fixed by removing the Tongsuo-specific loadSign* / loadEnc* APIs entirely. The reshaped patch no longer adds those methods.
2. Added a Licensing section: Thrift still depends only on OpenSSL by default; Tongsuo is an application-side choice (Apache 2.0, ASF Category A) and is not vendored or added to Thrift's build.

[hongzhi-gao]

Thanks for putting this together. Could we consider narrowing the upstream change to a generic TLS-provider compatibility layer, rather than making Tongsuo/NTLS a first-class Thrift backend?

The current patch adds Tongsuo-specific build plumbing, SSLProtocol::NTLS, and loadSign* / loadEnc* APIs to TSSLSocketFactory. That makes the Tongsuo/TLCP surface part of Thrift's public C++ API/ABI and ongoing build/test matrix.

An alternative might be to expose a small backend-neutral extension point around SSLContext / SSL_CTX creation and configuration, for example allowing applications to provide an already-created SSLContext/SSL_CTX, or a protected/public hook that creates and configures the context. Then downstream applications that need Tongsuo can link against it themselves and apply the NTLS/TLCP dual-certificate setup in their own factory/subclass, while Thrift only maintains the generic OpenSSL-compatible transport layer.

That would still let users avoid carrying a private transport fork, but keeps the provider-specific dependency and policy choices outside the core Thrift tree. Would you be open to reshaping the patch in that direction?

Done — the patch is reshaped to a backend-neutral SSLContext wrapper around an application-provided SSL_CTX, using the existing SSLContextFactory hook. Tongsuo/NTLS dual-cert setup lives in application (or local test) code; Thrift core stays OpenSSL-compatible and provider-agnostic.

[Jens-G]

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.

thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp

Lines 216 to 221 in 0870077

	SSLContext::SSLContext(SSL_CTX* ctx, bool takeOwnership)
	: ctx_(ctx), takeOwnership_(takeOwnership) {
	if (ctx_ == nullptr) {
	throw TSSLException("SSLContext: ctx must not be null");
	}
	}

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.

thrift/lib/cpp/src/thrift/transport/TSSLSocket.h

Lines 365 to 374 in 0870077

	class SSLContext {
	public:
	SSLContext(const SSLProtocol& protocol = SSLTLS);
	/**
	* Wrap an existing OpenSSL SSL_CTX.
	*
	* @param ctx OpenSSL context to wrap
	* @param takeOwnership If true (default), SSLContext frees ctx on destruction
	*/
	explicit SSLContext(SSL_CTX* ctx, bool takeOwnership = true);

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.

thrift/lib/cpp/test/SecurityTest.cpp

Lines 286 to 306 in 0870077

	context = std::make_shared<SSLContext>(raw, true);
	return context;
	});
	BOOST_CHECK(context->get() == raw);
	context.reset();
	}
	BOOST_AUTO_TEST_CASE(wrapped_ssl_context_null)
	{
	try
	{
	std::make_shared<SSLContext>(nullptr);
	BOOST_FAIL("Expected null SSL_CTX to throw");
	}
	catch (const TSSLException& ex)
	{
	BOOST_CHECK_EQUAL("SSLContext: ctx must not be null", std::string(ex.what()));
	}
	}
	BOOST_AUTO_TEST_CASE(custom_ssl_context_factory_validation)

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.

thrift/lib/cpp/src/thrift/transport/TSSLSocket.h

Lines 367 to 374 in 0870077

	SSLContext(const SSLProtocol& protocol = SSLTLS);
	/**
	* Wrap an existing OpenSSL SSL_CTX.
	*
	* @param ctx OpenSSL context to wrap
	* @param takeOwnership If true (default), SSLContext frees ctx on destruction
	*/
	explicit SSLContext(SSL_CTX* ctx, bool takeOwnership = true);

Verified: no SSL_OP_NO_TLSv1* in new constructor; existing floor confirmed at TSSLSocket.cpp:210-212; 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: "It is the responsibility of the code using TSSLSocketFactory to ensure that the factory lifetime exceeds the lifetime of any sockets it might create").

thrift/lib/cpp/src/thrift/transport/TSSLSocket.h

Lines 367 to 374 in 0870077

	SSLContext(const SSLProtocol& protocol = SSLTLS);
	/**
	* Wrap an existing OpenSSL SSL_CTX.
	*
	* @param ctx OpenSSL context to wrap
	* @param takeOwnership If true (default), SSLContext frees ctx on destruction
	*/
	explicit SSLContext(SSL_CTX* ctx, bool takeOwnership = true);

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

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Jens-G]

One housekeeping note: for a change of this scope, it is worth making sure the JIRA ticket captures the key design decisions and rationale in addition to what is in the commit messages. Commit messages are squashed on merge and can be hard to find later; JIRA comments and the ticket description remain a stable reference for anyone reviewing the issue in future. If there is technical context currently spread across individual commit messages (e.g. the rationale for the takeOwnership parameter semantics, or the TLS version floor decision), copying the relevant parts into the JIRA ticket before this is merged would be a good investment.