On Mon, 27 Oct 2025 04:16:03 GMT, Xue-Lei Andrew Fan <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Revert changes to UseStrongDHSizes test as ffdhe6144/8192 added back
>> - Updated comment in ServerHello and hybrid to upper-case in NamedGroup
>
> src/java.base/share/classes/sun/security/util/Hybrid.java line 134:
>
>> 132: right.initialize(rightSpec, random);
>> 133: initialized = true;
>> 134: } catch (Exception e) {
>
> It may be nice if not wrap InvalidParameterException twice.
Yes, updated the code.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 149:
>
>> 147: throw new ProviderException(e);
>> 148: }
>> 149: }
>
> The initialization block could be saved if call initialize in the
> constructor. See sun/security/ec/ECKeyPairGenerator.java
Moved it to the `KeyPairGeneratorImpl` constructor.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 178:
>
>> 176: throws InvalidKeySpecException {
>> 177: if (keySpec instanceof RawKeySpec rks) {
>> 178: byte[] key = rks.getKeyArr();
>
> null check on key may be missed. If the API expose to public in the future,
> the key length checking may be good before Arrays.copyOfRange.
Good catch. Added key length checking.
> test/jdk/java/security/spec/TestNamedParameterSpec.java line 44:
>
>> 42: "ML_DSA_44", "ML_DSA_65", "ML_DSA_87",
>> 43: "ML_KEM_512", "ML_KEM_768", "ML_KEM_1024",
>> 44: "X25519MLKEM768", "SecP256r1MLKEM768", "SecP384r1MLKEM1024",
>
> Does it sound like an option to use the similar naming style like
> "ML_KEM_512" for the new names? for example, X25519MLKEM768 ->
> X25519_ML_KEM_768
We don't need to change `TestNamedParameterSpec.java` test now as the
`NamedParameterSpec` instances for hybrid named groups are defined in
`Hybrid.java` as internal constants. We use a "two-part" naming style (left and
right algorithm), such as `X25519_MLKEM768`. It seems cleaner and more readable
than breaking it into multiple parts with additional underscores.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2505741676
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2505745109
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2505738721
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2505735771
