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

[Weijun Wang]

On Tue, 10 Sep 2024 20:13:08 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

>> Introduce an API for Key Derivation Functions (KDFs), which are cryptographic algorithms for deriving additional keys from a secret key and other data. See [JEP 478](https://openjdk.org/jeps/478).
>> 
>> Work was begun in [another PR](https://github.com/openjdk/jdk/pull/18924).
>
> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
> 
>   batch of review comments

Comments on `HkdfKeyDerivation.java`.

BTW, whenever the method declaration line is too long and you have to move the `throws` clause to the next line, we usually indent an extra 8 spaces.

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.

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`.

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.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 148:

> 146:         // JDK 17
> 147:         // Also, JEP 305 came out in JDK 14, so we can't declare a variable
> 148:         // in instanceof either

Personally I don't care about these restrictions. Backporters can modified to code to suit their releases.

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.

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.

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.

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 prevent you from accepting an empty key.

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.

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`?

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?

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

PR Review: https://git.openjdk.org/jdk/pull/20301#pullrequestreview-2298522282
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755520133
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755526106
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755589535
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755529935
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755533279
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755537612
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755553214
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755599916
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755604042
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755646348
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755610916