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

[Mark Powers]

On Tue, 8 Jul 2025 18:17:58 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232)
>
> src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 231:
> 
>> 229: 
>> 230:         var kdfParams = new PBKDF2Parameters();
>> 231:         String kdfAlgo = kdfParams.parseKDF(kdf);
> 
> nit: `parseKDF()` seems a bit redundant as the KDF name is already in the class name, i.e. `PBKDF2Parameter`. Maybe name it `init()` as this is what it does, i.e. initialize the `PBKDF2Parameters` obj w/ the `DerValue` argument.

fixed

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 111:
> 
>> 109: 
>> 110:     // the Hmac function
>> 111:     private String Hmac = null;
> 
> nit: if it's algorithm name, maybe "hmacName" or "hmacAlgo"? Also, variable name should start with lowercase.

Changed `Hmac` to `hmacAlgo`.

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 146:
> 
>> 144:         }
>> 145:         ObjectIdentifier OID = Info[1].data.getOID();
>> 146:             KnownOIDs o = KnownOIDs.findMatch(OID.toString());
> 
> nit: indentation seems off?

fixed

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 162:
> 
>> 160:         DerValue kdf = pBMAC1_params.data.getDerValue();
>> 161:         var kdfParams = new PBKDF2Parameters();
>> 162:         String kdfAlgo = kdfParams.parseKDF(kdf);
> 
> Maybe just use a `PBKDF2Parameters(DerValue) `constructor? Then retrieve the algorithm via a separate getter method.

That constructor doesn't exist. I don't know how I would do what you suggest.

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 174:
> 
>> 172:             throw new IOException("PMAC1 parameter parsing error: "
>> 173:                 + "missing keyLength field");
>> 174:         }
> 
> Where is these requirements on `keyLength` documented? I found them inside RFC 9579. But no such restriction in RFC 8018. If this is PKCS12-specific requirement, I am not sure if these checks should be enforced inside `PBMAC1Parameters` class. They can be done in the `PKCS12KeyStore` class when using this object, right?

You are right. The check belongs in the `PKCS12KeyStore` class.

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 36:
> 
>> 34: /**
>> 35:  * This class implements the parameter set used with password-based
>> 36:  * mac scheme 1 (PBKDF2), which is defined in PKCS#5 as follows:
> 
> "password-based mac scheme 1" should be "password-based key derivation function 2"?

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 73:
> 
>> 71: 
>> 72:     // the PBMAC1 algorithm name
>> 73:     private String pbmac1AlgorithmName = null;
> 
> It's probably clearer or easier to understand to match the field name to the ASN.1 param definition.

done

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 81:
> 
>> 79:     private int iCount = 0;
>> 80: 
>> 81:     // the key derivation function (default is HmacSHA1)
> 
> psuedo random function instead of key derivation function.

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 137:
> 
>> 135:                 throw new IOException("PBKDF2 parameter parsing error: "
>> 136:                         + "expecting the object identifier for a HmacSHA key "
>> 137:                         + "derivation function");
> 
> "key derivation function" -> "pseudorandom function"

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 170:
> 
>> 168:      * Returns size of key generated by PBKDF2.
>> 169:      *
>> 170:      * @return size of key generated by PBKDF2.
> 
> nit: add comment that -1 is returned if not found/set.

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201711440
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201726476
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201725103
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2202041150
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201724667
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201712853
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201713544
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201726873
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201714807
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2201715466