Code Review Request 8144566, Custom HostnameVerifier disables SNI extension

[Xuelei Fan]

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