Skip to content

Support standard TLS and mTLS in the C++ client - #18601

Open
hongzhi-gao wants to merge 13 commits into
apache:masterfrom
hongzhi-gao:feature/session-cpp-openssl
Open

Support standard TLS and mTLS in the C++ client#18601
hongzhi-gao wants to merge 13 commits into
apache:masterfrom
hongzhi-gao:feature/session-cpp-openssl

Conversation

@hongzhi-gao

@hongzhi-gao hongzhi-gao commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

Standard TLS and mTLS

Add CA verification and optional PEM client certificate/private key support to the C++ Tree, Table, SessionPool, and C APIs. TLS settings are preserved across reconnects and node discovery. TLCP is intentionally excluded from this PR.

OpenSSL packaging

Build checksum-pinned OpenSSL 3.5.8 from source by default on Linux, macOS, and Windows, and bundle its runtime libraries with the C++ SDK. A compatible system OpenSSL remains opt-in.

Tests

Run the C++ integration tests against real IoTDB servers in plain, TLS, and mTLS modes, including rejection of clients without certificates when client authentication is required.

This is the standard TLS/mTLS part split from #18597; TLCP can be handled independently.


This PR has:

  • been self-reviewed.
  • added documentation for modified behavior.
  • added comments explaining non-obvious intent.
  • added or updated tests for the changed paths.
  • been tested with a real IoTDB server.

Key changed/added classes (or packages if there are too many classes) in this PR
  • SslConfig
  • RpcSslUtils
  • SessionBuilder, TableSessionBuilder, and SessionPoolBuilder
  • C++ TLS integration-test scripts and fixtures

@HTHou

HTHou commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for splitting standard TLS/mTLS support into a separate PR and keeping OpenSSL as the default. I found two issues that I think should be addressed before merging:

  1. [P1] Make the macOS OpenSSL libraries relocatable.

    With source builds now enabled by default in FetchOpenSSL.cmake, OpenSSL's Darwin build rules embed an absolute install name under the build directory's _deps/openssl/install/lib/. The packaging code copies the dylibs but does not rewrite their install names or dependent-library references. Setting @loader_path as an RPATH on iotdb_session does not resolve absolute library references. As a result, tests on the build machine can pass while the extracted SDK fails to load on another machine with Library not loaded.

    Could we make these references relative using @rpath/@loader_path, including the dependency from libssl to libcrypto, and test the extracted SDK with the original OpenSSL build/install directory unavailable?

  2. [P2] Propagate changes from setSslConfig() to node discovery.

    Session::setSslConfig() only updates impl_->sslConfig_. However, the Session constructor has already created and started a NodesSupplier, which holds its own copy of the previous SSL configuration. With the new C API sequence ts_session_new() -> ts_session_set_ssl_config() -> ts_session_open(), the main connection uses TLS, but background node discovery continues attempting plaintext connections. Discovery therefore fails against a TLS-only server, leaving the endpoint list stale and preventing failover to newly added nodes that were not in the initial list.

    Could we defer starting node discovery until open(), or safely recreate it when the SSL configuration changes? A TLS/mTLS test that verifies endpoint discovery and failover would cover this path; the current single-node SHOW VERSION tests do not.

These findings are based on static review of f90b7228; I have not run a full build or the integration tests locally.

@HTHou HTHou left a comment

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.

Thanks for addressing the previous findings. The updated lifecycle introduces one additional correctness issue, and I have two follow-up comments on test coverage. Details are attached to the relevant lines. This is a static review; I have not run the full build locally.

Comment thread iotdb-client/client-cpp/src/session/Session.cpp
Comment thread iotdb-client/client-cpp/test/cpp/RpcSslIotdbE2eTest.cpp
Comment thread .github/workflows/multi-language-client.yml Outdated
@hongzhi-gao

Copy link
Copy Markdown
Contributor Author

Thanks for splitting standard TLS/mTLS support into a separate PR and keeping OpenSSL as the default. I found two issues that I think should be addressed before merging:

  1. [P1] Make the macOS OpenSSL libraries relocatable.
    With source builds now enabled by default in FetchOpenSSL.cmake, OpenSSL's Darwin build rules embed an absolute install name under the build directory's _deps/openssl/install/lib/. The packaging code copies the dylibs but does not rewrite their install names or dependent-library references. Setting @loader_path as an RPATH on iotdb_session does not resolve absolute library references. As a result, tests on the build machine can pass while the extracted SDK fails to load on another machine with Library not loaded.
    Could we make these references relative using @rpath/@loader_path, including the dependency from libssl to libcrypto, and test the extracted SDK with the original OpenSSL build/install directory unavailable?
  2. [P2] Propagate changes from setSslConfig() to node discovery.
    Session::setSslConfig() only updates impl_->sslConfig_. However, the Session constructor has already created and started a NodesSupplier, which holds its own copy of the previous SSL configuration. With the new C API sequence ts_session_new() -> ts_session_set_ssl_config() -> ts_session_open(), the main connection uses TLS, but background node discovery continues attempting plaintext connections. Discovery therefore fails against a TLS-only server, leaving the endpoint list stale and preventing failover to newly added nodes that were not in the initial list.
    Could we defer starting node discovery until open(), or safely recreate it when the SSL configuration changes? A TLS/mTLS test that verifies endpoint discovery and failover would cover this path; the current single-node SHOW VERSION tests do not.

These findings are based on static review of f90b7228; I have not run a full build or the integration tests locally.

Thanks for the review. Both original findings are addressed: Darwin OpenSSL IDs and dependencies are rewritten to @rpath and the package smoke test hides the original install directory; NodesSupplier initialization is deferred until open() so it receives the final SSL configuration. The follow-up fixes and coverage are summarized in the inline replies.

@HTHou HTHou left a comment

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.

Re-reviewed eec6aba, including SSL configuration propagation through the C/C++ APIs, SessionPool, discovery and reconnect paths, Session lifecycle guards, and the OpenSSL packaging changes. The previous findings have been addressed, and I found no additional blocking issues. I also checked the macOS source-OpenSSL and Windows CI logs: the plain, TLS and mTLS test phases passed. Approval is based on code review and CI evidence; I have not run a full local build. Thanks for the fixes and regression coverage.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants