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

[Weijun Wang]

On Fri, 13 Sep 2024 14:59:10 GMT, Kevin Driver <kdriver at 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:
> 
>   remove explicit zeroing in favor of finally blocks

My comments on `HKDFParameterSpec`.

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.

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?

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"?

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.

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?

src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 119:

> 117: 
> 118:         /**
> 119:          * Builds an {@code Extract} from the current state of the

Is it better to say "Builds an Extract object"? Same with the `@return` line and other methods.

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.

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?

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".

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.

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`.

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`.

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.

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

PR Review: https://git.openjdk.org/jdk/pull/20301#pullrequestreview-2303442839
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759075122
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759076323
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759077637
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759079953
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759080830
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759147114
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759082309
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759085923
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759087898
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759169705
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759172579
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759176153
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759178535