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

[Weijun Wang]

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

Some comments.

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

> 67:     private String pbmac1AlgorithmName = null;
> 68: 
> 69:     private byte[] salt = null;

Instead of including so many PBKDF2 components, can we just put one `PBKDF2Parameters` field here? We can also remove the `getPrf`, `getSalt`, and `getIterations` methods. IMO it's not awkward to call `pbmac1Params.getKdfParams().getPrf()` etc.

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)

Method names should start with a lowercase letter. If it's not used, remove it.

That said, in a different comment, I was hoping we can also construct a `PBMAC1Parameters` object using its components.

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

> 182:      * Returns a formatted string describing the parameters.
> 183:      */
> 184:     public String engineToString() {

Useless now. Therefore, `pbmac1AlgorithmName` is also useless.

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

> 188:         var skf = SecretKeyFactory.getInstance(
> 189:                 kdfHmac.equals("HmacSHA512") ?
> 190:                 "PBKDF2WithHmacSHA512" : "PBKDF2WithHmacSHA256");

The calculation of mac can be consolidated in one method, which is then called by both `processMacData` and `calculateMac`.

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

> 232:         String Hmac = null;
> 233: 
> 234:         if (newKeystore) {

What could happen if `newKeystore` is different? Is the only difference about the `And` in `macAlgorithm`? Can we just treat it in a consistent way no matter if a new keystore is created?

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

> 248:             }
> 249:         }
> 250:         // Fall back to old way of computing MAC

This is not a fallback. There are 2 different kinds of algorithms. If it starts with "PBEWith", PBMAC1 is used. If it starts with "HmacPBE", the old algorithm is used.

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

> 258: 
> 259:         var skf = SecretKeyFactory.getInstance(kdfHmac.equals("HmacSHA512") ?
> 260:                 "PBKDF2WithHmacSHA512" : "PBKDF2WithHmacSHA256");

Why not just use `"PBKDF2With" + kdfHmac`? What if `kdfHmac` is "HmacSHA384"?

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

> 260:                 "PBKDF2WithHmacSHA512" : "PBKDF2WithHmacSHA256");
> 261:         try {
> 262:             int keyLength = Hmac.equals("HmacSHA512") ? 64*8 : 32*8;

Use `Mac.getInstance(Hmac).getMacLength()`. There are other algorithms.

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

> 285:         return mData;
> 286:     }
> 287: 

For all methods below, unless one is used outside of this class, there is no need to create a getter method.

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

> 320:      * ASN.1 encoding.
> 321:      */
> 322:     public byte[] getEncoded() throws NoSuchAlgorithmException, IOException {

Since you have moved the decoding of PBKDF2-Params into its own class, are you going to move the encoding there as well? Ideally, a `PBKDF2Parameters` object can be either created using a `DerValue` or its components (salt, ic, keyLen), and then it has a `getEncoded()` method.

Same with the new `PBMAC1Parameters` class.

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

PR Review: https://git.openjdk.org/jdk/pull/24429#pullrequestreview-3281720001
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389276133
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389255889
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389268119
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389262830
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389223743
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389227816
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389230206
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389232306
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389237089
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389248160