On Wed, 17 Jul 2024 02:47:33 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. > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 21 commits: > > - Rework TLSBase for better testing > - Tests working > - Merge branch 'master' into nst-multi > - new changes > - remove frag issue > - Comments, remove thread, set NST default to 1, allow 0 > - comment cleanup > - Merge branch 'master' into nst-multi > - copyright & cleanup > - oops BAOS > - ... and 11 more: https://git.openjdk.org/jdk/compare/3796fdfc...35bfe799 This PR is huge now... didn't realize that not sending a ticket would require so many changes. Please double check the synchronization in `Cache`. Other than that I think this looks pretty good now. src/java.base/share/classes/sun/security/util/Cache.java line 280: > 278: // Locking is to protect QueueCacheEntry's from being removed from > the > 279: // cacheMap while another thread is adding new queue entries. > 280: private ReentrantLock lock; The cache currently uses `synchronized` for all data accesses (put, get, remove); please use synchronized instead of this lock. src/java.base/share/classes/sun/security/util/Cache.java line 341: > 339: continue; > 340: } > 341: if (currentEntry instanceof QueueCacheEntry<K,V> qe) { This code is unreachable now; QueueCacheEntry is not a SoftReference, so it will never be enqueued. src/java.base/share/classes/sun/security/util/Cache.java line 375: > 373: // If the top level entry expires, the whole queue is > expired. > 374: if (entry instanceof QueueCacheEntry<K,V> qe) { > 375: qe.clear(); not necessary; isValid already called invalidate, which called clear src/java.base/share/classes/sun/security/util/Cache.java line 384: > 382: qe.getQueue().forEach(e -> { > 383: if (e.isValid(time)) { > 384: qe.getQueue().remove(e); use removeIf instead src/java.base/share/classes/sun/security/util/Cache.java line 454: > 452: case QueueCacheEntry<K, V> qe -> { > 453: lock.lock(); > 454: qe.putValue(newEntry); this will add the entry to queue even if canQueue is false. What are the implications of that? src/java.base/share/classes/sun/security/util/Cache.java line 471: > 469: expirationTime, maxQueueSize)); > 470: } else { > 471: cacheMap.put(key, newEntry); if maxQueueSize == 0, we invalidate the previously stored entry. Should we do that here too? src/java.base/share/classes/sun/security/util/Cache.java line 764: > 762: if (entry.isValid(time)) { > 763: // Use SoftReference get() > 764: if (entry instanceof SoftCacheEntry<K,V> sce) { you can drop this `if`; `entry.getValue` below forwards to `get`. test/jdk/sun/security/ssl/SSLSessionImpl/MultiNSTClient.java line 29: > 27: * @library /javax/net/ssl/templates > 28: * @bug 8242008 > 29: * @summary Verifies multiple session tickets are PSKs are used by JSSE Suggestion: * @summary Verifies multiple PSKs are used by JSSE test/jdk/sun/security/ssl/SSLSessionImpl/MultiNSTClient.java line 35: > 33: * @run main/othervm MultiNSTClient -Djdk.tls.client.protocols=TLSv1.3 > -Djdk.tls.server.enableSessionTicketExtension=true > -Djdk.tls.client.enableSessionTicketExtension=true > 34: * @run main/othervm MultiNSTClient -Djdk.tls.client.protocols=TLSv1.3 > -Djdk.tls.server.enableSessionTicketExtension=false > -Djdk.tls.client.enableSessionTicketExtension=true > 35: * @run main/othervm MultiNSTClient -Djdk.tls.client.protocols=TLSv1.3 > -Djdk.tls.server.enableSessionTicketExtension=true > -Djdk.tls.client.enableSessionTicketExtension=false `jdk.tls.client.enableSessionTicketExtension` has no effect with `TLSv1.3`. Did you mean to run these tests with TLSv1.2? test/jdk/sun/security/ssl/SSLSessionImpl/MultiNSTParallel.java line 29: > 27: * @library /javax/net/ssl/templates > 28: * @bug 8242008 > 29: * @summary Verifies multiple session tickets are PSKs are used by TLSv1.3 Suggestion: * @summary Verifies multiple PSKs are used by TLSv1.3 test/jdk/sun/security/ssl/SSLSessionImpl/MultiNSTParallel.java line 199: > 197: System.err.println("released: " + t.getName()); > 198: } > 199: } catch (Exception e) { is this catch necessary? ------------- PR Review: https://git.openjdk.org/jdk/pull/19465#pullrequestreview-2205424437 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695642085 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695617478 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695622183 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695511949 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695646584 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695647717 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695652194 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695591595 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695537849 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695592220 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1695589727
