On Tue, 30 May 2023 22:03:44 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8301553: Support Password-Based Cryptography in SunPKCS11 (iteration #4) >> >> Co-authored-by: Francisco Ferrari <fferr...@redhat.com> >> Co-authored-by: Martin Balao <mba...@redhat.com> > > src/java.base/share/classes/sun/security/util/PBEUtil.java line 105: > >> 103: "needed for decryption"); >> 104: } >> 105: } > > Isn't there also default value for iteration count? 'params' can be > PBEParameterSpec (line 82) but its salt and iteration count values aren't > used comparing to the IvParameterSpec inside. Seems a bit inconsistent? @valeriepeng: I agree, `DEFAULT_ITERATIONS` should be used here and only here, so we consistently initialize any defaults in a single place. We'll update that. > src/java.base/share/classes/sun/security/util/PBEUtil.java line 182: > >> 180: // salt should be non-null per PBEParameterSpec >> 181: iCountInit = check(pbeParams.getIterationCount()); >> 182: saltInit = check(pbeParams.getSalt()); > > Why not override params with the IvParameterSpec inside the PBEParameterSpec > here? Then the PBES2Params.initialize(...) method does not need to handle > PBEParameterSpec type? Seems more consistent this way. @valeriepeng: I agree, the only downside I see is that we'll need to do the same `pbeParams.getParameterSpec()` extraction before [this `PBES2Params.initialize()` call](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java#L144-L146). But for this case, it's clearly more consistent to have the extraction here. We'll address this in the next iteration. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1213728201 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1213728793
