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

[Sean Mullan]

On Thu, 18 Sep 2025 18:35:20 GMT, Mark Powers <mpowers at openjdk.org> wrote:

>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232)
>
> 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.

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.

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.

src/java.base/share/classes/sun/security/pkcs12/MacData.java line 117:

> 115: 
> 116:         // Get the old salt.
> 117:         extraSalt = macData[1].getOctetString();

Do we need the `extraSalt` and `extraIterations` variables? What if you just put an `else` block here (if not PBMAC1) and then fill in the `macSalt` and `iterations`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2363905086
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2363978712
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2364032921
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2364043024