On Tue, 2 Dec 2025 14:25:36 GMT, Sergey Chernyshev <[email protected]> wrote:
>> Hi all, >> >> Let me propose a fix and a test case for JDK-8369950. >> >> The failure reproduces with BCJSSE provider and all implementations of >> SSLSocket other than SSLSocketImpl. >> >> In the test case an anonymous wrapper is used, over the standard >> SSLSocketImpl, to simulate an external JSSE provider. The test case shows >> the same behavior as in BCJSSE (failure due to non-LDH ASCII characters in >> the SNI host name). >> >> The fix avoids constructing SNIHostName when the URL host name is an IPv4 or >> IPv6 literal address. Other than that, all other FQDN host names that have >> invalid characters (non-LDH ASCII characters) still produce that exception. >> >> SNIHostName, as defined in >> https://github.com/openjdk/jdk/blob/873f8a696fa45c7d94a164be20cf3c797ce7f2a6/src/java.base/share/classes/javax/net/ssl/SNIHostName.java#L44-L66 >> >> has the fully qualified DNS hostname of the server. As follows from the >> section 3, "Server Name Indication", RFC 6066, `Literal IPv4 and IPv6 >> addresses are not permitted in "HostName"`. >> >> The fix mirrors the behavior of SSLSocketImpl, that avoids constructing the >> SNIHostName from literal addresses. Please see >> >> https://github.com/openjdk/jdk/blob/873f8a696fa45c7d94a164be20cf3c797ce7f2a6/src/java.base/share/classes/sun/security/ssl/Utilities.java#L110-L116 >> >> Testing: >> - standard jtreg tests goups showed no regressions >> - the new test passes with the fix and fails otherwise >> - passes also with BCJSSE in FIPS and standard mode >> >> <details><summary> BCJSSE standard</summary> >> >> >> STDOUT: >> STDERR: >> Dez. 01, 2025 2:44:02 PM org.bouncycastle.jsse.provider.PropertyUtils >> getBooleanSecurityProperty >> INFORMATION: Found boolean security property [keystore.type.compat]: true >> Dez. 01, 2025 2:44:02 PM org.bouncycastle.jsse.provider.PropertyUtils >> getStringSecurityProperty >> INFORMATION: Found string security property [jdk.tls.disabledAlgorithms]: >> SSLv3, TLSv1, TLSv1.1, DTLSv1.0, RC4, DES, MD5withRSA, DH keySize < 1024, EC >> keySize < 224, 3DES_EDE_CBC, anon, NULL, ECDH, TLS_RSA_*, rsa_pkcs1_sha1 >> usage HandshakeSignature, ecdsa_sha1 usage HandshakeSignature, dsa_sha1 >> usage HandshakeSignature >> Dez. 01, 2025 2:44:02 PM >> org.bouncycastle.jsse.provider.DisabledAlgorithmConstraints create >> WARNUNG: Ignoring unsupported entry in 'jdk.tls.disabledAlgorithms': >> rsa_pkcs1_sha1 usage HandshakeSignature >> Dez. 01, 2025 2:44:02 PM >> org.bouncycastle.jsse.provider.DisabledAlgorithmConstraints create >> WARNUNG: Ignoring unsupported entry in 'jdk.tl... > > Sergey Chernyshev has updated the pull request incrementally with two > additional commits since the last revision: > > - addressed more review comments > - addressed review comments src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 1: > 1: /* Copyright year needs a bump. src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 476: > 474: // host has been set previously for SSLSocketImpl > 475: if (!(s instanceof SSLSocketImpl) && > 476: !IPAddressUtil.isIPv4LiteralAddress(host) && This introduces a new behavior for both IPv4 and IPv6, yet we only verify the IPv6 behavior. Shouldn't we also verify that when the provided host is an IPv4 literal, `setServerNames()` is left untouched? src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 478: > 476: !IPAddressUtil.isIPv4LiteralAddress(host) && > 477: !(host.charAt(0) == '[' && > host.charAt(host.length() - 1) == ']' && > 478: > IPAddressUtil.isIPv6LiteralAddress(host.substring(1, host.length() - 1)) There is one more place in `sun.net.www`, which is in this very class, that (host.charAt(0) == '[' && host.charAt(host.length() - 1) == ']') { return host.substring(1, host.length() - 1) logic is practiced. Would it make sense to refactor this into a `private static Optional<String> ipv6FromHost(String host)` method, preferably with some short explanation in the method's Javadoc on why we do this? test/jdk/javax/net/ssl/HttpsURLConnection/SubjectAltNameIPv6.java line 1: > 1: /* FWIW, _without_ the proposed `HttpsClient` changes, I can reproduce java.lang.IllegalArgumentException: Contains non-LDH ASCII characters at java.base/java.net.IDN.toASCIIInternal(IDN.java:310) at java.base/java.net.IDN.toASCII(IDN.java:139) at java.base/javax.net.ssl.SNIHostName.<init>(SNIHostName.java:114) at java.base/sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:475) using this test against `master` (i.e., 7278d2e8e58). Put another way, AFAICT, fix and test is legit. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28577#discussion_r2580621834 PR Review Comment: https://git.openjdk.org/jdk/pull/28577#discussion_r2580726453 PR Review Comment: https://git.openjdk.org/jdk/pull/28577#discussion_r2580631079 PR Review Comment: https://git.openjdk.org/jdk/pull/28577#discussion_r2580718016
