The question about 8211018: Session Resumption without Server-Side State

[Volker Simonis]

Hi Sergey,

Thanks for starting this discussion. I've opened "8298381: Improve
handling of session tickets for multiple SSLContexts" [1] to track
this issue and submitted a pull request with a potential fix [2].
Let's continue the discussion there if you don't mind :)

Best regards,
Volker

[1] https://bugs.openjdk.org/browse/JDK-8298381
[2] https://github.com/openjdk/jdk/pull/11590

On Wed, Dec 7, 2022 at 10:01 PM Sergey Bylokhov <bylokhov at amazon.com> wrote:
>
> Hello, Security team.
>
> Right now I am working on some issues related to the sslContext and would like to clarify a few
> questions about how the feature implemented by the JDK-8211018 is intended to work.
> That feature is mostly implemented by this class:
> https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java
>
> Here is some questions about thread safeness of this code:
>
>   - The SessionTicketExtension class uses the currentKeyID to track the currently used keyID.
> https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java#L77
>     * Note that the field is static and could be shared across the threads. There is even a
> synchronization block in place where we increment the key:
>
> https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java#L175
>     * It seems wrong that it is synchronized on the "hc.sslContext.keyHashMap" since the static key
> can be shared across the different sslcontext's?
>     * In another place the synchronization is not used at all:
> https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java#L158
>     * Probably the intention was to use one currentKeyID per sslcontext?
>
>
>   - The hashmap to store the keys are located in the SSLContextImpl class:
> https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java#L74
>   * It seems that the usage of this map is not synchronized at all, does it mean that it cannot be
> used across the threads? -> And it is expected that one SSLContextImpl is created per thread?
>
>
>   - If the hashmap above can be used across the threads, then the code for "clean" the expired keys
> is unclear:
> https://github.com/openjdk/jdk/commit/94e1d7530ff03978be305211f6a1e6d14acb6d1c#diff-8eeef23d966ed2ebe6ecf02d16e549b8cef94f54bb17f6c9ac8d8dc151eed8fcR197
> ============
>      // Clean up any old keys, then return the current key
>      cleanup(hc);
>      return ssk;
> ============
>    Note that it is executed outside of the synchronized block, is that right?
>
>   - Another question is about using "SessionTicketExtension#getCurrentKey+cleanup" on a different
> threads. I think it is possible to get the next situation:
>     1. Thread1 call getCurrentKey() and return the key which is mostly ready to expire, but valid
> for now, This key started to be used  by the SessionTicketSpec#encrypt
>     2. Thread2 calls getCurrentKey() and creates a new key, then call "cleanup" and destroys the key
> used by Thread1, since it is expired now.
>     3. Thread1 is in trouble.
>
>
> Note that the specification does not have any hints about how the code in question should work
> concurrently.
>
> --
> Best regards, Sergey.