Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

Mon Aug 13 23:43:29 UTC 2018

Hi Xuelei,

 > Let's use two to emphasize the behaviors:
 > 1. both input and output streams should be closed in each side, and
 > 2. both client and server should perform #1.

SSLEngine.java
--------------
159:  Both sides (i.e. the peer) may not be a SSLEngine:

both the client and server applications should close the {@code 
SSLEngine} by calling {@link SSLEngine#closeInbound} and {@link 
SSLEngine#closeOutbound} and should send/receive any...
->
the application should close the {@code SSLEngine} by calling {@link 
SSLEngine#closeInbound} and {@link SSLEngine#closeOutbound} and should 
send/receive any...

SSLSocket.java
--------------
With all of the ways to close, trying to say anything here is going to 
get really ugly really fast.  SSLSocket.close(), InputStream.close() & 
OutputStream.close, and now Socket.shutdownInput() & 
Socket.shutdownOutput().  Even shutdownInput()/shutdownOutput() still 
throw UnsupportedOperationExceptions in JDK 10 all the way back to 1.4.

Can we just leave it at SSLSocket.close() and skip the 
Input/OutputStream.close() and shutdownInput/Output?  Or something like:

@apiNote
The TLS standard has evolved over the years, especially when it comes to 
closing Sockets and streams.  The most straightforward way to close a 
connection is for an application to call SSLSocket.close().  Some 
versions of the standard allow for independently closing the input or 
output sides of a connection, but may throw Exceptions if certain 
conditions aren't met.  If such a connection state is desired, the 
output stream should generally be closed before the input stream.

Once an {@code SSLSocket} is closed, it is not reusable: a new
{@code SSLSocket} must be created.

Brad

On 8/13/2018 11:50 AM, Xue-Lei Fan wrote:
> Hi Jamil,
> 
> Thanks for review.  One more step close to the integration.
> 
> On 8/13/2018 11:45 AM, Jamil Nimeh wrote:
>> Hi Xuelei,
>>
>>   * SSLSocket.java
>>       o 134: Nit - You can remove the first "both" in this sentence
>>         since you use it later with the input/output stream closure.
>>
> Let's use two to emphasize the behaviors:
> 1. both input and output streams should be closed in each side, and
> 2. both client and server should perform #1.
> 
> Thanks,
> Xuelei
> 
>> Looks good to me otherwise.
>>
>> --Jamil
>>
>> On 8/13/2018 11:31 AM, Xue-Lei Fan wrote:
>>> One more update:
>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.05/
>>>
>>> It is desired to make a note in SSLSocket and SSLEngine 
>>> specification, so that users have a good sense that an application 
>>> should close the input and output stream always.
>>>
>>> Updated for SSLEngine.java and SSLSocket.java only.  No changes in 
>>> other files.
>>>
>>> Please let me know your concerns by the end of today.
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 8/10/2018 4:02 PM, Jamil Nimeh wrote:
>>>> I'm good with the changes.
>>>>
>>>> --Jamil
>>>>
>>>> On 8/7/2018 5:24 PM, Xuelei Fan wrote:
>>>>> Hi Jamil,
>>>>>
>>>>> Thanks for comments.  Here is the updated webrev:
>>>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.04/
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>> On 8/7/2018 3:12 PM, Jamil Nimeh wrote:
>>>>>> Hi Xuelei, mostly small stuff:
>>>>>>
>>>>>>   * SSLEngineImpl.java
>>>>>>       o 717: Nit, inbout --> inbound
>>>>>>   * SSLEngineOutputRecord.java
>>>>>>       o 162, 169: Nit, applicatoin --> application
>>>>>>       o Same section: It looks like the "if" and "else if" clauses 
>>>>>> take
>>>>>>         the same actions with the same message.  Maybe just do "if
>>>>>>         (isClosed())" ?  Or were you planning to have different 
>>>>>> messages
>>>>>>         here?
>>>>>>   * SSLSocketOutputRecord.java
>>>>>>       o 58, 97, 204: Typo,  closedd --> closed
>>>>>>   * TLSEngineClosureTest.java
>>>>>>       o 2: Copyright date fix
>>>>>>       o 26: If this is a new test should the bug ID be different than
>>>>>>         8085979?
>>>>>>
>>>>>> --Jamil
>>>>>>
>>>>>>
>>>>>> On 08/07/2018 07:46 AM, Xuelei Fan wrote:
>>>>>>> New webrev:
>>>>>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.03/
>>>>>>>
>>>>>>> Thanks for a find of Tim Brooks, that the SSLEngine 
>>>>>>> inbound/outbound status is incorrect if closing during handshake. 
>>>>>>> The above webrev is trying to fix the problems. See more in the 
>>>>>>> OpenJDK thread:
>>>>>>> http://mail.openjdk.java.net/pipermail/security-dev/2018-August/017778.html 
>>>>>>>
>>>>>>>
>>>>>>> Please let me know your concerns before this Wednesday.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei
>>>>>>>
>>>>>>> On 8/3/2018 1:55 PM, Xuelei Fan wrote:
>>>>>>>> Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.02/
>>>>>>>>
>>>>>>>> In webrev.01, the socket close may be blocked by super class 
>>>>>>>> close synchronization.  Updated the SSLSocketImpl.java to use 
>>>>>>>> handshake only lock in the startHandshake() implementation.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>> On 8/1/2018 7:27 PM, Xuelei Fan wrote:
>>>>>>>>> Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.01/
>>>>>>>>>
>>>>>>>>> Integrated the fix for JDK-8208642, "Server initiated TLSv1.2 
>>>>>>>>> renegotiation fails if Java client allows TLSv1.3". 
>>>>>>>>> SSLHandshake.java is updated to use negotiated version so that 
>>>>>>>>> TLS 1.2 HelloRequest is acceptable in TLS 1.3 client side.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>> On 7/30/2018 10:24 AM, Xuelei Fan wrote:
>>>>>>>>>> <loop in net-dev as well>
>>>>>>>>>> Please let me know your concerns by the end of August 1st, 2018.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 7/30/2018 9:59 AM, Xuelei Fan wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> Please review the update for the TLS 1.3 half-close and 
>>>>>>>>>>> synchronization implementation:
>>>>>>>>>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> Unlike TLS 1.2 and prior versions, for TLS 1.3, the 
>>>>>>>>>>> close_notify is use to close the local write side and peer 
>>>>>>>>>>> read side only. After the close_notify get handles, the local 
>>>>>>>>>>> read side and peer write side may still be open.
>>>>>>>>>>>
>>>>>>>>>>> In this update, if an application calls 
>>>>>>>>>>> SSLEngine.closeInbound/Outbound() or 
>>>>>>>>>>> SSLSocket.shutdownInput/Output(), half-close will be used. 
>>>>>>>>>>> For compatibility, if SSLSocket.close() get called, a duplex 
>>>>>>>>>>> close will be tried.  In order to support duplex close, JDK 
>>>>>>>>>>> will use the user_canceled warning alert even the handshake 
>>>>>>>>>>> complete.
>>>>>>>>>>>
>>>>>>>>>>> In practice, an application may only close outbound even it 
>>>>>>>>>>> is intended to close the inbound as well, or close the 
>>>>>>>>>>> connection completely.  It works for TLS 1.2 and prior 
>>>>>>>>>>> versions.  But no more for TLS 1.3 because of the 
>>>>>>>>>>> close_notify behavior change in the TLS 1.3 specification.  
>>>>>>>>>>> The application may be hung and dead-waiting for read/close.  
>>>>>>>>>>> It could be solved by closing the inbound explicitly. In 
>>>>>>>>>>> order to mitigate the impact, a new System Property is 
>>>>>>>>>>> introduced, "jdk.tls.acknowledgeCloseNotify" if source code 
>>>>>>>>>>> update is not available.   If the System Property is set to 
>>>>>>>>>>> "true", if receiving the close_notify, a close_notify alert 
>>>>>>>>>>> will be responded.  It is a countermeasure of the TLS 1.3 
>>>>>>>>>>> half-close issues.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Xuelei
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>
>>>>
>>