RFR: 8343232: PKCS#12 KeyStore support for RFC 9879: Use of Password-Based Message Authentication Code 1 (PBMAC1) [v4]

[Mark Powers]

On Fri, 19 Sep 2025 17:43:41 GMT, Sean Mullan <mullan at openjdk.org> 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