RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v28]

[Kevin Driver]

On Wed, 11 Sep 2024 20:09:20 GMT, Weijun Wang <weijun at 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 55:
> 
>> 53: 
>> 54:     protected final int hmacLen;
>> 55:     protected final String hmacAlgName;
> 
> Make the 2 above private.

@wangweij: Resolved in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 107:
> 
>> 105:      *     if {@code alg} is empty or invalid
>> 106:      * @throws IllegalArgumentException
>> 107:      *     if {@code alg} is {@code null} or empty
> 
> The 2 exceptions above are not consistent. On the other hand, you throw NPE in the code when `alg == null`.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 143:
> 
>> 141:         throws InvalidAlgorithmParameterException {
>> 142:         List<SecretKey> ikms, salts;
>> 143:         byte[] inputKeyMaterial, salt, pseudoRandomKey, info;
> 
> It's always nice to clean up these intermediate bytes related to secret keys.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 255:
> 
>> 253:     }
>> 254: 
>> 255:     private byte[] consolidateKeyMaterial(List<SecretKey> keys)
> 
> Add a comment that this method throws an IKE if any of the keys is unextractable. Not necessarily in formal javadoc, but worth mentioning.

@wangweij: Resolved in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 274:
> 
>> 272:             }
>> 273:         } else if(keys != null) {
>> 274:                 return null;
> 
> Isn't an empty array better here? The null return is unexpected and has to be handled specially.

@wangweij: Resolved in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 299:
> 
>> 297:         throws InvalidKeyException, NoSuchAlgorithmException {
>> 298: 
>> 299:         if (salt == null) {
> 
> Also cover the empty `salt` case here. The `SecretKeySpec` creation below would fail.
> 
> Hint: when people call `addSalt(k)`, `k` can be empty. It doesn't have to be a `SecretKeySpec`. This is worth a test.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 346:
> 
>> 344:         }
>> 345:         hmacObj.init(pseudoRandomKey);
>> 346:         if (info == null) {
> 
> This cannot happen since you already checked it in both 2 callers. Or, you can move both the `info` and `length` checks from `deriveData` here.

@wangweij: Agreed. Removed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 356:
> 
>> 354:             while (i < rounds) {
>> 355:                 if (i > 0) {
>> 356:                     hmacObj.update(kdfOutput, Math.max(0,offset - hmacLen), hmacLen); // add T(i-1)
> 
> If `i > 0` is already true, I assume `Math.max(0,offset - hmacLen)` is always `offset - hmacLen`?

@wangweij: Resolved in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

> 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: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755777952
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755778150
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755779207
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755778302
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755778460
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755778964
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755779496
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755779866
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755779676