Code Review Request, JDK-8209333 Socket reset issue for TLS 1.3 socket close

[Jamil Nimeh]

Hi Xuelei, comments in-line.

On 12/17/2018 12:11 PM, Xue-Lei Fan wrote:
> On 12/17/2018 11:28 AM, Jamil Nimeh wrote:
>> Hi Xuelei, just a couple questions:
>>
>>   * SSLSocketImpl
>>       o 611: Are you sure conContext.inputRecord should go into a
>>         try-with-resources?  As far as I can tell, the inheritence chain
>>         is SSLSocketInputRecord->InputRecord and that directly or by
>>         extension implements the SSLRecord, Record and Closeable
>>         interfaces.  I thought that you needed to implement
>>         AutoCloseable in order to get that automatic closure benefit
>>         from leaving the try block.  Am I missing something?
> Closeable was updated to extend AutoCloseable, so using either 
> AutoCloseable or Closeable is fine for try-with-resources statement.
>     public interface Closeable extends AutoCloseable
Yep, I see what you're talking about.  I completely missed that 
interface relationship.
>
>>   * SSLContextTemplate
>>       o 505: Is there any benefit to initializing the SSLContext using a
>>         PKIXParameters object with the date fixed to sometime in between
>>         2018 and 2028 (the soonest expiration date of all the certs in
>>         the test).  This way you'd never have to worry about things
>>         expiring.  I know we don't do this in most of our tests, dunno
>>         why it just occurred to me now.
>>
> Good point!  It's a really nice enhancement!
>
> I had pushed this template in another bug fix. Let's do it separately. 
> I filed a new bug for tracking this enhancement.
>     https://bugs.openjdk.java.net/browse/JDK-8215509
Sounds good to me.  The review as a whole looks good.

--Jamil

>
> Thanks,
> Xuelei
>
>
>> The rest looks OK to me.
>>
>> --Jamil
>>
>> On 12/17/2018 9:52 AM, Xue-Lei Fan wrote:
>>> ping ...
>>>
>>> On 12/10/2018 1:14 PM, Xue-Lei Fan wrote:
>>>> Hi,
>>>>
>>>> Please review the TLS 1.3 half-close issue in JDK.
>>>>
>>>> http://cr.openjdk.java.net/~xuelei/8209333/webrev.00/
>>>>
>>>> While trying to duplex close a TLS connection upon the half-close 
>>>> policy, there might be pending receiving data in the closing side, 
>>>> and result in a TCP RST during closing. The TCP RST may then cause 
>>>> the peer reading failure.  For example:
>>>> 1. client and server establish a TLS 1.3 connection.
>>>> 2. server sending the post-handshake NewSessionTicket message.
>>>> 3. client send the application data, and then close the connection.
>>>> 4. as the client does not call to read the post-handshake message, 
>>>> the connection close will cause a TCP RST.
>>>> 5. server trying to read the client application data, but the 
>>>> socket may be impacted by the TCP RST, and the reading can fail.
>>>>
>>>> It would not be an issue any more if the client could read the 
>>>> post-handshake message, explicit or implicit.
>>>>
>>>> I would like applications consider to use half-close policy, and 
>>>> moving away from the duplex-close policy.
>>>>
>>>> The basic idea of the fix is trying to use up buffered network 
>>>> input before close the socket.  As is an implicit behavior to 
>>>> consume the post-handshake message, and mitigate the impact of it.
>>>>
>>>> This fix is not a perfect one.  It is just a workaround for 
>>>> duplex-close.  I'm open to hear more ideas.
>>>>
>>>> Thanks,
>>>> Xuelei
>>