webcrypto: Add ECDSA algorithm (chromium-side)

webcrypto: Add ECDSA algorithm using BoringSSL (chromium-side)

Adds:
 * EC key generation
 * EC key import from SPKI/PKCS8
 * EC key import from JWK
 * ECDSA signing
 * ECDSA verification
 * Tests

BUG=399094
Committed: https://crrev.com/b2ead6df9ea247e5e9a2f97199d2a25b52804f36
Cr-Commit-Position: refs/heads/master@{#304152}

[eroman, 2014-11-04 21:35:42]

eroman@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2014-11-05 23:31:20]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2014-11-05 23:31:21]

rsleevi@chromium.org changed required reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2014-11-05 23:31:21]

I'm gonna throw David under the bus on this one, and I'll act as secondary.

David, the WebCrypto spec regarding this is at
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#ecdsa

[davidben, 2014-11-07 00:17:21]

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ec_key_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:114: // ones.
http://crbug.com/389400
This is for checking id-ecDH and only allowing it for ECDH, right? (Note that,
currently, BoringSSL doesn't seem to know about id-ecDH at all.)

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:157:
jwk->SetBytes(member_name, CryptoData(BIGNUMToVector(value)));
JWK says of x/y that "The length of this octet string MUST be the full size of a
coordinate for the curve specified in the "crv" parameter". d is similarly
padded with zeros. WriteBIGNUM seems to call BN_bn2bin instead of
BN_bn2bin_padded, so the leading zeros are stripped.

To save you the trouble of generating them in a loop,
https://codereview.chromium.org/431453003 (wonderful; we have two JWK
implementations) includes a test for a key with a leading zero.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:163: crypto::ScopedBIGNUM*
out) {
Likewise, this should be strict about the size here.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:320:
algorithm.ecKeyImportParams();
Does this require a null check or is it already checked at another layer? (Ditto
for all the other instances in this file.)

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:380: // match expectations.
It looks like only ECDSA does anything with alg, not ECDH, so the comment
probably needs slight tweaking. (Does that mean its presence is an error or you
just ignore it?)

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:501: crypto::ScopedBIGNUM d;
Unused variable?

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:47: // ECDSA_size()
implementation it calls ec->ecdsa_meth->group_order_size(ec).
We could probably expose something of the sort. (And swap out that default
implementation's call to BN_num_bits with BN_num_bytes...)

Is WebCrypto intended to be used with keys with custom *_METHODs?

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:77: if (!ecdsa_sig.get())
  || der_data != &signature->front() + signature->size()

Not that BoringSSL should ever give back an invalid ECDSA_SIG, but we should
make sure there's no trailing input.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:90: 
This dance can just be two calls to BN_bn2bin_padded.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:152:
BN_bin2bn(signature.bytes() + order_size_bytes, order_size_bytes, NULL);
BN_bin2bn can return NULL. Dunno if you want to check for that... it's only on
allocation failure, but then again, this should be true[*] of ECDSA_SIG_new too.

[*] Probably? It's still using the macro-ridden old ASN.1 code, so who knows?

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:270: return rv >= 0 ?
Status::Success() : Status::OperationError();
Actually this just returns 0 or 1 now, as of
https://boringssl-review.googlesource.com/2064
https://boringssl-review.googlesource.com/1333

It does mean you can no longer distinguish the two failure cases, though it
doesn't look like this code cares.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/test/ecdsa_unittest.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/ecdsa_unittest.cc:46: EXPECT_TRUE(false) <<
"Unrecognized curve name: " << curve_str;
Probably ADD_FAILURE() instead of EXPECT_TRUE(false)

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/ecdsa_unittest.cc:275: // the exported JWK differs
from the inported one. In particular it contains
s/inports/imported/

[eroman, 2014-11-08 01:52:57]

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ec_key_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:114: // ones.
http://crbug.com/389400
On 2014/11/07 00:17:21, David Benjamin wrote:
> This is for checking id-ecDH and only allowing it for ECDH, right? (Note that,
> currently, BoringSSL doesn't seem to know about id-ecDH at all.)

Done.

I have delete the comment, it was a copy-paste from RSA which is not applicable
for ECDH.

For ECDSA WebCrypto says to use ecPublicKey which is consistent with what
BoringSSL already does.

It is true that when adding support for ECDH there will be tension with the spec
which says to recognize id-ecDH as well... But I will worry about that when
adding ECDH.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:157:
jwk->SetBytes(member_name, CryptoData(BIGNUMToVector(value)));
On 2014/11/07 00:17:21, David Benjamin wrote:
> JWK says of x/y that "The length of this octet string MUST be the full size of
a
> coordinate for the curve specified in the "crv" parameter". d is similarly
> padded with zeros. WriteBIGNUM seems to call BN_bn2bin instead of
> BN_bn2bin_padded, so the leading zeros are stripped.
> 
> To save you the trouble of generating them in a loop,
> https://codereview.chromium.org/431453003 (wonderful; we have two JWK
> implementations) includes a test for a key with a leading zero.

Thanks! Done.

(I hadn't read that carefully enough)

I have added tests where each of x, y, and d require zero-padding.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:163: crypto::ScopedBIGNUM*
out) {
On 2014/11/07 00:17:21, David Benjamin wrote:
> Likewise, this should be strict about the size here.

Done.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:320:
algorithm.ecKeyImportParams();
On 2014/11/07 00:17:21, David Benjamin wrote:
> Does this require a null check or is it already checked at another layer?
(Ditto
> for all the other instances in this file.)

This is guaranteed to be filled by the Blink layer.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:380: // match expectations.
On 2014/11/07 00:17:21, David Benjamin wrote:
> It looks like only ECDSA does anything with alg, not ECDH, so the comment
> probably needs slight tweaking. (Does that mean its presence is an error or
you
> just ignore it?)

Done.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:501: crypto::ScopedBIGNUM d;
On 2014/11/07 00:17:21, David Benjamin wrote:
> Unused variable?

Done.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:47: // ECDSA_size()
implementation it calls ec->ecdsa_meth->group_order_size(ec).
On 2014/11/07 00:17:21, David Benjamin wrote:
> We could probably expose something of the sort. (And swap out that default
> implementation's call to BN_num_bits with BN_num_bytes...)
> 
> Is WebCrypto intended to be used with keys with custom *_METHODs?

I have replaced this with a call to EC_GROUP_get_degree(), which I think
accomplishes what I want.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:77: if (!ecdsa_sig.get())
On 2014/11/07 00:17:21, David Benjamin wrote:
>   || der_data != &signature->front() + signature->size()
> 
> Not that BoringSSL should ever give back an invalid ECDSA_SIG, but we should
> make sure there's no trailing input.

Actually, see the comment below.

I have been finding EVP_DigestSignFinal() does not return the correct size to be
able to do this test (It appears to always just be ECDSA_size() and zero-added
when the DER is shorter).

Maybe there is a bug in my code? I will investigate further and open a bug if in
fact this is the case.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:90: 
On 2014/11/07 00:17:21, David Benjamin wrote:
> This dance can just be two calls to BN_bn2bin_padded.

Done.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:152:
BN_bin2bn(signature.bytes() + order_size_bytes, order_size_bytes, NULL);
On 2014/11/07 00:17:21, David Benjamin wrote:
> BN_bin2bn can return NULL. Dunno if you want to check for that... it's only on
> allocation failure, but then again, this should be true[*] of ECDSA_SIG_new
too.
> 
> [*] Probably? It's still using the macro-ridden old ASN.1 code, so who knows?

Done.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:270: return rv >= 0 ?
Status::Success() : Status::OperationError();
On 2014/11/07 00:17:21, David Benjamin wrote:
> Actually this just returns 0 or 1 now, as of
> https://boringssl-review.googlesource.com/2064
> https://boringssl-review.googlesource.com/1333
> 
> It does mean you can no longer distinguish the two failure cases, though it
> doesn't look like this code cares.

Done.

[davidben, 2014-11-10 20:24:00]

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:77: if (!ecdsa_sig.get())
On 2014/11/08 01:52:57, eroman wrote:
> On 2014/11/07 00:17:21, David Benjamin wrote:
> >   || der_data != &signature->front() + signature->size()
> > 
> > Not that BoringSSL should ever give back an invalid ECDSA_SIG, but we should
> > make sure there's no trailing input.
> 
> Actually, see the comment below.
> 
> I have been finding EVP_DigestSignFinal() does not return the correct size to
be
> able to do this test (It appears to always just be ECDSA_size() and zero-added
> when the DER is shorter).
> 
> Maybe there is a bug in my code? I will investigate further and open a bug if
in
> fact this is the case.

BoringSSL really shouldn't be padding that. At a glance the code seems fine
though. The length comes eventually from i2d_ECDSA_SIG's return value which
wouldn't know anything about ECDSA_size.

The NULL better case of EVP_DigestSignFinal only returns the maximum size of the
signature, but you seem to be accounting for that. Is it possible that you
weren't in a previous version and that's where you saw it? (I just threw
together a test program and EVP_DigestSignFinal seems to behave correctly.)

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:79: Status status =
GetEcGroupDegreeInBytes(key, &degree_bytes);
I don't think these are quiiiite the same, are they? One's the number of bits to
represent an element of the underlying field (so for P-*, the number of bits in
the prime) while the order of the group is the order of (the subgroup generated
by) the base point.

I believe the order is the one you want since an ECDSA signature is a pair of
numbers to multiply points on the curve by, not a point on the curve itself.
(And so they're taken modulo the order.) Looks like the order and degree are the
same size in bits in the NIST P-* curves, but I don't think this is necessarily
true in general?

I dunno. Math. :-)

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:90: return
Status::ErrorUnexpected();
Nit: I think if the condition stretched over two lines, we also put in braces
usually?

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:222: *signature_match = false;
Nit: If this is getting set on errors too, may as well lift it above
ErrorUnexpectedKeyType() too.

[eroman, 2014-11-10 20:55:52]

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:77: if (!ecdsa_sig.get())
On 2014/11/10 20:23:59, David Benjamin wrote:
> On 2014/11/08 01:52:57, eroman wrote:
> > On 2014/11/07 00:17:21, David Benjamin wrote:
> > >   || der_data != &signature->front() + signature->size()
> > > 
> > > Not that BoringSSL should ever give back an invalid ECDSA_SIG, but we
should
> > > make sure there's no trailing input.
> > 
> > Actually, see the comment below.
> > 
> > I have been finding EVP_DigestSignFinal() does not return the correct size
to
> be
> > able to do this test (It appears to always just be ECDSA_size() and
zero-added
> > when the DER is shorter).
> > 
> > Maybe there is a bug in my code? I will investigate further and open a bug
if
> in
> > fact this is the case.
> 
> BoringSSL really shouldn't be padding that. At a glance the code seems fine
> though. The length comes eventually from i2d_ECDSA_SIG's return value which
> wouldn't know anything about ECDSA_size.
> 
> The NULL better case of EVP_DigestSignFinal only returns the maximum size of
the
> signature, but you seem to be accounting for that. Is it possible that you
> weren't in a previous version and that's where you saw it? (I just threw
> together a test program and EVP_DigestSignFinal seems to behave correctly.)

I will debug this further and get back to you.
Note that it didn't happen every time, but every so often there would be 1 or 2
zeros at the end of the signature.

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:79: Status status =
GetEcGroupDegreeInBytes(key, &degree_bytes);
On 2014/11/10 20:24:00, David Benjamin wrote:
> I don't think these are quiiiite the same, are they? One's the number of bits
to
> represent an element of the underlying field (so for P-*, the number of bits
in
> the prime) while the order of the group is the order of (the subgroup
generated
> by) the base point.
> 
> I believe the order is the one you want since an ECDSA signature is a pair of
> numbers to multiply points on the curve by, not a point on the curve itself.
> (And so they're taken modulo the order.) Looks like the order and degree are
the
> same size in bits in the NIST P-* curves, but I don't think this is
necessarily
> true in general?
> 
> I dunno. Math. :-)

Thanks David. I thought it was the size of the order too, but simplified it to
using the degree after seeing this code in BoringSSL which was doing the same
thing:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

I will change it back to the order size so it is more general and won't break if
support for other curves is added.

[davidben, 2014-11-10 21:00:58]

+agl so he can comment on the ECC math below.

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:79: Status status =
GetEcGroupDegreeInBytes(key, &degree_bytes);
On 2014/11/10 20:55:52, eroman wrote:
> On 2014/11/10 20:24:00, David Benjamin wrote:
> > I don't think these are quiiiite the same, are they? One's the number of
bits
> to
> > represent an element of the underlying field (so for P-*, the number of bits
> in
> > the prime) while the order of the group is the order of (the subgroup
> generated
> > by) the base point.
> > 
> > I believe the order is the one you want since an ECDSA signature is a pair
of
> > numbers to multiply points on the curve by, not a point on the curve itself.
> > (And so they're taken modulo the order.) Looks like the order and degree are
> the
> > same size in bits in the NIST P-* curves, but I don't think this is
> necessarily
> > true in general?
> > 
> > I dunno. Math. :-)
> 
> Thanks David. I thought it was the size of the order too, but simplified it to
> using the degree after seeing this code in BoringSSL which was doing the same
> thing:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> 
> I will change it back to the order size so it is more general and won't break
if
> support for other curves is added.

Joy. We get that code from OpenSSL, so I will wave hands in their direction.

+agl. Is my understanding of ECC correct here, that degree and order sizes are
not necessarily interchangeable? (And thus we should probably go fix that
file...)

[agl, 2014-11-10 21:42:26]

agl@chromium.org changed reviewers:
+ agl@chromium.org

[agl, 2014-11-10 21:42:27]

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:79: Status status =
GetEcGroupDegreeInBytes(key, &degree_bytes);
On 2014/11/10 21:00:58, David Benjamin wrote:
> +agl. Is my understanding of ECC correct here, that degree and order sizes are
> not necessarily interchangeable? (And thus we should probably go fix that
> file...)

Right, r and s are both scalars. So it's the group order that's pertinent here.

While the group order is bounded by Hasse's theorem
(http://en.wikipedia.org/wiki/Hasse's_theorem_on_elliptic_curves), that bound
isn't sufficiently tight to ensure that the order is always the same length as
the field, even with cofactor=1. (Even though they are for P-{224|256|384|521}
curves.)

[eroman, 2014-11-10 22:02:59]

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:77: if (!ecdsa_sig.get())
On 2014/11/10 20:55:52, eroman wrote:
> On 2014/11/10 20:23:59, David Benjamin wrote:
> > On 2014/11/08 01:52:57, eroman wrote:
> > > On 2014/11/07 00:17:21, David Benjamin wrote:
> > > >   || der_data != &signature->front() + signature->size()
> > > > 
> > > > Not that BoringSSL should ever give back an invalid ECDSA_SIG, but we
> should
> > > > make sure there's no trailing input.
> > > 
> > > Actually, see the comment below.
> > > 
> > > I have been finding EVP_DigestSignFinal() does not return the correct size
> to
> > be
> > > able to do this test (It appears to always just be ECDSA_size() and
> zero-added
> > > when the DER is shorter).
> > > 
> > > Maybe there is a bug in my code? I will investigate further and open a bug
> if
> > in
> > > fact this is the case.
> > 
> > BoringSSL really shouldn't be padding that. At a glance the code seems fine
> > though. The length comes eventually from i2d_ECDSA_SIG's return value which
> > wouldn't know anything about ECDSA_size.
> > 
> > The NULL better case of EVP_DigestSignFinal only returns the maximum size of
> the
> > signature, but you seem to be accounting for that. Is it possible that you
> > weren't in a previous version and that's where you saw it? (I just threw
> > together a test program and EVP_DigestSignFinal seems to behave correctly.)
> 
> I will debug this further and get back to you.
> Note that it didn't happen every time, but every so often there would be 1 or
2
> zeros at the end of the signature.

Added the length check.

(I can no longer reproduce the issue, must have been a bug in my test setup).

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:79: Status status =
GetEcGroupDegreeInBytes(key, &degree_bytes);
On 2014/11/10 21:42:27, agl wrote:
> On 2014/11/10 21:00:58, David Benjamin wrote:
> > +agl. Is my understanding of ECC correct here, that degree and order sizes
are
> > not necessarily interchangeable? (And thus we should probably go fix that
> > file...)
> 
> Right, r and s are both scalars. So it's the group order that's pertinent
here.
> 
> While the group order is bounded by Hasse's theorem
> (http://en.wikipedia.org/wiki/Hasse), that bound
> isn't sufficiently tight to ensure that the order is always the same length as
> the field, even with cofactor=1. (Even though they are for P-{224|256|384|521}
> curves.)

Done -- using the order size.

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:90: return
Status::ErrorUnexpected();
On 2014/11/10 20:24:00, David Benjamin wrote:
> Nit: I think if the condition stretched over two lines, we also put in braces
> usually?

Done.

https://codereview.chromium.org/698363002/diff/120001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:222: *signature_match = false;
On 2014/11/10 20:24:00, David Benjamin wrote:
> Nit: If this is getting set on errors too, may as well lift it above
> ErrorUnexpectedKeyType() too.

I moved this to the end, for consistent style with
https://codereview.chromium.org/688983004/

[davidben, 2014-11-11 00:00:57]

lgtm

https://codereview.chromium.org/698363002/diff/220001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/220001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:75: return
Status::OperationError();
(It's a little unfortunate that this could either be OperationError because
malloc failed[*] or ErrorUnexpected because one would hope BoringSSL never
produces an invalid signature.)

[*] Well, I suppose malloc can't actually fail. Meh.

[eroman, 2014-11-11 01:26:27]

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/test/ecdsa_unittest.cc (right):

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/ecdsa_unittest.cc:46: EXPECT_TRUE(false) <<
"Unrecognized curve name: " << curve_str;
On 2014/11/07 00:17:21, David Benjamin wrote:
> Probably ADD_FAILURE() instead of EXPECT_TRUE(false)

Done.

https://codereview.chromium.org/698363002/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/ecdsa_unittest.cc:275: // the exported JWK differs
from the inported one. In particular it contains
On 2014/11/07 00:17:21, David Benjamin wrote:
> s/inports/imported/

Done.

https://codereview.chromium.org/698363002/diff/220001/content/child/webcrypto...
File content/child/webcrypto/openssl/ecdsa_openssl.cc (right):

https://codereview.chromium.org/698363002/diff/220001/content/child/webcrypto...
content/child/webcrypto/openssl/ecdsa_openssl.cc:75: return
Status::OperationError();
On 2014/11/11 00:00:57, David Benjamin wrote:
> (It's a little unfortunate that this could either be OperationError because
> malloc failed[*] or ErrorUnexpected because one would hope BoringSSL never
> produces an invalid signature.)
> 
> [*] Well, I suppose malloc can't actually fail. Meh.

I'll change it to an unexpected error. The main difference is error unexpected
is stuff I don't expect to ever happen. I should add histograms to validate this
assumption.

[eroman, 2014-11-13 22:55:48]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-11-13 22:56:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698363002/260001

[commit-bot: I haz the power, 2014-11-13 23:18:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-13 23:18:46]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[eroman, 2014-11-13 23:26:16]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-11-13 23:27:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698363002/280001

[commit-bot: I haz the power, 2014-11-13 23:56:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-13 23:56:16]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[eroman, 2014-11-14 00:15:30]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-11-14 00:17:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698363002/300001

[commit-bot: I haz the power, 2014-11-14 02:26:27]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2014-11-14 02:27:44]

Patchset 16 (id:??) landed as
https://crrev.com/b2ead6df9ea247e5e9a2f97199d2a25b52804f36
Cr-Commit-Position: refs/heads/master@{#304152}