RFR 8213009: Refactoring existing SunMSCAPI classes

Weijun Wang weijun.wang at oracle.com
Fri Dec 7 00:24:44 UTC 2018 

Hi Valerie,

I'll fix it in my local repo, run some test, and push the changeset.

Thanks,
Max

> On Dec 7, 2018, at 6:58 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> Hi, Max,
> 
> CSignature.java: one last nit, line 98 and 102 can be replaced by just one line outside the if-block.
> 
> Thanks,
> 
> Valerie
> 
> On 12/6/2018 6:33 AM, Weijun Wang wrote:
>> Webrev updated at
>> 
>>    https://cr.openjdk.java.net/~weijun/8213009/webrev.03
>> 
>> I reset messageDigest/messageDigestAlgorithm/needsReset and check for "key instanceof RSAPublicKey" in RSA::engineInitVerify.
>> 
>> Thanks
>> Max
>>    
>>> On Nov 27, 2018, at 10:13 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>> 
>>> 
>>> 
>>>> On Nov 27, 2018, at 9:55 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>> 
>>>> Hi Max,
>>>> 
>>>> Please find comments in line.
>>>> On 11/22/2018 6:04 AM, Weijun Wang wrote:
>>>>>> On Nov 22, 2018, at 9:14 AM, Valerie Peng <valerie.peng at oracle.com>
>>>>>> wrote:
>>>>>> 
>>>>>> Hi Max,
>>>>>> 
>>>>>> Here are some comments:
>>>>>> 
>>>>>> <src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CSignature.java>
>>>>>> - line 39, add PSS here as well.
>>>>>> - line 97, CSignature ctr, better to initialize all fields
>>>>>> 
>>>>> Which particular ones are you thinking about? privateKey and publicKey will be initialized later and messageDigest/messageDigestAlgorithm/needsReset will be useless when digestName == null.
>>>>> 
>>>> I was referring messageDigest/messageDigestAlgorithm/needsReset. My general preference is to set them to null/false even when not used, may help catch future coding problems.
>>> OK.
>>> 
>>>>>> - line 109, has key algorithm been checked by JCA already? If not, it should be checked here. Same goes for line 414, and 560
>>>>>> 
>>>>> Can I do this later? This sub-task is meant to be a cleanup.
>>>> My point for checking the key algorithm is to ensure that the specified key object is RSA type. After this cleanup and refactoring, the specified key should be checked to be of RSA type, i.e. line 111, 130. Otherwise, the casting to various RSA public/private key interfaces, e.g. java.security.interfaces.RSAPublicKey, may fail.
>>> OK, I can check for RSAPublicKey.
>>> 
>>> On the other hand, if I create a subclass for CRSAPrivateKey, its getModulus() and getPrivateExponent() can only throw out a ProviderException if the key is unextractable, and this breaks some existing code inside JDK.
>>> 
>>> Thanks
>>> Max
>>> 
>>>> Updates look fine.
>>>> 
>>>> Regards,
>>>> 
>>>> Valerie
>>>> 
>>>>> But I think I need to move more RSA related methods into the RSA subclass.
>>>>> 
>>>>> 
>>>>>> - with the class renaming, I think it's clearer to include "RSA" as part of the subclass names, e.g. "MD2withRSA" instead of just "MD2"
>>>>>> 
>>>>> OK.
>>>>> 
>>>>> 
>>>>>> - line 681: can be static, right? pkg-private?
>>>>>> 
>>>>> Yes. Looks like it can be private.
>>>>> 
>>>>> 
>>>>>> <src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CPublicKey.java>
>>>>>> - line 120: maybe a ProviderException instead of IllegalArgumentException as the 'alg' is not from user but rather inside the provider.
>>>>>> 
>>>>> Either is OK for me. Since it's only called inside the provider, it can even be an AssertionError.
>>>>> 
>>>>> 
>>>>>> - line 127, add @Override
>>>>>> 
>>>>> OK.
>>>>> 
>>>>> 
>>>>>> - line 140, must getPublicKeyBob be public? Maybe pkg private?
>>>>>> 
>>>>> Correct.
>>>>> 
>>>>> 
>>>>>> <src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CKeyStore.java>
>>>>>> - line 119: why not just use RSAPrivateCrtKey instead of Key? The caller has already did an instanceof check before this method.
>>>>>> 
>>>>> Correct. I think I should rename both setPrivateKey and generatePrivateKeyBlob to setRSAPrivateKey and generateRSAPrivateKeyBlob. If one day I starts supporting EC keys the methods will be quite different.
>>>>> 
>>>>> 
>>>>>> <test/jdk/sun/security/mscapi/KeyAlgorithms.java>
>>>>>> - would be nice to have a String constant for "RSA".
>>>>>> 
>>>>> Sure.
>>>>> 
>>>>> Thanks
>>>>> Max
>>>>> 
>>>>> 
>>>>>> Rest looks fine.
>>>>>> Thanks,
>>>>>> Valerie
>>>>>> 
>>>>>> 
>>>>>> On 11/15/2018 7:40 AM, Weijun Wang wrote:
>>>>>> 
>>>>>>> Oops, my copy/paste sequence goes wrong.
>>>>>>> 
>>>>>>> 
>>>>>>>> On Nov 15, 2018, at 11:38 PM, Weijun Wang <weijun.wang at oracle.com>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> Webrev updated at
>>>>>>>> 
>>>>>>>> 
>>>>>>>   https://cr.openjdk.java.net/~weijun/8213009/webrev.01/
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>>> More refactorings:
>>>>>>>> 
>>>>>>>> - getEncoded and getFormat of CKey removed, implemented in CPublicKey and CPrivateKey.
>>>>>>>> 
>>>>>>>> - CPublicKey has child class CRSAPublicKey, CKeyPairGenerator has child class RSA.
>>>>>>>> 
>>>>>>>> - CPublicKey and CPrivateKey now have a static of() method that can return a child instance.
>>>>>>>> 
>>>>>>>> - CCipher renamed to CRSACipher. I realized there won't be CECCipher.
>>>>>>>> 
>>>>>>>> Thanks
>>>>>>>> Max
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> On Nov 7, 2018, at 12:13 AM, Weijun Wang <weijun.wang at oracle.com>
>>>>>>>>> wrote:
>>>>>>>>> 
>>>>>>>>> Webrev updated at
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> https://cr.openjdk.java.net/~weijun/8213009/webrev.00/
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> The subtask id is now used.
>>>>>>>>> 
>>>>>>>>> The previous refactoring has removed the "RSA" algorithm info from some keys. This update adds them back.
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> Max
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> On Oct 25, 2018, at 4:38 PM, Weijun Wang <weijun.wang at oracle.com>
>>>>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>> Please review the change at
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> https://cr.openjdk.java.net/~weijun/8026953/webrev.00/
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> (I will use a sub-task id for this change but currently JBS is down).
>>>>>>>>>> 
>>>>>>>>>> The major change is renaming classes. Since we are going to support algorithms other than RSA, I've renamed the classes like RSAPrivateKey -> CPrivateKey. Classes that have the same name as JCA classes (like Key, KeyStore) are also renamed (to CKey, CKeyStore) so it's easy to tell them apart.
>>>>>>>>>> 
>>>>>>>>>> Others are not about renaming but they are also related to supporting other algorithms, and there is no behavior change. They include:
>>>>>>>>>> 
>>>>>>>>>> - CKey (plus its child classes CPublicKey and CPrivateKey) has a new field "algorithm". This field is used by CKeyStore::generateRSAKeyAndCertificateChain and its value is obtained from the public key algorithm in a cert [1].
>>>>>>>>>> 
>>>>>>>>>> - Child class named "RSA" of CKeyPairGenerator.
>>>>>>>>>> 
>>>>>>>>>> - Child class named "RSA" of CSignature. I also moved some RSA-related methods into this child class as overridden methods.
>>>>>>>>>> 
>>>>>>>>>> - CKeyStore::setPrivateKey's key parameter has a new type Key, but it still only accepts RSAPrivateCrtKey now.
>>>>>>>>>> 
>>>>>>>>>> Noreg-cleanup.
>>>>>>>>>> 
>>>>>>>>>> Thanks
>>>>>>>>>> Max
>>>>>>>>>> 
>>>>>>>>>> [1]
>>>>>>>>>> https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier

More information about the security-dev mailing list