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

[Valerie Peng]

On Thu, 18 Mar 2021 17:30:55 GMT, Ziyi Luo <luoziyi at openjdk.org> wrote:

>> This is a P2 regression introduced by JDK-8254717.
>> 
>> In `RSAKeyFactory.engineGetKeySpec`, when the key is a RSA key and the KeySpec is RSAPrivateKeySpec or RSAPrivateCrtKeySpec. The method behavior is described as follow:
>> 
>> X-axis: type of `keySpec`
>> Y-axis: type of `key`
>> 
>> Before JDK-8254717:
>> 
>> |  | RSAPrivateKeySpec.class | RSAPrivateCrtKeySpec.class |
>> |--|--|--|
>> | RSAPrivateKey | Return RSAPrivateKeySpec  | Throw `InvalidKeySpecException` |
>> | RSAPrivateCrtKey | Return RSAPrivateKeySpec | Return RSAPrivateKeyCrtSpec |
>> 
>> After JDK-8254717 (Green check is what we want to fix, red cross is the regression):
>> 
>> |  | RSAPrivateKeySpec.class | RSAPrivateCrtKeySpec.class |
>> |--|--|--|
>> | RSAPrivateKey | Throw `InvalidKeySpecException` ❌  | Throw `InvalidKeySpecException` |
>> | RSAPrivateCrtKey | Return RSAPrivateKeyCrtSpec ✅ | Return RSAPrivateKeyCrtSpec |
>> 
>> This commit fixes the regression.
>> 
>> 
>> ### Tests
>> 
>> * Jtreg: All tests under `java/security`, `sun/security`, `javax/crypto` passed
>> * JCK: All JCK-16 (I do not have jCK-17)tests under `api/java_security` passed
>
> 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");
            }

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

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