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

[Kevin Driver]

On Fri, 13 Sep 2024 15:30:04 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   remove explicit zeroing in favor of finally blocks
>
> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 48:
> 
>> 46:  * The caller may wish to provide a label (or other components) of
>> 47:  * the IKM without having access to all portions. The same feature is
>> 48:  * available for salts.
> 
> Maybe remove the `The caller...` sentence above? The next paragraph has more detail on it.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 65:
> 
>> 63:  *
>> 64:  *
>> 65:  *}
> 
> What are these blank lines for? What do they look in the rendered HTML?

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 96:
> 
>> 94:      * {@code HKDFParameterSpec}. As stated in the class description,
>> 95:      * {@code addIKM} and/or {@code addSalt} may be called as needed. Finally,
>> 96:      * the object is "built" by calling either {@code extractOnly} or
> 
> Maybe "an object is built"?

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 105:
> 
>> 103: 
>> 104:         List<SecretKey> ikms = new ArrayList<>();
>> 105:         List<SecretKey> salts = new ArrayList<>();
> 
> Make the 2 above private.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 114:
> 
>> 112:          * @return a {@code Builder} to mutate
>> 113:          */
>> 114:         private Builder createBuilder() {
> 
> Is this method useful?

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 129:
> 
>> 127: 
>> 128:         /**
>> 129:          * Builds an {@code ExtractThenExpand}.
> 
> In `extractOnly`, you have "from the current state...". Make them consistent.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 133:
> 
>> 131:          * @param info
>> 132:          *     the optional context and application specific information (may be
>> 133:          *     {@code null}); the byte array is copied to prevent subsequent
> 
> The RFC says "can be a zero-length string". Do we want to somehow mention that our null is their empty?

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 139:
> 
>> 137:          *
>> 138:          * @implNote HKDF implementations will enforce that the length is less
>> 139:          * than 255 * HMAC length.
> 
> Precisely, it's "not greater than".

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 158:
> 
>> 156:          * be assembled piece-meal or if part of the IKM is to be supplied by a
>> 157:          * hardware crypto device. This method appends to the existing list of
>> 158:          * values or creates a new list if there is none yet.
> 
> Precisely, this should be "multiple addIKM may be called when..." since even in the simplest case you still need to call this method once. In fact, I don't think we need to talk about this detail every time. If you really want to emphasize this, create a section header with an anchor the class spec, and inside each method just say "This method can be called multiple times on a single Builder object. See class_section_name".
> 
> Appending to or creating a list is implementation detail.

@wangweij: I have changed the wording, though I will leave these descriptions in-place. See: https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 248:
> 
>> 246: 
>> 247:     /**
>> 248:      * Returns a builder for building {@code Extract} and
> 
> Modify `builder` to `{@code Builder} object`.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 278:
> 
>> 276:      *     if {@code prk} is {@code null}
>> 277:      * @throws IllegalArgumentException
>> 278:      *     if {@code length} is not > 0
> 
> Modify `>` to `greater than`.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1.

> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 309:
> 
>> 307:          * <p>
>> 308:          * Input keying material values added by {@link Builder#addIKM(byte[])}
>> 309:          * are converted to a {@code SecretKeySpec} object.
> 
> Do we need to say "empty arrays were discarded"? This is more precise and JCK might test on it.

@wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. I kept the same verb tense that was already established.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759288227
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287918
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287670
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287409
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287252
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287053
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759286868
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759286669
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759286329
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759288567
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759288723
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759289114