On Sat, 13 Mar 2021 00:37:41 GMT, SalusaSecondus <github.com+829871+salusasecon...@openjdk.org> wrote:
>> I still cannot understand why CRT is always preferred. The original >> implementation also hadn't done that. > > I believe that the original implementation intended to do this but made a > mistake. This is why the original implementation (with the backwards > `isAssignableFrom` logic) first checked to see if it could use CRT (as it had > more information) and only afterwards fell back to seeing if it could use > `RSAPrivateKeySpec`. > > RSA CRT keys are much more efficient than normal RSA private keys and also > result in more a more standard compliant output when serialized to PKCS#8 > format (which technically requires the CRT parameters to be present). Thus, I > believe we should try to preserve the CRT parameters whenever possible for > our users. Now users who request an `RSAPrivateKeySpec` and then use it to > later create a new key (using `KeyFactory.generatePrivate`) can keep the > significant performance benefits for that private key. P11RSAKeyFactory.java should also be included into this fix as it's subject to the same RSAPrivateKeySpec vs RSAPrivateCrtKeySpec issue. 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 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/2949
