On Wed, 29 May 2024 18:53:55 GMT, Anthony Scarpino <ascarp...@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
