On Mon, 1 Dec 2025 16:13:52 GMT, Sean Mullan <[email protected]> wrote:
>> 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
>
> src/java.base/share/classes/com/sun/crypto/provider/DH.java line 66:
>
>> 64: * KEMs.
>> 65: */
>> 66: public class DH implements KEMSpi {
>
> This class includes more than a DH KEM wrapper implementation. The provider
> is registering all the hybrid algorithms, including the ML-KEM parts. It
> feels odd to be hiding the provider inside this class. I suggest creating a
> separate `HybridProvider` class that is in `sun.security.ssl` package and
> either encapsulating the DH impl inside that, or better yet, create a
> separate class for it.
Agreed.
> src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 28:
>
>> 26:
>> 27: import java.io.IOException;
>> 28: import java.security.*;
>
> Probably clearer not to use wildcard imports so we can see all the classes
> needed, same comment on line 32.
I vaguely remember somewhere the guidance that if there are <= 5 imports, you
should list them, otherwise use the wildcard.
> test/jdk/javax/net/ssl/TLSv13/HRRKeyShares.java line 33:
>
>> 31: * @summary Use two key share entries
>> 32: * @library /test/lib
>> 33: * @run main/othervm
>> -Djdk.tls.namedGroups=x25519,secp256r1,secp384r1,X25519MLKEM768,SecP256r1MLKEM768,SecP384r1MLKEM1024
>> HRRKeyShares
>
> Long line, break up into multiple lines.
I noted quite a few <= 80 comments throughout, so fully agree here! ;)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2578840216
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2578857585
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2578864356
