Repository navigation
[CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support - #3764
shellfish007 wants to merge 4 commits into
Conversation
SSLFactory.createEngine() called jdkSslContext.createSSLEngine() with no arguments. The JDK only includes the SNI extension in the TLS ClientHello when a peer hostname is provided via createSSLEngine(host, port). Without SNI, TLS termination/routing that depends on server name (e.g. Istio PASSTHROUGH/SIMPLE, or any SNI-based multi-tenant ingress) cannot route or differentiate connections. Thread peer hostname/port from TransportClientFactory.internalCreateClient() through TransportContext.initializePipeline() to SSLFactory.createSSLEngine(), which now calls jdkSslContext.createSSLEngine(peerHost, peerPort) for client connections. Existing server-side and no-arg callers are unaffected via backward-compatible overloads.
shellfish007
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds Server Name Indication (SNI) support for TLS client connections by threading the peer hostname/port through the Netty transport initialization path into SSLFactory so the JDK can emit SNI in the TLS ClientHello.
Changes:
- Add
TransportContext.initializePipeline(...)overloads that acceptpeerHost/peerPortand pass them to SSL setup. - Update
TransportClientFactory.internalCreateClient()to provide the remote peer host/port when initializing the client channel pipeline. - Extend
SSLFactory.createSSLEngine(...)with a peer host/port overload and useSSLContext#createSSLEngine(host, port)for client engines.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| common/src/main/java/org/apache/celeborn/common/network/TransportContext.java | Adds pipeline initialization overloads carrying peer host/port into SslHandler engine creation. |
| common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java | Introduces SSLEngine creation overload that uses createSSLEngine(peerHost, peerPort) for client-side SNI. |
| common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java | Passes remote address host/port into pipeline initialization for client connections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TransportChannelHandler clientHandler = | ||
| context.initializePipeline( | ||
| ch, decoder, true, address.getHostString(), address.getPort()); | ||
| clientRef.set(clientHandler.getClient()); |
There was a problem hiding this comment.
Non-blocking suggestion: the new SSLFactorySuiteJ tests cover the engine configuration, but they call SSLFactory directly. Could we also add a test in SslConnectivitySuiteJ that connects through TransportClientFactory using a DNS hostname and asserts the SNI observed by the server, for example via ExtendedSSLSession.getRequestedServerNames()? This would catch regressions where peerHost or peerPort is dropped along the TransportClientFactory -> TransportContext -> SSLFactory call chain.
I verified the current implementation locally with both TLS 1.2 and TLS 1.3, using pooled and unmanaged connections; the expected hostname reaches the server. This suggestion is for regression coverage and is non-blocking.
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
What changes were proposed in this pull request?
Thread peer hostname/port from
TransportClientFactory.internalCreateClient()throughTransportContext.initializePipeline()toSSLFactory.createSSLEngine().SSLFactory.createEngine()now callsjdkSslContext.createSSLEngine(peerHost, peerPort)for client connections, which makes the JDK include the SNI extension in the TLS ClientHello. Existing server-side and no-arg callers use backward-compatible overloads.Why are the changes needed?
SSLFactory.createEngine()calledjdkSslContext.createSSLEngine()with no arguments. The JDK only includes the SNI extension in the TLS ClientHello when a peer hostname is provided viacreateSSLEngine(host, port). Without SNI, TLS termination/routing that depends on server name (e.g. an SNI-based ingress/gateway such as Istio TLS PASSTHROUGH or SIMPLE mode) cannot route or differentiate connections by hostname. This fix is a prerequisite for shared-port multi-tenant routing through an SNI-based ingress gateway.Does this PR introduce any user-facing change?
No. Client
SSLEnginecreation now includes SNI; server-side and existing no-arg callers are unaffected.How was this patch tested?
Follows the existing
SSLFactory/TransportContextcall patterns with additive, backward-compatible overloads — no new config surface. Relies on CI for build/test verification.