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