RFR 8211018: Session Resumption without Server-Side State

[Xuelei Fan]

On 6/5/2019 12:41 PM, Anthony Scarpino wrote:
>>> http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02
>> SessionTicketExtension.java:
>> ----------------------------
> 
>>
>>   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.
> 
It's not only about the quality of SecureRandom. Some circumstances may 
want the key is not extractable, in a token, etc.

I would not worry about the NoSuchAlgorithmException exception as AES is 
  the "required" algorithm for KeyGenerator.  We don't support provider 
that does not implement required algorithms.

>>
>>   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.
It's a good question.  I don't know if it is really necessary.  The 
dependency could be very complicated.  To easy the job, I would like to 
cleanup all of the synchronized block in JSSE code, instead of make more 
evaluation now and later if the use of the synchronized block could be a 
problem or not.  As would easy our life a little bit.

> 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.
> 
As you are locking the keyState, an instance variable.  I think we could 
add new lock object in the same instance.  But,  we may not need to do 
that because we may need to adjust the code because of the following 
commented issue.

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

Xuelei