RFR 8226338: Updates to Stateless Resumption

[Xuelei Fan]

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