On Fri, 16 Aug 2024 01:19:15 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> addressed several review comments, namely: - renaming the getParameters
>> method - renaming the AlgorithmParameterSpec object - address some javadoc
>> exception messages - add some information to KDF class private constructor
>> javadocs - other general cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 226:
>
>> 224: byte[] extractResult = hkdfExtract(inputKeyMaterial,
>> (salt
>> 225:
>> == null) ? null : salt.getEncoded());
>> 226: pseudoRandomKey = new SecretKeySpec(extractResult,
>> "RAW");
>
> Use `hmacAlgName` instead of `RAW` as the key algorithm? Or is it
> well-established fact that most if not all Hmac impls doesn't care or check
> the key algorithm?
@wangweij felt this was advantageous.
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 369:
>
>> 367: throw new RuntimeException(sbe);
>> 368: }
>> 369: }
>
> `tLength` may not be necessary. Variables only used inside the loop can also
> be moved to the loop.
> The loop can be modified as:
>
>
> for (int i = 0, offset = 0; i < rounds; i++, offset += hmacLen) {
> // Calculate this round
> try {
> if (i > 0) {
> hmacObj.update(kdfOutput, offset - hmacLen, hmacLen); //
> add T(i-1)
> }
> hmacObj.update(info); // Add info
> hmacObj.update((byte) (i + 1)); // Add round
> number
> hmacObj.doFinal(kdfOutput, offset);
> } catch (ShortBufferException sbe) {
> // This really shouldn't happen given that we've
> // sized the buffers to their largest possible size up-front,
> // but just in case...
> throw new RuntimeException(sbe);
> }
> }
I'll need to double-check the logic of this snippet. For example, in the first
iteration of the loop, isn't `tLength` == 0? Yet, you have it always set to
`hmacLen`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720263369
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720260930
