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
>>>>>>
>>>>
>>>
>>
