Code Review Request 8144566, Custom HostnameVerifier disables SNI extension

[Sean Mullan]

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