On Mon, 9 Sep 2024 20:50:47 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> updated comments around locking mechanism
>
> src/java.base/share/classes/java/security/KDFParameters.java line 39:
>
>> 37: * the
>> 38: * {@link javax.crypto.KDF#getInstance(String, KDFParameters)
>> KDF.getInstance}
>> 39: * methods. The {@code getInstance} method returns a {@code KDF}.
>
> Suggest changing the above sentence to:
>
> The {@code getInstance} method returns a {@code KDF} that is initialized with
> the specified parameters.
@seanjmullan: Resolved in
https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
> src/java.base/share/classes/java/security/KDFParameters.java line 45:
>
>> 43: * parameters were not supplied and can be generated by the {@code KDF}
>> 44: * object, these may be supplied by the implementation. For additional
>> 45: * information, see: {@link javax.crypto.KDF#getParameters()}.
>
> I would simplify this as:
>
> The {@code KDFParameters} used for initialization are returned by {@link
> javax.crypto.KDF#getParameters()} and may contain additional default or
> random parameter values used by the underlying KDF implementation.
@seanjmullan: Resolved in
https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
> src/java.base/share/classes/javax/crypto/KDF.java line 225:
>
>> 223: * if {@code algorithm} is {@code null}
>> 224: */
>> 225: public static KDF getInstance(String algorithm)
>
> One thing that is missing from this method is an Implementation Note
> describing how the `jdk.security.provider.preferred` property affects the
> search algorithm. See any other JCE API - you can just copy/paste the text.
> Also affects the `getInstance` method that takes an algorithm and params.
@seanjmullan: Resolved in
https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
> src/java.base/share/classes/javax/crypto/KDF.java line 352:
>
>> 350: new InvalidAlgorithmParameterException(
>> 351: "No provider can be found that supports the
>> "
>> 352: + "specified parameters"));
>
> Slight tweak in message: s/specified parameters/specified algorithm and
> parameters/
@seanjmullan: Resolved in
https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
> src/java.base/share/classes/javax/crypto/KDF.java line 369:
>
>> 367: lastException =
>> 368: new NoSuchAlgorithmException(
>> 369: new InvalidAlgorithmParameterException(
>
> Slight tweak in message: s/specified parameters/specified algorithm and
> parameters/
@seanjmullan: Resolved in
https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
> src/java.base/share/classes/javax/crypto/KDF.java line 507:
>
>> 505: * <p>
>> 506: * The {@code deriveKey} method may be called multiple times on a
>> particular
>> 507: * {@code KDF} instance, but it is not considered thread-safe.
>
> My preference would be to remove this sentence and the same one in deriveData
> now that you have the "Concurrent Access" section in the Class Summary which
> I think describes the concurrency behavior of the whole API better.
@seanjmullan: Resolved in
https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755775749
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755776023
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755776724
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755776255
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755776497
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1755777144
