On Mon, 29 Sep 2025 03:45:38 GMT, Mark Powers <[email protected]> wrote:
>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232) > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > another day another iteration Some comments. src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 69: > 67: private String pbmac1AlgorithmName = null; > 68: > 69: private byte[] salt = null; Instead of including so many PBKDF2 components, can we just put one `PBKDF2Parameters` field here? We can also remove the `getPrf`, `getSalt`, and `getIterations` methods. IMO it's not awkward to call `pbmac1Params.getKdfParams().getPrf()` etc. src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 84: > 82: private int keyLength = -1; > 83: > 84: protected void Init(AlgorithmParameterSpec paramSpec) Method names should start with a lowercase letter. If it's not used, remove it. That said, in a different comment, I was hoping we can also construct a `PBMAC1Parameters` object using its components. src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 184: > 182: * Returns a formatted string describing the parameters. > 183: */ > 184: public String engineToString() { Useless now. Therefore, `pbmac1AlgorithmName` is also useless. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 190: > 188: var skf = SecretKeyFactory.getInstance( > 189: kdfHmac.equals("HmacSHA512") ? > 190: "PBKDF2WithHmacSHA512" : "PBKDF2WithHmacSHA256"); The calculation of mac can be consolidated in one method, which is then called by both `processMacData` and `calculateMac`. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 234: > 232: String Hmac = null; > 233: > 234: if (newKeystore) { What could happen if `newKeystore` is different? Is the only difference about the `And` in `macAlgorithm`? Can we just treat it in a consistent way no matter if a new keystore is created? src/java.base/share/classes/sun/security/pkcs12/MacData.java line 250: > 248: } > 249: } > 250: // Fall back to old way of computing MAC This is not a fallback. There are 2 different kinds of algorithms. If it starts with "PBEWith", PBMAC1 is used. If it starts with "HmacPBE", the old algorithm is used. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 260: > 258: > 259: var skf = > SecretKeyFactory.getInstance(kdfHmac.equals("HmacSHA512") ? > 260: "PBKDF2WithHmacSHA512" : "PBKDF2WithHmacSHA256"); Why not just use `"PBKDF2With" + kdfHmac`? What if `kdfHmac` is "HmacSHA384"? src/java.base/share/classes/sun/security/pkcs12/MacData.java line 262: > 260: "PBKDF2WithHmacSHA512" : "PBKDF2WithHmacSHA256"); > 261: try { > 262: int keyLength = Hmac.equals("HmacSHA512") ? 64*8 : 32*8; Use `Mac.getInstance(Hmac).getMacLength()`. There are other algorithms. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 287: > 285: return mData; > 286: } > 287: For all methods below, unless one is used outside of this class, there is no need to create a getter method. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 322: > 320: * ASN.1 encoding. > 321: */ > 322: public byte[] getEncoded() throws NoSuchAlgorithmException, > IOException { Since you have moved the decoding of PBKDF2-Params into its own class, are you going to move the encoding there as well? Ideally, a `PBKDF2Parameters` object can be either created using a `DerValue` or its components (salt, ic, keyLen), and then it has a `getEncoded()` method. Same with the new `PBMAC1Parameters` class. ------------- PR Review: https://git.openjdk.org/jdk/pull/24429#pullrequestreview-3281720001 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389276133 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389255889 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389268119 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389262830 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389223743 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389227816 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389230206 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389232306 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389237089 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2389248160
