RFR: 8298381: Improve handling of session tickets for multiple SSLContexts

[Volker Simonis]

On Sun, 11 Dec 2022 20:38:16 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Currently, TLS session tickets introduced by [JDK-8211018](https://bugs.openjdk.org/browse/JDK-8211018) in JDK 13 (i.e. `SessionTicketExtension$StatelessKey`) are generated in the class `SessionTicketExtension` and they use a single, global key ID (`currentKeyID`) for all `SSLContext`s.
>> 
>> This is problematic if more than one `SSLContext` is used, because every context which requests a session ticket will increment the global id `currentKeyID` when it creates a ticket. This means that in turn all the other contexts won't be able to find a ticket under the new id in their `SSLContextImpl` and create a new one (again incrementing `currentKeyID`). In fact, every time a ticket is requested from a different context, this will transitively trigger a new ticket creation in all the other contexts. We've observed millions of session ticket accumulating for some workloads.
>> 
>> Another issue with the curent implementation is that cleanup is racy because the underlying data structure (i.e. `keyHashMap` in `SSLContextImpl`) as well as the cleanup code itself are not threadsafe.
>> 
>> I therefor propose to move `currentKeyID` into the `SSLContextImpl` to solve these issues.
>> 
>> The following test program (contributed by Steven Collison (https://raycoll.com/)) can be used to demonstrate the current behaviour. It outputs the number of `StatelessKey` instances at the end of the program. Opening 1000 connections with a single `SSLContext` results in a single `StatelessKey` instance being created:
>> 
>> $ java -XX:+UseSerialGC -Xmx16m -cp ~/Java/ SSLSocketServerMultipleSSLContext 9999 1 1000
>> 605:             1             32  sun.security.ssl.SessionTicketExtension$StatelessKey (java.base at 20-internal)
>> 
>> The same example with the 1000 connections being opened alternatively on thwo different contexts will instead create 1000 `StatelessKey` instances:
>> 
>> $ java -XX:+UseSerialGC -Xmx16m -cp ~/Java/ SSLSocketServerMultipleSSLContext 9999 2 1000
>>   11:          1000          32000  sun.security.ssl.SessionTicketExtension$StatelessKey (java.base at 20-internal)
>> 
>> With my proposed patch, the numbers goes back to two instances again:
>> 
>> $ java -XX:+UseSerialGC -Xmx16m -cp ~/Java/ SSLSocketServerMultipleSSLContext 9999 2 1000
>> 611:             2             64  sun.security.ssl.SessionTicketExtension$StatelessKey (java.base at 20-internal)
>> 
>> 
>> I've attached the test program to the [JBS issue](https://bugs.openjdk.org/browse/JDK-8298381). If you think it makes sense, I can probably convert it into a JTreg test.
>
>> The same example with the 1000 connections being opened alternatively on two different contexts will instead create 1000 `StatelessKey` instances:
> 
> That's obviously not the expected behaviors.  It is a good catch for the `static currentKeyID` issue.
> 
> What do you think to move `SSLContextImpl.keyHashMap` into `SSLSessionContextImpl`?  I would like to have SSLContextImpl focusing on configuration.

Hi @XueleiFan, @ascarpino,

Thank you for your reviews and comments. I think I've addressed all of your suggestions in the latest version of the PR.

There's just one more thing I had to change. Now that I've moved all the logic to `SSLSessionContextImpl` I can't get a reference to `SSLContextImpl`s secure random instance any more when creating a new `StatelessKey`. For simplicity I've therefor decided to use the `KeyGenerator.init()` method which doesn't require an extra secure random instance. 

If you think that it is important that the `KeyGenerator` uses the same secure random instance like the corresponding `SSLContextImpl`, I can easily change `SSLSessionContextImpl::getKey()` to take a `SecureRandom` argument and pass `SSLContextImpl`s secure random instance down to  `SSLSessionContextImpl::getKey()` (and up to `StatelessKey::<init>`) when calling `getKey()` from `KeyState::getCurrentKey()`.

What do you think?

-------------

PR: https://git.openjdk.org/jdk/pull/11590