On Thu, 18 Sep 2025 18:35:20 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: > > comment from Sean src/java.base/share/classes/sun/security/pkcs12/MacData.java line 92: > 90: if (digestInfo[0].tag != DerValue.tag_Sequence) { > 91: throw new IOException("algid parse error, not a sequence"); > 92: } This check is not necessary, it was already checked by `AlgorithmId.parse` on line 83. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 105: > 103: } > 104: iterations = pbeSpec.getIterationCount(); > 105: macSalt = pbeSpec.getSalt(); Nit, use `this` for class fields (`this.iterations`, `this.macSalt`, etc) to keep method code consistent and helps distinguish from local variables. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 110: > 108: if (!(kdfHmac.equals("HmacSHA512") || > 109: kdfHmac.equals("HmacSHA256"))) { > 110: throw new IllegalArgumentException("unsupported PBMAC1 > Hmac"); Hmm. I don't think we need to limit the algorithms we support on loading. It is more about when storing new keystores since the current PBMAC1 algorithm syntax doesn't allow a mix of different Mac/Prf algs. Also,`engineLoad` is not defined to throw `IllegalArgumentException`, so if this is still needed, then `IOException` is probably more appropriate. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 117: > 115: > 116: // Get the old salt. > 117: extraSalt = macData[1].getOctetString(); Do we need the `extraSalt` and `extraIterations` variables? What if you just put an `else` block here (if not PBMAC1) and then fill in the `macSalt` and `iterations`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2363905086 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2363978712 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2364032921 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2364043024
