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

[SalusaSecondus]

On Mon, 15 Mar 2021 21:30:57 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

> P11RSAKeyFactory.java should also be included into this fix as it's subject to the same RSAPrivateKeySpec vs RSAPrivateCrtKeySpec issue.

Thank you. We'll fix this.

> 
> My personal take is that the isAssignableFrom() call may be more for checking if both keyspecs are the same. If the original impl (pre-8254717 code) really intend to return RSAPrivateCrtKeySpec for RSAPrivateCrtKey objs, then it should just check key type to be CRT and no need to call isAssignableFrom() with both RSAPrivateCrtKeySpec AND RSAPrivateKeySpec, just check the later would suffice. Thus, I'd argue the original intention is to return the keyspec matching the requested keyspec class.

I wondered this too, but if that were the case, I'd expect to see `RSA_PRIVCRT_KEYSPEC_CLS.equals(keySpec)` instead. It seems more likely that the code is just trying to ensure that some valid implementation of the KeySpec is returned rather than the specific one. 

> I can see the potential performance benefit of piggybacking the Crt info and generating Crt keys when RSAPrivateKeySpec is specified. However, the way it is coded in this PR seems a bit unclear. The InvalidKeySpecException definitely must be fixed. As for the RSAPrivateKeySpec->RSAPrivateCrtKeySpec change, it's a change of behavior comparing to pre-8254717 code which I am ok with if we can be sure that there is no undesirable side-effects.

I'm definitely up for ways to make this clearer. I'll ensure that the next version has some better comments to explain the logic flow so that future us doesn't need to guess about original intent in the way we are now.

I agree that this should be a safe change. Outside of some extremely sensitive unit tests (which is how I discovered this bug in the first place), I don't expect much code to depend on the *exact* class returned by this method. (Any code doing so would be wrong as well, but we still need to pay attention to it.)

We can see similar behavior elsewhere in the JDK where a more specialized class is returned by a method similar to this.
        AlgorithmParameters params = AlgorithmParameters.getInstance("EC");
        params.init(new ECGenParameterSpec("secp256r1"));
        ECParameterSpec spec = params.getParameterSpec(ECParameterSpec.class);
        System.out.println(spec);
        System.out.println(spec.getClass());

In this case we can see that [EcParameters](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/util/ECParameters.java#L201) returns an instance of the private class `sun.security.util.NamedCurve` specifically to preserve additional information about the parameters for later use within the JDK.

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

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