RFR: 8331008: KDF Implementation (Preview) [v28]

[Weijun Wang]

On Mon, 13 May 2024 03:46:50 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).
>
> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
> 
>   re-enable preview annotations

My second round code review.

src/java.base/share/classes/javax/crypto/KDF.java line 46:

> 44: 
> 45: /**
> 46:  * This class provides the functionality of a key derivation algorithm for JCE.

Or should it be "key derivation function"? The "F" in "KDF" is not mentioned.

Also, except for this first sentence, I suggest we only use the "KDF" name everywhere. I see too many "the key derivation algorithm" below.

src/java.base/share/classes/javax/crypto/KDF.java line 387:

> 385: 
> 386:     /**
> 387:      * Derive a key, returned as a {@code SecretKey}.

"Derives".

src/java.base/share/classes/javax/crypto/KDF.java line 407:

> 405:      *     invalid or incorrect for the type of key to be derived, or specifies
> 406:      *     a type of output that is not a key (e.g. raw data)
> 407:      */

What does "or specifies a type of output that is not a key (e.g. raw data)" mean? `KDFParameterSpec` has no field at all.

src/java.base/share/classes/javax/crypto/KDF.java line 454:

> 452: 
> 453:     /**
> 454:      * Obtain raw data from a key derivation function.

"Obtains".

src/java.base/share/classes/javax/crypto/KDF.java line 467:

> 465:      * @return a byte array whose length matches the length field in the
> 466:      *     processed {@code KDFParameterSpec} and containing the next bytes of
> 467:      *     output from the key derivation function

There is no length filed in `KDFParameterSpec`. There are also no "next bytes output". We don't have a key stream now.

src/java.base/share/classes/javax/crypto/KDFSpi.java line 59:

> 57:      * <p>
> 58:      * An {@code AlgorithmParameterSpec} may be specified for PRF algorithms that
> 59:      * may require this. Though no such KDF algorithms are currently defined,

You really want to say "Though no such KDF algorithms are currently defined"? First, you need to remove this when we invent one. Second, I think we allow 3rd party provider to support non-standard KDF algorithms.

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

> 37:  * operations of the HMAC-based Key Derivation Function (HKDF). The HKDF
> 38:  * function is defined in <a href="http://tools.ietf.org/html/rfc5869">RFC
> 39:  * 5869</a>.

Add enough examples on all 3 styles of parameters. Add more requirements to implementations. For example, a constructor with an `AlgorithmParameterSpec` argument must be provided but the argument must be `null`. `deriveKey` and `deriveData` must be thread-safe and their argumentmust be `HKDFParameterSpec`.

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

> 46:     /**
> 47:      * This builder helps with the mutation required by the {@code Extract}
> 48:      * scenario.

First sentence should be "A builder for building Extract and ExtractExpand objects", and then in a new paragraph, talk about collecting IKMs and salts. Explain why listed are used because this could be controversial. Finally, tell user to call one method to create an `Extract` object and call another to create `ExtractExpand`.

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

> 48:      * scenario.
> 49:      */
> 50:     final class Builder {

Add preview annotation to `Builder`. For safety, you can add it to `Expand`, `Extract`, and `ExtractExpand` as well.

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

> 67: 
> 68:         /**
> 69:          * Akin to a {@code Builder.build()} method for an {@code Extract}

Just say "Builds a ...".

src/java.base/share/classes/sun/security/util/Debug.java line 142:

> 140:         System.err.println("              only dump output for the specified list");
> 141:         System.err.println("              of JCA engines. Supported values:");
> 142:         System.err.println("              Cipher, KDF, KeyAgreement, KeyGenerator,");

Do we also need to add the option name itself? Somewhere neat line 100.

test/jdk/com/sun/crypto/provider/KDF/TestHKDFInitialization.java line 1:

> 1: /*

Why the class name? Is this only about initialization?

test/jdk/javax/crypto/KDF/Functions.java line 1:

> 1: /*

This one should probably be moved to `com/sun/crypto/provider/HKDF`.

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

PR Review: https://git.openjdk.org/jdk/pull/18924#pullrequestreview-2052779077
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598519889
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598525070
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598526782
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598528510
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598527991
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598531102
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598533592
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598537827
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598532505
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598538774
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598547147
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598549759
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1598550658