On Thu, 11 May 2023 19:32:50 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reintroduced Length for HSSPublicKey, added more @Override annotations
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 436:
>
>> 434: int sigArrLen = (12 + n * (p + 1) + m * h);
>> 435: if ((q >= (1 << h)) || (inLen < sigArrLen) ||
>> (checkExactLen && (inLen != sigArrLen))) {
>> 436: throw new InvalidParameterException("LMS signature
>> length is incorrect");
>
> Should be a `SignatureException`.
Changed.
> src/java.base/share/classes/sun/security/provider/HSS.java line 622:
>
>> 620: var val = new DerValue(new
>> ByteArrayInputStream(x.getEncoded()));
>> 621: val.data.getDerValue();
>> 622: return new HSSPublicKey(new
>> DerValue(val.data.getBitString()).getOctetString());
>
> The 2 lines above cannot detect wrong algorithm identifier and garbage data
> at the end. Now that you already have `parseBits` implementation, you should
> follow the usual `X509Key` convention to create a new `HSSPublicKey`
> constructor that takes in the whole encoding and call `decode` to decode it.
> See `ECKeyFactory` and `ECPublicKeyImpl.java` for an example.
Changed as suggested.
> src/java.base/share/classes/sun/security/provider/HSS.java line 728:
>
>> 726: @Override
>> 727: public int length() {
>> 728: return getKey().length();
>
> Debatable. `getKey` now contains 2 (OCTET STRING header) + 4 (L) + 4 (LMS
> type) + 4 (LM-OTS type) + 16 (I) + 32 (T) bytes. Should the 2 header bytes be
> included in the length? Should all fields other than T be included? The
> length is mainly used to compare strength and I suggest we refrain from
> implementing this method until a well-known definition is accepted for
> HSS/LMS. Table 2 of [NIST SP 800-57 Part
> 1](https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-57pt1r5.pdf)
> and they are defined by modulus size. In this sense, the size of the hash is
> more suitable to be defined as the size of the key.
Removed length, I agree that it doesn't make much sense.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192438720
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192437225
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1192438203
