RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v11]

Fri Aug 16 21:16:32 UTC 2024

On Wed, 14 Aug 2024 02:50:39 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   addressed several review comments, namely: - renaming the getParameters method - renaming the AlgorithmParameterSpec object - address some javadoc exception messages - add some information to KDF class private constructor javadocs - other general cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 86:
> 
>> 84:      *
>> 85:      * @throws InvalidAlgorithmParameterException
>> 86:      *     if the information contained within the {@code KDFParameterSpec} is
> 
> typo: `KDFParaneterSpec` should be `derivationParameterSpec`.
> BTW, I assume the javadoc is copied from `KDFSpi` class. I'd suggest to shorten it and match it to the actual implementation instead of the general description as in `KDFSpi`.
> For the last sentence, do you have an example of how the `derivationParameterSpec` typed `HKDFParameterSpec` can specify a type of output that is not a key?

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 242:
> 
>> 240:         }
>> 241:         throw new InvalidAlgorithmParameterException(
>> 242:             "an HKDF could not be initialized with the given KDFParameterSpec");
> 
> It's clearer to state that the given `KDFParameterSpec` must be `HKDFParameterSpec`. Also, given that KDF.getInstance() takes a KDFParameters parameters, I'd avoid the word "initialized" as it may confuse people which parameters you are talking about.
> I'd suggest something like "HKDF data/key derivatopn requires HKDFParameterSpec, not " + derivationParameterSpec.getClass()
> Also, for readability, it may be better to check the specified `derivationParameterSpec` is an instanceof `HKDFParameterSpec` in the beginning.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 279:
> 
>> 277: 
>> 278:     /**
>> 279:      * Perform the HMAC-Extract operation.
> 
> typo: 'HMAC' should be 'HKDF'

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 284:
> 
>> 282:      *     the input keying material used for the HKDF-Extract operation.
>> 283:      * @param salt
>> 284:      *     the salt value used for HKDF-Extract.  If no salt is to be used a
> 
> "If no salt is to be used a {@code null} value should be provided." should be "or {@code null} if no salt value is provided." as in the `hkdfExpand()` method javadoc.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 310:
> 
>> 308: 
>> 309:     /**
>> 310:      * Perform the HMAC-Expand operation.  At the end of the operation, the
> 
> typo: 'HMAC' should be 'HKDF'.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 312:
> 
>> 310:      * Perform the HMAC-Expand operation.  At the end of the operation, the
>> 311:      * keyStream instance variable will contain the complete KDF output based on
>> 312:      * the input values and desired length.
> 
> These lines are outdated? I can't find any `keyStream` instance variable.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 331:
> 
>> 329:      *     or derived during the generation of the PRK.
>> 330:      */
>> 331:     protected byte[] hkdfExpand(SecretKey prk, byte[] info, int outLen)
> 
> Same here, can be made 'private'.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 338:
> 
>> 336:         // Calculate the number of rounds of HMAC that are needed to
>> 337:         // meet the requested data.  Then set up the buffers we will need.
>> 338:         hmacObj.init(prk);
> 
> RFC5869 sec 2.3 states that "PRK - a pseudorandom key of at least HashLen octets". Shouldn't we check it before passing to to `hmacObj`?

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/javax/crypto/KDFSpi.java line 129:
> 
>> 127:      *     derivation parameters
>> 128:      *
>> 129:      * @return a byte array corresponding to a key built from the
> 
> Can't we just state 
> 
>> @return a byte array output by this KDF object using the derivation parameters.
> 
> No need to mention the "key built from ..." part. KDF output is bytes, we package it into key objects and not the other way around.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 262:
> 
>> 260:                 throw new NullPointerException(
>> 261:                     "salt must not be null");
>> 262:             }
> 
> Why not use `Objects.requireNonNull(T, String)`? Same goes to all other `addXXX()` methods.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 362:
> 
>> 360:          *
>> 361:          * @param prk
>> 362:          *     the pseudorandom key; may be {@code null}
> 
> Instead of stating "prk may be {@null}", how about narrow it down/clarify to "in the case of ExtractThenExpand, prk may be {@null} since the output of extract phase is used"

Numerous comments elsewhere in the code illustrate what's happening. Is your concern for readers of the javadoc? This is probably a valid suggestion.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 428:
> 
>> 426:          * <p>
>> 427:          * Note: {@code addIKMValue} and {@code addSaltValue} may be called
>> 428:          * afterward to supply additional values, if desired
> 
> What does this mean? {@code addIKMValue} and {@code addSaltValue} are methods of (@code Builder} class and do not belong to the {@code ExtractThenExpand} class. Copy-n-paste error?

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 443:
> 
>> 441:          *     if {@code length} is not > 0
>> 442:          */
>> 443:         private ExtractThenExpand(Extract ext, byte[] info, int length) {
> 
> Check {@code ext} to be not null?

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 444:
> 
>> 442:          */
>> 443:         private ExtractThenExpand(Extract ext, byte[] info, int length) {
>> 444:             // null-checked previously
> 
> nit: where is this checked? I didn't find it. The comment seems incorrect.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720360536
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720360709
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720361297
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720361445
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720361360
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720361525
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720361254
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720361697
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720356437
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720357246
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720359927
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720360092
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720360264
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720360406