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

Fri May 29 17:17:59 UTC 2020

Ok thanks.. no further questions.

Tony

On 5/29/20 9:54 AM, Xuelei Fan wrote:
> On 5/29/2020 8:59 AM, Anthony Scarpino wrote:
>> 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
>>
> Oops, I still missed the two above.  Updated in my local workspace.
> 
>> 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?
> Good question.  I double checked that the session ticket delivery and 
> writeRecord() are synchronized.  The set and use of 
> needHandshakeFinishedStatus are thread safe.
> 
>> Would it be better to only have the writeRecord() check?
>>
> The two block are used for different purpose.
> 
> If the client support session ticket 
> (psk_key_exchange_modes.psk_dhe_ke), the server will response with a 
> session ticket in the last handshake wrap().  The wrap() method will 
> return FINISHED status.  For this case, as the FINISHED status could be 
> caught, there is no need to have an additional wrap()  to return the 
> FINISHED handshake status any longer. The block in NewSessionTicket 
> 464-470 is used to indicate the case.
> 
> If the Firefox run in private mode, the psk_key_exchange_modes extension 
> is not used, and the server will not response with session ticket. Then, 
> the FINISHED status get missed if Certificate + CertificateVerify + 
> Finished hanshake messages wrapped in one record.  The block in 
> SSLEngineImpl.writeRecord() 167-172 is used to handle this case.
> 
> Thanks,
> Xuelei
> 
>> 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
>>