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

Thu May 28 03:44:12 UTC 2020

Tony and I had a private chat, and I understand his concerns better now. 
  The name of the variable TransportContext.needEmptySessionTicket is 
confusing.  I updated it to "needHandshakeFinishedStatus" per the 
suggestion, together with other change according to the comments.

Here is the new webrev:
    http://cr.openjdk.java.net/~xuelei/8240871/webrev.01/

Thanks,
Xuelei

On 5/26/2020 2:40 PM, Xuelei Fan wrote:
> On 5/26/2020 1:26 PM, Anthony Scarpino wrote:
>> 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.
> No dummy message created.  Only need to call wrap() or unwrap(), but not 
> data consumed or generated by the call to wrap() or unwrap(), no 
> application data consumed, no network data consumed, no application data 
> generated, no network data generated.
> 
>> 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.
> It is not caused by SSLEngine that does not fully reading the buffer. 
> Let me try again about what's the problem.
> 
> The client (Firefox) sends, Certificate and CerticateVerify and Finished 
> handshake messages in one record.  The record is encrypted.
> 1. One call to SSLEngine.unrwap() will read the record, and decrypt the 
> record.
> 2. One call to SSLEngine.unwrap() cannot read the Certificate and 
> CerticateVerify handshake message only, without reading the Finished 
> handshake message.  It means the unwrap() method will consume the record 
> data fully for all three handshake messages.
> 3. The Certificate and CerticateVerify should be handled in delegated 
> tasks, so the call to SSLEngine.unwrap() return NEED_TASK.
> 4. As the SSLEngine.unwrap() return NEED_TASK, it cannot return the 
> FINISHED status at the same time.
> 5. The FINISHED status is only be able to return with SSLEngine.unwrap() 
> or SSLEngine.wrap(), and cannot returned from delegated tasks.  So the 
> SSLEngine.getHandshakeStatus() after the tasks cannot be used to 
> indicate the FINISHED status.
> 6. Then FINISHED status is missed, applications like Tomcat run into 
> problem, like the bug described.
> 
> That's the problem of the bug as far as I can see.  I agreed it is not 
> good to have an additional wrap() or unwrap() that did nothing, but I 
> have no better idea.  It would be nice if we could have a fix in JDK 15, 
> considering the impact on Tomcat and Firefox.  I'm open if there is 
> alternative solution or workaround.
> 
> 
>> 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.
>>
> Added back.
> 
> Xuelei