Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]

["zaynt4606 (via GitHub)" <gi...@apache.org>]

zaynt4606 commented on code in PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#discussion_r4256036031


##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java:
##########
@@ -306,7 +306,9 @@ private TransportClient internalCreateClient(
         new ChannelInitializer<SocketChannel>() {
           @Override
           public void initChannel(SocketChannel ch) {
-            TransportChannelHandler clientHandler = context.initializePipeline(ch, decoder, true);
+            TransportChannelHandler clientHandler =
+                context.initializePipeline(
+                    ch, decoder, true, address.getHostString(), address.getPort());
             clientRef.set(clientHandler.getClient());

Review 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 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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org