On Fri, 30 Oct 2026 13:26:09 GMT, Weijun Wang <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused adapter from DHasKEM > > 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`. Added a comment. No move for the package. Moved `HybridProvider` in `DHasKEM`. > 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. Yes, passing the long hybrid name from SunJCE. > 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. Done. > 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. Done. > 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. Fixed the comment. > test/micro/org/openjdk/bench/javax/crypto/full/KEMBench.java line 1: > >> 1: /* > > KEM.DH is still in its own private provider. Same as in KeyPairGeneratorBench. Fixed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431516911 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431528041 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431526067 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431528631 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431526776 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431528933
