Webrev updated at https://cr.openjdk.java.net/~weijun/8213009/webrev.03
I reset messageDigest/messageDigestAlgorithm/needsReset and check for "key instanceof RSAPublicKey" in RSA::engineInitVerify. Thanks Max > On Nov 27, 2018, at 10:13 PM, Weijun Wang <weijun.w...@oracle.com> wrote: > > > >> 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 CRSAPrivateKey, 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 >
