On Fri, 26 Jul 2024 20:01:23 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: > > review comments src/java.base/share/classes/javax/crypto/KDF.java line 54: > 52: * <em>Algorithm</em>With<em>PRF</em>. For instance, a KDF implementation > of > 53: * HKDF using HMAC-SHA256 has an algorithm name of "HKDFWithHmacSHA256". > In some > 54: * cases the PRF portion of the algorithm field may be omitted if the KDF s/PRF/WithPRF/ src/java.base/share/classes/javax/crypto/KDF.java line 193: > 191: throw new NoSuchAlgorithmException( > 192: "Received an InvalidAlgorithmParameterException. Does > this " > 193: + "algorithm require an AlgorithmParameterSpec?", e); s/AlgorithmParameterSpec/KDFParameters/. Same comments below. src/java.base/share/classes/javax/crypto/KDF.java line 212: > 210: * if a provider is specified and it does not support the > specified KDF > 211: * algorithm, or if provider is {@code null} and there is no > provider > 212: * that supports a KDF implementation of the specified algorithm provider can't be null, so the last part of this doesn't make sense. Same comment in `getInstance(String, Provider)`. src/java.base/share/classes/javax/crypto/KDF.java line 217: > 215: * list > 216: * @throws NullPointerException > 217: * if the algorithm is {@code null} provider also can't be null. src/java.base/share/classes/javax/crypto/KDF.java line 248: > 246: * that supports a KDF implementation of the specified algorithm > 247: * @throws NullPointerException > 248: * if the algorithm is {@code null} provider also can't be null. src/java.base/share/classes/javax/crypto/KDF.java line 278: > 276: * the specified algorithm > 277: * @throws InvalidAlgorithmParameterException > 278: * if the {@code AlgorithmParameterSpec} is an invalid value s/AlgorithmParameterSpec/KDFParameters/. Same comment below. src/java.base/share/classes/javax/crypto/KDF.java line 412: > 410: * <p> > 411: * The {@code deriveKey} method may be called multiple times at the > same > 412: * time on a particular {@code KDF} instance. Remove "at the same time" as thread safety is no longer a requirement. Same comment for `deriveData`. src/java.base/share/classes/javax/crypto/KDF.java line 416: > 414: * Delayed provider selection is also supported such that the > provider > 415: * performing the derive is not selected until the method is called. > Once a > 416: * provider is selected, it cannot be changed. I think I would remove these sentences. This doesn't discuss the case if the provider is passed into `getInstance`, and it also doesn't discuss the case if a provider is not specified and`getProviderName` is called before `deriveKey`. You already explain these scenarios in the class summary, so I think that is probably sufficient. Same comment for deriveData. src/java.base/share/classes/javax/crypto/KDF.java line 420: > 418: * @param alg > 419: * the algorithm of the resultant {@code SecretKey} object (may > not be > 420: * {@code null}) You don't have to say "may not be null" since throwing NPE is specified below. Also `kdfParameterSpec` cannot be null either, so should be consistent. src/java.base/share/classes/javax/crypto/KDFSpi.java line 54: > 52: * <p> > 53: * A {@code KDFParameters} object may be specified for KDF algorithms > 54: * that require this. Suggest replacing "this" with "initialization parameters" to avoid confusion as to what this is referring to. I would also probably say "support" instead of "require" as some parameters may be optional. src/java.base/share/classes/javax/crypto/KDFSpi.java line 71: > 69: * Derives a key, returned as a {@code SecretKey}. > 70: * <p> > 71: * The {@code deriveKey} method may be called multiple times on a > particular s/deriveKey/engineDeriveKey/ src/java.base/share/classes/javax/crypto/KDFSpi.java line 72: > 70: * <p> > 71: * The {@code deriveKey} method may be called multiple times on a > particular > 72: * {@code KDF} instance. s/KDF/KDFSpi/ src/java.base/share/classes/javax/crypto/KDFSpi.java line 96: > 94: * Obtains raw data from a key derivation function. > 95: * <p> > 96: * The {@code deriveData} method may be called multiple times on a s/deriveData/engineDeriveData/ src/java.base/share/classes/javax/crypto/KDFSpi.java line 97: > 95: * <p> > 96: * The {@code deriveData} method may be called multiple times on a > 97: * particular {@code KDF} instance. s/KDF/KDFSpi/ test/jdk/com/sun/crypto/provider/KDF/Functions.java line 55: > 53: > 54: if (!Arrays.equals(prk.getEncoded(), expectedPrk)) { > 55: throw new Exception(); Suggest adding details to the exception messages as to what this is testing and why these comparisons fail. test/jdk/com/sun/crypto/provider/KDF/Functions.java line 66: > 64: test(HKDFParameterSpec.ofExtract().extractOnly()); > 65: test(HKDFParameterSpec.ofExtract().thenExpand(new byte[0], 32)); > 66: test(HKDFParameterSpec.ofExtract().addIKM(ikm).addSalt(new > byte[0]).extractOnly()); What are these calls testing? Can you add a comment for each? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695141729 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695164446 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695154671 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695155404 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695157824 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695162823 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695333402 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695346273 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695339742 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695114796 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695117975 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695118234 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695120209 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695120663 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695132194 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695135168
