RFR: 8298381: Improve handling of session tickets for multiple SSLContexts [v2]

David Schlosnagle duke at openjdk.org
Wed Dec 21 00:25:55 UTC 2022


On Tue, 20 Dec 2022 23:50:15 GMT, Volker Simonis <simonis 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.
>
> Volker Simonis has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Some refactoring and simplification. Moved most of the implementation from SessionTicketExtension to SSLSessionContextImpl
>  - Moving currentKeyID/keyHashMap to SSLSessionContextImpl as requested by @XueleiFan

src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 75:

> 73:     private int timeout;                // timeout in seconds
> 74: 
> 75:     private int currentKeyID = new SecureRandom().nextInt();

Could this instantiation of new SecureRandom become a concurrency bottleneck on entropy? Should the SecureRandom from SSLContext be injected to constructor?

src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 195:

> 193:                     // Suppress
> 194:                 }
> 195:                 keyHashMap.remove(entry.getKey());

Would it be worth using the entrySet iterator to remove the entry as opposed to looking it up? Should the key be removed from entries before destroying so that another thread doesn't read a destroyed key?

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

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



More information about the security-dev mailing list