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

[Valerie Peng]

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

>> 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).

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");
        }
    }

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

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