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 A few more comments. Still have to go through the test cases. src/java.base/share/classes/sun/security/ssl/ServerHello.java line 568: > 566: shc.serverHelloRandom = shm.serverRandom; > 567: > 568: // Key Encapsulation Mechanism (KEM): I really like the details you included here, but a little wordsmithing would improve readability. Can you please add an intro sentence like: "For key derivation, we'll either use the KA or a KEM model, depending on what group is used." Then add some of the lines from 579-584, then do the KEM and KA sections, and move the final sentence to the KEM section. I think this would read much cleaner. src/java.base/share/classes/sun/security/util/Hybrid.java line 57: > 55: public class Hybrid { > 56: > 57: public record SecretKeyImpl(SecretKey k1, SecretKey k2) implements > SecretKey { Minor nit: consider moving`SecretKeyImpl` to keep all the keys `PrivateKeyImpl`/`PublicKeyImpl` together? src/java.base/share/classes/sun/security/util/Hybrid.java line 139: > 137: > 138: @Override > 139: public void initialize(int keysize, SecureRandom random) { Is this method actually called? (I didn't see it in `KEMKeyExchange.KEMReceiverPossession`). I know the method is needed to fully extend the `KeyPairGeneratorSpi`, but it looks like we're doing the actual initialization of both sides in the constructor, and that is considered it's "default method of initialization." This duplicate code could probably be removed, and just be a NO-OP. src/java.base/share/classes/sun/security/util/Hybrid.java line 239: > 237: > 238: throw new InvalidKeySpecException( > 239: keySpec.getClass().getName() + " not supported"); Maybe add "KeySpec type: " to the Exception message? ------------- PR Review: https://git.openjdk.org/jdk/pull/27614#pullrequestreview-3512839879 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2567059770 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2566607441 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2566570776 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2566652347
