RFR 8226338: Updates to Stateless Resumption

[Xuelei Fan]

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

NewSessionTicket.java
---------------------
-    189    if (m.remaining() < 14) {
+    189    if (m.remaining() < 9) {

I did not get the point of this update.  I think 14 should be fine so 
that to fail earlier if no sufficient data.


  317 public byte[] produce(ConnectionContext context) ...
  324 public byte[] produce(ServerHandshakeContext shc) ...
  433 public byte[] produce(PostHandshakeContext hc) ...

I did not get the point to use three methods here.  Read more inlines, 
please.

On 7/15/2019 4:04 PM, Anthony Scarpino wrote:
> 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.
> 
Did you mean send a new NST of the boundValues is changed?  What if the 
previous NST was used for resumption?

Thanks,
Xuelei

> 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
>>
>