On Thu, 5 May 2022 19:38:06 GMT, Valerie Peng <[email protected]> wrote:
>> This change refactors the PBES2Core and PKCS12PBECipherCore classes in
>> SunJCE provider as requested in the bug record. Functionality should remain
>> the same with a clearer and simplified code/control flow with less lines of
>> code. This should improve readability and maintenance. I enhanced one
>> existing regression test to test more scenarios. This test would pass before
>> the proposed change and continues to pass with the proposed changes.
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyright year for PBES2Core.java
Is it possible to rewrite `PKCS12PBECipherCore.java` so that the implementation
inside is `CipherCore` instead of `CipherSpi`? I'd rather create a `CipherSpi`
child inside this package as the base for all implementations that does nothing
more but simply is able to expose all those `engineXYZ` methods to other
classes in the same package. Sigh.
src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229:
> 227: if (key instanceof javax.crypto.interfaces.PBEKey
> pbeKey) {
> 228: salt = check(pbeKey.getSalt()); // may be null
> 229: iCount = check(pbeKey.getIterationCount()); // may
> be 0
It seems the return value is never 0.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 183:
> 181: "for PBEWithSHA1And" + algo);
> 182: }
> 183: }
How about using switch/case or at least put the `if` in same indentation level?
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 214:
> 212:
> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
> 214: SecureRandom random, CipherSpi cipher)
Why rename `cipherImpl` to `cipher`? I think `cipher` is usually a `Cipher`
object and `cipherImpl` is a good name for a `CipherSpi` object.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 215:
> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
> 214: SecureRandom random, CipherSpi cipher)
> 215: throws InvalidKeyException, InvalidAlgorithmParameterException {
Indent the line above.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 314:
> 312: } else if (cipher instanceof DESedeCipher tripleDes)
> {
> 313: tripleDes.engineInit(opmode, cipherKey, ivSpec,
> random);
> 314: } else {
Can we combine the 2 if above? What else type can it be?
-------------
PR: https://git.openjdk.java.net/jdk/pull/8521
