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

[Valerie Peng]

On Thu, 3 Apr 2025 22:58:39 GMT, Mark Powers <mpowers 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.

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.

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?

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.

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?

src/java.base/share/classes/javax/crypto/spec/PBMAC1ParameterSpec.java line 41:

> 39:  * @since 26
> 40:  */
> 41: public class PBMAC1ParameterSpec implements AlgorithmParameterSpec {

PKCS#12 states that other KDF may be used, e.g. Scrypt. The current API seems to be hardcoded to PBKDF2, e.g. salt, iteration count, keyLength, prf, etc. Have you considered separating it out? I know that encapsulating all these different possibilities can be complicated. If this class doesn't have to be under javax.crypto,spec, then we can change it as we see fit. Otherwise, we will need spending more time designing the API...

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"?

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.

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.

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"

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193162616
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193992823
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193991671
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2194002205
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193980876
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2194070511
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193439672
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193515579
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193998601
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193531917
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2193548268