> On Nov 27, 2018, at 9:55 AM, Valerie Peng <valerie.p...@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.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.
>>
> 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 CRSAPublicKey, 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.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
