On Wed, 11 Sep 2024 20:58:01 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> batch of review comments
>
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 341:
>
>> 339: // Calculate the number of rounds of HMAC that are needed to
>> 340: // meet the requested data. Then set up the buffers we will
>> need.
>> 341: if (CipherCore.getKeyBytes(pseudoRandomKey).length < hmacLen) {
>
> Why call a method when you already had `prk` the bytes? Also, moving this
> check before the `SecretKeySpec` creation also prevents you from accepting an
> empty key.
@wangweij: we have to create the SecretKeySpec in order to use the prk with the
HMAC, so I decided to re-use the check and exception handling from
`CipherCore`. Originally this method accepted a `SecretKey`, but I was
requested to change it to accept a `byte[]` by @valeriepeng. With the original
method signature, it would have been simpler.
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 365:
>
>> 363: System.arraycopy(tmp, 0, kdfOutput, offset,
>> 364: outLen - offset);
>> 365: offset = outLen;
>
> Care to clean up `tmp` here?
@wangweij: Will do. This snippet was taken from another reviewer.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755741646
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755743884
