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

Weijun Wang weijun at openjdk.org
Mon Aug 26 18:01:22 UTC 2024 

On Fri, 23 Aug 2024 21:48:44 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:
> 
>   code review comments and test renaming

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 175:

> 173:                 (HKDFParameterSpec.Expand) derivationParameterSpec;
> 174:             // set this value in the "if"
> 175:             if ((pseudoRandomKey = anExpand.prk()) == null) {

Will not happen. Throw an `AssertionError` instead.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 194:

> 192:                                      length);
> 193:             } catch (InvalidKeyException ike) {
> 194:                 throw (InvalidAlgorithmParameterException) new InvalidAlgorithmParameterException(

IAPE has a ctor with cause as an argument.

src/java.base/share/classes/java/security/KDFParameters.java line 43:

> 41:  * <p>
> 42:  * The {@code KDFParameters} used for initialization can be retrieved via
> 43:  * {@link javax.crypto.KDF#getParameters()}.

Since you add this line here right after the "initialized with" line, there should be some words about the difference between the user-provided params and actual params.

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

> 96:                                                           "Provider");
> 97:     private static final boolean skipDebug = Debug.isOn("engine=")
> 98:                                              && !Debug.isOn("kdf");

Add some debug output, especially on exceptions for all different reasons during delayed provider selection.

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

> 315:      *     if no {@code Provider} supports a {@code KDFSpi} implementation for
> 316:      *     the specified algorithm
> 317:      * @throws InvalidAlgorithmParameterException

No IAPE is thrown in the current implementation.

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

> 317:      * @throws InvalidAlgorithmParameterException
> 318:      *     if the initialization parameters are inappropriate for this
> 319:      *     {@code KDF} or if no provider can be found which supports the

What does "this KDF" mean? This is a static method and there is no instance yet.

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

> 471:      * @param alg
> 472:      *     the algorithm of the resultant {@code SecretKey} object
> 473:      * @param derivationParameterSpec

I prefer a short name like `input`. I know the type is `AlgorithmParameterSpec` but it's actually not parameter.

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

> 474:      *     the object describing the inputs to the derivation function
> 475:      *
> 476:      * @return a {@code SecretKey} object corresponding to a key built from the

I suggest just `@return the derived key`.

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

> 516:      *     the object describing the inputs to the derivation function
> 517:      *
> 518:      * @return a byte array corresponding to the KDF output and according to

I suggest `@return the derived key in its raw bytes`. This also implies my earlier suggestion on the relation between the output of the 2 derive methods.

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

> 50:  * {@code null} to be passed, otherwise an {@code InvalidAlgorithmParameterException}
> 51:  * may be thrown. On the other hand, implementations which require
> 52:  * {@code KDFParameters} may throw an {@code InvalidAlgorithmParameterException}

This is not only `may`, this is `must` or `should`.

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

> 52:  * {@code KDFParameters} may throw an {@code InvalidAlgorithmParameterException}
> 53:  * upon receiving a {@code null} value. Furthermore, implementations
> 54:  * may supply default values for {@code KDFParameters}, mutating the

The `mutating` word is suspicious. The object is very likely to be immutable. Just say a different object should be returned in the next sentence.

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

> 53:  * upon receiving a {@code null} value. Furthermore, implementations
> 54:  * may supply default values for {@code KDFParameters}, mutating the
> 55:  * object. In that case, {@link KDFSpi#engineGetParameters()} would supply

Change `would` to `should`. This is requirement for the implementation.

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

> 1: /*

Rename all `key material` to `keying material`, both input and output. That's the name used in the RFC.

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

> 155:          * <p>
> 156:          * This supports the use-case where a label can be applied to the IKM
> 157:          * but the actual value of the IKM is not yet available.

I feel the two paragraphs above are repeated too many times. Better describe them in the class spec.

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

> 158:          * <p>
> 159:          * An implementation should concatenate the input key materials into a
> 160:          * single value once all components are available.

The above is a requirement for implementations and should be better to be moved to `ikms`. You can keep the line here with `would` instead of `should`.

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

> 280:      * @param length
> 281:      *     the length of the output key material (must be greater than 0 and
> 282:      *     less than 255 * HMAC length)

The maximum size of `length` is not checked in this class but it's worth mentioning. Also, the size of `prk` also has a minimum size that is not checked here. I suggest talking about both in the method spec.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731607816
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731607112
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731586598
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731609569
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731593121
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731592305
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731594606
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731595042
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731595804
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731597548
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731598663
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731597229
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731606140
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731602285
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731601431
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1731604829

More information about the security-dev mailing list