Hi Valerie Updated webrev at
https://cr.openjdk.java.net/~weijun/8213009/webrev.02 Changes: - CSignature.java: * Mentions "RSASSA-PSS" in class spec of CSignature. * CSignature.RSA child classes renamed, For example, Raw -> NONEwithRSA, SHA1 -> SHA1withRSA. References (both class name and string) also updated in SunMSCAPI.java. * Move CSignature::generatePublicKeyBlob into child class, CSignature$RSA::generatePublicKeyBlob. * Move CSignature::engineInitVerify into child class, CSignature$RSA::engineInitVerify - CKeyStore.java *Rename generatePrivateKeyBlob to generateRSAPrivateKeyBlob and setPrivateKey to setRSAPrivateKey (thus no more instanceof check). - CPublicKey.java * When CPublicKey.of(alg,...) sees an unknown alg, AssertionError("Unsupported algorithm: " + alg) is now thrown. * Add @Override to toString. * Make CPublicKey::getPublicKeyBlob package private. - Extract "RSA" as a constant in the test. - Many coding style changes, especially, moving open braces to the end of the previous lines. Thanks Max > On Nov 22, 2018, at 10:04 PM, Weijun Wang <weijun.w...@oracle.com> 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. > >> - 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 >> >
