On Thu, 8 Dec 2022 13:09:11 GMT, Volker Simonis <simo...@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@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@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@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. src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 97: > 95: keyHashMap.put(Integer.valueOf(newID), key); > 96: currentKeyID = newID; > 97: cleanupSessionKeys(); Does synchronization on sslContext around cleanupSessionKeys is needed? It will block all threads which will try to get the new key, and it seems does not make it safe since the ConcurrentHashMap is used now. src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java line 187: > 185: } else { > 186: newNum = currentKeyID + 1; > 187: } Any idea why the code used zero after MAX_VALUE before? ------------- PR: https://git.openjdk.org/jdk/pull/11590
