> 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
>
