On Fri, 5 Dec 2025 16:37:04 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 two additional > commits since the last revision: > > - Updates with Weijun's comments > - Comments added for possession creation per handshake src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 91: > 89: // The order of shares in the concatenation for group name > 90: // X25519MLKEM768 has been reversed. This is due to IETF > 91: // historical reasons. Can we just change this to something like "as per the current draft RFC?" "historical reasons" is too vague. The draft is the real reason. src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 152: > 150: } > 151: > 152: static final class Handler private? src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 261: > 259: > 260: X448(56, 56, > 261: "XDH", "XDH", NamedParameterSpec.X448), unneeded comma, move the semicolon below to the end of this "XDH" line (currently line 261). src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 268: > 266: private final String keyAlgorithm; > 267: private final AlgorithmParameterSpec spec; > 268: nit: extra line? src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 331: > 329: } > 330: > 331: private static class HybridService extends Provider.Service { Reminder: this would also be moved to the new Provider file.... src/java.base/share/classes/sun/security/ssl/Hybrid.java line 61: > 59: * in a single TLS hybrid named group. > 60: * It implements: > 61: * - Hybrid KeyPair generation This seems to be a mix of javadoc and markdown. If anyone ever does generate javadocs for these internal classes, this will not render well. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 319: > 317: } > 318: > 319: private static byte[] concat(byte[]... inputs) { @wangweij did the following in `com.sun.crypto.provider.DHKEM.java` and `c.s.c.p.HPKE.java`: private static byte[] concat(byte[]... inputs) { ByteArrayOutputStream o = new ByteArrayOutputStream(); Arrays.stream(inputs).forEach(o::writeBytes); return o.toByteArray(); } Since this is the third usage in the last couple years, maybe put this in a utility class instead and adjust the other instances? src/java.base/share/classes/sun/security/ssl/NamedGroup.java line 314: > 312: try { > 313: // Skip AlgorithmParameters for KEMs (not supported) > 314: if (namedGroupSpec == NamedGroupSpec.NAMED_GROUP_KEM) { Can you also put in a comment here as to why you're doing these `getInstances`. It looks like you're checking for the existence of the `KeyFactory`, but the exception logger only talks about `AlgorithmParameters`. src/java.base/share/classes/sun/security/ssl/ServerHello.java line 572: > 570: // model, depending on what key exchange group is used. > 571: // > 572: // In JSSE for KA flow, the server usually generates its key Minor doc nit here. For KA flows, the server first receives the client's share, THEN generates its key share. Then we finally come here. Just reverse the order and you should be good. src/java.base/share/classes/sun/security/ssl/ServerHello.java line 581: > 579: // Traditional Key Agreement (KA): > 580: // - Both peers generate a key share and exchange it. > 581: // - Each peer computes a shared secret upon receiving the We currently do the share generation in the KeyShare extension, and calculate the shared secret here in ServerHello. It's not immediate, but this suggests it is. Maybe change: upon receiving -> sometime after receiving src/java.base/share/classes/sun/security/ssl/ServerHello.java line 585: > 583: // > 584: // Key Encapsulation Mechanism (KEM): > 585: // The decapsulator (the client) publishes a public key, and I would suggest rewording this to be something like: // Key Encapsulation Mechanism (KEM): // The client push a public key via a KeyShareExtension, which // the server uses to: // // - generate the shared secret // - encapsulate the message which is sent to the client in another // KeyShareExtension // // The derived shared secret must be stored in a KEMSenderPossession // so it can be retrieved for handshake traffic secret derivation later. src/java.base/share/classes/sun/security/ssl/ServerHello.java line 635: > 633: > 634: if (handshakeSecret == null) { > 635: SSLKeyDerivation handshakeKD = > ke.createKeyDerivation(shc); Since we only end up with one possession, can we move this up into the for loop? Each of the various possession types goes through the `for` loop, so seems this would be a tiny bit clearer. for () { if (pos instanceof ...) { handshakeSecret =... } else { SSLKeyDerivation handshakeKD = ... } } ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594536917 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594576033 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594584547 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594584936 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594585786 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594587476 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594622712 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594471547 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594491151 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594492123 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594495875 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2594508194
