On Wed, 14 Aug 2024 02:50:39 GMT, Valerie Peng <valer...@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
