On 7/4/19 10:14 PM, Xuelei Fan wrote:
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.
I'll see if I can come up with a "is*" name
NewSessionTicket.java
---------------------
104 // opaque ticket<1..2^16-1>;
108 if (m.remaining() < 6) {
Per section 3.3, RFC 5077, line 104 should be:
opaque ticket<0..2^16-1>;
I'm glad you saw that because it was confusing me why a zero buffer was
allowed in 5077
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.
So I assume you are talking about the zero length ticket. I never
noticed that 1.3 changed behavior away from zero length tickets. It
would appear the spec writings made every attempt to make NST usage
confusing between versions.
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?
Hmm.. I remember making this change, and I thought it was a good idea,
but clearly it does nothing.
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.
Yes, the 1.3 non-zero length sure complicates the code.. Very unfortunate.
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);
I didn't noticed logger accepted exceptions.
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?
It was my understanding from previous discussions that we were going to
take a wait and see approach with boundValues and NST.
When I took a glance at how boundValues were added, they appear to be
outside of the normal flow of the ConnectionContext.
Thanks,
Xuelei
4) Be more graceful in case of stateless ticket generation failure
http://cr.openjdk.java.net/~ascarpino/8226338/webrev.00/
Tony
