On Thu, 5 Sep 2024 19:23:04 GMT, Sean Mullan <[email protected]> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> change impl class to use byte arrays rather than SecretKey objects where
>> possible
>
> src/java.base/share/classes/javax/crypto/KDF.java line 89:
>
>> 87: * the {@code deriveKey} or {@code deriveData} method is called, and a
>> provider
>> 88: * is chosen that supports the parameters passed to the {@code
>> deriveKey} or
>> 89: * {@code deriveData} method. However, if {@code getProviderName} or
>
> This sentence is repeating what you said in the first sentence of the
> previous paragraph, so I don't think it is needed.
>
> Some suggested re-wordings (and a typo fix on `getKDFParameters` method
> name), starting this paragraph as:
>
> "If the {@code getProviderName} or {@code getParameters} method is called
> before the {@code deriveKey} or {@code deriveData} methods, the first
> provider supporting the KDF algorithm and optional {@code KDFParameters} is
> chosen. This provider may not support the key material that is subsequently
> passed to the deriveKey or deriveData methods. Therefore, it is recommended
> not to call the {@code getProviderName} or {@code getParameters} methods
> until after a key derivation operation. Once a provider is selected, it
> cannot be changed."
Addressed in:
https://github.com/openjdk/jdk/pull/20301/commits/59b1743fd225ff34e6bcce055fd47a887ed22a08.
Please indicate if resolved.
> src/java.base/share/classes/javax/crypto/KDF.java line 350:
>
>> 348: lastException = new NoSuchAlgorithmException(
>> 349: new InvalidAlgorithmParameterException(
>> 350: "newInstance failed to provide a KDFSpi for
>> the "
>
> The caller does not need to know about internal methods named `newInstance`
> so these should not be exposed in exception messages. How about something
> simpler like "No provider can be found that supports the specified
> parameters". Same comment on line 365.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/59b1743fd225ff34e6bcce055fd47a887ed22a08.
Please indicate if resolved.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1746238483
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1746239212
