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

[Mark Powers]

On Thu, 4 Sep 2025 19:58:26 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> Mark Powers has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   a few more comments
>
> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 115:
> 
>> 113:     protected void engineInit(AlgorithmParameterSpec paramSpec)
>> 114:         throws InvalidParameterSpecException
>> 115:     {
> 
> nit: move the "{" to the end of line 114, same goes for other methods.

done

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 156:
> 
>> 154:         DerValue kdf = pBMAC1_params.data.getDerValue();
>> 155:         var kdfParams = new PBKDF2Parameters();
>> 156:         String kdfAlgo = kdfParams.init(kdf);
> 
> Hmm, it's somewhat obscure to return the prf algorithm as the result of `PBKDF2Parameters.init(...) `call.
> Is there a reason for a separate `init(...)` call? How about just `PBKDF2Parameters(kdf)` and retrieve the `prfAlgo` (instead of  the "kdfAlgo" name) separately just like `salt`, `iCount` and `keyLength`?

This has been changed to `PBKDF2Parameters(kdf) `by an earlier comment.  Are you suggesting to change `kdfAlgo` to `prfAlgo` or something else?

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 165:
> 
>> 163:             throw new IOException("PBMAC1 parameter parsing "
>> 164:                     + "error: missing keyLength field");
>> 165:         }
> 
> Isn't this a PKCS#12 restriction? If so, this should be moved to PKCS12KeyStore class?

The `keyLength` is present check should stay for now since it is a MUST. We can revisit this later if we switch to a different parameter spec. Without a `PBMAC1ParameterSpec` there is no way to pass `keyLength` to PKCS12.

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 73:
> 
>> 71: 
>> 72:     // AlgorithmIdentifier
>> 73:     private String prf = null;
> 
> Set the default value to "HmacSHA1" here instead of when parsing the DER encoding?

I set the default value of `kdfAlgo` to "HmacSHA1".

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 77:
> 
>> 75:     private byte[] salt = null;
>> 76: 
>> 77:     private int iterationCount = 0;
> 
> nit: order the fields matching the ASN.1 definition?

done

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 81:
> 
>> 79:     // the pseudorandom function (default is HmacSHA1)
>> 80:     private ObjectIdentifier kdfAlgo_OID =
>> 81:             ObjectIdentifier.of(KnownOIDs.HmacSHA1);
> 
> This field is not really used? This can just be a local variable when parsing the DER encoding.

It's now referenced by PBMAC1Parameters because of an earlier comment so I can't remove it.

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 142:
> 
>> 140:         } else {
>> 141:             kdfAlgo = "HmacSHA1";
>> 142:         }
> 
> Can be removed if setting the `prf` default value.

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826986
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353825760
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353825902
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826428
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353825603
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826085
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2353826678