RFR: 8298390: Implementing ML-KEM key encapsulation mechanism [v20]
Valerie Peng
valeriep at openjdk.org
Wed Nov 13 02:20:14 UTC 2024
On Fri, 8 Nov 2024 18:04:24 GMT, Ben Perez <bperez at 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
More information about the security-dev
mailing list