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

[Xue-Lei Andrew Fan]

On Wed, 21 Dec 2022 15:05:13 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 one additional commit since the last revision:
> 
>   Optimized initialisation of currentKeyID and deletion of expired session keys as proposed by @schlosna

src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 80:

> 78: 
> 79:     protected SessionTicketExtension.StatelessKey getKey() {
> 80:         SessionTicketExtension.StatelessKey ssk = serverCache.getKey();

I may change the serverCache from private to package private, and move the getKey() methods into SSLSessionContextImpl:

-    private final SSLSessionContextImpl serverCache;
+    final SSLSessionContextImpl serverCache;


Then the caller could use sslContext.serverCache.getKey() directly.  BTW, I may change the name from getKey to getStatelessKey() for readability.

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

> 26: package sun.security.ssl;
> 27: 
> 28: import java.security.SecureRandom;

This import is not used.

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

> 75: 
> 76:     private int currentKeyID;           // RFC 5077 session ticket key name
> 77:     private final Map<Integer,          // Maps session keys to session state

What do you think if adjusting the comment a little bit?

    // The current session ticket encryption key ID
    private int currentKeyID;

    // Session ticket encryption keys and IDs map
    private final Map<Integer,

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

> 77:     private final Map<Integer,          // Maps session keys to session state
> 78:             SessionTicketExtension.StatelessKey> keyHashMap = new ConcurrentHashMap<>();
> 79: 

It may be not necessary to allocate the map for client session context.  It may be fine to me the map allocation to constructor.


private final Map<Integer, SessionTicketExtension.StatelessKey> keyHashMap;
SSLSessionContextImpl(boolean server) {
    ....
    if (server) {
        keyHashMap = ...
        currentKeyID = ...
    } else {
        keyHashMap = empty map
    }
}

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

> 180: 
> 181:     // Package-private, used only from SSLContextImpl::engineInit() to initialie currentKeyID.
> 182:     void initCurrentKeyID(int keyID) {

The ID will work as if it is unique in the context, granted with synchronization.  It may be not necessary to use secure number for it.   The ID could be assigned at constructor (see comment above, use number zero or the current time, etc.), and thus this method and the caller code could be removed, I think.

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

> 185: 
> 186:     // Package-private, used only from SSLContextImpl::getKey() to create a new session key.
> 187:     int getCurrentKeyID() {

This method could be removed if you want to move SSLContextImpl.getKey() into this class.

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

> 189:     }
> 190: 
> 191:     private void cleanupSessionKeys() {

What do you think if using CleanupStatelessKeys as the method name?

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

> 224:         return keyHashMap.get(id);
> 225:     }
> 226: 

It may need to reconsider these 3 methods you want to move SSLContextImpl.getKey() into this class.

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

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