RFR: 8298381: Improve handling of session tickets for multiple SSLContexts

[Volker Simonis]

On Thu, 8 Dec 2022 18:52:31 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

> I have asked some of the next questions already [here](https://mail.openjdk.org/pipermail/security-dev/2022-December/033797.html). Would like to mention some of them;
> 
>     * The main question I have: is it safe to assume that the [SSLContextImpl](https://github.com/openjdk/jdk/pull/11590/files#diff-fc40f443cd9d2236d4279b83e6b6e662771d08faa571851f9de3de4925a2c36c) can be shared across the threads? 

I can't answer this for sure but at least I haven't found any documentation which would prevent multi-threaded usage. On the other hand, `SSLContextImpl` contains the following implementation note:

 * Implementation note: Instances of this class and the child classes are
 * immutable, except that the context initialization (SSLContext.init()) may
 * reset the key, trust managers and source of secure random.

So this comment will probably have to be removed once we move the session ticket key (i.e. `currentKeyID`) into this class. But the session key hash map has already lived in `SSLContextImpl` before, so the claim has already been violated.

> *..If yes, it seems it lacks synchronization here and there. Like this patch added a synchronization around getNextKey/cleanup/destroy methods but it does not prevent usage of the key after creation and destroying it by different threads.

You're right, but that's actually an improvement compared to the initial implementation where cleanup/destroy wasn't synchronized at all :)

With regards to the missing synchronization of key usage and key destruction, I think this patch doesn't change the existing behavior because it wasn't synchronized before either. On the other hand, this patch fixes the implementation such that it only creates new tickets every hour (i.e. `jdk.tls.server.statelessKeyTimeout`) per context by default. This means that each `keyHashMap` should not contain more than 24 valid session keys by default (i.e. `jdk.tls.server.sessionTicketTimeout`/`jdk.tls.server.statelessKeyTimeout`) and cleanup should be called at most once per hour. Even if a session key which is in use will be destroyed, the worst case consequence would be that the verification of a corresponding session ticket could fail. But that is not critical because the SSL handshake would just fail and fall back to full session renegotiation in that case. 

>     * Is it safe to have duplicated currentKeyID per ssl context and use that during encryption/description as part of the "Additional Authentication Data"?

With this change there will be no duplicate key IDs per context, only different contexts might eventually use the same session key (i.e. `currentKeyID`). But that should still be quite rare, because they both start from a different random number. And even if it happens, I don't see a problem with it. According to [RFC 5077](https://www.rfc-editor.org/rfc/rfc5077), the key ID is an opaque data structure which is transmitted in plain text. It is only used by the server to identify the correct private key for decrypting the clients session ticket. It's up to the application to make sure that clients with session tickets will connect to the correct context which is capable of decrypting the ticket. Notice, that this was already the case in the original implementation where different contexts had different key IDs but still every context could only decrypt session tickets with keys it had issued before. I think "[JDK-8250965: Distributed TLS sessions implementation](https://bugs.openjdk.o
 rg/browse/JDK-8250965)" is an attempt to solve this problem more generally.

> 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?

I have no clue, but as the keys can be initially negative anyway (i.e. if `SecureRandom().nextInt()` returns a negative number), I don't see a reason to restrict them to positive values after they wrap around.

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

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