Code Review Request 8144566, Custom HostnameVerifier disables SNI extension

[Xuelei Fan]

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