On Tue, 20 May 2025 03:01:06 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:
>
> Log error and return null no compress/decompress failure
This looks promising. What ticket sizes have you observed when no client
certificates were involved?
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 324:
> 322: b = Record.getBytes8(buf);
> 323: if (b.length > 0) {
> 324: String alg = new String(b);
Please remove the algorithm names from the session ticket. They were not used,
and I don't see any reason to start using them now.
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 421:
> 419:
> 420: SSLPossession pos = X509Authentication.createPossession(
> 421: hc, certAlgs);
Are you trying to select the same certificate that was used in the original
handshake? That's an interesting approach. Are you confident that this will
produce the same possession as the original handshake, assuming that the
ClientHello is the same and the KeyManager certificates did not change?
I think you only need the certAlg of the first certificate in the chain for
createPossession.
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 424:
> 422:
> 423: if (pos == null) {
> 424: throw hc.conContext.fatal(Alert.HANDSHAKE_FAILURE,
I don't think using `fatal` is the right choice here; ideally we should fall
back to a full handshake instead. Consider setting invalidated=true instead
src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java line
238:
> 236: byte compressed = 0;
> 237: if (data.length >= MIN_COMPRESS_SIZE) {
> 238: data = compress(data);
we should probably handle the null return here.
src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java line
250:
> 248: result[3] = (byte)(key.num);
> 249: System.arraycopy(iv, 0, result, Integer.BYTES,
> iv.length);
> 250: result[Integer.BYTES + iv.length] = compressed;
this byte should be authenticated. Either pass it to updateAAD, or to doFinal
src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java line
262:
> 260: }
> 261:
> 262: ByteBuffer decrypt(HandshakeContext hc) throws IOException {
does it really throw?
src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java line
336:
> 334: int count = 0;
> 335: int b;
> 336: while ((b = gis.read()) >= 0) {
use gis.readAllBytes()
-------------
PR Review: https://git.openjdk.org/jdk/pull/25310#pullrequestreview-2853321629
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097538961
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097556759
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097506777
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097431800
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097401544
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097432527
PR Review Comment: https://git.openjdk.org/jdk/pull/25310#discussion_r2097412283
