Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]
On Sat, 20 Mar 2021 18:43:34 GMT, SalusaSecondus wrote: >> Mike, >> >> From what I can find, if you try to get a spec from a non-extractable key >> you'll get an `InvalidKeySpecException`. >> 1. `C_GetAttributeValue`will throw a `PKCS11Exception` >> 2. The `PKCS11Exception` gets caught in >> [P11KeyFactory](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyFactory.java#L98-L99) >> which rethrows it as an `InvalidKeySpecException`. > > We seem to have a choice and I'm not sure the best way to approach this. > > 1. We trust the properties in `P11Key` and just ask it if the values are both > sensitive and extractable. [1] > 2. But if we already trust P11Key, why not also trust that it properly > implements the RSAPrivateKey interfaces [2]. This is the strategy used by the > snippet I posted earlier (delegating to `implGetSoftwareFactory()`) > 3. We don't trust P11Key except to use getKeyId(), this yields the current > design where we pull the attributes every time the factory needs them. > > We should probably reduce calls to `C_GetAttributeValue` as they may be very > slow. At the least they cross the JNI boundary and at worst they interact > with a slow piece of hardware (possibly over a network). The current design > will have two calls in a worst case, but is likely to have only one call the > vast majority of the time. > > [1] > https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L92 > [2] > https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L375-L406 P11PrivateKey is private so we cannot check that. Our options to figure out if something is sensitive are: 1. See if it doesn't implement `RSAPrivateKey` (this yields the prior snippet with `implGetSoftwareFactory()`) 2. Try to access the attributes directly from the token (this yields the current solution which we're modifying) 3. Check the value of `p11Key.extractable` (which is package-private and thus visible) The smallest change would be to keep our strategy as 2. While I like it the least (my favorite is number 1) it has the smallest risk of changing some undocumented behavior on a PKCS#11 device that we're unfamiliar with (and not testing because we may not have the hardware costing tens of thousands of dollars). Option 3 somewhat spits the difference between 1 and 2. - PR: https://git.openjdk.java.net/jdk/pull/2949
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]
On 3/20/2021 2:46 PM, SalusaSecondus wrote:
On Sat, 20 Mar 2021 17:52:00 GMT, SalusaSecondus
wrote:
@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!
Mike,
From what I can find, if you try to get a spec from a non-extractable key
you'll get an `InvalidKeySpecException`.
1. `C_GetAttributeValue`will throw a `PKCS11Exception`
2. The `PKCS11Exception` gets caught in
[P11KeyFactory](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyFactory.java#L98-L99)
which rethrows it as an `InvalidKeySpecException`.
We seem to have a choice and I'm not sure the best way to approach this.
1. We trust the properties in `P11Key` and just ask it if the values are both
sensitive and extractable. [1]
2. But if we already trust P11Key, why not also trust that it properly
implements the RSAPrivateKey interfaces [2]. This is the strategy used by the
snippet I posted earlier (delegating to `implGetSoftwareFactory()`)
3. We don't trust P11Key except to use getKeyId(), this yields the current
design where we pull the attributes every time the factory needs them.
We should probably reduce calls to `C_GetAttributeValue` as they may be very
slow. At the least they cross the JNI boundary and at worst they interact with
a slow piece of hardware (possibly over a network). The current design will
have two calls in a worst case, but is likely to have only one call the vast
majority of the time.
[1]
https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L92
[2]
https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L375-L406
-
PR: https://git.openjdk.java.net/jdk/pull/2949
Actually, the important lines are 63-66 and 365-373:
* If the components are not accessible, we use a generic class that
* only implements PrivateKey (or SecretKey). Whether the components of a
* key are extractable is automatically determined when the key object is
* created.
attributes = getAttributes(session, keyID, attributes, new
CK_ATTRIBUTE[] {
new CK_ATTRIBUTE(CKA_TOKEN),
new CK_ATTRIBUTE(CKA_SENSITIVE),
new CK_ATTRIBUTE(CKA_EXTRACTABLE),
});
if (attributes[1].getBoolean() || (attributes[2].getBoolean()
== false)) {
return new P11PrivateKey
(session, keyID, algorithm, keyLength, attributes);
}
If the key is non-extractable, then the only attributes will be these
three and the underlying type will be P11Key.P11PrivateKey rather than
one of the RSA variants.
Simple check at the top.
Mike
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]
On Sat, 20 Mar 2021 17:52:00 GMT, SalusaSecondus wrote: >> @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! > > Mike, > > From what I can find, if you try to get a spec from a non-extractable key > you'll get an `InvalidKeySpecException`. > 1. `C_GetAttributeValue`will throw a `PKCS11Exception` > 2. The `PKCS11Exception` gets caught in > [P11KeyFactory](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyFactory.java#L98-L99) > which rethrows it as an `InvalidKeySpecException`. We seem to have a choice and I'm not sure the best way to approach this. 1. We trust the properties in `P11Key` and just ask it if the values are both sensitive and extractable. [1] 2. But if we already trust P11Key, why not also trust that it properly implements the RSAPrivateKey interfaces [2]. This is the strategy used by the snippet I posted earlier (delegating to `implGetSoftwareFactory()`) 3. We don't trust P11Key except to use getKeyId(), this yields the current design where we pull the attributes every time the factory needs them. We should probably reduce calls to `C_GetAttributeValue` as they may be very slow. At the least they cross the JNI boundary and at worst they interact with a slow piece of hardware (possibly over a network). The current design will have two calls in a worst case, but is likely to have only one call the vast majority of the time. [1] https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L92 [2] https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L375-L406 - PR: https://git.openjdk.java.net/jdk/pull/2949
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]
On 3/20/2021 1:54 PM, SalusaSecondus wrote:
On Thu, 18 Mar 2021 20:25:59 GMT, Ziyi Luo wrote:
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!
Mike,
From what I can find, if you try to get a spec from a non-extractable key
you'll get an `InvalidKeySpecException`.
1. `C_GetAttributeValue`will throw a `PKCS11Exception`
2. The `PKCS11Exception` gets caught in
[P11KeyFactory](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyFactory.java#L98-L99)
which rethrows it as an `InvalidKeySpecException`.
-
PR: https://git.openjdk.java.net/jdk/pull/2949
Given that, I'd refactor the code to pull the CKA_SENSITIVE and
CKA_EXPORTABLE attributes first and throw a more specific message if the
key is not extractable rather than having to fail twice before throwing
the error. (I.e., you try both combos of the attributes and both are
failing on the inability to pull the private exponent).
Either that or fail early by checking the error code of the first thrown
PKCS11Exception against CKR_ATTRIBUTE_SENSITIVE.
} catch (final PKCS11Exception ex) {
if (ex.getErrorCode() == PKCS11Constants.CKR_ATTRIBUTE_SENSITIVE) {
throw new InvalidKeySpecException ("Sensitive key may not be
extracted", ex);
}
// bubble this up if RSAPrivateCrtKeySpec is specified
// otherwise fall through to RSAPrivateKeySpec
if (!keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
throw ex;
}
} finally {
key.releaseKeyID();
}
Later, Mike
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]
On Thu, 18 Mar 2021 20:25:59 GMT, Ziyi Luo wrote: >> 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! Mike, >From what I can find, if you try to get a spec from a non-extractable key >you'll get an `InvalidKeySpecException`. 1. `C_GetAttributeValue`will throw a `PKCS11Exception` 2. The `PKCS11Exception` gets caught in [P11KeyFactory](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyFactory.java#L98-L99) which rethrows it as an `InvalidKeySpecException`. - PR: https://git.openjdk.java.net/jdk/pull/2949
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v4]
On Fri, 19 Mar 2021 19:42:10 GMT, SalusaSecondus
wrote:
>> 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");
>> }
>> }
>
> I think that this code will work, but since there already is
> [P11RSAPrivateKey and
> P11RSAPrivateNonCRTKey](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L375-L406)
> (both of which implement the standard `RSAPrivateKey` and `RSAPrivateCrtKey`
> interfaces respectively), can we just depend on and simplify our code? (My
> concern is that there is some unknown reason for calling
> `C_GetAttributeValue` directly here.)
>
> For example (based on the non-P11 code above)
> } 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 if (key instanceof RSAPrivateKey) {
> 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(),
>
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v5]
> 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: Address Valerie's feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/2949/files - new: https://git.openjdk.java.net/jdk/pull/2949/files/defbf46b..da25a691 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2949=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2949=03-04 Stats: 89 lines in 2 files changed: 34 ins; 35 del; 20 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
