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

[Jamil Nimeh]

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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180813/3f5b6b1c/attachment.htm>