RFR 8226338: Updates to Stateless Resumption

[Anthony Scarpino]

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