RFR: 8328608: Multiple NewSessionTicket support for TLS [v4]

[Daniel Jeliński]

On Sat, 3 Aug 2024 00:46:05 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 incrementally with two additional commits since the last revision:
> 
>  - revert to synchronized
>  - code review changes

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

> 365:                 // If this is a queue, check for some expired entries
> 366:                 if (entry instanceof QueueCacheEntry<K,V> qe) {
> 367:                     qe.getQueue().removeIf(e -> e.isValid(time));

Shouldn't this be `e -> !e.isValid(time)`?

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

> 466:     }
> 467: 
> 468:     synchronized public V get(Object key) {

please revert the modifier order change

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

> 474: 
> 475:         if (lifetime > 0 && !entry.isValid(System.currentTimeMillis())) {
> 476:             removeImpl(key);

you can revert to `cacheMap.remove(key);`; `invalidate` is already done by `isValid`

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

> 476:             removeImpl(key);
> 477:             if (DEBUG) {
> 478:                 System.out.println("Ignoring expired entry: ");

did you intend to print the entry contents here?

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

> 492:     }
> 493: 
> 494:     public void remove(Object key) {

this still needs to be synchronized because of `emptyQueue`

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

> 497:     }
> 498: 
> 499:     private synchronized void removeImpl(Object key) {

once you change `remove`, this doesn't need to be synchronized, because all callers are already synchronized.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722244026
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722247219
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722252662
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722251527
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722253359
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722254245