Code Review Request 8144566, Custom HostnameVerifier disables SNI extension

[Sean Mullan]

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