RFR: 8328608: Multiple NewSessionTicket support for TLS [v3]
Daniel Jeliński
djelinski at openjdk.org
Mon Jul 29 18:24:43 UTC 2024
On Wed, 17 Jul 2024 02:47:33 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.
>
> 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
More information about the security-dev
mailing list