RFR: 8298387: Implementing ML-DSA signature algorithm [v3]

[Weijun Wang]

On Tue, 8 Oct 2024 18:16:47 GMT, Ben Perez <bperez at openjdk.org> wrote:

>> Java implementation of ML-DSA, the FIPS 204 post-quantum signature scheme https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.204.pdf. Depends on https://github.com/openjdk/jdk/pull/21167
>
> Ben Perez has updated the pull request incrementally with one additional commit since the last revision:
> 
>   renamed internal keyGen/sign/verify functions to be same as spec

Another comment: I noticed you didn't `mod q` everywhere. The result should be equivalent and I hope they are always "normalized" at encoding.

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 35:

> 33: 
> 34: public class ML_DSA {
> 35:     private static final int mlDsa_q = 8380417;

`static final` constants are usually in all UPPER_CASE in Java. Also, `t0CoeffSize` seems also a constant.

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 706:

> 704:                 }
> 705:             }
> 706:         }

Add a comment here that `shift` must be zero now so we should have output all bits.

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 710:

> 708:     }
> 709: 
> 710:     public int[][] t1Unpack(byte[] v) {

Maybe worth saying this is `SimpleBitUnpack` always with the same argument.

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 811:

> 809:     public ML_DSA_PublicKey pkDecode(byte[] pk) {
> 810:         byte[] rho = new byte[mlDsaASeedLength];
> 811:         System.arraycopy(pk, 0, rho, 0, mlDsaASeedLength);

Maybe use `Arrays.copyOfRange` to make it more clear.

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 916:

> 914:             this.xof = xof;
> 915:             this.bitsPerCall = bitsPerCall;
> 916:             bitMask = (1 << bitsPerCall) - 1;

Add some comments about the limit of `bitsPerCall`, looks like cannot exceed 31.

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 1174:

> 1172:         int result = implMlDsaAlmostNtt(coeffs, montZetasForVectorNtt);
> 1173:         int[] check = coeffs.clone();
> 1174:         result = implMlDsaMontMulByConstant(coeffs,  montRModQ);

In FIPS 204, NTT does not end with multiplying a constant. Why do you need one?

src/java.base/share/classes/sun/security/provider/ML_DSA.java line 1202:

> 1200:     public static int[] mlDsaInverseNtt(int[] coeffs) {
> 1201:         int result = implMlDsaAlmostInverseNtt(coeffs, montZetasForVectorInverseNtt);
> 1202:         result = implMlDsaMontMulByConstant(coeffs, montDimInverse);

In FIPS 204, the constant is 8347681. Why do you use 16382?

-------------

PR Review: https://git.openjdk.org/jdk/pull/21364#pullrequestreview-2360756958
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795691383
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795694178
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795695028
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795695988
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795697317
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795700210
PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795702588