RFR 8211018: Session Resumption without Server-Side State

[Anthony Scarpino]

On 5/24/19 8:51 AM, Xuelei Fan wrote:
> SSLSessionContext.java
> ----------------------
> As comment in the CSR review thread, I may not define the 
> jdk.tls.server.sessionTicketTimeout property, and use one session 
> timeout (SSLSessionContext.getSessionTimeout()) instead.
> 
> The ticket timeout may be not necessary, read more please.
> 
> SessionTicketExtension.java
> ---------------------------
> As comment in the CSR review thread, I may not define the key timeout. 
> Instead, I may use key usage limit.  As it is stateless server, I may 
> not use the stateless session timeout as well, for less states.  The key 
> rotation scheme might be able to take place of timeout.  For client 
> side, the session context session timeout and management may be 
> sufficient for the client side cache cleanup.

The spec says the keys need to be rotated regularly.  Of course 
"regularly" is up for interpretation.  If a usage limit is implemented 
and the server is not frequently used, it's possible to have the same 
key used for the entire span of the session timeout.  I don't feel that 
is often enough.

> 
> StatelessKey.gcmspec:
> If I read it right, the same key and iv are used for every ticket 
> encryption.  This behavior is vulnerable.  The IV should be unique for 
> each encryption.

GCM as a whole was probably the wrong choice

> 
> I would like to avoid to create thread in the fundamental API 
> implementation if possible.  As the thread (KeyState.run()) is for 
> invalid key cleanup only, the cleanup can be moved to the get methods.

How is this thread in the API?  It is only started when the key is 
generated and not by a API call and it's the internal parts of the 
implementation.

> 
> For each StatelessKey, one thread is created.  I was wondering if there 
> are any chance that there are multiple threads to manage the stateless 
> keys, and potential memory leak?

Every time a new session key.  Depending on the property the default is 
once an hour. It's possible multiple threads can be used if the timing 
is just right, but that only mean it runs through the list a few times. 
Hardly a performance issue.  Each key is put in the hashmap, so there is 
no memory leak.  Maybe I can trim it down to a static thread that runs 
when needed, that would eliminate the multiple threads, but maybe add 
more locks.

> 
> Anyway, I will try to avoid to use internal thread.  I may use a key 
> rotation scheme similar to cookie manager (use two keys, one for legacy 
> and one for the current key, see HelloCookieManager.java).

The thread is one piece for clean up, if I remove it from the thread, I 
will need to be put into the main code path.

The keys have a longer life, the defaults are 1 hour and can last up to 
24hrs.  Those can be changed with the properties in a way that would 
require more keys to be stored.  So rotating keys will involve some sort 
of Map or storage object.  HelloCookieManager just needs to remember the 
previous secret and for a short period of time as the response from the 
client could be only a few seconds.

As for the for the lock mechanism around the rotation.  I don't see much 
difference between you using the ReentrantLock vs the synchronized block 
I have.  I'm trying to minimize the amount of time in the lock 
generating the key.  I'm not so concerned about a few extra keys being 
generated.  Putting in more locks just means more chances for the main 
code path getting stopped.

I don't see the significants to this other than being personal coding 
styles.

> 
> If the key is invalid, an exception will be thrown and then fail back to 
> full handshake.  Exception thrown and catch are expensive, I may not 
> exception for the failback to full handshake.

If there is an exception the full handshake is going to be more 
expensive than the exception.  I could probably code away the exception.

> 
> Let's discuss these issues firstly before we moving on with more code 
> review.
> 
> Hope it helps.
> 
> Thanks,
> Xuelei
> 
> 
> On 5/21/2019 5:35 PM, Anthony Scarpino wrote:
>> Hi all,
>>
>> I’ve updated in-place some recent changes due to some additional testing
>>
>> Tony
>>
>>> On May 16, 2019, at 2:30 PM, Anthony Scarpino 
>>> <anthony.scarpino at oracle.com> wrote:
>>>
>>> I'm asking for a review of this rather large change to add support 
>>> stateless tickets in the TLS 1.3 5077 RFC.
>>> https://bugs.openjdk.java.net/browse/JDK-8211018
>>>
>>> http://cr.openjdk.java.net/~ascarpino/stateless/webrev.00
>>>
>>> thanks
>>>
>>> Tony
>>