On Tue, 10 Sep 2024 20:13:08 GMT, Kevin Driver <kdri...@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
