On Thu, 4 Dec 2025 00:03:49 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 one additional > commit since the last revision: > > Remove null check to not assume key is returned Some comments. src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 214: > 212: if (from == 0 && to == params.secretLen) { > 213: return key; > 214: } else if ("RAW".equalsIgnoreCase(key.getFormat())) { I think it's not worth supporting key slicing because it should never happen. Otherwise there might be a programming error. Throw an AssertionError here. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 406: > 404: // getFormat() returns "RAW" if both keys are X509Key; otherwise > null. > 405: @Override > 406: public String getFormat() { My understanding is that this will always return "RAW" even if `left` or `right` comes from 3rd-party providers and is not `X509Key`. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 420: > 418: if (left instanceof X509Key xk1 && right instanceof X509Key > xk2) { > 419: return concat(xk1.getKeyAsBytes(), xk2.getKeyAsBytes()); > 420: } else { Here, we should be prepared for 3rd-party providers -- just call `getEncoded` and strip the ASN.1 headers of algorithm identifiers. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 459: > 457: > 458: static boolean isXDH(String name) { > 459: return name != null && name.equals("X25519"); Can `name` in the two methods above be null? This might be hiding a bug. Also, I think it's not worth putting them into a separate class `APS`. src/java.base/share/classes/sun/security/ssl/KAKeyDerivation.java line 184: > 182: KEM.getInstance(algorithmName, provider) : > 183: KEM.getInstance(algorithmName); > 184: KEM.Encapsulator e = kem.newEncapsulator(pk); `newEncapsulator` is called without an `SecureRandom` argument. Is this intended? Otherwise, we had a chance to pass in a user-provided random when `KEMSenderPossession` is created and it can be passed here. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 116: > 114: private final PublicKey publicKey; > 115: > 116: KEMReceiverPossession(NamedGroup namedGroup, SecureRandom > random) { `random` is ignored. Is this intended? ------------- PR Review: https://git.openjdk.org/jdk/pull/27614#pullrequestreview-3540372956 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2589356191 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2589475112 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2589478265 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2589389536 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2589439762 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2589412239
