On Fri, 13 Sep 2024 19:39:04 GMT, Kevin Driver <kdri...@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:
> 
>   refine wording on DPS getInstance with params exception

Comments on the API spec in `KDF.java`. I'll need some more time to look at the 
implementation.

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

> 192:      * otherwise {@code null} is returned.
> 193:      *
> 194:      * @see <a href="#DelayedProviderSelection">Delayed Provider 
> Selection</a>

Why `@see DPS`? The paragraph above has nothing to do with it.

If you meant the warning not to call it too early (which I think is not 
necessary), why not add the same to the `getProviderName` method?

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

> 205:      * Returns a {@code KDF} object that implements the specified 
> algorithm.
> 206:      *
> 207:      * @see <a href="#DelayedProviderSelection">Delayed Provider 
> Selection</a>

Put `@see` at the end. Put `@implNote` before `@param`. This is the order they 
are rendered in HTML javadoc.

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

> 264:      *     list
> 265:      * @throws NullPointerException
> 266:      *     if the {@code algorithm} or {@code provider} is {@code null}

Remove the `the` word. Same below.

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

> 343:      *     if at least one {@code Provider} supports a {@code KDFSpi}
> 344:      *     implementation for the specified algorithm but none of them
> 345:      *     support the specified parameters

One of the two `@throws` above uses `supports a {@code KDF} implementation` and 
the other uses `supports a {@code KDFSpi} implementation`. We'd better choose 
the same class name.

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

> 424:      * @throws InvalidAlgorithmParameterException
> 425:      *     if the specified provider does not support a {@code KDFSpi}
> 426:      *     implementation for the specified algorithm and parameters

Do not mention `algorithm` in this `@throws`. It's already covered by `@throws 
NSAE`. Same below.

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

> 528:      *     results in something invalid
> 529:      * @throws NoSuchAlgorithmException
> 530:      *     if {@code alg} is empty or invalid

Is it easy to tell precisely what falls into "combination of alg and 
derivationSpec results in something invalid" and what falls into "alg is 
invalid"?

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

> 553: 
> 554:     /**
> 555:      * Obtains raw data from a key derivation function.

The first sentences of the two `derive` methods use different verbs: `Derives` 
and `Obtains`. Is it possible to use a same one?

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

PR Review: https://git.openjdk.org/jdk/pull/20301#pullrequestreview-2305207448
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759845500
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759869223
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759877457
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759878466
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759878857
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759883966
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759882929

Reply via email to