On Sat, 11 May 2024 02:06:09 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: > > commenting out until better understood -- causing failures src/java.base/share/classes/javax/crypto/KDF.java line 1: > 1: /* Note: many of my comments apply to multiple methods, so please check all other methods when fixing. src/java.base/share/classes/javax/crypto/KDF.java line 46: > 44: > 45: /** > 46: * This class provides the functionality of a key derivation algorithm > for JCE. Suggest adding a sentence describing what key derivation does. src/java.base/share/classes/javax/crypto/KDF.java line 144: > 142: * > 143: * <p>This is the same name that was specified in one of the > 144: * {@code getInstance} calls that created this {@code KDF} object. I don't think it is necessary to include this sentence. src/java.base/share/classes/javax/crypto/KDF.java line 163: > 161: > 162: /** > 163: * Creates an instance of the {@code KDF} object. Suggest rewording as "Returns a {@code KDF} object that implements the specified algorithm." src/java.base/share/classes/javax/crypto/KDF.java line 166: > 164: * > 165: * @param algorithm > 166: * the key derivation algorithm to use This should include a pointer to the KDF section of the Standard Algorithm Names specification once you set that up. src/java.base/share/classes/javax/crypto/KDF.java line 171: > 169: * > 170: * @throws NoSuchAlgorithmException > 171: * if no {@code Provider} supports a {@code KDFSpi} > implementation for s/KDFSpi/KDF/ src/java.base/share/classes/javax/crypto/KDF.java line 174: > 172: * the specified algorithm > 173: * @throws NullPointerException > 174: * if the algorithm is {@code null} s/the algorithm/{@code algorithm}/ src/java.base/share/classes/javax/crypto/KDF.java line 190: > 188: /** > 189: * Creates an instance of the {@code KDF} object with a specific > 190: * {@code Provider}. Suggest rewording as "Returns a {@code KDF} object that implements the specified algorithm from the specified security provider." src/java.base/share/classes/javax/crypto/KDF.java line 195: > 193: * the key derivation algorithm to use > 194: * @param provider > 195: * the provider to use for this key derivation It looks like provider can be null. If so, you should specify that, e.g. "If null, this method is equivalent to getInstance(String)." src/java.base/share/classes/javax/crypto/KDF.java line 201: > 199: * @throws NoSuchAlgorithmException > 200: * if no {@code Provider} supports a {@code KDFSpi} > implementation for > 201: * the specified algorithm This exception reason looks to be copied from getInstance(String), but is different for this method. It should be: "if a provider is specified and it does not support the specified KDF algorithm, or if provider is null and there is no provider that supports a KDF implementation of the specified algorithm" src/java.base/share/classes/javax/crypto/KDF.java line 223: > 221: /** > 222: * Creates an instance of the {@code KDF} object using a supplied > 223: * {@code Provider} instance. Suggest rewording as "Returns a {code KDF} object that implements the specified algorithm from the specified security provider." src/java.base/share/classes/javax/crypto/KDF.java line 252: > 250: > 251: /** > 252: * Creates an instance of the {@code KDF} object. Suggest rewording as "Returns a {@code KDF} object that implements the specified algorithm and is initialized with the specified parameters." src/java.base/share/classes/javax/crypto/KDF.java line 258: > 256: * @param algParameterSpec > 257: * the {@code AlgorithmParameterSpec} used to configure this > KDF's > 258: * algorithm or {@code null} if no additional parameters were > provided. s/were/are/ no period at end src/java.base/share/classes/javax/crypto/KDF.java line 395: > 393: * <p> > 394: * The {@code deriveKey} method may be called multiple times once a > 395: * {@code KDF} object is initialized. I think this sentence is important enough that it should be repeated in the class summary. src/java.base/share/classes/javax/crypto/KDF.java line 398: > 396: * <p> > 397: * Delayed provider selection is also supported such that the > provider > 398: * performing the derive is not selected until the method is called. Delayed provider selection is an important enough topic that it probably should be in the class summary. However it is complicated to word correctly as there is also the case if someone calls `getProviderName` beforehand which locks the provider to the first one supporting the algorithm. I would probably also avoid "delayed provider" as that is not a term currently used in the javadocs. Suggest something like: If a provider is not specified in the getInstance method when instantiating a KDF object, the provider is selected the first time the deriveKey or deriveData method is called and a provider is chosen that supports the parameters passed to the deriveKey or deriveData method, for example the initial key material. However, if getProviderName is called before calling the deriveKey or deriveData methods, the first provider supporting the KDF algorithm is chosen which may not be the desired one; therefore it is recommended to not call getProviderName until after a key derivation operation. src/java.base/share/classes/javax/crypto/KDF.java line 401: > 399: * > 400: * @param alg > 401: * the algorithm of the resultant {@code SecretKey} object This method should throw NPE if alg is null. src/java.base/share/classes/javax/crypto/KDF.java line 403: > 401: * the algorithm of the resultant {@code SecretKey} object > 402: * @param kdfParameterSpec > 403: * derivation parameters Can this be null? If so, you should specify that null is allowed. src/java.base/share/classes/javax/crypto/KDF.java line 414: > 412: * > 413: */ > 414: public SecretKey deriveKey(String alg, KDFParameterSpec > kdfParameterSpec) Are there cases where the parameters are correct, but the derive operation can still fail for other reasons? If so, I'm not sure we should be wrapping those in InvalidParameterSpecException. That exception should be thrown by the method initially when it inspects the parameters and before the derive operation begins. This is why I mentioned previously if it makes sense to add a DerivationException class. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651531 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651374 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644343 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644752 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644842 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644961 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597645015 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597646828 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597645773 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597646383 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597646492 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597647056 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597647294 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651283 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597650550 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651071 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597650904 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597652741
