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

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
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

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

Commit messages:
 - Fix jcheck whitespace error
 - 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec

Changes: https://git.openjdk.java.net/jdk/pull/2949/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2949&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263404
  Stats: 21 lines in 1 file changed: 3 ins; 5 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2949.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2949/head:pull/2949

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang-2
On Thu, 11 Mar 2021 19:51:19 GMT, Ziyi Luo <[hidden email]> 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

My understanding is that the problem here is the 2 `isAssignableFrom` checks are in wrong order. The parent class RSA_PRIV_KEYSPEC_CLS should be checked first.

BTW, please add a regression test to the fix. Thanks.

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
On Fri, 12 Mar 2021 15:39:48 GMT, Weijun Wang <[hidden email]> 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
>
> My understanding is that the problem here is the 2 `isAssignableFrom` checks are in wrong order. The parent class RSA_PRIV_KEYSPEC_CLS should be checked first.
>
> BTW, please add a regression test to the fix. Thanks.

If we check the parent class `RSA_PRIV_KEYSPEC_CLS` first, then the JDK will return instances of `RSAPrivateKeySpec` even when it *could* return an instance of `RSAPrivateCrtKeySpec`. If the underlying RSA key has CRT parameters, then it seems preferable to return the more detailed and specific `RSAPrivateCrtKeySpec` if possible. (Not opportunistically returning `RSAPrivateCrtKeySpec` is actually one of the specific things that the prior change (which introduced this regression) is trying to fix.

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
In reply to this post by Ziyi Luo
> 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 incrementally with one additional commit since the last revision:

  Add one test case for the regression fixed by 8263404

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2949/files
  - new: https://git.openjdk.java.net/jdk/pull/2949/files/0ebb72d0..e9a76978

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2949&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2949&range=00-01

  Stats: 62 lines in 1 file changed: 61 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2949.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2949/head:pull/2949

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
In reply to this post by Weijun Wang-2
On Fri, 12 Mar 2021 15:39:48 GMT, Weijun Wang <[hidden email]> 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
>
> My understanding is that the problem here is the 2 `isAssignableFrom` checks are in wrong order. The parent class RSA_PRIV_KEYSPEC_CLS should be checked first.
>
> BTW, please add a regression test to the fix. Thanks.

Hi @wangweij Thanks for your review. As @SalusaSecondus commented, RSAPrivateKeyCrtSpec should be favored over RSAPrivateKeySpec when the PrivateKey is a Crt Key. I just modified our JTreg test to include all four cases described in the PR description.

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang-2
On Fri, 12 Mar 2021 22:04:35 GMT, Ziyi Luo <[hidden email]> wrote:

>> My understanding is that the problem here is the 2 `isAssignableFrom` checks are in wrong order. The parent class RSA_PRIV_KEYSPEC_CLS should be checked first.
>>
>> BTW, please add a regression test to the fix. Thanks.
>
> Hi @wangweij Thanks for your review. As @SalusaSecondus commented, RSAPrivateKeyCrtSpec should be favored over RSAPrivateKeySpec when the PrivateKey is a Crt Key. I just modified our JTreg test to include all four cases described in the PR description.

I still cannot understand why CRT is always preferred. The original implementation also hadn't done that.

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
On Fri, 12 Mar 2021 22:06:40 GMT, Weijun Wang <[hidden email]> wrote:

>> Hi @wangweij Thanks for your review. As @SalusaSecondus commented, RSAPrivateKeyCrtSpec should be favored over RSAPrivateKeySpec when the PrivateKey is a Crt Key. I just modified our JTreg test to include all four cases described in the PR description.
>
> 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.

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
On Sat, 13 Mar 2021 00:37:41 GMT, SalusaSecondus <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
In reply to this post by Ziyi Luo
On Fri, 12 Mar 2021 22:00:18 GMT, Ziyi Luo <[hidden email]> 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 incrementally with one additional commit since the last revision:
>
>   Add one test case for the regression fixed by 8263404

test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 80:

> 78:         // === Case 1: private key is RSAPrivateCrtKey, expected spec is RSAPrivateKeySpec
> 79:         // === Expected: return RSAPrivateCrtKeySpec
> 80:         // Since RSAPrivateCrtKeySpec inherits from RSAPrivateKeySpec, we'd expect this next line to return an instance of RSAPrivateKeySpec

Typo? I think you mean RSAPrivateCrtKeySpec?

test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 83:

> 81:         // (because the private key has CRT parts).
> 82:         KeySpec spec = factory.getKeySpec(pair.getPrivate(), RSAPrivateKeySpec.class);
> 83:         if (!(spec instanceof RSAPrivateCrtKeySpec)) {

The generated key is implementation specific, you should check the key type before checking the returned key spec?

test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 99:

> 97:         // InvalidKeySpecException should not be thrown
> 98:         KeySpec notCrtSpec = factory.getKeySpec(notCrtKey, RSAPrivateKeySpec.class);
> 99:         if (notCrtSpec instanceof RSAPrivateCrtKeySpec) {

Just to be safe, check the returned keyspec is RSAPrivateKeySpec?

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
On Mon, 15 Mar 2021 21:35:16 GMT, Valerie Peng <[hidden email]> wrote:

>> Ziyi Luo has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add one test case for the regression fixed by 8263404
>
> test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 80:
>
>> 78:         // === Case 1: private key is RSAPrivateCrtKey, expected spec is RSAPrivateKeySpec
>> 79:         // === Expected: return RSAPrivateCrtKeySpec
>> 80:         // Since RSAPrivateCrtKeySpec inherits from RSAPrivateKeySpec, we'd expect this next line to return an instance of RSAPrivateKeySpec
>
> Typo? I think you mean RSAPrivateCrtKeySpec?

Yup. Thank you for the catch.

> test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 83:
>
>> 81:         // (because the private key has CRT parts).
>> 82:         KeySpec spec = factory.getKeySpec(pair.getPrivate(), RSAPrivateKeySpec.class);
>> 83:         if (!(spec instanceof RSAPrivateCrtKeySpec)) {
>
> The generated key is implementation specific, you should check the key type before checking the returned key spec?

How about we specifically use the ` SunRsaSign` provider then (which does generate an `RSAPrivateCrtKey`)?

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
In reply to this post by Valerie Peng-2
On Mon, 15 Mar 2021 21:30:57 GMT, Valerie Peng <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
In reply to this post by Greg Rubin
On Mon, 15 Mar 2021 21:37:59 GMT, SalusaSecondus <[hidden email]> wrote:

>> test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 80:
>>
>>> 78:         // === Case 1: private key is RSAPrivateCrtKey, expected spec is RSAPrivateKeySpec
>>> 79:         // === Expected: return RSAPrivateCrtKeySpec
>>> 80:         // Since RSAPrivateCrtKeySpec inherits from RSAPrivateKeySpec, we'd expect this next line to return an instance of RSAPrivateKeySpec
>>
>> Typo? I think you mean RSAPrivateCrtKeySpec?
>
> Yup. Thank you for the catch.

This is not a typo but I found the sentence very confusing. I'll update the comment.
What I meant: `keySpec` is RSAPrivateKeySpec

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
In reply to this post by Ziyi Luo
> 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 incrementally with one additional commit since the last revision:

  Fix P11RSAKeyFactory and add one more jtreg test

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2949/files
  - new: https://git.openjdk.java.net/jdk/pull/2949/files/e9a76978..065e1138

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2949&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2949&range=01-02

  Stats: 132 lines in 3 files changed: 98 ins; 10 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2949.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2949/head:pull/2949

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
In reply to this post by Ziyi Luo
> 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

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

Changes: https://git.openjdk.java.net/jdk/pull/2949/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2949&range=03
  Stats: 211 lines in 4 files changed: 162 ins; 15 del; 34 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2949.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2949/head:pull/2949

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
On Thu, 18 Mar 2021 17:30:55 GMT, Ziyi Luo <[hidden email]> 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

This looks to cover the cases and fixes we talked about.

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Ziyi Luo
On Thu, 18 Mar 2021 18:57:57 GMT, SalusaSecondus <[hidden email]> 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
>
> This looks to cover the cases and fixes we talked about.

@valeriepeng Sorry for the delay. There were unknown Windows build failure during the pre-submit tests that I have to rebase my commits on top of the  master tip. This new revision should cover all comments you left before. Thank you!

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
In reply to this post by Ziyi Luo
On Thu, 18 Mar 2021 17:30:55 GMT, Ziyi Luo <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
On Thu, 18 Mar 2021 23:17:51 GMT, Valerie Peng <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
On Fri, 19 Mar 2021 03:18:35 GMT, SalusaSecondus <[hidden email]> wrote:

>> 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).

Well, for `P11RSAKeyFactory`, I think some minor modification may be needed given the additional P11PrivateKey type.
I'd expect it to be something like:
        // must be either RSAPrivateKeySpec or RSAPrivateCrtKeySpec
        if (keySpec.isAssignableFrom(RSAPrivateCrtKeySpec.class)) {
            session[0] = token.getObjSession();
            CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
                new CK_ATTRIBUTE(CKA_MODULUS),
                new CK_ATTRIBUTE(CKA_PUBLIC_EXPONENT),
                new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
                new CK_ATTRIBUTE(CKA_PRIME_1),
                new CK_ATTRIBUTE(CKA_PRIME_2),
                new CK_ATTRIBUTE(CKA_EXPONENT_1),
                new CK_ATTRIBUTE(CKA_EXPONENT_2),
                new CK_ATTRIBUTE(CKA_COEFFICIENT),
            };
            long keyID = key.getKeyID();
            try {
                token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
                KeySpec spec = new RSAPrivateCrtKeySpec(
                    attributes[0].getBigInteger(),
                    attributes[1].getBigInteger(),
                    attributes[2].getBigInteger(),
                    attributes[3].getBigInteger(),
                    attributes[4].getBigInteger(),
                    attributes[5].getBigInteger(),
                    attributes[6].getBigInteger(),
                    attributes[7].getBigInteger()
                );
                return keySpec.cast(spec);
            } catch (final PKCS11Exception ex) {
                // bubble this up if RSAPrivateCrtKeySpec is specified
                // otherwise fall through to RSAPrivateKeySpec
                if (!keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
                    throw ex;
                }
            }  finally {
                key.releaseKeyID();
            }

            attributes = new CK_ATTRIBUTE[] {
                new CK_ATTRIBUTE(CKA_MODULUS),
                new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
            };
            keyID = key.getKeyID();
            try {
                token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
            } finally {
                key.releaseKeyID();
            }

            KeySpec spec = new RSAPrivateKeySpec(
                attributes[0].getBigInteger(),
                attributes[1].getBigInteger()
            );
            return keySpec.cast(spec);
        } else { // PKCS#8 handled in superclass
            throw new InvalidKeySpecException("Only RSAPrivate(Crt)KeySpec "
                + "and PKCS8EncodedKeySpec supported for RSA private keys");
        }
    }

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

PR: https://git.openjdk.java.net/jdk/pull/2949
Reply | Threaded
Open this post in threaded view
|

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

Michael StJohns
On 3/19/2021 2:24 PM, Valerie Peng wrote:

>
> some* reason (even if I cannot figure out why).
> Well, for `P11RSAKeyFactory`, I think some minor modification may be needed given the additional P11PrivateKey type.
> I'd expect it to be something like:
>          // must be either RSAPrivateKeySpec or RSAPrivateCrtKeySpec
>          if (keySpec.isAssignableFrom(RSAPrivateCrtKeySpec.class)) {
>              session[0] = token.getObjSession();
>              CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
>                  new CK_ATTRIBUTE(CKA_MODULUS),
>                  new CK_ATTRIBUTE(CKA_PUBLIC_EXPONENT),
>                  new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),

If the PKCS11 private key has the CKA_SENSITIVE attribute set to true or
CKA_EXPORTABLE set to false, you can't retrieve the above attribute. 
AIRC, the contract for getting a Key from an unextractable PKCS11
private key is to return a key that implements both PrivateKey and
RSAKey, but doesn't implement either of the RSAPrivateKey interfaces.  
I don't know what the contract is for producing KeySpec's from
unextractable keys.

Mike


>                  new CK_ATTRIBUTE(CKA_PRIME_1),
>                  new CK_ATTRIBUTE(CKA_PRIME_2),
>                  new CK_ATTRIBUTE(CKA_EXPONENT_1),
>                  new CK_ATTRIBUTE(CKA_EXPONENT_2),
>                  new CK_ATTRIBUTE(CKA_COEFFICIENT),
>              };
>              long keyID = key.getKeyID();
>              try {
>                  token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
>                  KeySpec spec = new RSAPrivateCrtKeySpec(
>                      attributes[0].getBigInteger(),
>                      attributes[1].getBigInteger(),
>                      attributes[2].getBigInteger(),
>                      attributes[3].getBigInteger(),
>                      attributes[4].getBigInteger(),
>                      attributes[5].getBigInteger(),
>                      attributes[6].getBigInteger(),
>                      attributes[7].getBigInteger()
>                  );
>                  return keySpec.cast(spec);
>              } catch (final PKCS11Exception ex) {
>                  // bubble this up if RSAPrivateCrtKeySpec is specified
>                  // otherwise fall through to RSAPrivateKeySpec
>                  if (!keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
>                      throw ex;
>                  }
>              }  finally {
>                  key.releaseKeyID();
>              }
>
>              attributes = new CK_ATTRIBUTE[] {
>                  new CK_ATTRIBUTE(CKA_MODULUS),
>                  new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
>              };
>              keyID = key.getKeyID();
>              try {
>                  token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
>              } finally {
>                  key.releaseKeyID();
>              }
>
>              KeySpec spec = new RSAPrivateKeySpec(
>                  attributes[0].getBigInteger(),
>                  attributes[1].getBigInteger()
>              );
>              return keySpec.cast(spec);
>          } else { // PKCS#8 handled in superclass
>              throw new InvalidKeySpecException("Only RSAPrivate(Crt)KeySpec "
>                  + "and PKCS8EncodedKeySpec supported for RSA private keys");
>          }
>      }
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2949


123