On Tue, 14 May 2024 19:49:43 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: > > throw an exception if algorithm parameter spec is passed src/java.base/share/classes/javax/crypto/KDF.java line 281: > 279: /** > 280: * Creates an instance of the {@code KDF} object with a specific > 281: * {@code Provider}. Suggest rewording like other getInstance methods: "Returns a {@code KDF} object that implements the specified algorithm from the specified provider and is initialized with the specified parameters." src/java.base/share/classes/javax/crypto/KDF.java line 332: > 330: /** > 331: * Creates an instance of the {@code KDF} object using a supplied > 332: * {@code Provider} instance. Suggest rewording like other getInstance methods: "Returns a {@code KDF} object that implements the specified algorithm from the specified provider and is initialized with the specified parameters." src/java.base/share/classes/javax/crypto/KDF.java line 391: > 389: * <p> > 390: * The {@code deriveKey} method may be called multiple times on a > particular > 391: * {@code KDF} instance. s/multiple times/multiple times at the same time/ I also think this thread-safety requirement should be specified in the class summary. (I think I mentioned this in another comment) Same comment for `deriveData`. src/java.base/share/classes/javax/crypto/KDF.java line 395: > 393: * Delayed provider selection is also supported such that the > provider > 394: * performing the derive is not selected until the method is called. > Once a > 395: * provider is selected, it cannot be changed. You may have missed one of my earlier comments: > 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. > Yes, maybe add at end: "Once a provider is selected, it cannot be changed." src/java.base/share/classes/javax/crypto/KDF.java line 401: > 399: * {@code null}) > 400: * @param kdfParameterSpec > 401: * derivation parameters (may not be {@code null} Normally, we only say if parameters may be null and don't say parameters may not be null, since the `@throws NullPointerException` makes this case clear. So I would remove the words "(may not be null)". Same comment for deriveData. src/java.base/share/classes/javax/crypto/KDF.java line 460: > 458: } > 459: // should never reach here > 460: return null; I think this could reach here if `lastException` is `null`. In any case, it's safer to throw an exception here instead of returning null as this is clearly an error condition. Something like: throw new InvalidParameterSpecException ("No installed provider supports the deriveKey method with these parameters"); Same comment for `deriveData`. src/java.base/share/classes/javax/crypto/KDF.java line 464: > 462: > 463: /** > 464: * Obtains raw data from a key derivation function. Is there a better word than "Obtains" you can use here? How about: "Derives and returns key material as a byte array, which can be used for entropy or if key material is required in that form." src/java.base/share/classes/javax/crypto/KDF.java line 478: > 476: * @return a byte array whose length matches the specified length in > the > 477: * processed {@code KDFParameterSpec} and containing the output > from the > 478: * key derivation function There is no length parameter in `KDFParameterSpec`. Suggest making this more generic as you did for `deriveKey`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600644237 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600644587 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600657005 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600662736 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600664674 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600678440 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600679240 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600681960
