RFR: 8298127: HSS/LMS Signature Verification [v5]

[Ferenc Rakoczi]

On Fri, 5 May 2023 19:34:32 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   adding key translation, finally block, removing 24-byte LMOTS parameters
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 37:
> 
>> 35: import java.util.Arrays;
>> 36: import java.util.Objects;
>> 37: 
> 
> Can you add some comments describing this class?

Comment added.

> src/java.base/share/classes/sun/security/provider/HSS.java line 38:
> 
>> 36: import java.util.Objects;
>> 37: 
>> 38: public class HSS extends SignatureSpi {
> 
> Make the class `final`?

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 43:
> 
>> 41: 
>> 42:     @Deprecated
>> 43:     protected void engineSetParameter(String param, Object value) {
> 
> Technically, this API is not defined to throw UOE. I think throwing IPE is better and would be compliant with the specification. (I am aware that there are other JDK SignatureSpi implementations that already throw UOE for this method, but they should be fixed).

Changed to throw InvalidParameterException.

> src/java.base/share/classes/sun/security/provider/HSS.java line 48:
> 
>> 46: 
>> 47:     @Deprecated
>> 48:     protected AlgorithmParameters engineGetParameter(String param) {
> 
> Same comment as above.

Changed to throw InvalidParameterException.

> src/java.base/share/classes/sun/security/provider/HSS.java line 56:
> 
>> 54:     }
>> 55: 
>> 56:     protected byte[] engineSign() throws SignatureException {
> 
> This API is not defined to throw UOE. Suggest throwing `SignatureException` instead with a message "Signing is not supported".
> 
> Though, technically this method should never be called, as `Signature.sign()` will throw a `SignatureException` because it has not been initialized for signing.

Done.

> src/java.base/share/classes/sun/security/util/RawKeySpec.java line 30:
> 
>> 28: import java.security.spec.KeySpec;
>> 29: 
>> 30: public class RawKeySpec  implements KeySpec {
> 
> Nit, should be one space between `RawKeySpec` and `implements`.

Changed.

> src/java.base/share/classes/sun/security/util/RawKeySpec.java line 30:
> 
>> 28: import java.security.spec.KeySpec;
>> 29: 
>> 30: public class RawKeySpec  implements KeySpec {
> 
> Can you add some comments describing this class?

Added.

> src/java.base/share/classes/sun/security/util/RawKeySpec.java line 31:
> 
>> 29: 
>> 30: public class RawKeySpec  implements KeySpec {
>> 31:     final private byte[] keyArr;
> 
> Put `private` before `final`.

Changed the order.

> src/java.base/share/classes/sun/security/util/RawKeySpec.java line 33:
> 
>> 31:     final private byte[] keyArr;
>> 32:     /**
>> 33:      * The sole constructor
> 
> Nit: add period at end of sentence and an empty line after this (before the `@param`).

Added period.

> src/java.base/share/classes/sun/security/util/RawKeySpec.java line 37:
> 
>> 35:      */
>> 36:     public RawKeySpec(byte[] key) {
>> 37:         keyArr = key.clone();
> 
> Does this need to be cloned if it is an internal class?

Yes, I think so. If someone wants to test with several different keys by first creating RawKeySpec objects from an array in which a few bytes are changed between the calls and and then use these KeySpecs to create the actual keys, without the cloning they will end up with the same keys. So an immutable object is better.

> src/java.base/share/classes/sun/security/util/RawKeySpec.java line 41:
> 
>> 39: 
>> 40:     /**
>> 41:      * Getter function
> 
> Nit: add period at end of sentence and an empty line after this (before the @return).

Added period.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453794
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187454052
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453859
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453935
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453983
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453454
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453499
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453735
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453545
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453600
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453680