HTHou commented on PR #18130:
URL: https://github.com/apache/iotdb/pull/18130#issuecomment-4900823518
I reviewed this PR locally against head
`696ca4872a2a5c03635048a8a990c49b7e1d54b5`. I think there are a few issues
worth addressing before merge:
1. `trustStore` does not actually enable server certificate verification in
the Thrift socket path.
`RpcSslUtils::createTlsClientContext()` / `createTlcpClientContext()`
load the trust store and set `SSL_VERIFY_PEER`, but
`RpcSslUtils::createSslSocketFactory()` then calls
`factory->authenticate(false)`. In Thrift
`TSSLSocketFactory::authenticate(false)` resets the context verify mode to
`SSL_VERIFY_NONE`, so the configured trust store is effectively bypassed for
normal `Session` / `ThriftConnection` connections. This means the documented
TLS one-way/server-authentication and TLCP trust-store behavior is not
enforced. The current handshake tests use `SSL_connect` directly, so they do
not cover this Thrift socket factory path.
Could we make this conditional, for example call `authenticate(true)`
when `config.effectiveTrustStore()` is set and keep `authenticate(false)` only
for the legacy no-trust-store mode? It would also be good to add a
Session/Thrift-level negative test where a wrong CA must fail.
2. The bundled Tongsuo source is not pinned to an immutable artifact.
`TONGSUO_GIT_REF` defaults to `8.4-stable`, and `FetchOpenSSL.cmake`
downloads the branch tarball without an expected hash. Since that branch can
move, the same IoTDB commit can produce different SDK binaries over time, which
is risky for release reproducibility and dependency auditing. Please consider
pinning a tag/commit and verifying the tarball hash.
3. The packaged SDK examples are inconsistent with the updated examples
README.
`examples/README.md` documents `cpp_tls_example`, `cpp_ntls_example`,
`tls_tree_example`, and `c_ntls_example`, but `src/assembly/client-cpp.xml`
only packages the older example sources. Also, `examples/CMakeLists.txt` only
creates the new TLS/TLCP example targets in the in-tree build path. After users
unpack the released SDK zip, the documented TLS/TLCP examples are not
available/buildable from the package.
4. Architectural question: should Tongsuo be the default SSL provider for
all C++ SDK builds?
Since most users only need standard TLS, would it be possible to keep the
default SDK on upstream OpenSSL and provide a Tongsuo/TLCP-specific build
flavor only when TLCP is requested? For example, an explicit provider
option/classifier such as `IOTDB_SSL_PROVIDER=TONGSUO` or a separate TLCP
package could keep the default TLS path simpler, reduce the default
dependency/build-toolchain surface, and limit the impact of Tongsuo to users
who actually need TLCP.
--
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]
