On Mon, 21 Oct 2024 20:07:10 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Ben Perez has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert "ML-DSA for jarsigner"
>>
>> This reverts commit cc231109513d0f3a939f0bff92a890ff921d94e0.
>
> src/java.base/share/classes/sun/security/provider/ML_DSA.java line 1:
>
>> 1: /*
>
> Does this class need to be public? Many methods are also public - do they
> need to be?
No need, but Mark's test might be using it.
> src/java.base/share/classes/sun/security/provider/ML_DSA_Provider.java line
> 33:
>
>> 31: import java.util.Arrays;
>> 32:
>> 33: public class ML_DSA_Provider {
>
> This class isn't a `Provider`. Can we name it something else,
> like`ML_DSA_Impl`?
I understand what you meant, but `ML_DSA` is the actual implementation and this
class just expose the functions as `Spi`s. It's more like a glue between the
implementation and JCA.
> src/java.base/share/classes/sun/security/provider/ML_DSA_Provider.java line
> 82:
>
>> 80: }
>> 81:
>> 82: public static class KPG5 extends KPG {
>
> What is the numbering scheme used here, why is this named KPG5?
The number maps to security levels of each parameter set. See `security_level`
in `ML_DSA`.
> src/java.base/share/classes/sun/security/provider/ML_DSA_Provider.java line
> 90:
>
>> 88: public static class KF extends NamedKeyFactory {
>> 89: public KF() {
>> 90: super("ML-DSA", "ML-DSA-44", "ML-DSA-65", "ML-DSA-87");
>
> Add a comment with the default as you do for `KPG`.
There is no concept of default parameter set for `KeyFactory` or `Signature`.
They are just supported parameter sets.
> src/java.base/share/classes/sun/security/provider/ML_DSA_Provider.java line
> 118:
>
>> 116: public static class SIG extends NamedSignature {
>> 117: public SIG() {
>> 118: super("ML-DSA", "ML-DSA-44", "ML-DSA-65", "ML-DSA-87");
>
> Add a comment with the default as you do for KPG.
See above.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1809502192
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1809502333
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1809505229
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1809508685
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1809509093
