Skip to content

THRIFT-6073: Add optional TLCP/NTLS support to C++ TSSLSocket via Tongsuo#3606

Open
hongzhi-gao wants to merge 1 commit into
apache:masterfrom
hongzhi-gao:feature/cpp-tongsuo-ntls
Open

THRIFT-6073: Add optional TLCP/NTLS support to C++ TSSLSocket via Tongsuo#3606
hongzhi-gao wants to merge 1 commit into
apache:masterfrom
hongzhi-gao:feature/cpp-tongsuo-ntls

Conversation

@hongzhi-gao

@hongzhi-gao hongzhi-gao commented Jun 26, 2026

Copy link
Copy Markdown

Summary

Add optional TLCP/NTLS support to the C++ TSSLSocket stack by allowing libthrift to be built against Tongsuo, an OpenSSL-compatible TLS library with NTLS/TLCP support.

This enables Thrift C++ services to run over TLCP without maintaining a private fork of the transport layer.

Background

Thrift C++ already provides SSL/TLS through TSSLSocket / TSSLSocketFactory, backed by OpenSSL. In deployments that require TLCP (Transport Layer Cryptography Protocol, commonly implemented as NTLS in Tongsuo/BabaSSL), the existing API is not sufficient:

  • TLCP uses a dual-certificate model (separate signing and encryption credentials), not the single cert/key pair assumed by loadCertificate() / loadPrivateKey().
  • TLCP cipher suites and handshake behavior differ from standard TLS (e.g. SM2/SM3/SM4).

Teams using Thrift in TLCP-regulated environments today must either patch Thrift locally or replace the transport entirely. Both options increase maintenance cost and reduce interoperability with upstream Thrift.

Tongsuo is API-compatible with OpenSSL for most Thrift SSL usage, so this can be integrated as an opt-in build path rather than a breaking change to the default OpenSSL workflow.

What changed

  • CMake: -DWITH_TONGSUO=ON and -DTONGSUO_ROOT_DIR=... to link against Tongsuo instead of system OpenSSL; runtime detection sets THRIFT_HAVE_NTLS when NTLS is available.
  • TSSLSocketFactory: new SSLProtocol::NTLS and dual-certificate APIs (loadSign* / loadEnc*, including buffer variants).
  • Tests: TNTLSSocketTest with SM2 dual-cert fixtures under test/keys/ntls/.

Existing OpenSSL builds and standard TLS behavior are unchanged when -DWITH_TONGSUO is not enabled.

Test plan

  • Build with -DWITH_TONGSUO=ON -DTONGSUO_ROOT_DIR=<prefix> -DBUILD_TESTING=ON -DBUILD_COMPILER=ON
  • ctest -R TNTLSSocketTest
    • one-way NTLS handshake + echo
    • mutual authentication with dual certificates
    • dual certificates loaded from memory buffers

Tongsuo must be built with ./config enable-ntls before running NTLS tests.

Enable building libthrift against Tongsuo via -DWITH_TONGSUO and expose
NTLS dual-certificate APIs on TSSLSocketFactory for TLCP workloads.
@mergeable mergeable Bot added c++ Pull requests that update C++ code testsuite build and general CI cmake, automake and build system changes labels Jun 26, 2026
@hongzhi-gao hongzhi-gao changed the title Add Tongsuo NTLS support to C++ TSSLSocket Add optional TLCP/NTLS support to C++ TSSLSocket via Tongsuo Jun 26, 2026
@hongzhi-gao hongzhi-gao changed the title Add optional TLCP/NTLS support to C++ TSSLSocket via Tongsuo THRIFT-6073: Add optional TLCP/NTLS support to C++ TSSLSocket via Tongsuo Jun 26, 2026
@Jens-G

Jens-G commented Jun 26, 2026

Copy link
Copy Markdown
Member

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.

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");
}

  1. 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 👎.

TLSv1_0 = 3, // Supports TLSv1_0 or later.
TLSv1_1 = 4, // Supports TLSv1_1 or later.
TLSv1_2 = 5, // Supports TLSv1_2 or later.
NTLS = 6, // Tongsuo NTLS/TLCP; requires dual signing and encryption certificates.

@HTHou HTHou Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should 6 be TLSv1_3?

@HTHou

HTHou commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build and general CI cmake, automake and build system changes c++ Pull requests that update C++ code testsuite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants