On Tue, 4 Jun 2024 15:04:56 GMT, Jamil Nimeh <jni...@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. > > src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 503: > >> 501: this.preSharedKey = new SecretKeySpec(b, alg); >> 502: // Get identity len >> 503: i = buf.get(); > > Given that this is just a small change to a large function it may not be > important to change, but a lot of these X-byte lengths followed by arrays > might be simplified with Record.getBytes8 or the other similar routines that > handle longer length fields. But you wouldn't end up with a null byte[] when > the length is zero, so that's a change, not necessarily a down-side. I think that's something to consider as a new RFE as it would have a scope outside of this change > src/java.base/share/classes/sun/security/util/Cache.java line 683: > >> 681: >> 682: // Limit the number of queue entries. >> 683: private static final int MAXQUEUESIZE = 10; > > What do you think about making this tunable through the constructor, perhaps > with a default value of 10? I thought about this, but found no reason as this is an internal API. `QueueCacheEntry<>` is only for TLS client NewSessionTicket situation and the API would have to set that at the time the whole cache is built. Even with that, it would still be a hard coded number, just somewhere else. Unless we have yet another system property. I don’t see a need for individual tuning at this time and we can always up the hardcoded limit if more are needed. If it is necessary in the future we can revisit this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1638775456 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1628164520
