> On Nov 22, 2018, at 9:14 AM, Valerie Peng <valerie.p...@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.w...@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.w...@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.w...@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
> 

Reply via email to