On Wed, 8 Jan 2025 01:18:22 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 v... > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > typo fix in comment > > Co-authored-by: Michael McMahon > <70538289+michael-mc-ma...@users.noreply.github.com> Marked as reviewed by dfuchs (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/22927#pullrequestreview-2540151487
