On Thu, 22 Dec 2022 15:00:18 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. > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Moved stateless key logic from SSLContextImpl to SSLSessionContextImpl and > addressed comments by @XueleiFan and @ascarpino src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 95: > 93: // Should be "randomly generated" according to RFC 5077, > 94: // but doesn't necessarily has to be a true random number. > 95: currentKeyID = this.hashCode(); As the hashCode() is called in the constructor, I'm not very sure if it works as expected. Maybe, the system nano time could be used instead. src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 97: > 95: currentKeyID = this.hashCode(); > 96: } else { > 97: keyHashMap = null; I may use an unmodifiable empty map so that we don't have to worry about null pointer exception. `keyHashMap = Map.of();` src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java line 163: > 161: SSLSessionContextImpl serverCache = > 162: > (SSLSessionContextImpl)hc.sslContext.engineGetServerSessionContext(); > 163: return serverCache.getKey(); I think the `HandshakeContext hc` could be passed as a parameter for the getKey() method, and thus you have a way to get the secure random for StatelessKey(). return serverCache.getKey(hc); -> SessionTicketExtension.StatelessKey getKey(HandshakeContext hc) { -> ssk = new SessionTicketExtension.StatelessKey(hc, newID); -> StatelessKey(HandshakeContext hc, int newNum) { -> kg.init(KEYLEN, hc.sslContext.getSecureRandom()); ------------- PR: https://git.openjdk.org/jdk/pull/11590
