On Mon, 13 May 2024 03:46:50 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: > > re-enable preview annotations My second round code review. 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. Or should it be "key derivation function"? The "F" in "KDF" is not mentioned. Also, except for this first sentence, I suggest we only use the "KDF" name everywhere. I see too many "the key derivation algorithm" below. src/java.base/share/classes/javax/crypto/KDF.java line 387: > 385: > 386: /** > 387: * Derive a key, returned as a {@code SecretKey}. "Derives". src/java.base/share/classes/javax/crypto/KDF.java line 407: > 405: * invalid or incorrect for the type of key to be derived, or > specifies > 406: * a type of output that is not a key (e.g. raw data) > 407: */ What does "or specifies a type of output that is not a key (e.g. raw data)" mean? `KDFParameterSpec` has no field at all. src/java.base/share/classes/javax/crypto/KDF.java line 454: > 452: > 453: /** > 454: * Obtain raw data from a key derivation function. "Obtains". src/java.base/share/classes/javax/crypto/KDF.java line 467: > 465: * @return a byte array whose length matches the length field in the > 466: * processed {@code KDFParameterSpec} and containing the next > bytes of > 467: * output from the key derivation function There is no length filed in `KDFParameterSpec`. There are also no "next bytes output". We don't have a key stream now. src/java.base/share/classes/javax/crypto/KDFSpi.java line 59: > 57: * <p> > 58: * An {@code AlgorithmParameterSpec} may be specified for PRF > algorithms that > 59: * may require this. Though no such KDF algorithms are currently > defined, You really want to say "Though no such KDF algorithms are currently defined"? First, you need to remove this when we invent one. Second, I think we allow 3rd party provider to support non-standard KDF algorithms. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 39: > 37: * operations of the HMAC-based Key Derivation Function (HKDF). The HKDF > 38: * function is defined in <a href="http://tools.ietf.org/html/rfc5869";>RFC > 39: * 5869</a>. Add enough examples on all 3 styles of parameters. Add more requirements to implementations. For example, a constructor with an `AlgorithmParameterSpec` argument must be provided but the argument must be `null`. `deriveKey` and `deriveData` must be thread-safe and their argumentmust be `HKDFParameterSpec`. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 48: > 46: /** > 47: * This builder helps with the mutation required by the {@code > Extract} > 48: * scenario. First sentence should be "A builder for building Extract and ExtractExpand objects", and then in a new paragraph, talk about collecting IKMs and salts. Explain why listed are used because this could be controversial. Finally, tell user to call one method to create an `Extract` object and call another to create `ExtractExpand`. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 50: > 48: * scenario. > 49: */ > 50: final class Builder { Add preview annotation to `Builder`. For safety, you can add it to `Expand`, `Extract`, and `ExtractExpand` as well. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 69: > 67: > 68: /** > 69: * Akin to a {@code Builder.build()} method for an {@code Extract} Just say "Builds a ...". src/java.base/share/classes/sun/security/util/Debug.java line 142: > 140: System.err.println(" only dump output for the > specified list"); > 141: System.err.println(" of JCA engines. Supported > values:"); > 142: System.err.println(" Cipher, KDF, KeyAgreement, > KeyGenerator,"); Do we also need to add the option name itself? Somewhere neat line 100. test/jdk/com/sun/crypto/provider/KDF/TestHKDFInitialization.java line 1: > 1: /* Why the class name? Is this only about initialization? test/jdk/javax/crypto/KDF/Functions.java line 1: > 1: /* This one should probably be moved to `com/sun/crypto/provider/HKDF`. ------------- PR Review: https://git.openjdk.org/jdk/pull/18924#pullrequestreview-2052779077 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598519889 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598525070 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598526782 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598528510 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598527991 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598531102 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598533592 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598537827 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598532505 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598538774 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598547147 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598549759 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598550658
