RFR 8211018: Session Resumption without Server-Side State

[Xuelei Fan]

Hi Tony,

There are a lot of pretty good designs in the update, for example, the 
cooperation of the session timeout and key rotation timeout.

My following comments are mainly about the issues I can find.  Most of 
them are minors.

On 6/3/2019 5:42 PM, Anthony Scarpino wrote:
> http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02
SessionTicketExtension.java:
----------------------------
   81     private static int currentKey = new SecureRandom().nextInt();
When I read the code, the first question came to me was: if a integer is 
strong enough to be a key.  It took me a while to understand that 
currentKey is a key id rather than a key.  Would you mind rename the 
filed with a straightforward name?  For example currentKeyId?

   83     // Time in milliseconds until key is invalid and will be 
destroyed.
Looks like there is no code related to this comment line.  Is it a 
duplicated line?


   93                     keyTimeout = TIMEOUT_DEFAULT;
Nice to have a debug log for incorrect setting.

   78     private static int keyTimeout = TIMEOUT_DEFAULT;
I did not find the assignment to this filed other than the 
initialization static block.  Could this filed be 'final'?

  117             byte[] k = new byte[KEYLEN];
  118             random.nextBytes(k);
  119             key = new SecretKeySpec(k, "AES");
I think the stateless key is pretty important.  Plaintext key may not 
meet the security requirement in some circumstances.

What do you think if using KeyGenerator APIs?

   80     private static final HashMap<Integer, StatelessKey> keystate = 
new HashMap<>();
This 'keystate' is a global variable.  As means different SSLContext 
shares the same stateless keys.  Different SSLContext could use 
different session timeout.  I would prefer to make it a SSLContext 
sensible variable, and then when SSLContext object is 
releases/destroyed, and stateless keys will be released/destroyed as well.

  108     static final class StatelessKey {
Could this class be private?

  149             StatelessKey ssk = getKey(hc, currentKey);
The above line will check if the key is invalid via getKey() method. 
Because of the implementation details of isExpired() and isInvalid(), I 
think it should be sufficient if not checking isInvalid().  Otherwise, I 
was little bit confusing about the logic here.

  166             synchronized (keystate) {
If needed, I would suggest to replace 'synchronized' with explicit locks 
(java.util.concurrent.locks). For more information, please refer to 
JDK-8221882 or Project Loom.

  186             cleanup(getSessionTimeout(hc));
  191         static void cleanup(int sessionTimeout) {
  202                     keystate.remove(i);
If different SSLContext use different session timeout, the key may be 
valid in one context but not in another.  If look the three lines above 
together, the key cleanup method may not work as expected.

I would suggest to use SSLContext sensible key state cache.  For 
example, put the cache in the SSLContextImpl instance.

BTW, would you mind limit each line no more than 80 characters?

I will review the rest of this class tomorrow.

Thanks,
Xuelei