On Fri, 19 Sep 2025 17:43:41 GMT, Sean Mullan <[email protected]> wrote:
>> Mark Powers has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> comment from Sean
>
> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 92:
>
>> 90: if (digestInfo[0].tag != DerValue.tag_Sequence) {
>> 91: throw new IOException("algid parse error, not a sequence");
>> 92: }
>
> This check is not necessary, it was already checked by `AlgorithmId.parse` on
> line 83.
You are right. The check has been removed.
> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 105:
>
>> 103: }
>> 104: iterations = pbeSpec.getIterationCount();
>> 105: macSalt = pbeSpec.getSalt();
>
> Nit, use `this` for class fields (`this.iterations`, `this.macSalt`, etc) to
> keep method code consistent and helps distinguish from local variables.
fixed
> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 110:
>
>> 108: if (!(kdfHmac.equals("HmacSHA512") ||
>> 109: kdfHmac.equals("HmacSHA256"))) {
>> 110: throw new IllegalArgumentException("unsupported PBMAC1
>> Hmac");
>
> Hmm. I don't think we need to limit the algorithms we support on loading. It
> is more about when storing new keystores since the current PBMAC1 algorithm
> syntax doesn't allow a mix of different Mac/Prf algs.
>
> Also,`engineLoad` is not defined to throw `IllegalArgumentException`, so if
> this is still needed, then `IOException` is probably more appropriate.
We don't have a `PBMAC1ParameterSpec` to pass key length from
`PBMAC1Parameters` to `PKCS12KeyStore`. By the time you get to
`PKCS12KeyStore`, the only hint for key length is the suffix on the Hmac. I
think we need this check, otherwise we will end up with a not very useful
"failed PKCS12 integrity checking" message.
I'll change `IllegalArgumentException` to `IOException`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2365842435
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2365842471
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2365842488
