On Thu, 4 Sep 2025 19:58:26 GMT, Valerie Peng <[email protected]> wrote:
>> Mark Powers has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> a few more comments
>
> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java
> line 115:
>
>> 113: protected void engineInit(AlgorithmParameterSpec paramSpec)
>> 114: throws InvalidParameterSpecException
>> 115: {
>
> nit: move the "{" to the end of line 114, same goes for other methods.
done
> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java
> line 156:
>
>> 154: DerValue kdf = pBMAC1_params.data.getDerValue();
>> 155: var kdfParams = new PBKDF2Parameters();
>> 156: String kdfAlgo = kdfParams.init(kdf);
>
> Hmm, it's somewhat obscure to return the prf algorithm as the result of
> `PBKDF2Parameters.init(...) `call.
> Is there a reason for a separate `init(...)` call? How about just
> `PBKDF2Parameters(kdf)` and retrieve the `prfAlgo` (instead of the "kdfAlgo"
> name) separately just like `salt`, `iCount` and `keyLength`?
This has been changed to `PBKDF2Parameters(kdf) `by an earlier comment. Are
you suggesting to change `kdfAlgo` to `prfAlgo` or something else?
> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java
> line 165:
>
>> 163: throw new IOException("PBMAC1 parameter parsing "
>> 164: + "error: missing keyLength field");
>> 165: }
>
> Isn't this a PKCS#12 restriction? If so, this should be moved to
> PKCS12KeyStore class?
The `keyLength` is present check should stay for now since it is a MUST. We can
revisit this later if we switch to a different parameter spec. Without a
`PBMAC1ParameterSpec` there is no way to pass `keyLength` to PKCS12.
> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 73:
>
>> 71:
>> 72: // AlgorithmIdentifier
>> 73: private String prf = null;
>
> Set the default value to "HmacSHA1" here instead of when parsing the DER
> encoding?
I set the default value of `kdfAlgo` to "HmacSHA1".
> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 77:
>
>> 75: private byte[] salt = null;
>> 76:
>> 77: private int iterationCount = 0;
>
> nit: order the fields matching the ASN.1 definition?
done
> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 81:
>
>> 79: // the pseudorandom function (default is HmacSHA1)
>> 80: private ObjectIdentifier kdfAlgo_OID =
>> 81: ObjectIdentifier.of(KnownOIDs.HmacSHA1);
>
> This field is not really used? This can just be a local variable when parsing
> the DER encoding.
It's now referenced by PBMAC1Parameters because of an earlier comment so I
can't remove it.
> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 142:
>
>> 140: } else {
>> 141: kdfAlgo = "HmacSHA1";
>> 142: }
>
> Can be removed if setting the `prf` default value.
done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826986
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353825760
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353825902
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826428
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353825603
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826085
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826678
