Add ECPrivateKey for Elliptic Curve keypair generation.

Add ECPrivateKey for Elliptic Curve keypair generation.

The implementation uses NSS on all platforms unless USE_OPENSSL is defined
(which is only stubbed out in this CL).

BUG=88782
TEST=ECPrivateKeyUnitTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109188

[mattm, 2011-10-28 04:27:41]

Main annoyance is needing the "public value" in order to import the key in NSS. 
I did a very simple hack of stuffing that into the exported byte array,
wondering if you have any idea if it's possible to avoid needing it?

Second thing:  Why does the win build have unresolved symbols for the ASN1
template?  I can try to dig into this on my win box tomorrow if no one knows
right off.

[wtc, 2011-11-03 02:17:50]

Review comments on Patch Set 6:

Excellent work!  And sorry for the late review.

I reviewed ec_private_key_nss.cc carefully and thought about
the problems you ran into.  I believe I figured out a good
solution to the ConvertToPublicKey problem.  But the need to
use EncryptedPrivateKeyInfo still remains.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:9: #include "build/build_config.h"

Include "build/build_config.h" with the Chromium headers
below.

In the new style, the platform-specific ifdefs are after
the Chromium headers, so it is not necessary to include
"build/build_config.h" first.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:22: struct SECKEYPublicKeyStr;

Please do

  typedef struct SECKEYPrivateKeyStr SECKEYPrivateKey;
  typedef struct SECKEYPublicKeyStr SECKEYPublicKey;

and use SECKEYPrivateKey and SECKEYPublicKey below.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:27: // Encapsulates an EC private key. Can be used to
generate new keys, export

EC => elliptic curve (EC)

otherwise "EC" is never defined in this file.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:38: static ECPrivateKey* Create();

Please add a TODO comment about adding a 'curve' parameter.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:47: // ExportPrivateKey for the format description.

API DESIGN: I recommend that this class exports and imports
public key and private key using separate std::vector<uint8>
parameters.

Then the origin-bound certificate store will store them in
our special format.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:72: bool ExportPrivateKey(std::vector<uint8>* output);

We should name this function ExportEncryptedPrivateKey,
and it should take a 'password' parameter.

I also suggest that the public key be a separate output
parameter.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:84: // methods above instead.

Nit: this class doesn't have any Find*() methods.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:42: // Like PK11_ImportEncryptedPrivateKeyInfo, but
hardcoded for EC, and returns

These two functions should be put in a file with an NSS
license header under src/crypto/third_party/nss.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:46: SECStatus
ImportEncryptedPrivateKeyInfoAndReturnKey(

Ideally this function should simply return SECKEYPrivateKey*
rather than using an output parameter.  But I found two
existing PK11_ImportXXXAndReturnKey functions and they both
use an output parameter like this.  So this should be consistent.
Sigh.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:70: CK_MECHANISM_TYPE cryptoMechType =
PK11_GetPBECryptoMechanism(

Since you are following Google style, name this variable
crypto_mech_type.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:131: CKA_NETSCAPE_DB, &item);

The CKA_NETSCAPE_DB attribute is only used by NSS's built-in
soft token, so this method won't work in general.

Fortunately, I believe we don't need the ConvertToPublicKey
function.  See my very last comment in this file.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:156: // Seems to be okay to leave pubk->u.ec.size =
0.

Based on code inspection, I believe you're right.  I saw no
NSS code that uses pubk->u.ec.size, and I saw NSS code that
sets pubk->u.ec.size to 0:
http://mxr.mozilla.org/security/search?string=ec.size

But I figured out how to set this field properly:
  pubk->u.ec.size = SECKEY_ECParamsToKeySize(&pubk->u.ec.DEREncodedParams);

Can you find out if this sets pubk->u.ec.size to 256?

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:189: NOTIMPLEMENTED();

Please add a comment to explain why this is not implemented
(if USE_NSS is not defined, we initialize NSS with no
databases, so we can't create permanent keys).

Same with CreateSensitiveFromPrivateKeyInfo below.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:247:
SECKEY_DestroyEncryptedPrivateKeyInfo(encrypted, PR_TRUE);

Nit: save the return value of SEC_ASN1EncodeItem in a local
variable, and call SECKEY_DestroyEncryptedPrivateKeyInfo
before testing the return value.  This avoid the duplicate
SECKEY_DestroyEncryptedPrivateKeyInfo calls.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:253: // XXX: do we care about cross build
compatibility (like loading keys

Replace XXX with TODO.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:280: output->push_back(der_pubkey->data[i]);

Can we use the assign or insert method to avoid byte-by-byte
copying?  Please fix the original code in
rsa_private_key_nss.cc the same way.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:294: EnsureNSSInit();

This EnsureNSSInit() call is redundant.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:353: SECItem public_value = {siBuffer};

Nit: initialize this to {siBuffer, NULL, 0} for consistency.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:360: unsigned char* buf = const_cast<unsigned
char*>(&input[0]);

Nit: I prefer moving the const_cast to lines 366 and 369 to
make it clear it's necessitated by NSS's SECItem.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:363: if (public_value.len >= input.size())

I think this should be
  if (1 + public_value.len >= input.size())
to account for byte 0 (the length byte).

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:370: der_key.len = &input.back() - buf + 1;

input.size() - 1 - public_value.len is easier to understand.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:402: result->public_key_ =
ConvertToPublicKey(result->key_);

IMPORTANT: this ConvertToPublicKey call is not necessary
because we have the public_value.

Can we use SECKEY_DecodeDERSubjectPublicKeyInfo
and SECKEY_ExtractPublicKey?  This requires changing
ExportPrivateKey to export an encoded SubjectPublicKeyInfo
instead of just public_key_->u.ec.publicValue.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_opens...
File crypto/ec_private_key_openssl.cc (right):

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_opens...
crypto/ec_private_key_openssl.cc:48: }

Also need stubs for ExportValue and ExportECParams.

[mattm, 2011-11-04 02:39:14]

updated.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:9: #include "build/build_config.h"
On 2011/11/03 02:17:50, wtc wrote:
> 
> Include "build/build_config.h" with the Chromium headers
> below.
> 
> In the new style, the platform-specific ifdefs are after
> the Chromium headers, so it is not necessary to include
> "build/build_config.h" first.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:22: struct SECKEYPublicKeyStr;
On 2011/11/03 02:17:50, wtc wrote:
> 
> Please do
> 
>   typedef struct SECKEYPrivateKeyStr SECKEYPrivateKey;
>   typedef struct SECKEYPublicKeyStr SECKEYPublicKey;
> 
> and use SECKEYPrivateKey and SECKEYPublicKey below.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:27: // Encapsulates an EC private key. Can be used to
generate new keys, export
On 2011/11/03 02:17:50, wtc wrote:
> 
> EC => elliptic curve (EC)
> 
> otherwise "EC" is never defined in this file.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:38: static ECPrivateKey* Create();
On 2011/11/03 02:17:50, wtc wrote:
> 
> Please add a TODO comment about adding a 'curve' parameter.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:47: // ExportPrivateKey for the format description.
On 2011/11/03 02:17:50, wtc wrote:
> 
> API DESIGN: I recommend that this class exports and imports
> public key and private key using separate std::vector<uint8>
> parameters.
> 
> Then the origin-bound certificate store will store them in
> our special format.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:72: bool ExportPrivateKey(std::vector<uint8>* output);
On 2011/11/03 02:17:50, wtc wrote:
> 
> We should name this function ExportEncryptedPrivateKey,
> and it should take a 'password' parameter.
> 
> I also suggest that the public key be a separate output
> parameter.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:84: // methods above instead.
On 2011/11/03 02:17:50, wtc wrote:
> 
> Nit: this class doesn't have any Find*() methods.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:42: // Like PK11_ImportEncryptedPrivateKeyInfo, but
hardcoded for EC, and returns
On 2011/11/03 02:17:50, wtc wrote:
> 
> These two functions should be put in a file with an NSS
> license header under src/crypto/third_party/nss.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:70: CK_MECHANISM_TYPE cryptoMechType =
PK11_GetPBECryptoMechanism(
On 2011/11/03 02:17:50, wtc wrote:
> 
> Since you are following Google style, name this variable
> crypto_mech_type.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:156: // Seems to be okay to leave pubk->u.ec.size =
0.
On 2011/11/03 02:17:50, wtc wrote:
> 
> Based on code inspection, I believe you're right.  I saw no
> NSS code that uses pubk->u.ec.size, and I saw NSS code that
> sets pubk->u.ec.size to 0:
> http://mxr.mozilla.org/security/search?string=ec.size
> 
> But I figured out how to set this field properly:
>   pubk->u.ec.size = SECKEY_ECParamsToKeySize(&pubk->u.ec.DEREncodedParams);
> 
> Can you find out if this sets pubk->u.ec.size to 256?

Yes, that did set it to 256.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:189: NOTIMPLEMENTED();
On 2011/11/03 02:17:50, wtc wrote:
> 
> Please add a comment to explain why this is not implemented
> (if USE_NSS is not defined, we initialize NSS with no
> databases, so we can't create permanent keys).
> 
> Same with CreateSensitiveFromPrivateKeyInfo below.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:247:
SECKEY_DestroyEncryptedPrivateKeyInfo(encrypted, PR_TRUE);
On 2011/11/03 02:17:50, wtc wrote:
> 
> Nit: save the return value of SEC_ASN1EncodeItem in a local
> variable, and call SECKEY_DestroyEncryptedPrivateKeyInfo
> before testing the return value.  This avoid the duplicate
> SECKEY_DestroyEncryptedPrivateKeyInfo calls.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:253: // XXX: do we care about cross build
compatibility (like loading keys
On 2011/11/03 02:17:50, wtc wrote:
> 
> Replace XXX with TODO.

Removed the comment, since I hope there shouldn't be any compatibility issues
now.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:280: output->push_back(der_pubkey->data[i]);
On 2011/11/03 02:17:50, wtc wrote:
> 
> Can we use the assign or insert method to avoid byte-by-byte
> copying?  Please fix the original code in
> rsa_private_key_nss.cc the same way.

done (I'll do rsa_private_key_nss.cc and rsa_private_key.cc (it has some too)
separately)

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:294: EnsureNSSInit();
On 2011/11/03 02:17:50, wtc wrote:
> 
> This EnsureNSSInit() call is redundant.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:353: SECItem public_value = {siBuffer};
On 2011/11/03 02:17:50, wtc wrote:
> 
> Nit: initialize this to {siBuffer, NULL, 0} for consistency.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:360: unsigned char* buf = const_cast<unsigned
char*>(&input[0]);
On 2011/11/03 02:17:50, wtc wrote:
> 
> Nit: I prefer moving the const_cast to lines 366 and 369 to
> make it clear it's necessitated by NSS's SECItem.

done-ish (code is a bit different now)

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:402: result->public_key_ =
ConvertToPublicKey(result->key_);
On 2011/11/03 02:17:50, wtc wrote:
> 
> IMPORTANT: this ConvertToPublicKey call is not necessary
> because we have the public_value.
> 
> Can we use SECKEY_DecodeDERSubjectPublicKeyInfo
> and SECKEY_ExtractPublicKey?  This requires changing
> ExportPrivateKey to export an encoded SubjectPublicKeyInfo
> instead of just public_key_->u.ec.publicValue.

Done.

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_opens...
File crypto/ec_private_key_openssl.cc (right):

http://codereview.chromium.org/8413024/diff/12001/crypto/ec_private_key_opens...
crypto/ec_private_key_openssl.cc:48: }
On 2011/11/03 02:17:50, wtc wrote:
> 
> Also need stubs for ExportValue and ExportECParams.

Done.

[Ryan Sleevi, 2011-11-04 03:21:25]

A lot of nits - overall LGTM, but I think there is good opportunity for some
RAII re-use.

I think this class may be a little weird once agl's
http://codereview.chromium.org/8431007/ lands, but I think this naming makes the
most sense.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:113: SECKEYPublicKey* public_key_;
ScopedSECKEYPrivateKey key_
ScopedSECKEYPublicKey public_key_

From crypto/scoped_nss_types.h

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:51: SECKEY_DestroyPublicKey(public_key_);
Don't need this when using the Scoped types

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:68: NOTIMPLEMENTED();
If this will never be implemented, should it be a NOTIMPLEMENTED?

I thought those were for "not quite ready" implementations

Same for line 101

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:120: SECKEYEncryptedPrivateKeyInfo* encrypted =
PK11_ExportEncryptedPrivKeyInfo(
nit? typedef scoped_ptr_malloc<SECKEYEncryptedPrivateKeyInfo,
SECKEY_DestroyEncryptedPrivateKeyInfo> ScopedSECKEYEncryptedPrivateKeyInfo

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:125: 1, // iterations.
Could you add a comment here as to why iterations is 1 (it's because it's not
really meant to be 'encrypted', it's just a transport thing.

I'm surprised NSS doesn't complain about low iteration counts - does this work
the same in the FIPS mode?

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:130: << PORT_GetError();
Is PORT_GetError correct here, or should it be PR_GetError?

I wonder if we shouldn't surface GetNSSErrorMessage from crypto/nss_util.cc to
public API and use that everywhere here. Perhaps TODO for later.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:149: ExportPublicKey(subject_public_key_info);
This method can fail. Perhaps

return ExportPublicKey(subject_public_key_info);

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:157: NOTREACHED();
Does this need to be NOTREACHED? Seems a valid (handled) edge condition, curious
the motivation.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:174: }
nit: Brace on same line

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:189: NOTREACHED();
Same here for NOTREACHED()

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:195: if (!SECITEM_AllocItem(NULL, &ec_parameters,
(2 + oid_data->oid.len))) {
Comment to explain the magic 2 here and on line 203

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:212: SECITEM_FreeItem(&ec_parameters, PR_FALSE);
Perhaps we should have a StackSECItem to go along with the ScopedSECItem, to
pass PR_FALSE as the last param.

Or you could just template<> the existing ScopedSECItem, then

StackSECItem stack_item;
SECItem item;
stack_item.reset(&item);

Perhaps that's no better, just looking for places where we can RAII (the
templates will optimize out, the only thing would be symbol size)

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_opens...
File crypto/ec_private_key_openssl.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_opens...
crypto/ec_private_key_openssl.cc:12: }
nit: closing brace on the same line

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
File crypto/ec_private_key_unittest.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:18: }
nit: If/when OpenSSL is implemented, I think you should also add some gold tests
that the two can use to test interoperability between the implementations.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:22: // value and elliptic curve params.
nit: Much like the existing tests for RSA, this is probably going to cause
issues for the memory tester bots. And of course, randomness can be a source of
test in-determinism.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:46: std::vector<uint8> pubkey2;
nit: The naming here (key as suffix) seems to conflict with the naming of the
variables at 34-37/66-70

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:89: TEST(ECPrivateKeyUnitTest,
BadPasswordTest) {
Explanation as to what the test covers?

More importantly, why test a bad password, if your iteration count for
encrypting is only 1? Is this meant to convey some security property?

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.cc
File crypto/third_party/nss/ec.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.c...
crypto/third_party/nss/ec.cc:24: *   Douglas Stebila <douglas@stebila.ca>, Sun
Microsystems Laboratories
Does any of this license truly apply?

[wtc, 2011-11-04 21:52:49]

Response to rsleevi's comments on Patch Set 7:

rsleevi: thank you for your thoughtful comments.  I responded
to some of your comments below.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:68: NOTIMPLEMENTED();

On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> If this will never be implemented, should it be a NOTIMPLEMENTED?

I think you're right.  NOTREACHED() seems more appropriate.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:120: SECKEYEncryptedPrivateKeyInfo* encrypted =
PK11_ExportEncryptedPrivKeyInfo(

On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> nit? typedef scoped_ptr_malloc<SECKEYEncryptedPrivateKeyInfo,
> SECKEY_DestroyEncryptedPrivateKeyInfo> ScopedSECKEYEncryptedPrivateKeyInfo

Unless this function becomes longer, using this
scoped_ptr_malloc seems to be a net loss...

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:130: << PORT_GetError();

On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Is PORT_GetError correct here, or should it be PR_GetError?

PORT_GetError and PR_GetError are the same:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/sec...

In code that uses NSS, it looks nicer to call PORT_GetError.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:157: NOTREACHED();

On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Does this need to be NOTREACHED?

This is copied from crypto/rsa_private_key_nss.cc.  I agree
this NOTREACHED() can be removed.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:195: if (!SECITEM_AllocItem(NULL, &ec_parameters,
(2 + oid_data->oid.len))) {

On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Comment to explain the magic 2 here and on line 203

The magic 2 here is indeed not obvious.  The magic 2 on
line 203 is obvious, following the assignments on lines
201-202.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
File crypto/ec_private_key_unittest.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:22: // value and elliptic curve params.

On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> nit: Much like the existing tests for RSA, this is probably going to cause
> issues for the memory tester bots.

RSA key pair generation is much slower than EC key pair
generation because an RSA key pair requires two random
*prime* numbers.  An EC key pair just need a random number.
So I don't think this test will time out when running
under valgrind or other memory check tools.

[wtc, 2011-11-04 22:41:26]

Patch Set 7 LGTM.  I suggest some changes below.

Almost all of my suggested changes are about coding style.
The only substantive issue is that ExportEncryptedPrivateKey
should not export the public key; call ExportPublicKey to do
that.

http://codereview.chromium.org/8413024/diff/15002/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/crypto.gyp#newcode99
crypto/crypto.gyp:99: 'third_party/nss/ec.cc',

For the header file, I suggest naming it something like
chromium-nss.h or chromium-pk11pub.h.

For the .cc file, I suggest naming it after the original
NSS source file pk11akey.c.  So you can name it pk11akey.cc
or chromium-pk11akey.cc.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:36: // Create a new random instance. Can return NULL if
initialization fails.

Nit: please go through this header file to use descriptive
rather than imperative verbs in the function comments, as
recommended by the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:49: // block and an X509 SubjectPublicKeyInfo block.

Nit: X509 => X.509

Please fix all occurrences in this file.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:77: bool ExportEncryptedPrivateKey(

Please make ExportEncryptedPrivateKey export just the private
key.  Use a separate ExportPublicKey call to export the public
key.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:114: SECItem password_secitem = {

Nit: shorten the variable name suffix to _item.  (This is the
convention used inside NSS.)

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:136: SECItem* rv = SEC_ASN1EncodeItem(

Nit: name this variable 'encoded_item'.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:195: if (!SECITEM_AllocItem(NULL, &ec_parameters,
(2 + oid_data->oid.len))) {
Nit: In C++ you should be able to allocate a variable-sized array on the stack:
  unsigned char parameters_buf[2 + oid_data->oid.len];
  SECKEYECParams ec_parameters = {
    siDEROID, parameters_buf, 2 + oid_data->oid.len
  };

Then you can avoid the SECITEM_AllocItem and SECITEM_FreeItem calls.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:236: SECItem subject_public_key_info_secitem = {

Nit: use the _item suffix in the variable name.
You can just name this 'spki_item' or 'encoded_spki',
because you already use the "spki" abbreviation below.

Similarly, encrypted_private_key_info_secitem can be
shortened to epki_item or encoded_epki.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:278: SECItem password_secitem = {

Nit: password_secitem => password_item

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.cc
File crypto/third_party/nss/ec.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.c...
crypto/third_party/nss/ec.cc:46: SECStatus
ImportEncryptedECPrivateKeyInfoAndReturnKey(

Please add a comment to note this is based on the
PK11_ImportEncryptedPrivateKeyInfo function in
mozilla/security/nss/lib/pk11wrap/pk11akey.c.

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h
File crypto/third_party/nss/ec.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h...
crypto/third_party/nss/ec.h:24: *   Douglas Stebila <douglas@stebila.ca>, Sun
Microsystems Laboratories

Let's remove lines 22-24.  They are a little confusing.

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h...
crypto/third_party/nss/ec.h:40: // This file contains some helper functions for
our elliptic curve crypto code.

If you rename this header to something more generic like
chromium-nss.h or chromium-pk11pub.h, this comment should
be updated.

[mattm, 2011-11-08 02:12:27]

updated

http://codereview.chromium.org/8413024/diff/15002/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/crypto.gyp#newcode99
crypto/crypto.gyp:99: 'third_party/nss/ec.cc',
On 2011/11/04 22:41:26, wtc wrote:
> 
> For the header file, I suggest naming it something like
> chromium-nss.h or chromium-pk11pub.h.
> 
> For the .cc file, I suggest naming it after the original
> NSS source file pk11akey.c.  So you can name it pk11akey.cc
> or chromium-pk11akey.cc.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:36: // Create a new random instance. Can return NULL if
initialization fails.
On 2011/11/04 22:41:26, wtc wrote:
> 
> Nit: please go through this header file to use descriptive
> rather than imperative verbs in the function comments, as
> recommended by the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:49: // block and an X509 SubjectPublicKeyInfo block.
On 2011/11/04 22:41:26, wtc wrote:
> 
> Nit: X509 => X.509
> 
> Please fix all occurrences in this file.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:77: bool ExportEncryptedPrivateKey(
On 2011/11/04 22:41:26, wtc wrote:
> 
> Please make ExportEncryptedPrivateKey export just the private
> key.  Use a separate ExportPublicKey call to export the public
> key.

Done.  Misunderstood your comment on the original review.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:113: SECKEYPublicKey* public_key_;
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> ScopedSECKEYPrivateKey key_
> ScopedSECKEYPublicKey public_key_
> 
> From crypto/scoped_nss_types.h

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:51: SECKEY_DestroyPublicKey(public_key_);
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Don't need this when using the Scoped types

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:68: NOTIMPLEMENTED();
On 2011/11/04 21:52:49, wtc wrote:
> 
> On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> > If this will never be implemented, should it be a NOTIMPLEMENTED?
> 
> I think you're right.  NOTREACHED() seems more appropriate.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:114: SECItem password_secitem = {
On 2011/11/04 22:41:26, wtc wrote:
> 
> Nit: shorten the variable name suffix to _item.  (This is the
> convention used inside NSS.)

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:125: 1, // iterations.
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Could you add a comment here as to why iterations is 1 (it's because it's not
> really meant to be 'encrypted', it's just a transport thing.
> 
> I'm surprised NSS doesn't complain about low iteration counts - does this work
> the same in the FIPS mode?

Since this has a password arg now I guess it also makes sense to expose the
iterations as an argument too.  Then using the iteration count of 1 can be moved
up to a higher level where it makes more sense.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:136: SECItem* rv = SEC_ASN1EncodeItem(
On 2011/11/04 22:41:26, wtc wrote:
> 
> Nit: name this variable 'encoded_item'.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:157: NOTREACHED();
On 2011/11/04 21:52:49, wtc wrote:
> 
> On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> > Does this need to be NOTREACHED?
> 
> This is copied from crypto/rsa_private_key_nss.cc.  I agree
> this NOTREACHED() can be removed.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:174: }
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> nit: Brace on same line

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:189: NOTREACHED();
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Same here for NOTREACHED()

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:195: if (!SECITEM_AllocItem(NULL, &ec_parameters,
(2 + oid_data->oid.len))) {
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Comment to explain the magic 2 here and on line 203

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:195: if (!SECITEM_AllocItem(NULL, &ec_parameters,
(2 + oid_data->oid.len))) {
On 2011/11/04 22:41:26, wtc wrote:
> Nit: In C++ you should be able to allocate a variable-sized array on the
stack:
>   unsigned char parameters_buf[2 + oid_data->oid.len];
>   SECKEYECParams ec_parameters = {
>     siDEROID, parameters_buf, 2 + oid_data->oid.len
>   };
> 
> Then you can avoid the SECITEM_AllocItem and SECITEM_FreeItem calls.

Hm, seems variable length arrays aren't in (standard) C++, but are in C99.  The
style guide doesn't allow them.  However the idea is good, I switched to using a
vector instead.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:236: SECItem subject_public_key_info_secitem = {
On 2011/11/04 22:41:26, wtc wrote:
> 
> Nit: use the _item suffix in the variable name.
> You can just name this 'spki_item' or 'encoded_spki',
> because you already use the "spki" abbreviation below.
> 
> Similarly, encrypted_private_key_info_secitem can be
> shortened to epki_item or encoded_epki.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:278: SECItem password_secitem = {
On 2011/11/04 22:41:26, wtc wrote:
> 
> Nit: password_secitem => password_item

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_opens...
File crypto/ec_private_key_openssl.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_opens...
crypto/ec_private_key_openssl.cc:12: }
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> nit: closing brace on the same line

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
File crypto/ec_private_key_unittest.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:18: }
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> nit: If/when OpenSSL is implemented, I think you should also add some gold
tests
> that the two can use to test interoperability between the implementations.

Good idea.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:22: // value and elliptic curve params.
On 2011/11/04 21:52:49, wtc wrote:
> 
> On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> > nit: Much like the existing tests for RSA, this is probably going to cause
> > issues for the memory tester bots.
> 
> RSA key pair generation is much slower than EC key pair
> generation because an RSA key pair requires two random
> *prime* numbers.  An EC key pair just need a random number.
> So I don't think this test will time out when running
> under valgrind or other memory check tools.

Yeah, from the linux_valgrind try run:
ECPrivateKeyUnitTest.InitRandomTest (1201 ms)
RSAPrivateKeyUnitTest.InitRandomTest (36355 ms)

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:46: std::vector<uint8> pubkey2;
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> nit: The naming here (key as suffix) seems to conflict with the naming of the
> variables at 34-37/66-70

I thought about that, but the alternatives I thought of seem overly verbose or
don't read very naturally.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:89: TEST(ECPrivateKeyUnitTest,
BadPasswordTest) {
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Explanation as to what the test covers?
> 
> More importantly, why test a bad password, if your iteration count for
> encrypting is only 1? Is this meant to convey some security property?

No, just a functional test, wrong password should return NULL and not blow up or
leak memory or something.

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.cc
File crypto/third_party/nss/ec.cc (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.c...
crypto/third_party/nss/ec.cc:24: *   Douglas Stebila <douglas@stebila.ca>, Sun
Microsystems Laboratories
On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> Does any of this license truly apply?

This is the license block from pk11akey.c

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.c...
crypto/third_party/nss/ec.cc:46: SECStatus
ImportEncryptedECPrivateKeyInfoAndReturnKey(
On 2011/11/04 22:41:26, wtc wrote:
> 
> Please add a comment to note this is based on the
> PK11_ImportEncryptedPrivateKeyInfo function in
> mozilla/security/nss/lib/pk11wrap/pk11akey.c.

Done.

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h
File crypto/third_party/nss/ec.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h...
crypto/third_party/nss/ec.h:24: *   Douglas Stebila <douglas@stebila.ca>, Sun
Microsystems Laboratories
On 2011/11/04 22:41:26, wtc wrote:
> 
> Let's remove lines 22-24.  They are a little confusing.

Done.  (did you mean just in the header or in the .cc too?)

[wtc, 2011-11-08 23:21:18]

Patch Set 11 LGTM.

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:113: SECKEYPublicKey* public_key_;
On 2011/11/08 02:12:27, mattm wrote:
> On 2011/11/04 03:21:25, Ryan Sleevi wrote:
> > ScopedSECKEYPrivateKey key_
> > ScopedSECKEYPublicKey public_key_
> > 
> > From crypto/scoped_nss_types.h
> 
> Done.

This was not done. :-)

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h
File crypto/third_party/nss/ec.h (right):

http://codereview.chromium.org/8413024/diff/15002/crypto/third_party/nss/ec.h...
crypto/third_party/nss/ec.h:24: *   Douglas Stebila <douglas@stebila.ca>, Sun
Microsystems Laboratories
On 2011/11/08 02:12:27, mattm wrote:
> On 2011/11/04 22:41:26, wtc wrote:
> > 
> > Let's remove lines 22-24.  They are a little confusing.
> 
> Done.  (did you mean just in the header or in the .cc too?)

I meant just in the header.

These three people contributed ECC code to NSS.  The
declaration of this function is not ECC specific, and
this header is intended to declare all or most functions
we copied from NSS, so it isn't necessary to acknowledge
the ECC contributors in this header.

On the other hand, pk11akey.cc has ECC related code, so
we should acknowledge the ECC contributors.

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:76: // block and the public key as an X.509
SubjectPublicKeyInfo block.

|password| and |iterations| should be documented.  A safe
value of |iterations| should be recommended.

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:69: NOTREACHED();

Perhaps we should remove the CreateSensitive() and
CreateSensitiveFromEncryptedPrivateKeyInfo() methods from this
class, and add them back when we really need them.

You can do this later (so this CL won't drag on too long).

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:192: // The in addition to the oid data, the
encoding requires one byte for the

Typo: The in => In

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_unitt...
File crypto/ec_private_key_unittest.cc (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:14: // TODO(mattm): When that happens, also
add some golden test data to test

I don't know what "golden" test data means.  Is that a well-known
term?

http://codereview.chromium.org/8413024/diff/26006/crypto/third_party/nss/chro...
File crypto/third_party/nss/chromium-nss.h (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/third_party/nss/chro...
crypto/third_party/nss/chromium-nss.h:41: // This file contains some helper
functions for our elliptic curve crypto code.

Make this comment more generic (such as "some functions we
copied from NSS"), or just remove this comment.

This header is intended to declare all or most functions we
copied from NSS, hence the name chromium-nss.h.

http://codereview.chromium.org/8413024/diff/26006/crypto/third_party/nss/chro...
crypto/third_party/nss/chromium-nss.h:48: // See
https://bugzilla.mozilla.org/show_bug.cgi?id=211546

Please add a note that this function can be replaced by
PK11_ImportEncryptedPrivateKeyInfoAndReturnKey when we
use NSS 3.13.2 or later.

[mattm, 2011-11-09 04:11:34]

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:76: // block and the public key as an X.509
SubjectPublicKeyInfo block.
On 2011/11/08 23:21:18, wtc wrote:
> 
> |password| and |iterations| should be documented.  A safe
> value of |iterations| should be recommended.

Done.

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_nss.cc
File crypto/ec_private_key_nss.cc (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_nss.c...
crypto/ec_private_key_nss.cc:192: // The in addition to the oid data, the
encoding requires one byte for the
On 2011/11/08 23:21:18, wtc wrote:
> 
> Typo: The in => In

Done.

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_unitt...
File crypto/ec_private_key_unittest.cc (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/ec_private_key_unitt...
crypto/ec_private_key_unittest.cc:14: // TODO(mattm): When that happens, also
add some golden test data to test
On 2011/11/08 23:21:18, wtc wrote:
> 
> I don't know what "golden" test data means.  Is that a well-known
> term?

I guess it's a somewhat common term, though now that I think about it this isn't
quite correct use of it.  (It's basically a term for using the code to generate
some output and then subsequently comparing in the test that the same output is
generated, like layout tests.)  In this case we would be using the code to
generate the test data, but it's more of an input than something we compare
against the output, so maybe it's arguable whether the term still applies. 
Changed.

http://codereview.chromium.org/8413024/diff/26006/crypto/third_party/nss/chro...
File crypto/third_party/nss/chromium-nss.h (right):

http://codereview.chromium.org/8413024/diff/26006/crypto/third_party/nss/chro...
crypto/third_party/nss/chromium-nss.h:41: // This file contains some helper
functions for our elliptic curve crypto code.
On 2011/11/08 23:21:18, wtc wrote:
> 
> Make this comment more generic (such as "some functions we
> copied from NSS"), or just remove this comment.
> 
> This header is intended to declare all or most functions we
> copied from NSS, hence the name chromium-nss.h.

oops, done

http://codereview.chromium.org/8413024/diff/26006/crypto/third_party/nss/chro...
crypto/third_party/nss/chromium-nss.h:48: // See
https://bugzilla.mozilla.org/show_bug.cgi?id=211546
On 2011/11/08 23:21:18, wtc wrote:
> 
> Please add a note that this function can be replaced by
> PK11_ImportEncryptedPrivateKeyInfoAndReturnKey when we
> use NSS 3.13.2 or later.

Done.

[commit-bot: I haz the power, 2011-11-09 04:13:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/8413024/31028

[commit-bot: I haz the power, 2011-11-09 05:08:55]

Change committed as 109188