On Mon, 6 Jan 2025 12:48:25 GMT, Jaikiran Pai <j...@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
