On Fri, 30 Oct 2026 08:05:56 GMT, Hai-May Chao <[email protected]> wrote:
>> This feature enables SunJSSE to use a TLS 1.3 hybrid named group implemented >> directly by a security provider as JCA `KeyPairGenerator` and `KEM` >> services. It also moves the JDK implementations of `X25519MLKEM768`, >> SecP256r1MLKEM768, and `SecP384r1MLKEM1024` into SunJCE provider. >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused adapter from DHasKEM Some comments. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 1: > 1: /* The 3 algorithms we support now always have 1. EC curve always on left side 2. X25519 only on right side 3. X448 not used 4. ML-KEM is the only PQC There are quite some hardcoded assumptions on this and it might change in the future (for example, https://datatracker.ietf.org/doc/html/draft-preussmattsson-tls-mlkem1024-x448). We don't have time to do everything right now but at least add a comment on this. Also, I am not sure if this class should stay in `ssl` package or in `com.sun.crypto.provider`. It is a little strange that SunJCE refers to a class outside. If you move `Hybrid`, `DHasKEM` and `HybridProvider` need to be moved too. Either side is OK for me. BTW, `HybridProvider` should be renamed and since it's so small now you can make it an inner class of `DHasKEM`. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 117: > 115: // provider selection. > 116: return KeyFactory.getInstance(name, provider); > 117: } Why force using ML-KEM in SunJCE? There could be users having their own ML-KEM provider but we should not expect their provider having the hybrid KEMs. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 178: > 176: kp2.getPublic()), > 177: new PrivateKeyImpl("Hybrid", kp1.getPrivate(), > 178: kp2.getPrivate())); Now that this is public, I suggest we use the long hybrid name instead of `Hybrid` as the algorithm of the keys. Then, we can check the name in `engineNewEncapsulator` and `engineNewDecapsulator` of `KEMImpl` below to make sure it's the same. We can even add a test for this. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 210: > 208: } > 209: > 210: PublicKey decode(PublicKey inKey) throws InvalidKeyException { We can change the return type to `PublicKeyImpl` so it's clearer. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 312: > 310: @Override > 311: public EncapsulatorSpi engineNewEncapsulator(PublicKey publicKey, > 312: AlgorithmParameterSpec spec, SecureRandom secureRandom) > throws We should make sure `spec` is null. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 327: > 325: @Override > 326: public DecapsulatorSpi engineNewDecapsulator(PrivateKey > privateKey, > 327: AlgorithmParameterSpec spec) We should make sure the `spec` argument is null in `engineNewEncapsulator`. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 435: > 433: @Override > 434: public String getAlgorithm() { > 435: return "Generic"; This is hardcoded. Maybe we should use the `algorithm` argument of `encapsulate` and `decapsulate`. Does not matter for JSSE usage. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 543: > 541: } > 542: > 543: // getEncoded() returns an empty byte array because there is no Not empty, but null. src/java.base/share/classes/sun/security/ssl/HybridProvider.java line 38: > 36: // It exposes the internal DH-as-KEM adapter to Hybrid.KEMImpl through > 37: // the public KEM API without exposing KEM.DH as a system provider > 38: // service. The ML-KEM component is obtained from SunJCE. No need to mention ML-KEM source, if you agree with my previous comment that it does not have to come from SunJCE. ------------- PR Review: https://git.openjdk.org/jdk/pull/33433#pullrequestreview-5712668458 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4424096848 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4423962667 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4424379221 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4424136365 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4423928317 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4423933445 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4424447130 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4423922969 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4424263741
