On Fri, 19 Mar 2021 19:42:10 GMT, SalusaSecondus
<github.com+829871+salusasecon...@openjdk.org> 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(),
> rsaKey.getPrivateExponent(),
> rsaKey.getParams()
> ));
> } else { // New P11 code starts here and handles the
> P11PrivateKey case
> throw new InvalidKeySpecException("RSA Private key is not
> extractable");
> } // End new P11 code
> } else {
> throw new InvalidKeySpecException
> ("KeySpec must be RSAPrivate(Crt)KeySpec or "
> + "PKCS8EncodedKeySpec for RSA private keys");
> }
I've used your code for both because it looks like a good solution.
One thing which puzzles me is that the following implementation for the P11
code passes all tests and avoids any interaction with P11 library at all. I
didn't want to use it because I'm concerned that there is an untested edge-case
we'd bump into.
<T extends KeySpec> T implGetPrivateKeySpec(P11Key key, Class<T> keySpec,
Session[] session) throws PKCS11Exception, InvalidKeySpecException {
if (key instanceof RSAPrivateKey) {
try {
return implGetSoftwareFactory().getKeySpec(key, keySpec);
} catch (final InvalidKeySpecException e) {
throw e;
} catch (final GeneralSecurityException e) {
throw new InvalidKeySpecException("Could not generate KeySpec",
e);
}
} else {
throw new InvalidKeySpecException("Key is not extractable");
}
}
-------------
PR: https://git.openjdk.java.net/jdk/pull/2949
