RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]

[Weijun Wang]

On Thu, 5 May 2022 19:38:06 GMT, Valerie Peng <valeriep at openjdk.org> 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