On Fri, 12 May 2023 14:27:18 GMT, Ferenc Rakoczi <d...@openjdk.org> wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC >> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with >> SHA-256/192 parameters recommended. > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Removed Length from HSSPublicKey, changed the handling of X509 encoded keys > in the factory, did some more beautification. Except for putting 3 lines into the try block for `engineVerify`, I only have some format comments. There are quite some long lines in `HSS.java`. Better make them less than 80 characters. Everything else looks fine to me. Thanks for the hard work. src/java.base/share/classes/sun/security/provider/HSS.java line 38: > 36: > 37: /* > 38: * This class implements the Hierarchical Signature System using the Add the short name "(HSS)" after the long name. src/java.base/share/classes/sun/security/provider/HSS.java line 96: > 94: HSSSignature sig = new HSSSignature(signature, pubKey); > 95: LMSPublicKey lmsPubKey = pubKey.lmsPublicKey; > 96: boolean result = true; Please put all 3 lines above into the `try` block so we make sure `messageStream.reset()` is always called. src/java.base/share/classes/sun/security/provider/SHA2.java line 51: > 49: > 50: private static final int ITERATION = 64; > 51: private static final int BLOCKSIZE = 64; I'm not sure if it's worth defining this. A lot of other numbers (like 56 and 120) are hardcoded and it's not easy to modify this number to implement a different algorithm. src/java.base/share/classes/sun/security/provider/SunEntries.java line 221: > 219: addWithAlias(p, "KeyFactory", "DSA", > 220: "sun.security.provider.DSAKeyFactory", attrs); > 221: addWithAlias(p, "KeyFactory", "HSS/LMS", > "sun.security.provider.HSS$KeyFactoryImpl", attrs); This line can be shorter. Overall, it will be nice to keep lines (including comment lines) shorter than 80 chars. If the code does look really ugly, better no more than 120 chars. src/java.base/share/classes/sun/security/util/RawKeySpec.java line 32: > 30: /** > 31: * This is a KeySpec that is used to specify a key by its byte array > implementation. > 32: * It is intended to be used in testing algorithms where the algorithm > specification describes the key in this form. This comment line can be shorter. ------------- PR Review: https://git.openjdk.org/jdk/pull/13691#pullrequestreview-1425204741 PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192829509 PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192830859 PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192828015 PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192828938 PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192829128
