-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Additional fix for IPv6-derived SNI hostnames #2778
Conversation
// Replace colons with dots to satisfy SNIHostName validation | ||
return address instanceof Inet6Address ? hostAddress.replace(':', '.') : hostAddress; | ||
// Replace colons with dots, and percentages with underscores, to satisfy SNIHostName validation | ||
return address instanceof Inet6Address ? hostAddress.replace(':', '.').replace('%', '_') : hostAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Underscore is also a non-LDH ASCII characters. Please add a test to validate the fix. You can change visibility of toHostAddress
to pkg-private for a unit test or you can implement a full integration test using HttpClients.forResolvedAddress(ipv6Address)
Motivation: PR #2575 replaced colons with dots, but we may also see percentage-sign characters – `%` – in the IPv6 text representation, which would give us errors like the following: ``` java.security.cert.CertificateException: Illegal given domain name: localhost-0.0.0.0.0.0.0.0%0 at java.base/sun.security.util.HostnameChecker.matchDNS(HostnameChecker.java:193) at java.base/sun.security.util.HostnameChecker.match(HostnameChecker.java:103) at java.base/sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:455) at java.base/sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:429) at java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:283) at java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:141) at io.netty.handler.ssl.EnhancingX509ExtendedTrustManager.checkServerTrusted(EnhancingX509ExtendedTrustManager.java:69) ... Caused by: java.lang.IllegalArgumentException: Contains non-LDH ASCII characters at java.base/java.net.IDN.toASCIIInternal(IDN.java:296) at java.base/java.net.IDN.toASCII(IDN.java:122) at java.base/javax.net.ssl.SNIHostName.<init>(SNIHostName.java:99) at java.base/sun.security.util.HostnameChecker.matchDNS(HostnameChecker.java:191) ... 36 more ``` Modifications: Add an additional `replace(char, char)` call, to replace the colons with underscores. The `replace(char, char)` method is well-optimised in the JDK, so I don't think we can do better with a single loop over an extracted char-array. Tests were added using a variety of synthetic inet addresses, and all available host addresses, verifying that they can all be converted to valid SNI hostnames. Result: Users see expected `CertificateException`: No subject alternative DNS name matching <...> found.
@idelpivnitskiy Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Motivation:
PR #2575 replaced colons with dots, but we may also see percentage-sign characters –
%
– in the IPv6 text representation, which would give us errors like the following:Modifications:
Add an additional
replace(char, char)
call, to replace the colons with underscores. Thereplace(char, char)
method is well-optimised in the JDK, so I don't think we can do better with a single loop over an extracted char-array.Tests were added using a variety of synthetic inet addresses, and all available host addresses, verifying that they can all be converted to valid SNI hostnames.
Result:
Users see expected
CertificateException
: No subject alternative DNS name matching <...> found.