On Fri, 8 Nov 2024 18:04:24 GMT, Ben Perez <bpe...@openjdk.org> wrote:
>> Java implementation of ML-KEM, the [FIPS >> 203](https://csrc.nist.gov/pubs/fips/203/final) post-quantum KEM scheme. >> 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: > > no classpath exception in test copyright header Note the bot warning about title mismatch... This should be fixed. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 383: > 381: private final int mlKem_du; > 382: private final int mlKem_dv; > 383: public final int encapsulationSize; Does this really have to be public? Maybe pkg private will do? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 424: > 422: public record K_PKE_KeyPair( > 423: K_PKE_DecryptionKey privateKey, K_PKE_EncryptionKey > publicKey) { > 424: } java.security.KeyPair has `publicKey` as the 1st argument and `privateKey` as the 2nd argument. I'd prefer following the same convention for consistency sake. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 451: > 449: > 450: /* > 451: Key check functions from the beginning of sections 7.2 and 7.3 of > the spec nit: separate out the comments and add them to individual functions below. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 471: > 469: } > 470: } > 471: return null; Why return null? Why not just use `void` as return type? Same for the `checkPrivateKey(...)` method. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 500: > 498: */ > 499: public ML_KEM_KeyPair generateKemKeyPair( > 500: byte[] kem_d, byte[] kem_z) nit: should be able to fit onto the same line as the method name? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 502: > 500: byte[] kem_d, byte[] kem_z) > 501: throws NoSuchAlgorithmException, DigestException { > 502: var mlKemH = MessageDigest.getInstance(HASH_H_NAME); Is `NoSuchAlgorithmException` possible to happen? If not, maybe it should be handled (e.g. as in `checkPrivateKey()`). Same goes for `DigestException`. It seems a bit strange to see these exceptions for generating KEM key pairs. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 526: > 524: public ML_KEM_EncapsulateResult encapsulate( > 525: ML_KEM_EncapsulationKey encapsulationKey, byte[] > randomMessage) > 526: throws NoSuchAlgorithmException, InvalidKeyException { Same comment as earlier, is NoSuchAlgorithmException really possible? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 546: > 544: public byte[] decapsulate(ML_KEM_DecapsulationKey decapsulationKey, > 545: K_PKE_CipherText cipherText) > 546: throws NoSuchAlgorithmException, Same `NoSuchAlgorithmException` comment as above. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 684: > 682: System.arraycopy(sigma, 0, prfSeed, 0, sigma.length); > 683: > 684: var kPkePRFeta1 = new SHAKE256(64 * mlKem_eta1); Would it be faster to do `mlKem_eta1 << 6` instead of `64 * mlKem_eta1`? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 940: > 938: // expressions > 939: private short[][] mlKemVectorInverseNTT(short[][] vector) { > 940: for (int i = 0; i < mlKem_k; i++) { with an additional `mlKem_k` argument, this method can be made "static"? Same goes for other `mlKemXXX()` methods. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM.java line 946: > 944: } > 945: > 946: // @IntrinsicCandidate indentation is off? Same goes for line 970, etc. src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 38: > 36: import javax.crypto.DecapsulateException; > 37: > 38: public final class ML_KEM_Provider { I find the Provider name here very confusing. Can we change it to something like ML_KEM_Impls or ML_KEM_Services? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 53: > 51: } > 52: > 53: public static class KPG extends NamedKeyPairGenerator { Consider using "sealed" to limit the inheritance to the known sub-classes? And mark the sub-classes final? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 106: > 104: } > 105: > 106: public static class KF extends NamedKeyFactory { Consider using "sealed" to limit the inheritance to the known sub-classes? And mark the sub-classes final? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 133: > 131: } > 132: > 133: public static class K extends NamedKEM { Consider using "sealed" to limit the inheritance to the known sub-classes? And mark the sub-classes final? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 137: > 135: > 136: @Override > 137: public byte[][] implEncapsulate(String name, byte[] > encapsulationKey, Object ek, SecureRandom secureRandom) { nit: break lines exceeding 80 chars into 2? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 137: > 135: > 136: @Override > 137: public byte[][] implEncapsulate(String name, byte[] > encapsulationKey, Object ek, SecureRandom secureRandom) { Does this method really need to be "public"? Same goes for `implDecapsulate()`. Actually, all `implXXX()` methods are changed from `protected` to `public`, is this really necessary? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Provider.java line 168: > 166: decapsulateResult = mlKem.decapsulate( > 167: new > ML_KEM.ML_KEM_DecapsulationKey(decapsulationKey), kpkeCipherText); > 168: } catch (NoSuchAlgorithmException | InvalidKeyException | > DecapsulateException e) { Why change `DecapsulationException` into `ProviderException`? src/java.base/share/classes/com/sun/crypto/provider/SHA3Parallel.java line 37: > 35: import static sun.security.provider.SHA3.keccak; > 36: > 37: public class SHA3Parallel { Why not merge this with `sun.security.provider.SHA3` class? A separate class in a different package seems harder to track... src/java.base/share/classes/com/sun/crypto/provider/SHA3Parallel.java line 122: > 120: } > 121: > 122: public static final class Shake256Parallel extends SHA3Parallel { I didn't find usage of this class in this PR? Is this for future usage? ------------- PR Comment: https://git.openjdk.org/jdk/pull/21478#issuecomment-2472210220 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1838668194 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1838873506 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1838884648 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1838890268 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1838901711 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1838914982 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839011218 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839014327 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839076932 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839103652 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839101907 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1835199392 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839195334 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839196789 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839196940 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839196114 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839219656 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839234905 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839270281 PR Review Comment: https://git.openjdk.org/jdk/pull/21478#discussion_r1839264511
