On Mon, 15 Mar 2021 21:30:57 GMT, Valerie Peng <valer...@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
