On Thu, 9 May 2024 15:04:53 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> change algorithm standard name for HKDFs in SunJCE provider
>
> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 70:
>
>> 68: */
>> 69: public Extract extractOnly() {
>> 70: if (this.ikms.isEmpty() && this.salts.isEmpty()) {
>
> I don't think this check is necessary? While it's probably unsafe to provide
> no IKM, providing no salt is quite common. Anyway, no need to restrict on
> both, IMHO
I agree. Also, if we do want to validate arguments (and I don't know if we need
to), then I think the `Extract` constructor should be responsible for doing
that, not the `Builder`. Doing it in `Extract` is safer since it is done after
the fields are cloned.
> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 108:
>
>> 106: *
>> 107: * @param ikm
>> 108: * the ikm value (null values will not be added)
>
> Are you sure about allowing `null`? Any valid use case? Same question for the
> other `add` methods.
`null` should not be silently ignored. The method should throw
`NullPointerException` (and that should be added to the javdoc in an `@throws`
clause).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1596861849
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1596866987
