Hi Valerie, The updated webrev is at
https://cr.openjdk.java.net/~weijun/8213010/webrev.01/ I haven't merged importPublicKey and importECPublicKey because the content is so different and we expect someday to remove the CAPI method at all. The interdiff.patch has some problem since I haven't really changed SunMSCAPI.java. Maybe a bug of the interdiff tool. Thanks Max > On Dec 12, 2018, at 9:18 AM, Weijun Wang <weijun.w...@oracle.com> wrote: > > 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 >>> >
