On Mon, 24 Nov 2025 07:51:40 GMT, Hai-May Chao <[email protected]> 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
