On Fri, 19 Mar 2021 03:18:35 GMT, SalusaSecondus
<github.com+829871+salusasecon...@openjdk.org> 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
