SSLEngine weird behavior in 11+21?

[Xuelei Fan]

On 7/31/2018 12:13 PM, Simone Bordet wrote:
> Hi,
> 
> On Tue, Jul 31, 2018 at 5:10 PM Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>
>> Hi,
>>
>> On 7/31/2018 6:43 AM, Xuelei Fan wrote:
>>> Current jdk11 tip with your patch:
>>> 1. client.closeOutbound() then goes into NEED_WRAP.
>>> 2. Client wraps 24 bytes, result is CLOSED, then goes into NEED_UNWRAP.
>>> 3. Server unwraps 24 bytes, result is CLOSED, then goes into NEED_WRAP.
>>> 4. Server wraps 0 bytes and stays in NEED_WRAP (?)
>>
>> In my testing (OpenJDK,
>> test/jdk/javax/net/ssl/TLSv1/TLSEnginesClosureTest.java), for #4, the
>> server could wrap the close_notify alert message for TLS 1.2 and prior
>> versions (CLOSED/NOT_HANDSHAKING); and wrap data for TLS 1.3
>> (OK/NOT_HANDSHAKING, half-close).
>>
>> Are you using TLS 1.3 with no data in your test case in #4?  Because of
>> the half-close policy,  it may be the expected behavior if no
>> application data can be delivered.
> 
> The problem with 4 between TLS 1.2 and your latest patch is that
> before there was no need to call server.closeOutbound(): as the server
> received the close_notify from the client, it was moving to NEED_WRAP
> and if wrap() was called it would generate the close_notify reply.
> With your latest patch, you _have_ to call server.closeOutbound()
> otherwise 4 will always generate 0 bytes and spin loop.
> 
> That is why I prefer 2 to go into CLOSED+NOT_HANDSHAKING.
> When it goes into CLOSE+NEED_UNWRAP, the application will follow the
> instructions of SSLEngine and attempt an unwrap() immediately, while
> instead it should stop wrapping/unwrapping and write the close_notify
> to the server.
> 
Hm, I see your point.  Because of the half-close policy, I think the 
server could keep sending messages other than the close_notify alert. 
My concern of the CLOSED+NOT_HANDSHAKING is that it does not indicate 
the following operations.  Applications may call unwrap(), and run into 
problems.  It looks that it is tolerable to use wrap() with no 
application data (generate 0 bytes).

>> For TLS 1.3
>> ------------------------
>> Trying to close engines from Client to Server
>> Client wrapped 24 bytes.
>> Client handshake status is NEED_UNWRAP Result is CLOSED
>> Server unwrapping 24 bytes...
>> Server handshake status is NEED_WRAP Result is CLOSED
>> Server wrapped 16406 bytes.
>> Server handshake status is NEED_WRAP Result is OK
>> ------------------------
> 
> The above tells me that the server did not generate yet the
> close_notify reply because it is still in NEED_WRAP.
Yes, but it is the desired behavior to me.  For the TLS 1.3 half close 
policy, it is not expected to generate close_notify reply unless the 
server close its outbound.  So the server can keep sending the 
application data after the read close, as it write side is open.

The TLS 1.3 spec says:
   "Each party MUST send a "close_notify" alert before closing its write
    side of the connection, unless it has already sent some error alert.
    This does not have any effect on its read side of the connection."

And the SSLEngine.closeOutbound() spec says:
    "Signals that no more outbound application data will be sent on
     this SSLEngine."

I think it is fine to keep the write side open while the read side closed.

In this update, for TLS 1.2 connection, the duplex-close behavior is 
reserved: as the server received the close_notify, it moving to 
NEED_WRAP; and the a close_notify will be delivered, and then duplex 
close the connection.

However, for TLS 1.3 connection, if there is no call to 
server.closeOutbound(), the server write side keeps open even the read 
side has been closed.

In this update, for TLS 1.3, I tried to support half-close for:
1. SSLEngine.closeIn/Outbound()
2. SSLSocket.shutdownIn/Output()

and duplex-close for:
3. SSLSocket.close()

Unfortunately, it does introduce a compatibility issue that an existing 
application may not close both inbound and outbound if the connection is 
desired to be duplex-closed.  In order to mitigate the impact, a new 
System Property in introduced, "jdk.tls.acknowledgeCloseNotify".  This 
compatibility issues could be mitigated by closing both inbound and 
outbound explicitly, or set the property to "true".  If the property 
value is "true", a corresponding close_notify alert will be sent when 
receiving a close_notify alert, and therefore the connection will be 
duplex closed.

Does it make sense to you?

Xuelei

> Just to repeat myself I would prefer this:
> 
>> Client called closeOutbound() status is NEED_WRAP
>> Client wrapped 24 bytes.
>> Client handshake status is NOT_HANDSHAKING Result is CLOSED
>> Server unwrapping 24 bytes...
>> Server handshake status is NOT_HANDSHAKING Result is CLOSED
>> Server wrapped 16406 bytes.
>> Server handshake status is NOT_HANDSHAKING Result is OK
>> Server called closeOutbound() status is NEED_WRAP
>> Server wraps 24 bytes
>> Server handshake status is NOT_HANDSHAKING Result is CLOSED
> 
> Thanks!
>