On Thu, 18 Mar 2021 23:17:51 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> 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");
> }
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).
-------------
PR: https://git.openjdk.java.net/jdk/pull/2949
