On Wed, 11 Sep 2024 20:09:20 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 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
