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

[SalusaSecondus]

On Fri, 19 Mar 2021 19:42:10 GMT, SalusaSecondus <github.com+829871+SalusaSecondus at openjdk.org> wrote:

>> Well, for `P11RSAKeyFactory`, I think some minor modification may be needed given the additional P11PrivateKey type.
>> I'd expect it to be something like:
>>         // must be either RSAPrivateKeySpec or RSAPrivateCrtKeySpec
>>         if (keySpec.isAssignableFrom(RSAPrivateCrtKeySpec.class)) {
>>             session[0] = token.getObjSession();
>>             CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
>>                 new CK_ATTRIBUTE(CKA_MODULUS),
>>                 new CK_ATTRIBUTE(CKA_PUBLIC_EXPONENT),
>>                 new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
>>                 new CK_ATTRIBUTE(CKA_PRIME_1),
>>                 new CK_ATTRIBUTE(CKA_PRIME_2),
>>                 new CK_ATTRIBUTE(CKA_EXPONENT_1),
>>                 new CK_ATTRIBUTE(CKA_EXPONENT_2),
>>                 new CK_ATTRIBUTE(CKA_COEFFICIENT),
>>             };
>>             long keyID = key.getKeyID();
>>             try {
>>                 token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
>>                 KeySpec spec = new RSAPrivateCrtKeySpec(
>>                     attributes[0].getBigInteger(),
>>                     attributes[1].getBigInteger(),
>>                     attributes[2].getBigInteger(),
>>                     attributes[3].getBigInteger(),
>>                     attributes[4].getBigInteger(),
>>                     attributes[5].getBigInteger(),
>>                     attributes[6].getBigInteger(),
>>                     attributes[7].getBigInteger()
>>                 );
>>                 return keySpec.cast(spec);
>>             } catch (final PKCS11Exception ex) {
>>                 // bubble this up if RSAPrivateCrtKeySpec is specified
>>                 // otherwise fall through to RSAPrivateKeySpec
>>                 if (!keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
>>                     throw ex;
>>                 }
>>             }  finally {
>>                 key.releaseKeyID();
>>             }
>> 
>>             attributes = new CK_ATTRIBUTE[] {
>>                 new CK_ATTRIBUTE(CKA_MODULUS),
>>                 new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
>>             };
>>             keyID = key.getKeyID();
>>             try {
>>                 token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
>>             } finally {
>>                 key.releaseKeyID();
>>             }
>> 
>>             KeySpec spec = new RSAPrivateKeySpec(
>>                 attributes[0].getBigInteger(),
>>                 attributes[1].getBigInteger()
>>             );
>>             return keySpec.cast(spec);
>>         } else { // PKCS#8 handled in superclass
>>             throw new InvalidKeySpecException("Only RSAPrivate(Crt)KeySpec "
>>                 + "and PKCS8EncodedKeySpec supported for RSA private keys");
>>         }
>>     }
>
> 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");
        }
    }

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

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