On Tue, 27 May 2025 14:00:07 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> The stateless session ticket is included in the ClientHello message, either
>> in the stateless_ticket extension (pre-TLS1.3), or in the pre_shared_key
>> extension (TLS1.3). With the current construction, the ticket is often the
>> largest contributor to the ClientHello message size. For example, in
>> HttpClient tests we observed a case where a non-resumption ClientHello
>> occupied 360 bytes, and the session ticket (pre_shared_key identity)
>> included in a resumption ClientHello occupied 1600+ bytes.
>>
>> ClientHello messages that do not fit in a single packet on the network can
>> greatly increase the handshake time on lossy networks. Ideally we would like
>> the ClientHello message to always fit in a single packet.
>>
>> When using QUIC as the underlying protocol, one packet can hold
>> approximately 1100 byte payload. Getting the session ticket size below 700
>> bytes should be sufficient to make the ClientHello fit in a single packet
>>
>> Things done in this PR to reduce the ticket size in order of importance:
>>
>> 1. Remove local certificates.
>> 2. Compress tickets with the size 600 bytes or larger.
>> 3. Remove `peerSupportedSignAlgs`.
>> 4. Remove `pskIdentity`
>> 5. PreSharedKey is only needed by TLSv1.3, masterSecret is only needed by
>> pre-TLSv1.3
>> 6. Remove `statusResponses`
>>
>> Tickets with a chain of 2 RSA peer certificates are still above 700 bytes
>> (about 1KB), but they are significantly reduced from prior size of about 3KB.
>
> Artur Barashev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update comments. Optimize imports.
A few minor suggested nits.
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 289:
> 287: * < length in bytes> Peer certificate
> 288: * < 1 byte > Number of Local Certificate entries
> 289: * < 1 byte > Local Certificate algorithm length
Can we add a minor doc note here to say these fields are repeated for each cert
(local/peer)?
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 326:
> 324: }
> 325:
> 326: this.useExtendedMasterSecret = false;
This is probably not needed, but ok to stay.
If you like this style, then you could explicitly add the default
`this.preSharedKey = null` for the TLSv1.3+ case around line 340.
test/jdk/sun/security/ssl/SSLSessionImpl/ResumeChecksServer.java line 107:
> 105:
> 106: switch (testMode) {
> 107: case BASIC:
Minor nit, Oracle Code Style is (was?) to keep case at the same level as the
switch. I noticed this was updated in all the different cases here.
If you're using IJ, you'll need to set the Settings->Editor->Code
Style->Java->switch statement/expression->Indent case branches
test/jdk/sun/security/ssl/SSLSessionImpl/ResumeChecksServer.java line 177:
> 175:
> 176: switch (mode) {
> 177: case BASIC:
Same indentation issue here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25310#pullrequestreview-2872621088
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2110223363
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2112802016
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2112970553
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2112970801
