RFR: 8314323: Implement JEP 527: TLS 1.3 Hybrid Key Exchange [v9]

[Bradford Wetmore]

On Mon, 24 Nov 2025 07:51:40 GMT, Hai-May Chao <hchao at openjdk.org> wrote:

>> Implement hybrid key exchange support for TLS 1.3 by adding three post-quantum hybrid named groups: X25519MLKEM768, SecP256r1MLKEM768, and SecP384r1MLKEM1024.
>> Please see [JEP 527](https://openjdk.org/jeps/527) for details about this change.
>
> Hai-May Chao has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Update names to uppercase
>  - Remove fallback in engineGeneratePublic
>  - Change default named group list to have only X25519MLKEM768

(I am not all the way through the code, so this may already been addressed, but I didn't see it in my travels so far, or in a quick search of this PR's git diffs.)

`draft-ietf-tls-ecdhe-mlkem-03` has an additional 7'ish checks that need to be performed (IETF wording : **MUST**), but I didn't see them addressed.  e.g.

`
For all groups, the client/server MUST perform XXX ...and abort with an illegal_parameter/internal_error alert if it fails.
`

I did see a `DecapsulateException`, but that wasn't translated into the right `Alert` back in the JSSE layer.

(See the draft for the various client/server/alert combos.)

src/java.base/share/classes/com/sun/crypto/provider/DH.java line 64:

> 62:  * key exchange. It models DH/ECDH/XDH as KEMs, like post-quantum algorithms,
> 63:  * so DH/ECDH/XDH can be used in hybrid key exchange, alongside post-quantum
> 64:  * KEMs.

The purpose of this class from the opening javadoc was confusing on my first several reads.  I expected that this class just created a `KEM` layer for DH, but surprise(!), it also crammed in a Hybrid provider implementation too!

My preference is to break the `DH` and `Provider`+`HybridService` code into separate files for cleaner abstractions, but if not, then maybe use something like this for the description?


/**
 * The DH class presents a KEM abstraction layer over traditional 
 * DH-based key exchange, which can be used for either straight
 * DH/ECDH/XDH or TLS hybrid key exchanges.
 * 
 * This class can be alongside standard full post-quantum KEMs
 * when hybrid implementations are required.
 */

src/java.base/share/classes/com/sun/crypto/provider/DH.java line 75:

> 73:         private static final long serialVersionUID = 0L;
> 74:         private ProviderImpl() {
> 75:             super("InternalJCE", PROVIDER_VER, "");

Maybe include an `info` String here?

src/java.base/share/classes/com/sun/crypto/provider/DH.java line 75:

> 73:         private static final long serialVersionUID = 0L;
> 74:         private ProviderImpl() {
> 75:             super("InternalJCE", PROVIDER_VER, "");

Wondering if we might use another name here for the Provider?

Perhaps `InternalSunJCE`, `InternalJCEKEM`, `InternalJCEHybridJSSE`, `InternalJCEHybridJSSEKEM`, or something similar?

This may not be necessary, but would help indicate what this is.

src/java.base/share/classes/com/sun/crypto/provider/DH.java line 82:

> 80:             // The order of shares in the concatenation for group name
> 81:             // X25519MLKEM768 has been reversed. This is due to historical
> 82:             // reasons.

`...due to IETF historical reasons...`, not JSSE reasons.

src/java.base/share/classes/com/sun/crypto/provider/DH.java line 86:

> 84:                     "right", "X25519");
> 85:             putService(new HybridService(this, "KeyPairGenerator",
> 86:                     "X25519MLKEM768", "sun.security.util.Hybrid$KeyPairGeneratorImpl",

Is there a reason why `Hybrid` is in `sun.security.util` instead of `com.sun.crypto.provider`?    This is the only place it's used, so `c.s.c.p` seems to be a more natural place for it, but maybe I'm just not far enough into the guts of the code yet.

src/java.base/share/classes/com/sun/crypto/provider/DH.java line 251:

> 249:                 "XDH", "XDH", NamedParameterSpec.X448),
> 250:         ;
> 251:         private final int Nsecret;

Minor nits:  lower case name starting letters.  

Maybe `nSecret`/`secretLen` and `nPK`/`publicKeyLen`

(Is the N prefix a Solaris convention?)

src/java.base/share/classes/sun/security/ssl/KeyShareExtension.java line 580:

> 578:                     shc.handshakeKeyExchange = ke;
> 579:                     shc.handshakePossessions.add(pos);
> 580: 

Noticed several <=80 lines throughout the code.  Will mention just here.

It helps when reviewing side-by-side not having to vertically scroll.

Thanks.

src/java.base/share/classes/sun/security/util/Hybrid.java line 35:

> 33: import javax.crypto.KEMSpi;
> 34: import javax.crypto.SecretKey;
> 35: import java.io.ByteArrayOutputStream;

import no longer needed.

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

PR Review: https://git.openjdk.org/jdk/pull/27614#pullrequestreview-3480768887
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562101424
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562089452
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562722660
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2563032371
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562842297
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562757554
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562067883
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2562124446