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
