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

[Valerie Peng]

On Sat, 20 Mar 2021 17:41:45 GMT, SalusaSecondus <github.com+829871+SalusaSecondus at openjdk.org> wrote:

>> I think that this code will work, but since there already is [P11RSAPrivateKey and P11RSAPrivateNonCRTKey](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L375-L406) (both of which implement the standard `RSAPrivateKey` and `RSAPrivateCrtKey` interfaces respectively), can we just depend on and simplify our code? (My concern is that there is some unknown reason for calling `C_GetAttributeValue` directly here.)
>> 
>> For example (based on the non-P11 code above)
>>         } 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 if (key instanceof RSAPrivateKey) {
>>                     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 { // New P11 code starts here and handles the P11PrivateKey case
>>                     throw new InvalidKeySpecException("RSA Private key is not extractable");
>>                 } // End new P11 code
>>             } else {
>>                 throw new InvalidKeySpecException
>>                         ("KeySpec must be RSAPrivate(Crt)KeySpec or "
>>                         + "PKCS8EncodedKeySpec for RSA private keys");
>>             }
>
> I've used your code for both because it looks like a good solution.
> 
> One thing which puzzles me is that the following implementation for the P11 code passes all tests and avoids any interaction with P11 library at all. I didn't want to use it because I'm concerned that there is an untested edge-case we'd bump into.
> 
>     <T extends KeySpec> T implGetPrivateKeySpec(P11Key key, Class<T> keySpec,
>             Session[] session) throws PKCS11Exception, InvalidKeySpecException {
>         if (key instanceof RSAPrivateKey) {
>             try {
>                 return implGetSoftwareFactory().getKeySpec(key, keySpec);
>             } catch (final InvalidKeySpecException  e) {
>                 throw e;
>             } catch (final GeneralSecurityException e) {
>                 throw new InvalidKeySpecException("Could not generate KeySpec", e);
>             }
>         } else {
>             throw new InvalidKeySpecException("Key is not extractable");
>         }
>     }

Well, when the key object has all the attribute values available, things will often work regardless which path you took.

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

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