Hi Valerie, > On Dec 12, 2018, at 6:21 AM, Valerie Peng <valerie.p...@oracle.com> wrote: > > Hi Max, > > <CSignature.java> > > - Comments (line 60-63) missed SHA224withECDSA?
Oops. > > - Line 430: should be "ECPublicKey" Oops again. > > - Line 919, 922: is it really necessary to have two methods with algorithm > name argument? It seems they are functionally the same but one calls CAPI vs > CNG. Can they be merged? Yes. > > <CKey.java> > > - generateECBlob() method (line 110-148), is the extra 1 due to the sign bit > added by the BigInteger.toByteArray()? I recall BigInteger adds an extra 00 > byte sometimes to indicate the positive value, but why is there an extra 1? > Are the bytes for x, y, bs longer than the allocated "keyLen" length? You're right. I was thinking about the sign bit but got it wrong. > > <security.cpp> > > - line 627, shouldn't 'C' be at buffer[1]? If not, what is at buffer[1]? len > value is not checked, not sure how useful it is. It's a Unicode algorithm name, so buffer[1] is zero. I didn't check len because 32 is always enough for an algorithm name and there are at least 3 bytes there. Thanks, Max > > Thanks, > > Valerie > > On 12/3/2018 7:14 AM, Weijun Wang wrote: >> Please take a review at >> >> https://cr.openjdk.java.net/~weijun/8213010/webrev.00/ >> >> A Windows keystore is now able to load EC keys and uses them in signing and >> verifying with SHA<n>withECDSA. >> >> Not supported: >> >> 1. No EC KeyPairGenerator yet. >> >> 2. Cannot store a EC key (from SunEC) into a Windows keystore. I still >> haven't figured out how to call NCryptImportKey, NCryptCreatePersistedKey >> and CertAddCertificateContextToStore together correctly to associate a EC >> private key to a cert and store them. >> >> 3. SHA<n>withECDSAinP1363Format not supported, but it's easy to add them. >> >> 4. NONEwithECDSA not supported. >> >> Currently I can only use certmgr.exe to import a pkcs12 file and then run a >> manual test with it. Therefore no automatic test is included. Like RSA >> support in SunMSCAPI, Signature::initSign only support native keys. >> Signature::initVerify supports both native and SunEC keys. That said, since >> we do not have EC KeyPairGenerator yet you won't meet a real native EC >> public key. >> >> Thanks >> Max >>
