RFR 8226338: Updates to Stateless Resumption

[Anthony Scarpino]

I've updated the webrev

http://cr.openjdk.java.net/~ascarpino/8226338/webrev.01/

As I need to push this by wednesday, please review it soon.

There are fixes for the comments that were made, added a NST for 
boundVaules, a very basic test to make sure the post handshake NST is 
sent after boundValues have changed.

There is no change to invalidate() as these is not enough time to 
address that.

Tony

On 7/7/19 9:14 PM, Anthony Scarpino wrote:
> 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
>