SSLSessionImpl.java
-------------------
526 boolean checkSessionTicket(HandshakeContext hc) {
As the return value is a boolean type, the method is better to use
"isSomething". Otherwise, the meaning of the code
"!checkSessionTicket()" is clear and need to read the implementation code.
NewSessionTicket.java
---------------------
104 // opaque ticket<1..2^16-1>;
108 if (m.remaining() < 6) {
Per sectio 3.3, RFC 5077, line 104 should be:
opaque ticket<0..2^16-1>;
To be better understand the number 6, as you are already there, would
you please update line 104 as well?
I did not get the idea of the update of
T13NewSessionTicketMessage.T13NewSessionTicketMessage(). It looks like
that the updated code does not comply to TLS 1.3 specification of
NewSessionTicket.
453 NewSessionTicketMessage nstm;
454 nstm = new T12NewSessionTicketMessage(shc,
sessionTimeoutSeconds,
Maybe better to use one statement.
453 NewSessionTicketMessage nstm =
454 new T12NewSessionTicketMessage(shc,
sessionTimeoutSeconds,
499 if (nstm.ticket.length == 0) {
Empty ticket should be not allowed in TLS 1.3.
ServerHelloDone.java
--------------------
110 if (shc.statelessResumption) {
111
shc.handshakeConsumers.put(SSLHandshake.NEW_SESSION_TICKET.id,
112 SSLHandshake.NEW_SESSION_TICKET);
113 }
I think the NewSessionTicket is produced by server and consumed in
client side. I did not get the idea to have NST consumer in server
side. Am I missing something?
SessionTicketExtension.java
---------------------------
260 if (!hc.handshakeSession.checkSessionTicket(hc)) {
261 return new byte[0];
262 }
From the above line, I guess why you use empty ticket for TLS 1.3,
although empty ticket is not allowed in TLS 1.3.
I may suggest never use empty ticket. Always use a proper ticket in a
proper NewSessionTicket message, at least in format. When there is a
problem to retrieve the values, cache the session and prefer to use the
case rather than the ticket for session resumption. As may be a little
bit more interop friendly.
295 if (SSLLogger.isOn &&
SSLLogger.isOn("ssl,handshake")) {
296 SSLLogger.fine("Encryption failed." +
e.getMessage());
297 e.printStackTrace(); // Helps with session.write()
298 }
Line 297 does not use SSLLogger. It could be:
SSLLogger.fine("Encryption failed", e);
On 7/2/2019 10:35 AM, Anthony Scarpino wrote:
Hi,
I need a code review on some updates to the stateless resumption.
1) Changing peerSupportedSignAlgs from a String[] to Collection[]
2) Additional items added to the stateless ticket
3) Not provide a stateless ticket when the masterkey is not accessible
(FIPS) or when boundValues are used
I did not whether the update works if bound values are set after the
handshake completed (i.e., after the NewSessionTicket has been produced
and used). Did you have a test for the case?
Thanks,
Xuelei
4) Be more graceful in case of stateless ticket generation failure
http://cr.openjdk.java.net/~ascarpino/8226338/webrev.00/
Tony
