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