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

Sun May 12 14:55:15 UTC 2024

On Sat, 11 May 2024 02:06:09 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:
> 
>   commenting out until better understood -- causing failures

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

> 1: /*

Note: many of my comments apply to multiple methods, so please check all other methods when fixing.

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.

Suggest adding a sentence describing what key derivation does.

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

> 142:      *
> 143:      * <p>This is the same name that was specified in one of the
> 144:      * {@code getInstance} calls that created this {@code KDF} object.

I don't think it is necessary to include this sentence.

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

> 161: 
> 162:     /**
> 163:      * Creates an instance of the {@code KDF} object.

Suggest rewording as "Returns a {@code KDF} object that implements the specified algorithm."

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

> 164:      *
> 165:      * @param algorithm
> 166:      *     the key derivation algorithm to use

This should include a pointer to the KDF section of the Standard Algorithm Names specification once you set that up.

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

> 169:      *
> 170:      * @throws NoSuchAlgorithmException
> 171:      *     if no {@code Provider} supports a {@code KDFSpi} implementation for

s/KDFSpi/KDF/

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

> 172:      *     the specified algorithm
> 173:      * @throws NullPointerException
> 174:      *     if the algorithm is {@code null}

s/the algorithm/{@code algorithm}/

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

> 188:     /**
> 189:      * Creates an instance of the {@code KDF} object with a specific
> 190:      * {@code Provider}.

Suggest rewording as "Returns a {@code KDF} object that implements the specified algorithm from the specified security provider."

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

> 193:      *     the key derivation algorithm to use
> 194:      * @param provider
> 195:      *     the provider to use for this key derivation

It looks like provider can be null. If so, you should specify that, e.g. "If null, this method is equivalent to getInstance(String)."

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

> 199:      * @throws NoSuchAlgorithmException
> 200:      *     if no {@code Provider} supports a {@code KDFSpi} implementation for
> 201:      *     the specified algorithm

This exception reason looks to be copied from getInstance(String), but is different for this method. It should be: "if a provider is specified and it does not support the specified KDF algorithm, or if provider is null and there is no provider that supports a KDF implementation of the specified algorithm"

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

> 221:     /**
> 222:      * Creates an instance of the {@code KDF} object using a supplied
> 223:      * {@code Provider} instance.

Suggest rewording as "Returns a {code KDF} object that implements the specified algorithm from the specified security provider."

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

> 250: 
> 251:     /**
> 252:      * Creates an instance of the {@code KDF} object.

Suggest rewording as "Returns a {@code KDF} object that implements the specified algorithm and is initialized with the specified parameters."

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

> 256:      * @param algParameterSpec
> 257:      *     the {@code AlgorithmParameterSpec} used to configure this KDF's
> 258:      *     algorithm or {@code null} if no additional parameters were provided.

s/were/are/
no period at end

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

> 393:      * <p>
> 394:      * The {@code deriveKey} method may be called multiple times once a
> 395:      * {@code KDF} object is initialized.

I think this sentence is important enough that it should be repeated in the class summary.

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

> 396:      * <p>
> 397:      * Delayed provider selection is also supported such that the provider
> 398:      * performing the derive is not selected until the method is called.

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.

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

> 399:      *
> 400:      * @param alg
> 401:      *     the algorithm of the resultant {@code SecretKey} object

This method should throw NPE if alg is null.

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

> 401:      *     the algorithm of the resultant {@code SecretKey} object
> 402:      * @param kdfParameterSpec
> 403:      *     derivation parameters

Can this be null? If so, you should specify that null is allowed.

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

> 412:      *
> 413:      */
> 414:     public SecretKey deriveKey(String alg, KDFParameterSpec kdfParameterSpec)

Are there cases where the parameters are correct, but the derive operation can still fail for other reasons? If so, I'm not sure we should be wrapping those in InvalidParameterSpecException. That exception should be thrown by the method initially when it inspects the parameters and before the derive operation begins.

This is why I mentioned previously if it makes sense to add a DerivationException class.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651531
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651374
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644343
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644752
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644842
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597644961
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597645015
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597646828
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597645773
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597646383
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597646492
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597647056
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597647294
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651283
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597650550
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597651071
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597650904
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597652741