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

Fri May 29 15:59:46 UTC 2020

Thanks for the changes Xuelei, it is much clearer to me now.  Two minor 
things and a question.

NewSessionTicket:323.  The comment talks about an empty session ticket,

SSLEngineImpl:167.  Comment uses old variable name

One question, NewSessionTicket 464-470 and SSLEngineImpl.writeRecord() 
167-172 both set needHandshakeFinishedStatus false. If the NST code sets 
needHandshakeFinishedStatus before the asynchronous call to 
writeRecord(), couldn't that not change the status?  Would it be better 
to only have the writeRecord() check?

thanks

Tony

On 5/27/20 8:44 PM, Xuelei Fan wrote:
> 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