RFR JDK-8240871: SSLEngine handshake status immediately after the handshake can be NOT_HANDSHAKING rather than FINISHED with TLSv1.3

[Anthony Scarpino]

On 5/13/20 1:44 PM, Xuelei Fan wrote:
> On 5/13/2020 9:41 AM, Anthony Scarpino wrote:
>> On 4/30/20 10:19 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Could I get the following update reviewed:
>>>      http://cr.openjdk.java.net/~xuelei/8240871/webrev.00/
>>>
>>> For TLS 1.3 full handshake, if the last handshake flight wraps the 
>>> Finished together with other handshake message, for example client 
>>> certificate, the flight could be wrapped and encrypted in one record 
>>> and delegated tasks would be used.  There is no chance to return 
>>> FINISHED handshake status with SSLEngine.(un)wrap(). However, per the 
>>> HandshakeStatus.FINISHED specification, this handshake status is only 
>>> generated by a call to SSLEngine.wrap()/unwrap() and it is never 
>>> generated by SSLEngine.getHandshakeStatus().
>>>
>>> In order to workaround this case for TLS 1.3, the FINISHED status 
>>> could present with SSLEngine.wrap() while delivering of the 
>>> NewSessionTicket post-handshake message.  If this post-handshake 
>>> message is not needed, a follow-on SSLEngine.wrap() should be called 
>>> to indicate the FINISHED handshake status.  Although this special 
>>> SSLEngine.wrap() should not consume or produce any application or 
>>> network data.
>>>
>>> I also clean up some debug log, names and code style a little bit.
>>>
>>> The update could be confirmed with Tomcat and Firefox in private 
>>> mode, as described in the bug description.  As this case happens only 
>>> when psk_key_exchange_modes does not present, which is not a behavior 
>>> supported by JDK, I did not find a workaround for a new regression 
>>> test yet.  I added the labels, noreg-external and noreg-hard.
>>>
>>> Thanks,
>>> Xuelei
>>
>> I do not fully understand the situation, mostly because of SSLEngine 
>> semantics.  In normal operation, does is HandshakeStatus.FINISHED 
>> returned when Finished is received or after the NewSessionTicket message?
> Not exactly.  For TLS 1.2, FINISHED will be returned with unwrap() of 
> the finished handshake message.  However, for TLS 1.3, FINISHED will be 
> returned any longer, because the finished handshake message is wrapped 
> with certificate message in one record.
> 
> For TLS 1.3:
> 1. client send certificate, certificate verify and finished handshake 
> message in one record.
> 2. server call unwrap(), and return NEED_TASK to handle the certificate 
> and certificate verify.
> 
> So, no more FINISHED for the unwrap() return.
> 
> It is fine if there is a after NewSessionTicket message.  The wrap() for 
> the post-handshake message will return FINISHED.
> 
> The bug reported is a special one that the Firefox is run in private 
> mode, which does not request NewSessionTicket.  So there is no 
> post-handshake generated and sent in server side.  Then, there is no 
> FINISHED can be used if applications depends on it.
> 
> To workaround the case, a dummy wrap() or unwrap() could be used to get 
> the FINISHED.  The wrap() or unwrap() actually do nothing but return the 
> FINISHED status.
>

I don't want to be problematic, but I don't really agree with creating 
dummy messages to generate wrap/unwrap operations in the TLS code.  If 
SSLEngine is doing something wrong with not fully reading the buffer, 
then I feel it's SSLEngine that should be fixed to handle the situation 
right.  Maybe not put the finished message or put all these messages 
together.

Maybe Brad may know of a way out of this problem?  If creating a dummy 
message is the only way to fix this, then I'm ok with it.  It is just 
not a clean fix in my mind.


>> My understanding would have been after Finished because NST is suppose 
>> to be a post handshake message.  So in this case there is no problem, 
>> correct?
>>
> Correct.
> 
>> I'm trying to figure out why you need an empty NST.  Is the problem 
>> when a number of messages are bundled together.  For example, the 
>> Finished message with a partial NST, then Finished isn't processed and 
>> both sides are waiting?  Or do both sides continue normal traffic, 
>> it's jut the HandshakeStatus.FINISHED is one operation behind?
>>
> It should be fine as empty NST is just a signal to indicate the next 
> call to wrap().  The next call to wrap() just use the signal for the 
> return of FINISHED status, not network data produced, delivered or 
> consumed.
> 
>>
>> One code comment so far:
>> 433:  The debug message purpose was to say the NST is a stateless 
>> ticket and not a preshared key.  Can we keep "stateless" in the message?
>>
> NewSessionTicket.java?  Sure, I may just want to shrink to one line.  It 
> was not intended.

shrinking the message is fine, it just needs to be clear which message 
ticket type got sent.

thanks

Tony

> 
> Xuelei