On Fri, 23 Aug 2024 21:48:44 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: > > code review comments and test renaming src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 175: > 173: (HKDFParameterSpec.Expand) derivationParameterSpec; > 174: // set this value in the "if" > 175: if ((pseudoRandomKey = anExpand.prk()) == null) { Will not happen. Throw an `AssertionError` instead. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 194: > 192: length); > 193: } catch (InvalidKeyException ike) { > 194: throw (InvalidAlgorithmParameterException) new > InvalidAlgorithmParameterException( IAPE has a ctor with cause as an argument. src/java.base/share/classes/java/security/KDFParameters.java line 43: > 41: * <p> > 42: * The {@code KDFParameters} used for initialization can be retrieved via > 43: * {@link javax.crypto.KDF#getParameters()}. Since you add this line here right after the "initialized with" line, there should be some words about the difference between the user-provided params and actual params. src/java.base/share/classes/javax/crypto/KDF.java line 98: > 96: "Provider"); > 97: private static final boolean skipDebug = Debug.isOn("engine=") > 98: && !Debug.isOn("kdf"); Add some debug output, especially on exceptions for all different reasons during delayed provider selection. src/java.base/share/classes/javax/crypto/KDF.java line 317: > 315: * if no {@code Provider} supports a {@code KDFSpi} > implementation for > 316: * the specified algorithm > 317: * @throws InvalidAlgorithmParameterException No IAPE is thrown in the current implementation. src/java.base/share/classes/javax/crypto/KDF.java line 319: > 317: * @throws InvalidAlgorithmParameterException > 318: * if the initialization parameters are inappropriate for this > 319: * {@code KDF} or if no provider can be found which supports the What does "this KDF" mean? This is a static method and there is no instance yet. src/java.base/share/classes/javax/crypto/KDF.java line 473: > 471: * @param alg > 472: * the algorithm of the resultant {@code SecretKey} object > 473: * @param derivationParameterSpec I prefer a short name like `input`. I know the type is `AlgorithmParameterSpec` but it's actually not parameter. src/java.base/share/classes/javax/crypto/KDF.java line 476: > 474: * the object describing the inputs to the derivation function > 475: * > 476: * @return a {@code SecretKey} object corresponding to a key built > from the I suggest just `@return the derived key`. src/java.base/share/classes/javax/crypto/KDF.java line 518: > 516: * the object describing the inputs to the derivation function > 517: * > 518: * @return a byte array corresponding to the KDF output and > according to I suggest `@return the derived key in its raw bytes`. This also implies my earlier suggestion on the relation between the output of the 2 derive methods. src/java.base/share/classes/javax/crypto/KDFSpi.java line 52: > 50: * {@code null} to be passed, otherwise an {@code > InvalidAlgorithmParameterException} > 51: * may be thrown. On the other hand, implementations which require > 52: * {@code KDFParameters} may throw an {@code > InvalidAlgorithmParameterException} This is not only `may`, this is `must` or `should`. src/java.base/share/classes/javax/crypto/KDFSpi.java line 54: > 52: * {@code KDFParameters} may throw an {@code > InvalidAlgorithmParameterException} > 53: * upon receiving a {@code null} value. Furthermore, implementations > 54: * may supply default values for {@code KDFParameters}, mutating the The `mutating` word is suspicious. The object is very likely to be immutable. Just say a different object should be returned in the next sentence. src/java.base/share/classes/javax/crypto/KDFSpi.java line 55: > 53: * upon receiving a {@code null} value. Furthermore, implementations > 54: * may supply default values for {@code KDFParameters}, mutating the > 55: * object. In that case, {@link KDFSpi#engineGetParameters()} would supply Change `would` to `should`. This is requirement for the implementation. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 1: > 1: /* Rename all `key material` to `keying material`, both input and output. That's the name used in the RFC. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 157: > 155: * <p> > 156: * This supports the use-case where a label can be applied to > the IKM > 157: * but the actual value of the IKM is not yet available. I feel the two paragraphs above are repeated too many times. Better describe them in the class spec. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 160: > 158: * <p> > 159: * An implementation should concatenate the input key materials > into a > 160: * single value once all components are available. The above is a requirement for implementations and should be better to be moved to `ikms`. You can keep the line here with `would` instead of `should`. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 282: > 280: * @param length > 281: * the length of the output key material (must be greater than 0 > and > 282: * less than 255 * HMAC length) The maximum size of `length` is not checked in this class but it's worth mentioning. Also, the size of `prk` also has a minimum size that is not checked here. I suggest talking about both in the method spec. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731607816 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731607112 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731586598 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731609569 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731593121 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731592305 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731594606 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731595042 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731595804 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731597548 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731598663 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731597229 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731606140 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731602285 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731601431 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731604829
