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

[Mark Powers]

On Tue, 7 Oct 2025 23:36:39 GMT, Thomas Fitzsimmons <fitzsim at openjdk.org> wrote:

>> After innumerable iterations, this encoding method will be removed from `PBMAC1Parameters`.
>> That said, keyLength now comes from the Mac before being encoded in `MacData`:
>> 
>> 
>> Mac m = Mac.getInstance(hmac);
>> int keyLength = m.getMacLength()*8;
>> 
>> If this doesn't work, we have some other serious issues.
>
> The `> 0` check is still there in MacData.getEncoded:
> 
> 
>         if (this.pbmac1Keystore) {
>         [...]
>             // encode derived key length
>             if (this.keyLength > 0) {
>                 pBKDF2_params.putInteger(this.keyLength / 8); // derived key length (in octets)
>             }
> 
> 
> How about making sure it is greater than zero in the constructor instead, something like:
> 
> 
> --- a/src/java.base/share/classes/sun/security/pkcs12/MacData.java
> +++ b/src/java.base/share/classes/sun/security/pkcs12/MacData.java
> @@ -122,6 +122,10 @@ class MacData {
>          }
>          if (algName.equals("PBMAC1")) {
>              this.pbmac1Keystore = true;
> +            if (keyLength <= 0) {
> +                throw new IllegalArgumentException("the keyLength " +
> +                        "parameter for PBMAC1 must be greater than 0");
> +            }
>          }
>          algid = AlgorithmId.get(algName);
>  
> @@ -333,9 +337,7 @@ byte[] getEncoded() throws NoSuchAlgorithmException, IOException {
>              pBKDF2_params.putInteger(this.iterations);
>  
>              // encode derived key length
> -            if (this.keyLength > 0) {
> -                pBKDF2_params.putInteger(this.keyLength / 8); // derived key length (in octets)
> -            }
> +            pBKDF2_params.putInteger(this.keyLength / 8); // derived key length (in octets)
>              pBKDF2_params.write(DerValue.tag_Sequence, kdfHmac);
>              tmp3.putOID(pkcs5PBKDF2_OID);
>              tmp3.write(DerValue.tag_Sequence, pBKDF2_params);
> 
> 
> Should `keyLength` also be initialized in the `MacData(DerInputStream derin)` constructor?

This code has changed a lot recently. I agree that the `> 0` check allows the `keyLength` field to be omitted - in violation of the spec. I will soon push code without the ` > 0` check and`keyLength / 8` operation. `keyLength` will be set to `Mac.getMacLength()`.

To answer your second question about `MacData(DerInputStream derin)`, the `keyLength` will be set to the value in the input stream. An exception will be thrown by `MacData` if `keyLength` is not present in the input stream.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2417918527