8369950: TLS connection to IPv6 address fails with BCJSSE due to IllegalArgumentException#28577
8369950: TLS connection to IPv6 address fails with BCJSSE due to IllegalArgumentException#28577sercher wants to merge 9 commits intoopenjdk:masterfrom
Conversation
…galArgumentException
|
👋 Welcome back schernyshev! A progress list of the required criteria for merging this PR into |
|
@sercher This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 161 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@djelinski, @dfuch, @myankelev, @vy) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
|
Please make sure to run tier2 before integrating. |
@djelinski Thank you for review! |
@dfuch Thanks Daniel. The new test is in tier2, so far it passes in macOS and Linux. Windows takes more time with tier2... |
|
/integrate |
|
Hello Sergey, the copyright year on |
| if (!(s instanceof SSLSocketImpl) && | ||
| !IPAddressUtil.isIPv4LiteralAddress(host) && | ||
| !(host.charAt(0) == '[' && host.charAt(host.length() - 1) == ']' && | ||
| IPAddressUtil.isIPv6LiteralAddress(host.substring(1, host.length() - 1)) |
There was a problem hiding this comment.
The host value here comes from URL.getHost() which specifies that it returns an IPv6 address enclosed in []brackets. So what you have here looks fine to me.
One additional thing I would suggest is to make this protected String host field of this class final. It currently gets assigned in the constructor of the HttpClient and HttpsClient and making this final would give an extra assurance that its value will always be coming from URL.getHost() call.
These 2 sun.net.www.http.HttpClient and HttpsClient classes are internal to the JDK, so changing this protected field to final shouldn't cause any issues for application code.
There was a problem hiding this comment.
@jaikiran I think this deserves a separate issue. I could file a bug for this.
There was a problem hiding this comment.
Yes, it's OK with me if you don't change this field to final in this PR.
| * terminate all hung threads after its timeout has expired, | ||
| * currently 3 minutes by default, but you might try to be | ||
| * smart about it.... | ||
| */ |
There was a problem hiding this comment.
I am not too familiar with these style of tests, but it looks like there are several similar ones in the javax/net/ssl area. Would you know if these comments are still accurate and up to date? In other words, do we need these comments in this test?
There was a problem hiding this comment.
Let's clean this up in a follow-up PR. There's 60+ files with the same comment.
There was a problem hiding this comment.
Yes, doing this as a separate PR is fine with me.
There was a problem hiding this comment.
@jaikiran Let me clean up the comments that are not anymore accurate in this particular PR. I personally try to avoid this type of changes that only touch the whitespace or comments blocks, including the copyright header ones, because they increase the number of connections between unrelated changes, that makes the fix less portable.
There was a problem hiding this comment.
@sercher, it's OK if you leave the test in the current form. Just fixing the @summary test on the test definition should be enough. I have spoken to others and the agreement is that since we already have similar tests in this area, it probably is a better thing to let this test stay in this manner instead of devicing some new way to test this. Sorry about the previous guidance to update these test comments.
| BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(sslOS)); | ||
| bw.write("HTTP/1.1 200 OK\r\n\r\n\r\n"); | ||
| bw.flush(); | ||
| Thread.sleep(5000); |
There was a problem hiding this comment.
Is the sleep() necessary for this test?
There was a problem hiding this comment.
Again, copy/paste. Deserves a separate PR to clean up.
| * Fork off the other side, then do your work. | ||
| */ | ||
| SubjectAltNameIPv6() throws Exception { | ||
| if (separateServerThread) { |
There was a problem hiding this comment.
Are these if/else blocks needed or could we simplify the test to just expect the server and client to run as separate threads?
There was a problem hiding this comment.
I spoke to Daniel and I agree with him that it's OK to leave this in current form and if needed clean up as a follow up when doing the same for the rest of these tests.
| * @test | ||
| * @bug 8369950 | ||
| * @library /test/lib | ||
| * @summary TLS connection to IPv6 address fails with BCJSSE due to IllegalArgumentException |
There was a problem hiding this comment.
Nit - as per the jtreg tag order recommendation, the @summary should come before the @library https://openjdk.org/jtreg/tag-spec.html#ORDER
There was a problem hiding this comment.
Also, this test doesn't exercise the BouncyCastle provider. Would it be better to change this line to say:
@summary Test that the HttpsURLConnection does not set IPv6 address literals for SNI hostname during TLS handshake
There was a problem hiding this comment.
Nit - as per the jtreg tag order recommendation, the
@summaryshould come before the@libraryhttps://openjdk.org/jtreg/tag-spec.html#ORDER
Thanks @jaikiran , i will update the order and the summary text. The BouncyCastle came here from the title of the issue.
Thanks @jaikiran. That's correct. I will update the headers. |
openjdk
bot
removed
sponsor
| !(host.charAt(0) == '[' && host.charAt(host.length() - 1) == ']' && | ||
| IPAddressUtil.isIPv6LiteralAddress(host.substring(1, host.length() - 1)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The same thought occurred to me but I'd rather keep refactoring at a minimum in this PR.
| } | ||
|
|
||
| SSLSocketFactory sf = new SimpleSSLContext().get().getSocketFactory(); | ||
| URL url = new URL("https://[::1]:" + serverPort + "/index.html"); |
There was a problem hiding this comment.
| URL url = new URL("https://[::1]:" + serverPort + "/index.html"); | |
| URL url = | |
| new URI("https://[::1]:" + serverPort + "/index.html").toURL(); |
There was a problem hiding this comment.
I think the block is better readable without it.
There was a problem hiding this comment.
It's deprecated since version 20 I think
There was a problem hiding this comment.
Ahh, ok, i read it wrong, apologies. URI is the modern one, exactly.
| void doServerSide() throws Exception { | ||
| SSLServerSocketFactory sslssf = | ||
| new SimpleSSLContext().get().getServerSocketFactory(); | ||
| SSLServerSocket sslServerSocket = |
There was a problem hiding this comment.
Nit: could you please keep the lines under 80 characters long. There are a few instances in this file
| */ | ||
| SubjectAltNameIPv6() throws Exception { | ||
| startServer(); | ||
| startClient(); |
There was a problem hiding this comment.
Do you think it might be better to call doClientSide here directly?
| /* | ||
| * Is the server ready to serve? | ||
| */ | ||
| volatile static boolean serverReady = false; |
There was a problem hiding this comment.
I think this might be better to make this a CountDownLatch
| } | ||
|
|
||
| void startServer() throws Exception { | ||
| serverThread = new Thread() { |
There was a problem hiding this comment.
| serverThread = new Thread() { | |
| serverThread = new Thread(() -> { |
What do you think?
There was a problem hiding this comment.
This makes sense. I will add it manually in a single commit.
|
/contributor add @myankelev |
|
@sercher |
|
Thanks for considering me as a contributor to this ticket! I really appreciate it. Overall the ticket looks good, thank you for your updates! |
|
@myankelev Thank you for in-depth review. |
| BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(sslOS)); | ||
| bw.write("HTTP/1.1 200 OK\r\n\r\n\r\n"); | ||
| bw.flush(); | ||
| sslSocket.close(); |
There was a problem hiding this comment.
Now that you removed the Thread.sleep, you can't close the socket without reading the request off the input stream; it will cause intermittent connection reset failures on Windows. You can use the readOneRequest method.
There was a problem hiding this comment.
Thanks @djelinski for spotting this. Added the method.
test/jdk/javax/net/ssl/HttpsURLConnection/SubjectAltNameIPv6.java
Outdated
Show resolved
Hide resolved
test/jdk/javax/net/ssl/HttpsURLConnection/SubjectAltNameIPv6.java
Outdated
Show resolved
Hide resolved
| * @comment Add -Djavax.net.debug=all to the following line to enable SSL debugging | ||
| * @run main/othervm SubjectAltNameIPv6 | ||
| * @comment Insert -Djavax.net.debug=all into the following lines to enable SSL debugging | ||
| * @run main/othervm SubjectAltNameIPv6 127.0.0.1 |
There was a problem hiding this comment.
Since we also test against IPv4, I'd remove the mention of IPv6 from the class name (e.g., SubjectAltNameIP) and @summary.
| */ | ||
| conn.setSSLSocketFactory(wrapSocketFactory(sf)); | ||
| conn.setSSLSocketFactory(wrapSocketFactory(sf, | ||
| sslSocket -> clientSSLSocket = sslSocket)); |
There was a problem hiding this comment.
Shall we first assert that clientSSLSocket == null before assignment?
There was a problem hiding this comment.
The method doClientSide() is called from constructor, the clientSSLSocket is non-static and was set to null. Therefore, it's the only assignment of clientSSLSocket per instance. Or do you mean the check must be in the the lambda-expr?
There was a problem hiding this comment.
I mean something like:
| sslSocket -> clientSSLSocket = sslSocket)); | |
| sslSocket -> { | |
| assertNull(clientSSLSocket); | |
| clientSSLSocket = sslSocket; | |
| })); |
To avoid double-assignment and eventually causing verification of the wrong value.
There was a problem hiding this comment.
I made assertEquals instead, otherwise it shows an incorrect message - it prints the left hand operand as "expected" value, which in the case of assertNull is in the right hand operand.
| conn.getInputStream(); | ||
|
|
||
| var sniSN = clientSSLSocket.getSSLParameters().getServerNames(); | ||
| if( sniSN != null && !sniSN.isEmpty()) { |
test/jdk/javax/net/ssl/HttpsURLConnection/SubjectAltNameIP.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel Jelinski <djelinski1@gmail.com>
Co-authored-by: Volkan Yazıcı <volkan.yazici@oracle.com>
|
/integrate |
|
/sponsor |
|
Going to push as commit 7da9153.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
6 participants
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
jdk/src/java.base/share/classes/javax/net/ssl/SNIHostName.java
Lines 44 to 66 in 873f8a6
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
jdk/src/java.base/share/classes/sun/security/ssl/Utilities.java
Lines 110 to 116 in 873f8a6
Testing:
BCJSSE standard
BCJSSE FIPS
Progress
Issue
Reviewers
Contributors
<myankelevich@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28577/head:pull/28577$ git checkout pull/28577Update a local copy of the PR:
$ git checkout pull/28577$ git pull https://git.openjdk.org/jdk.git pull/28577/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28577View PR using the GUI difftool:
$ git pr show -t 28577Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28577.diff
Using Webrev
Link to Webrev Comment