RFR: 8328608: Multiple NewSessionTicket support for TLS

[Daniel Jeliński]

On Wed, 29 May 2024 18:53:55 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

> Hi
> 
> This change is to improve TLS 1.3 session resumption by allowing a TLS server to send more than one resumption ticket per connection and clients to store more.  Resumption is a quick way to use an existing TLS session to establish another session by avoiding the long TLS full handshake process.  In TLS 1.2 and below, clients can repeatedly resume a session by using the session ID from an established connection.  In TLS 1.3, a one-time "resumption ticket" is sent by the server after the TLS connection has been established.  The server may send multiple resumption tickets to help clients that rapidly resume connections.  If the client does not have another resumption ticket, it must go through the full TLS handshake again.  The current implementation in JDK 23 and below, only sends and store one resumption ticket.
> 
> The number of resumption tickets a server can send should be configurable by the application developer or administrator. [RFC 8446](https://www.rfc-editor.org/rfc/rfc8446) does not specify a default value.  A system property called `jdk.tls.server.newSessionTicketCount` allows the user to change the number of resumption tickets sent by the server.  If this property is not set or given an invalid value, the default value of 3 is used. Further details are in the CSR.
> 
> A large portion of the changeset is on the client side by changing the caching system used by TLS.  It creates a new `CacheEntry<>` type called `QueueCacheEntry<>` that will store multiple values for a Map entry.

Changes requested by djelinski (Reviewer).

src/java.base/share/classes/sun/security/ssl/NewSessionTicket.java line 388:

> 386:             /*
> 387:              * This thread addresses a Windows only networking issue found with
> 388:              * SSLSocketBruteForceClose. A client that quickly closes after

Thanks for bringing it up. Using a thread to delay sending the messages only hides the problem; if the client closes the connection without reading the NST messages, the connection will be reset. Should we work on a proper fix instead?

src/java.base/share/classes/sun/security/ssl/NewSessionTicket.java line 397:

> 395:              * and server are on different machines.
> 396:              */
> 397:             Thread nstThread = Thread.ofVirtual().name("NST").start(() -> {

Please don't use threads during handshake.

src/java.base/share/classes/sun/security/ssl/NewSessionTicket.java line 415:

> 413:                 } catch (IOException e) {
> 414:                     // Low exception likelihood this is data requires not
> 415:                     // reply an IO errors will be handled by other messages.

Could you rephrase this?

src/java.base/share/classes/sun/security/ssl/NewSessionTicket.java line 510:

> 508:     }
> 509: 
> 510:         /**

unintended

src/java.base/share/classes/sun/security/util/Cache.java line 300:

> 298:         }
> 299: 
> 300:         cacheMap = new ConcurrentHashMap<>();

With LinkedHashMap we were removing least recently used entries on overflow (see the implementation of `put`); with ConcurrentHashMap we will remove entries in random order. Is that intentional? If it is, you might want to review the class's JavaDoc.

src/java.base/share/classes/sun/security/util/Cache.java line 395:

> 393:                 entry.invalidate();
> 394:             }
> 395:             while (queue.poll() != null);

Can we keep the original code? IntelliJ complains about both versions, but the comment makes it more clear that the empty loop is intentional.

src/java.base/share/classes/sun/security/util/Cache.java line 701:

> 699:         }
> 700: 
> 701:         public V getValue() {

This method should call SoftReference.get() somewhere to let the GC know that this cache entry is still used

src/java.base/share/classes/sun/security/util/Cache.java line 741:

> 739:             // is a one for one entry swap, locking isn't necessary and plus or
> 740:             // minus a few entries is not critical.
> 741:             if (queue.size() > MAXQUEUESIZE) {

Suggestion:

            if (queue.size() >= MAXQUEUESIZE) {

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

PR Review: https://git.openjdk.org/jdk/pull/19465#pullrequestreview-2117840823
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639488568
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639523446
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639489471
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639522755
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639529424
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639539288
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639541832
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639543646