RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]

[SalusaSecondus]

On Thu, 18 Mar 2021 23:17:51 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> Ziyi Luo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>> 
>>  - Fix P11RSAKeyFactory and add one more jtreg test
>>  - Add one test case for the regression fixed by 8263404
>>  - Fix jcheck whitespace error
>>  - 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec
>
> src/java.base/share/classes/sun/security/rsa/RSAKeyFactory.java line 430:
> 
>> 428:             } else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS)) {
>> 429:                 throw new InvalidKeySpecException
>> 430:                         ("RSAPrivateCrtKeySpec can only be used with CRT keys");
> 
> If we are basing the decision on type of key, it may be clearer to do something like below:
>         } else if (key instanceof RSAPrivateKey) {
>             if (keySpec.isAssignableFrom(PKCS8_KEYSPEC_CLS)) {
>                 return keySpec.cast(new PKCS8EncodedKeySpec(key.getEncoded()));
>             } else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS)) {
>                 if (key instanceof RSAPrivateCrtKey) {
>                     RSAPrivateCrtKey crtKey = (RSAPrivateCrtKey)key;
>                     return keySpec.cast(new RSAPrivateCrtKeySpec(
>                         crtKey.getModulus(),
>                         crtKey.getPublicExponent(),
>                         crtKey.getPrivateExponent(),
>                         crtKey.getPrimeP(),
>                         crtKey.getPrimeQ(),
>                         crtKey.getPrimeExponentP(),
>                         crtKey.getPrimeExponentQ(),
>                         crtKey.getCrtCoefficient(),
>                         crtKey.getParams()
>                     ));
>                 } else { // RSAPrivateKey (non-CRT)
>                     if (!keySpec.isAssignableFrom(RSA_PRIV_KEYSPEC_CLS)) {
>                         throw new InvalidKeySpecException
>                             ("RSAPrivateCrtKeySpec can only be used with CRT keys");
>                     }
> 
>                     // fall through to RSAPrivateKey (non-CRT)
>                     RSAPrivateKey rsaKey = (RSAPrivateKey) key;
>                     return keySpec.cast(new RSAPrivateKeySpec(
>                         rsaKey.getModulus(),
>                         rsaKey.getPrivateExponent(),
>                         rsaKey.getParams()
>                     ));
>                 }
>             } else {
>                 throw new InvalidKeySpecException
>                         ("KeySpec must be RSAPrivate(Crt)KeySpec or "
>                         + "PKCS8EncodedKeySpec for RSA private keys");
>             }

I like this flow and think that it's clearer.

One question: Do you think we could use this code almost character for character in the `P11RSAKeyFactory`? The keys handled should be descendants of `RSAPrivateCrtKey`, `RSAPrivateKey`, or `P11PrivateKey` (which would need some special handling). I didn't want to refactor it that much even though I think that it would work because I assume it must have been written to use the underlying PKCS#11 properties for *some* reason (even if I cannot figure out why).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2949