Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27195002:
openssl: Implement crypto::ECPrivateKey. (Closed)
|
Created:
7 years, 2 months ago by digit1 Modified:
7 years, 2 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionopenssl: Implement crypto::ECPrivateKey.
BUG=306176
R=rsleevi@chromium.org, wtc@chromium.org, agl@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229153
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address nits. #Patch Set 3 : Trivial rebase #
Total comments: 2
Patch Set 4 : Trivial rebase. #Patch Set 5 : Fix mac build by adding one \n at the end of a source file. #Messages
Total messages: 14 (0 generated)
Patch set 1 LGTM. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl.cc File crypto/ec_private_key_openssl.cc (right): https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:23: typedef int (*ExportFunctionWithBio)(BIO* bio, void* key); 1. Nit: ExportFunctionWithBio => ExportWithBioFunction or KeyExportFunction so that the type name ends in "Function"? Update: I see this needs to be distinct from ExportFunctionWithData. I don't have a good suggestion right now. 2. It seems that |bio| is an output argument. The Style Guide says: When defining a function, parameter order is: inputs, then outputs. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:26: bool ExportKeyWithBio(void* key, Nit: can |key| be declared as const void* ? https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:53: // the number of bytes required to export the data. Nit: Add "return -1 on failure". https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:56: // Hlper to export |key| into |output| via the specified export function. Typo: Hlper => Helper https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:68: unsigned char* data = reinterpret_cast<unsigned char*>(&output->front()); We can assume uint8 is the same as unsigned char and omit the reinterpret_cast<unsigned char*> cast. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:115: } Nit: omit curly braces. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:149: // ImportEncryptedECPrivateKeyInfoAndReturnKey(). I took a quick look. When NSS stores a private key, it uses the corresponding public key as the lookup "key". Specifically, the SHA-1 hash of the public key becomes the CKA_ID attribute of the private key, and then the public key itself is also stored as an attribute of the private key. I think this is why the NSS code requires |subject_public_key_info|. So, it is OK for this file to ignore |subject_public_key_info|. You can save it in a member so that ECPrivateKey::ExportPublicKey can simply return it. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_unittes... File crypto/ec_private_key_unittest.cc (right): https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_unittes... crypto/ec_private_key_unittest.cc:12: // TODO(mattm): also add some exported keys from each to test Nit: remove "also".
https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl.cc File crypto/ec_private_key_openssl.cc (right): https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:32: // OpenSSLErrStackTracer err_tracer(FROM_HERE); Uncomment or delete? https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:56: // Hlper to export |key| into |output| via the specified export function. // Helper... https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:149: // ImportEncryptedECPrivateKeyInfoAndReturnKey(). I think NSS might be using it to avoid recalculating the public key. But that's no big deal.
https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl.cc File crypto/ec_private_key_openssl.cc (right): https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:23: typedef int (*ExportFunctionWithBio)(BIO* bio, void* key); I've changed it to 'ExportBioFunction' and added a note that explains that, since this is used with OpenSSL API functions, this doesn't follows the Chromium style guide. On the other hand, 'const void* key' works. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:26: bool ExportKeyWithBio(void* key, On 2013/10/14 21:45:07, wtc wrote: > > Nit: can |key| be declared as const void* ? Done. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:32: // OpenSSLErrStackTracer err_tracer(FROM_HERE); Deleted. Thanks. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:53: // the number of bytes required to export the data. On 2013/10/14 21:45:07, wtc wrote: > > Nit: Add "return -1 on failure". Done. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:56: // Hlper to export |key| into |output| via the specified export function. On 2013/10/14 21:47:31, agl wrote: > // Helper... Done. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:68: unsigned char* data = reinterpret_cast<unsigned char*>(&output->front()); On 2013/10/14 21:45:07, wtc wrote: > > We can assume uint8 is the same as unsigned char and omit the > reinterpret_cast<unsigned char*> cast. Done. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:115: } On 2013/10/14 21:45:07, wtc wrote: > > Nit: omit curly braces. Done. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_openssl... crypto/ec_private_key_openssl.cc:149: // ImportEncryptedECPrivateKeyInfoAndReturnKey(). Thanks, I think I'd prefer to ignore it if it is not used at all, otherwise at would have to check that it matches the private key information, just to catch clients that pass invalid arguments, and I don't think it's worth it here. https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_unittes... File crypto/ec_private_key_unittest.cc (right): https://codereview.chromium.org/27195002/diff/1/crypto/ec_private_key_unittes... crypto/ec_private_key_unittest.cc:12: // TODO(mattm): also add some exported keys from each to test On 2013/10/14 21:45:07, wtc wrote: > > Nit: remove "also". Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/27195002/11001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/27195002/30001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Patch set 3 LGTM. https://codereview.chromium.org/27195002/diff/30001/crypto/ec_private_key_ope... File crypto/ec_private_key_openssl.cc (right): https://codereview.chromium.org/27195002/diff/30001/crypto/ec_private_key_ope... crypto/ec_private_key_openssl.cc:149: // as a lookup key when storing the private one in its store). Nit: move this note to the beginning of the function, perhaps after the subject_public_key_info.empty() test?
https://codereview.chromium.org/27195002/diff/30001/crypto/ec_private_key_ope... File crypto/ec_private_key_openssl.cc (right): https://codereview.chromium.org/27195002/diff/30001/crypto/ec_private_key_ope... crypto/ec_private_key_openssl.cc:149: // as a lookup key when storing the private one in its store). On 2013/10/15 23:13:25, wtc wrote: > > Nit: move this note to the beginning of the function, perhaps after the > subject_public_key_info.empty() test? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/27195002/52001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/27195002/66001
Message was sent while issue was closed.
Change committed as 229153 |
