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

[Sean Mullan]

On Tue, 14 May 2024 19:49:43 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:
> 
>   throw an exception if algorithm parameter spec is passed

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

> 279:     /**
> 280:      * Creates an instance of the {@code KDF} object with a specific
> 281:      * {@code Provider}.

Suggest rewording like other getInstance methods: "Returns a {@code KDF} object that implements the specified algorithm from the specified provider and is initialized with the specified parameters."

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

> 330:     /**
> 331:      * Creates an instance of the {@code KDF} object using a supplied
> 332:      * {@code Provider} instance.

Suggest rewording like other getInstance methods: "Returns a {@code KDF} object that implements the specified algorithm from the specified provider and is initialized with the specified parameters."

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

> 389:      * <p>
> 390:      * The {@code deriveKey} method may be called multiple times on a particular
> 391:      * {@code KDF} instance.

s/multiple times/multiple times at the same time/

I also think this thread-safety requirement should be specified in the class summary. (I think I mentioned this in another comment)

Same comment for `deriveData`.

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

> 393:      * Delayed provider selection is also supported such that the provider
> 394:      * performing the derive is not selected until the method is called. Once a
> 395:      * provider is selected, it cannot be changed.

You may have missed one of my earlier comments:

> Delayed provider selection is an important enough topic that it probably should be in the class summary. However it is complicated to word correctly as there is also the case if someone calls `getProviderName` beforehand which locks the provider to the first one supporting the algorithm. I would probably also avoid "delayed provider" as that is not a term currently used in the javadocs. Suggest something like:
> 
> If a provider is not specified in the getInstance method when instantiating a KDF object, the provider is selected the first time the deriveKey or deriveData method is called and a provider is chosen that supports the parameters passed to the deriveKey or deriveData method, for example the initial key material. However, if getProviderName is called before calling the deriveKey or deriveData methods, the first provider supporting the KDF algorithm is chosen which may not be the desired one; therefore it is recommended to not call getProviderName until after a key derivation operation.

> Yes, maybe add at end: "Once a provider is selected, it cannot be changed."

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

> 399:      *     {@code null})
> 400:      * @param kdfParameterSpec
> 401:      *     derivation parameters (may not be {@code null}

Normally, we only say if parameters may be null and don't say parameters may not be null, since the `@throws NullPointerException` makes this case clear. So I would remove the words "(may not be null)".

Same comment for deriveData.

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

> 458:         }
> 459:         // should never reach here
> 460:         return null;

I think this could reach here if `lastException` is `null`. In any case, it's safer to throw an exception here instead of returning null as this is clearly an error condition. Something like:


            throw new InvalidParameterSpecException
                ("No installed provider supports the deriveKey method with these parameters");


Same comment for `deriveData`.

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

> 462: 
> 463:     /**
> 464:      * Obtains raw data from a key derivation function.

Is there a better word than "Obtains" you can use here? How about:

"Derives and returns key material as a byte array, which can be used for entropy or if key material is required in that form."

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

> 476:      * @return a byte array whose length matches the specified length in the
> 477:      *     processed {@code KDFParameterSpec} and containing the output from the
> 478:      *     key derivation function

There is no length parameter in `KDFParameterSpec`. Suggest making this more generic as you did for `deriveKey`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600644237
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600644587
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600657005
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600662736
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600664674
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600678440
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600679240
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600681960