On Tue, 8 Jul 2025 18:17:58 GMT, Valerie Peng <[email protected]> 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
