On Thu, 9 May 2024 20:24:19 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> some code review comments
>
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 80:
>
>> 78: * @throws InvalidAlgorithmParameterException
>> 79: * if the initialization parameters are inappropriate for this
>> {@code KDFSpi}
>> 80: */
>
> This constructor should contain `hmacAlgName` as a parameter, and its
> assignment moved here, thus make it final.
Yes, had the same comment.
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 227:
>
>> 225: this.ikms = anExtract.ikms();
>> 226: this.salts = anExtract.salts();
>> 227: if (isNullOrEmpty(ikms) && isNullOrEmpty(salts)) {
>
> As I said earlier, I think this restriction is not necessary.
Yes, it is not possible for the params to be null as
`HKDFParameterSpec.Extract` is final and always passes in non-null lists to
them.
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 244:
>
>> 242: HKDFParameterSpec.Expand anExpand =
>> (HKDFParameterSpec.Expand) kdfParameterSpec;
>> 243: // set this value in the "if"
>> 244: if ((pseudoRandomKey = anExpand.prk()) == null) {
>
> This could not happen here.
Right, `HKDFParameterSpec.Expand` is final and is never created with a null prk.
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 249:
>
>> 247: }
>> 248: // set this value in the "if"
>> 249: if ((info = anExpand.info()) == null) {
>
> I would disallow null when the parameter is created.
info is optional though.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1599017285
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1599040966
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1599045337
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1599052304
