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

[Sean Mullan]

On Mon, 29 Sep 2025 03:45:38 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:
> 
>   another day another iteration

src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 26:

> 24:  */
> 25: 
> 26: package com.sun.crypto.provider;

This class can be moved to the `sun/security/pkcs12` package and made package-private as it is only referenced by `PKCS12KeyStore`. We can always move it again later if necessary.

src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 58:

> 56:  * -- PBKDF2
> 57:  *
> 58:  * {@link PBKDF2Parameters}

I don't think this link works since `PBKDF2Parameters` is in another package.

Note: javadoc is not generated by default for internal classes. While it is still helpful to write class/method comments in the javadoc format, for something like the above, it might be more useful to say something like "See sun.security.util.PBKDF2Parameters for ..." so it is more readable as a comment.

src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 84:

> 82:     private int keyLength = -1;
> 83: 
> 84:     protected void Init(AlgorithmParameterSpec paramSpec)

I don't think you need a separate method. Put this code in the constructor. Same for the other `Init` method. Then you can also make the fields final.

src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 86:

> 84:     protected void Init(AlgorithmParameterSpec paramSpec)
> 85:         throws InvalidParameterSpecException {
> 86:         if (!(paramSpec instanceof PBEParameterSpec)) {

Use the instanceof pattern (see JEP 394) to avoid the need to cast on lines 90 and 91.

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

> 49:  * @author Sharon Liu
> 50:  */
> 51: 

It would be useful to add the ASN.1 definition here for reference.

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

> 172:     }
> 173: 
> 174:     void processMacData(AlgorithmParameterSpec params,

Can be static?

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

> 176:             throws  Exception {
> 177:         final String kdfHmac;
> 178:         final String Hmac;

Use lower-case as first letter of variable names, s/Hmac/hmac

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

> 184:             kdfHmac = macAlgorithm;
> 185:             Hmac = macAlgorithm;
> 186:         }

Did you consider adding another kdfHmac parameter to the method and passing in the correct values?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392601034
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392616902
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392624684
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392641323
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392648607
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392679324
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392669962
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392695506