RFR 8213009: Refactoring existing SunMSCAPI classes

Weijun Wang weijun.wang at oracle.com
Mon Nov 26 03:07:39 UTC 2018


Hi Valerie

Updated webrev at

   https://cr.openjdk.java.net/~weijun/8213009/webrev.02

Changes:

- CSignature.java:

  * Mentions "RSASSA-PSS" in class spec of CSignature.

  * CSignature.RSA child classes renamed, For example, Raw -> NONEwithRSA, SHA1 -> SHA1withRSA. References (both class name and string) also updated in SunMSCAPI.java.

  * Move CSignature::generatePublicKeyBlob into child class, CSignature$RSA::generatePublicKeyBlob.

  * Move CSignature::engineInitVerify into child class, CSignature$RSA::engineInitVerify

- CKeyStore.java

   *Rename generatePrivateKeyBlob to generateRSAPrivateKeyBlob and setPrivateKey to setRSAPrivateKey (thus no more instanceof check).

- CPublicKey.java

  * When CPublicKey.of(alg,...) sees an unknown alg, AssertionError("Unsupported algorithm: " + alg) is now thrown.

  * Add @Override to toString.

  * Make CPublicKey::getPublicKeyBlob package private.

- Extract "RSA" as a constant in the test.

- Many coding style changes, especially, moving open braces to the end of the previous lines.

Thanks
Max


> On Nov 22, 2018, at 10:04 PM, Weijun Wang <weijun.wang at oracle.com> 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.
> 
>> - 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.
> 
> 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