Hi Valerie, I'll fix it in my local repo, run some test, and push the changeset.
Thanks, Max > On Dec 7, 2018, at 6:58 AM, Valerie Peng <[email protected]> wrote: > > Hi, Max, > > CSignature.java: one last nit, line 98 and 102 can be replaced by just one > line outside the if-block. > > Thanks, > > Valerie > > On 12/6/2018 6:33 AM, Weijun Wang wrote: >> 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 <[email protected]> wrote: >>> >>> >>> >>>> On Nov 27, 2018, at 9:55 AM, Valerie Peng <[email protected]> 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 <[email protected]> >>>>>> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>>> 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 <[email protected]> >>>>>>>>>> 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
