SSLEngine weird behavior in 11+21?

[Xuelei Fan]

The Status.CLOSED specification is defined as "The operation just closed 
this side of the SSLEngine, or the operation could not be completed 
because it was already closed.".   My reading of the spec, the CLOSED 
status means half-close.   If wrap() status is CLOSED, it means write 
side close; and unwrap() CLOSED is for read side close.

I may prefer to:
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.closeOutbound() then goes into NEED_WRAP.
5. Server wraps 24 bytes, result is CLOSED, then goes into NOT_HANDSHAKING.
6. Client unwraps 24 bytes, result is CLOSED, then goes into 
NOT_HANDSHAKING.

Xuelei

On 7/31/2018 6:43 AM, Xuelei Fan wrote:
> Hi Simone,
> 
> Thanks for the quick feedback.  Read more in-lines, please.
> 
> On 7/31/2018 3:10 AM, Simone Bordet wrote:
>> Hi,
>>
>> On Tue, Jul 31, 2018 at 11:39 AM Simone Bordet 
>> <simone.bordet at gmail.com> wrote:
>>>
>>> Hi,
>>> On Mon, Jul 30, 2018 at 8:08 PM Xuelei Fan <xuelei.fan at oracle.com> 
>>> wrote:
>>>> Would you mind look at the code I posted in the following thread:
>>>> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017708.html 
>>>>
>>>
>>> JDK 11+21:
>>> 1. client.closeOutbound() then goes into NEED_WRAP.
>>> 2. Client wraps 24 bytes, result is CLOSED, then goes into 
>>> NOT_HANDSHAKING (?)
>>> 3. Server unwraps 24 bytes, result is CLOSED, then goes into NEED_WRAP.
>>> 4. Server wraps 24 bytes, result is CLOSED, then goes into 
>>> NOT_HANDSHAKING.
>>> 5. Client unwraps 0 bytes (?)
>>>
>>> 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 (?)
>>>
>>> I don't think this is right.
>>>
>>> While I previously complained about step 2 going into NOT_HANDSHAKING,
>>> if you now support full half close, then I think this may be
>>> reasonable, as the server may still send data and only later issue a
>>> close_notify.
>>> However, NEED_UNWRAP like it is now is also reasonable.
>>>
>>> At step 3, after the server unwraps the close_notify, the server
>>> should either stay in NOT_HANDSHAKING *and* require a call to
>>> closeOutbound() (which will move the state to NEED_WRAP), or it should
>>> go into NEED_WRAP *and* produce the close_notify.
>>> As it is now, SSLEngine tells the application to wrap(), but it wraps
>>> 0 bytes, but tells again the application to wrap(), but still produces
>>> 0 bytes, so it's going to be a tight spin loop - not good.
>>
>> For completeness, calling server.closeOutbound() at step 4. correctly
>> moves SSLEngine into NEED_WRAP and a subsequent wrap() produces the 24
>> bytes of the close_notify and result CLOSED, then goes into
>> NOT_HANDSHAKING.
>>
>> I think the current behavior (with your patch) needs to be fixed.
>> Since you implemented half-close, my preference would be this:
>>
>> 1. client.closeOutbound() then goes into NEED_WRAP.
>> 2. Client wraps 24 bytes, result is CLOSED, then goes into 
>> NOT_HANDSHAKING.
> It might be a problem for application to make a right decision if using 
> CLOSED status and NOT_HANDSHAKING handshake status together.
> 
> As the write side is close, the client now only be able to receiving 
> data from peer.  I think NEED_UNWRAP may be more appropriate for 
> application development.
> 
> However, the CLOSED status may be confusing as the client may still want 
> to read something later.
> 
> I may prefer to use OK/NEED_UNWRAP.  What do you think?
> 
>> 3. Server unwraps 24 bytes, result is CLOSED, then goes into 
>> NOT_HANDSHAKING.
> Same reason as above, I may prefer to use OK/NEED_WRAP.
> 
>> 4. server.closeOutbound() then goes into NEED_WRAP.
>> 5. Server wraps 24 bytes, result is CLOSED, then goes into 
>> NOT_HANDSHAKING.
>> 6. Client unwraps 24 bytes, result is CLOSED, then goes into 
>> NOT_HANDSHAKING.
>>
> Agreed.
> 
> If no objections, I will make the update:
> 
> 1. client.closeOutbound() then goes into NEED_WRAP.
> 2. Client wraps 24 bytes, result is OK, then goes into NEED_UNWRAP.
> 3. Server unwraps 24 bytes, result is OK, then goes into NEED_WRAP.
> 4. server.closeOutbound() then goes into NEED_WRAP.
> 5. Server wraps 24 bytes, result is CLOSED, then goes into NOT_HANDSHAKING.
> 6. Client unwraps 24 bytes, result is CLOSED, then goes into 
> NOT_HANDSHAKING.
> 
> Thanks,
> Xuelei