RFR 8211018: Session Resumption without Server-Side State

[Anthony Scarpino]

On 6/4/19 9:02 PM, Xuelei Fan wrote:
> 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?

Sure, I can see how that could be confusing

> 
>    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  > duplicated line?

It was an orphaned comment

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

sure

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

ok

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

While I would argue that if SecureRandom is broken there are a lot 
bigger problems with the system, I'll use the API and have the old 
random code as a backup in the catch because an NoSuchAlgorithmException 
is not an acceptable or clean result to deal with in the code.

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

Yeah, that shouldn't be static
> 
>   108     static final class StatelessKey {
> Could this class be private?

yes

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

yes

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

The questions is if it's necessary to change what maybe infrequently 
locked code for Project Loom.  The only way around this is putting a 
lock in SSLContextImpl and accessing it through hc.sslcontext.  With all 
this code being static the lock has to come through the context now. 
Another less desirable option is one static lock for all contexts.

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