On Thu, 9 May 2024 19:46:39 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). > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > some code review comments For `HkdfKeyDerivation`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 53: > 51: * and Expand-only variants. > 52: */ > 53: abstract class HkdfKeyDerivation extends KDFSpi { How about just name it `HKDF`? src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 54: > 52: */ > 53: abstract class HkdfKeyDerivation extends KDFSpi { > 54: All fields below should be local variables inside methods for thread-safety, except for `hmacLen` and `hmacAlgName`, which can be made final. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 59: > 57: protected String hmacAlgName; > 58: protected List<SecretKey> ikms; > 59: protected List<SecretKey> salts; Not sure if the 3 below should be `SecretKey` or `byte[]`. The latter has a benefit that when empty can be empty instead of null. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 66: > 64: protected int length; > 65: > 66: protected enum HKDFTYPES { We don't usually use plural. make it `HKDF_TYPE`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 70: > 68: } > 69: > 70: protected HKDFTYPES HKDFTYPE; This is not a constant, make it `hldfType`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 80: > 78: * @throws InvalidAlgorithmParameterException > 79: * if the initialization parameters are inappropriate for this > {@code KDFSpi} > 80: */ This constructor should contain `hmacAlgName` as a parameter, and its assignment moved here, thus make it final. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 101: > 99: protected SecretKey engineDeriveKey(String alg, KDFParameterSpec > kdfParameterSpec) > 100: throws InvalidParameterSpecException { > 101: Too many duplicate code with `engineDerivedata`. Please consider let one call the other, or, you can create a `deriveInternal` method which is then called by the 2 engine methods. If you do this, there is also no need for a separate `inspectKDFParameterSpec` method, which then make it easy to only have local variables. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 189: > 187: // perform expand > 188: try { > 189: return Arrays.copyOf(hkdfExpand(this.pseudoRandomKey, > this.info, this.length), If length is already correct, there is no need to call `copyOf`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 223: > 221: // A switch would be nicer, but we may need to backport this > before JDK 17 > 222: // Also, JEP 305 came out in JDK 14, so we can't declare a > variable in instanceof either > 223: if (kdfParameterSpec instanceof HKDFParameterSpec.Extract) { `kdfParameterSpec instanceof HKDFParameterSpec.Extract anExtract`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 227: > 225: this.ikms = anExtract.ikms(); > 226: this.salts = anExtract.salts(); > 227: if (isNullOrEmpty(ikms) && isNullOrEmpty(salts)) { As I said earlier, I think this restriction is not necessary. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 244: > 242: HKDFParameterSpec.Expand anExpand = > (HKDFParameterSpec.Expand) kdfParameterSpec; > 243: // set this value in the "if" > 244: if ((pseudoRandomKey = anExpand.prk()) == null) { This could not happen here. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 249: > 247: } > 248: // set this value in the "if" > 249: if ((info = anExpand.info()) == null) { I would disallow null when the parameter is created. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 365: > 363: // Calculate the number of rounds of HMAC that are needed to > 364: // meet the requested data. Then set up the buffers we will > need. > 365: hmacObj.init(prk); prk size must be checked somewhere. ------------- PR Review: https://git.openjdk.org/jdk/pull/18924#pullrequestreview-2048746355 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595929751 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595932007 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595933601 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595931078 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595930649 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595935623 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595943274 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595944111 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595948410 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595948979 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595949546 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595949980 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595951294
