SSLEngine weird behavior in 11+21?

[Xuelei Fan]

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