RFR: 8346705: SNI not sent with Java 22+ using java.net.http.HttpClient.Builder#sslParameters

[Michael McMahon]

On Mon, 6 Jan 2025 12:48:25 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review of this change which fixes a regression that was introduced in Java 22 release in the implementation of `java.net.http.HttpClient`? This addresses https://bugs.openjdk.org/browse/JDK-8346705.
> 
> A client intiating a TLS handshake (through the `ClientHello`) can send a "Server Name Indication" to tell the server, the name of the server it is communicating with (https://www.rfc-editor.org/rfc/rfc6066#section-3). When a `java.net.http.HttpClient` is establishing a (TLS) connection to serve a HTTP request, it determines the SNI server name to send in the handshake.
> 
> In Java 22, through https://bugs.openjdk.org/browse/JDK-8309910, we did an (internal implementation detail) change to allow for the internal implementation classes to keep track of what SNI server name was used for the connection, so that that detail can be used for other purposes after the connection has been established. That change wasn't meant to affect the logic that determined the SNI server name to be sent across in the TLS handshake. However, due to an oversight in the refactoring, a regression got introduced which caused the HttpClient to no longer send a SNI server name for a certain scenario (described below).
> 
> The implementation of the HttpClient first looks at the request URI to determine the SNI. If the request URI host is a IP address literal, then the HttpClient implementation looks at the `HttpClient.sslParameters()` to see if the application has configured any ServerName(s) through `SSLParameters.setServerNames(...)`. If any are set, then the HttpClient uses those ServerName(s) and sends them in the SNI, else it doesn't set the SNI. On the other hand, if the URI host is not a IP address literal, then the HttpClient implementation doesn't use any application configured ServerName(s) and instead uses the request URI host as the SNI name and sends it in the TLS handshake.
> 
> The oversight in JDK-8309910 caused a regression where if the request URI host is a IP address literal, then we no longer looked at the application configured ServerName(s), thus causing the HttpClient to no longer set the SNI server name in the TLS handshake. This can cause (and indeed does cause) the TLS handshakes to fail if the server rejects the handshake due to the missing SNI.
> 
> The commit in this PR addresses this regression by making sure that the HttpClient continues to behave the way it used to before JDK-8309910. A new jtreg test case has been introduced to verify this existing be...

Looks fine. Just one minor typo suggestion

Marked as reviewed by michaelm (Reviewer).

src/java.net.http/share/classes/jdk/internal/net/http/AbstractAsyncSSLConnection.java line 155:

> 153:      * <p>
> 154:      * The given {@code serverName} is given preference, and if it is not null and
> 155:      * is not a IP address literal, then the returned list will contain only one

Suggestion:

     * is not an IP address literal, then the returned list will contain only one

-------------

PR Review: https://git.openjdk.org/jdk/pull/22927#pullrequestreview-2534958384
PR Review: https://git.openjdk.org/jdk/pull/22927#pullrequestreview-2534961945
PR Review Comment: https://git.openjdk.org/jdk/pull/22927#discussion_r1905801099