On Wed, 6 May 2026 06:05:13 GMT, Volkan Yazici <[email protected]> wrote:
>> Per [RFC 6066 "3. Server Name Indication"], disallow IP literals in >> `SNIHostName::new`. >> >> While the following two call-sites could be simplified by removing IP >> literal checks, I've refrained from doing so because delegating some of the >> checks to an exception catching mechanism would impact the performance: >> >> sun.security.ssl.Utilities::rawToSNIHostName >> sun.net.www.protocol.https.HttpsClient::afterConnect >> >> [RFC 6066 "3. Server Name Indication"]: >> https://www.rfc-editor.org/rfc/rfc6066.html#page-6 >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Volkan Yazici has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Merge remote-tracking branch 'upstream/master' into sni > - Apply review feedback > - Fix variable shadowing issue in `SNIHostName` > - Enrich tests > - Use `sun.security.x509.DNSName` in strict checks > - Merge remote-tracking branch 'upstream/master' into sni > - Improve deprecation message > - Big facelift > - Add `ofString` static factory method > - Disallow IP literals in `SNIHostName::new` src/java.base/share/classes/javax/net/ssl/SNIHostName.java line 81: > 79: * Creates an {@code SNIHostName} using the specified hostname. > 80: * <p> > 81: * A valid SNI hostname is a DNS hostname, which is either an > ASCII-encoded I wonder if "DNS hostname" could link to somewhere authoritative that defines a DNS hostname. If you could find a suitable target then the first usage in the class description could link to it. src/java.base/share/classes/javax/net/ssl/SNIHostName.java line 105: > 103: * StandardCharsets#US_ASCII ASCII}. > 104: * {@link IDN#toASCII(String, int) IDN.toASCII(hostname, > IDN.USE_STD3_ASCII_RULES)} > 105: * is used to enforce the restrictions on ASCII characters in > hostnames (see This is confusing when rendered because it reads the space between ASCII and IDN.toASCII isn't very obvious (if you generate the API docs then you'll see what I mean). Maybe change it to "The IDN.toASCII(..) method is used to enforce .." and that will avoid have the captialized words running into each other. src/java.base/share/classes/javax/net/ssl/SNIHostName.java line 179: > 177: * depending on their context; e.g., propagate the failure, or > report the > 178: * issue and skip {@linkplain SSLParameters#setServerNames(List) the > SNI > 179: * server name configuration}. Instead of "Users are expected" then it might be clearer to say "Code using this method is expected to ..." src/java.base/share/classes/javax/net/ssl/SNIHostName.java line 208: > 206: * constitute a DNS hostname, which is either an ASCII-encoded > hostname or > 207: * an {@linkplain IDN Internationalized Domain Name (IDN)}. A decoded > 208: * hostname string is considered illegal if it: "Once the specific value is decoded ..." is confusing. I think you'll need to re-phase this to say the input is decoded into a host name string so there is no ambiguity on who and when the bytes are decoded. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30747#discussion_r3193759675 PR Review Comment: https://git.openjdk.org/jdk/pull/30747#discussion_r3193792071 PR Review Comment: https://git.openjdk.org/jdk/pull/30747#discussion_r3193797230 PR Review Comment: https://git.openjdk.org/jdk/pull/30747#discussion_r3193847815
