Updated webrev looks fine to me.
--Sean
On 04/20/2016 11:18 AM, Xuelei Fan wrote:
Thanks for the comments, all looks reasonable to me.
Updated webrev: http://cr.openjdk.java.net/~xuelei/8144566/webrev.02/
Thanks,
Xuelei
On 4/20/2016 9:10 PM, Sean Mullan wrote:
* SSLSocketImpl.java
2100 // ONLY used by ClientHandshaker for the server hostname during
handshaling
typo: handshaking
2114 synchronized private void useImplicitHost(boolean noSniUpdate) {
the modifier order should be "private synchronized ..."
See: http://cr.openjdk.java.net/~alundblad/styleguide/#toc-modifiers
2115 if ((host != null) && (host.length() != 0)) {
2116 return;
2117 }
This seems redundant. You already check this before you call the method.
2150 // No explicitly specified hostname, no sserver name
indication.
typo: server
2610 if (sniNames.isEmpty()) {
2611 // need no SNI extension
2612 noSniExtension = true;
2613 } // Otherwise, need not to set noSniExtension
2614
Could write more compactly as:
noSniExtension = sniNames.isEmpty();
same comment on lines 2620-4
* BestEffortOnLazyConnected.java, ImpactOnSNI.java
2 * Copyright (c) 2015, Oracle and/or its affiliates. All rights
reserved.
2016
I know this is just a test, but it seems like most of these methods and
fields should be private.
--Sean
On 04/06/2016 06:52 PM, Xuelei Fan wrote:
Ping ...
On 1/20/2016 9:14 AM, Xuelei Fan wrote:
Ping ...
On 12/8/2015 8:12 PM, Xuelei Fan wrote:
Good catch!
I copied the comment here:
----------
SocketFactory sslsf = SSLSocketFactory.getDefault();
SSLSocket ssls = (SSLSocket) sslsf.createSocket();
ssls.connect(new InetSocketAddress(
"bugs.openjdk.java.net", 443), 0);
ssls.startHandshake();
No SNI is sent in this case.
----------
Although this is not a regression, but this is a very good catch.
However, I don't think the code path is a best practice, as the actual
behavior depends on providers behaviors and platform behaviors too
much.
I would recommend to use SSLParameters.setServerNames() to specify
the
server names explicitly.
But, best effort should be made for the default server names for such
code paths. Here is the webrev that also fixes this code path issue:
http://cr.openjdk.java.net/~xuelei/8144566/webrev.01/
The fix gets a little bit complicated because of the need to consider
the SSLParameters.setServerNames() impact in the code path:
SSLSocket ssls = (SSLSocket) sslsf.createSocket();
// before the connection, need to consider the impact:
// Get the SSLParameters from the socket, or create on the fly?
// Call ssls.setSSLParameters(params) or not?
ssls.connect(socketAddress);
// after the connection, need to consider the impact:
// Call ssls.setSSLParameters(params) or not?
Basically, the implementation honors the server name set by
SSLParameters.setServerNames().
Bernd's comment:
Isnt this codepath used as a workaround for dynamically disabling
SNI? Using connect(host..) instead of SSLSocket(host) was at
least a common, well known workaround.
If the code path is really used in practice, the
SSLParameters.setServerNames() would better be called actually to
disable SNI explicitly.
SocketFactory sslsf = SSLSocketFactory.getDefault();
SSLSocket ssls = (SSLSocket) sslsf.createSocket();
ssls.connect(new InetSocketAddress(
"bugs.openjdk.java.net", 443), 0);
sslParameters.setServerNames(emptyList);
ssls.setSSLParameters(sslParameters);
ssls.startHandshake();
Thank you, Bernd and Brad, for the feedback.
Thanks,
Xuelei
On 12/8/2015 8:21 AM, Bradford Wetmore wrote:
Please see my comment in the bug. I haven't verified this, but it
seems
the problem might be generic to the codepath through SSLSocket, not
just
Https.
Brad
On 12/6/2015 4:32 AM, Xuelei Fan wrote:
Hi,
Please review the update for JDK-8144566:
http://cr.openjdk.java.net/~xuelei/8144566/webrev.00/
For HttpsURLConnection, the server name may be set after the TLS
connection and handshake has been initialized. As may result in that
the server name does not present at TLS ClientHello messages.
This fix resets the server name for the initialized handshake for
above
cases.
Thanks,
Xuelei
