Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received
Xue-Lei Fan
xuelei.fan at oracle.com
Mon Aug 13 18:50:23 UTC 2018
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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>
>>>
>
More information about the security-dev
mailing list