Support generation of EC keys using P256 curve and support ECDSA certs.

Support generation of EC keys using P256 curve and support ECDSA certs.

This CL started life here: https://webrtc-codereview.appspot.com/51189004

BUG=webrtc:4685, webrtc:4686
R=hbos@webrtc.org, juberti@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/b6d4ec418504fd947c6f96829c73180e9487e203

[torbjorng (webrtc), 2015-06-15 14:47:40]

torbjorng@webrtc.org changed reviewers:
+ hbos@webrtc.org, juberti@webrtc.org, tommi@webrtc.org

[torbjorng (webrtc), 2015-06-15 14:52:11]

On 2015/06/15 14:47:40, torbjorng1 wrote:
> mailto:torbjorng@webrtc.org changed reviewers:
> + mailto:hbos@webrtc.org, mailto:juberti@webrtc.org, mailto:tommi@webrtc.org

Please take a look.  I'd like to land this in its current state.

I will make a separate CL for unittests.

[tommi, 2015-06-15 21:02:42]

Great stuff.  Just a couple of questions but otherwise looks good.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc
File webrtc/base/nssidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:83: LOG(LS_ERROR) << "Key type requested not
understood";
What about having something like this at the top of the function?:

CHECK(key_type == KT_RSA || key_type == KT_ECDSA);

And then skip this else clause.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:141: ASSERT(certificate_ != nullptr);
Can we switch over to using DCHECK instead of ASSERT?  (I'm fine with doing that
in another cl)

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:228: const SECHashObject* ho;
nit: initialize to nullptr

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:317: bool NSSCertificate::Equals(const
NSSCertificate* tocompare) const {
nit: to_compare

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:445: LOG(LS_ERROR) << "Couldn't set hashing
algorithm";
Are these errors something we should expect to happen or are they "the world
must be ending" type of errors?
If so, I'd rather have [D]CHECKs in place of error handlers+gotos and errors
that the caller can't be expected to handle.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:538: if (private_key.find("-----BEGIN RSA PRIVATE
KEY-----") == 0) {
nit: could use startswith as suggested in the other review

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/opensslidentity.cc
File webrtc/base/opensslidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/opensslidentity.c...
webrtc/base/opensslidentity.cc:78: LOG(LS_ERROR) << "Key type requested not
understood";
[D]CHECK instead?

[juberti1, 2015-06-16 02:44:31]

On 2015/06/15 at 14:52:11, torbjorng wrote:
> On 2015/06/15 14:47:40, torbjorng1 wrote:
> > mailto:torbjorng@webrtc.org changed reviewers:
> > + mailto:hbos@webrtc.org, mailto:juberti@webrtc.org, mailto:tommi@webrtc.org
> 
> Please take a look.  I'd like to land this in its current state.
> 
> I will make a separate CL for unittests.

I am not OK with separate CL for unittests. Unit testing is part of writing the
code.

Also believe KT_DEFAULT should be used in place of KT_RSA in most, if not all
tests.

[juberti1, 2015-06-16 02:45:05]

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc
File webrtc/base/nssidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:83: LOG(LS_ERROR) << "Key type requested not
understood";
On 2015/06/15 at 21:02:42, tommi (webrtc) wrote:
> What about having something like this at the top of the function?:
> 
> CHECK(key_type == KT_RSA || key_type == KT_ECDSA);
> 
> And then skip this else clause.

Seems kind of drastic. If we added another key type, and then didn't update
nssidentity, would we want to crash in that case?

I agree with CHECK when there is no sensible way to recover, but the caller
needs to handle a NULL return here regardless.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:141: ASSERT(certificate_ != nullptr);
On 2015/06/15 at 21:02:42, tommi (webrtc) wrote:
> Can we switch over to using DCHECK instead of ASSERT?  (I'm fine with doing
that in another cl)

I think that should be done globally (pthatcher/pbos talked about this IIRC)

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:437: SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION,
This seems like it could be factored to simply set the OID in a local var, and
then use common code for SetAlgorithmID and DerSignData below.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:445: LOG(LS_ERROR) << "Couldn't set hashing
algorithm";
On 2015/06/15 at 21:02:42, tommi (webrtc) wrote:
> Are these errors something we should expect to happen or are they "the world
must be ending" type of errors?
> If so, I'd rather have [D]CHECKs in place of error handlers+gotos and errors
that the caller can't be expected to handle.

Caller needs to handle failure because keygen can fail (insufficient entropy
pool, bad params)

[torbjorng (webrtc), 2015-06-16 14:11:51]

Issues addressed.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc
File webrtc/base/nssidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:228: const SECHashObject* ho;
On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> nit: initialize to nullptr

A cleanup in old code, but OK...

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:317: bool NSSCertificate::Equals(const
NSSCertificate* tocompare) const {
On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> nit: to_compare

I don't understand this comment.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:317: bool NSSCertificate::Equals(const
NSSCertificate* tocompare) const {
On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> nit: to_compare

I will defer this cleanup in old to separate CL.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:437: SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION,
On 2015/06/16 02:45:04, juberti1 wrote:
> This seems like it could be factored to simply set the OID in a local var, and
> then use common code for SetAlgorithmID and DerSignData below.

Done.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:445: LOG(LS_ERROR) << "Couldn't set hashing
algorithm";
On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> Are these errors something we should expect to happen or are they "the world
> must be ending" type of errors?
> If so, I'd rather have [D]CHECKs in place of error handlers+gotos and errors
> that the caller can't be expected to handle.

Some of these "goto fail" could probably not happen, but some of them are for
real (for reasons mentioned by juberti) and need to be handled by caller.  If we
determine a cleanup is needed, let's put that in a separate CL and let's not
forget that we will BoringSSL everything.  :-)

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:538: if (private_key.find("-----BEGIN RSA PRIVATE
KEY-----") == 0) {
On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> nit: could use startswith as suggested in the other review

Done.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/opensslidentity.cc
File webrtc/base/opensslidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/opensslidentity.c...
webrtc/base/opensslidentity.cc:78: LOG(LS_ERROR) << "Key type requested not
understood";
On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> [D]CHECK instead?

Caller needs handle this, making no change.

[tommi, 2015-06-16 14:38:01]

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc
File webrtc/base/nssidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:83: LOG(LS_ERROR) << "Key type requested not
understood";
On 2015/06/16 at 02:45:04, juberti1 wrote:
> On 2015/06/15 at 21:02:42, tommi (webrtc) wrote:
> > What about having something like this at the top of the function?:
> > 
> > CHECK(key_type == KT_RSA || key_type == KT_ECDSA);
> > 
> > And then skip this else clause.
> 
> Seems kind of drastic. If we added another key type, and then didn't update
nssidentity, would we want to crash in that case?
> 
> I agree with CHECK when there is no sensible way to recover, but the caller
needs to handle a NULL return here regardless.

If we add a new key type and don't need to handle it here, then it sounds like
it would be by design and a CHECK() isn't the right thing to do.  I was
wondering whether this implementation would always be able to handle all types
and therefore shouldn't expect anything else to come along.

https://codereview.webrtc.org/1189583002/diff/1/webrtc/base/nssidentity.cc#ne...
webrtc/base/nssidentity.cc:317: bool NSSCertificate::Equals(const
NSSCertificate* tocompare) const {
On 2015/06/16 at 14:11:51, torbjorng1 wrote:
> On 2015/06/15 21:02:42, tommi (webrtc) wrote:
> > nit: to_compare
> 
> I don't understand this comment.

the naming rules would be to use an underscore in between the two words as is
actually done elsewhere in this file (e.g. "cert_list").  The variable name
here, doesn't follow that.

[torbjorng (webrtc), 2015-06-23 15:22:00]

Please take another look.

The CL now includes changes to the tests for ECDSA.

The tests fall in three categories:

Some tests now test both RSA and ECDSA.  Since the RFC says
that we MUST support both, testing both makes sense.

Other tests use KT_DEFAULT, which means they currently use
RSA. In all cases here, I have manually made sure things work
also for ECDSA.  We could generalize these to tests both RSA
and RCDSA at some point.

A final set of tests use KT_RSA, even if I prepared them for
being more general. These tests have some TLS1.0 and even
SSL2 assumptions, which means they expect RSA.  These
protocols are quite obsolete.  We might want to generalize these tests at some
point (perhaps by simply ripping out
SSL2/TLS1.0 code).

[juberti1, 2015-06-23 19:54:37]

On 2015/06/23 at 15:22:00, torbjorng wrote:
> Please take another look.
> 
> The CL now includes changes to the tests for ECDSA.
> 
> The tests fall in three categories:
> 
> Some tests now test both RSA and ECDSA.  Since the RFC says
> that we MUST support both, testing both makes sense.
> 
> Other tests use KT_DEFAULT, which means they currently use
> RSA. In all cases here, I have manually made sure things work
> also for ECDSA.  We could generalize these to tests both RSA
> and RCDSA at some point.
> 
> A final set of tests use KT_RSA, even if I prepared them for
> being more general. These tests have some TLS1.0 and even
> SSL2 assumptions, which means they expect RSA.  These
> protocols are quite obsolete.  We might want to generalize these tests at some
point (perhaps by simply ripping out
> SSL2/TLS1.0 code).

I looked but I didn't see any assumptions on SSL2(ancient!). There are some
mentions of TLS 1.0 but EC cipher suites should still work fine in that context,
AFAIK.

[torbjorng (webrtc), 2015-06-24 08:51:19]

> I looked but I didn't see any assumptions on SSL2(ancient!). There are some
> mentions of TLS 1.0 but EC cipher suites should still work fine in that
context,
> AFAIK.

For OpenSSL (i.e., not Linux or ChromeOS) we have ASSERTs which enforce the
"default cipher" for DTLS 1.0.  This is per webrtc/base/opensslstreamadapter.cc
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA and per nssstreamadapter.cc similar, but
AES_128.

I am not sure that removing the ASSERTs is correct, as the test is set up to
trigger TLS 1.0 where I suppose using elliptic curves is not correct (caveat: I
have not read the TLS 1.0 RFC).

I didn't debug NSS very carefully, but there we hit RSA specific code inside the
lib, in functions prefixed "ssl2".  (This is indeed ancient, and the code in
question only conditionally runs SSL2; it falls back to SSL1 under some
conditions!)

[torbjorng (webrtc), 2015-06-26 13:41:30]

Now the tests pass for both RSA and ECDSA keys. We explicitly
exercise both in one unittest, but leave the other as
KT_DEFAULT.

Please take a look.

[juberti1, 2015-06-26 19:00:52]

On 2015/06/24 at 08:51:19, torbjorng wrote:
> > I looked but I didn't see any assumptions on SSL2(ancient!). There are some
> > mentions of TLS 1.0 but EC cipher suites should still work fine in that
context,
> > AFAIK.
> 
> For OpenSSL (i.e., not Linux or ChromeOS) we have ASSERTs which enforce the
"default cipher" for DTLS 1.0.  This is per webrtc/base/opensslstreamadapter.cc
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA and per nssstreamadapter.cc similar, but
AES_128.
> 
> I am not sure that removing the ASSERTs is correct, as the test is set up to
trigger TLS 1.0 where I suppose using elliptic curves is not correct (caveat: I
have not read the TLS 1.0 RFC).

ECDHE = elliptic curves. So I am very confused by this - you are saying that
using elliptic curves is not correct, when the test is testing that ECDHE is
being used.

Clearly removing the asserts is incorrect. But I think the only thing that needs
to be done here is to make the function that gets the default cipher take
KeyType as an argument. 
 
> I didn't debug NSS very carefully, but there we hit RSA specific code inside
the lib, in functions prefixed "ssl2".  (This is indeed ancient, and the code in
question only conditionally runs SSL2; it falls back to SSL1 under some
conditions!)

I don't think this analysis is correct. The SSL1/2/3 code inside of NSS simply
refers to different generations of APIs, not the protocol versions. (confusing,
of course).

Anyway - I think that for all tests above webrtc/base, we should be using
KT_DEFAULT.

[juberti1, 2015-06-26 19:16:02]

Adding jbauch as CC - he did most of the work on making sure the right cipher
suites are used.

Generally, I think this is pretty close, but the tests still need work in a few
places.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/nssstreamadap...
File webrtc/base/nssstreamadapter.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/nssstreamadap...
webrtc/base/nssstreamadapter.cc:73: TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,
this should be ECDHE

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/nssstreamadap...
webrtc/base/nssstreamadapter.cc:74: TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA
we don't want the AES-256 variant. this should be
 TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/ssladapter_un...
File webrtc/base/ssladapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/ssladapter_un...
webrtc/base/ssladapter_unittest.cc:401: TEST_F(SSLAdapterTestDTLS_RSA,
TestDTLSConnect) {
I think we need to add DTLS with ECDSA tests here.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity.cc
File webrtc/base/sslidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity.c...
webrtc/base/sslidentity.cc:158: KeyType key_type) {
I think it probably makes sense to put KeyType into SSLIdentityParams rather
than having it as a separate field.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity_u...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:49: // Confirmed to work with KT_RSA and
KT_ECDSA.
This needs to specifically test both KT_RSA and KT_ECDSA behavior - using
KT_DEFAULT isn't correct in this particular test.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:64: // Both NSSIdentity::Generate and
OpenSSLIdentity::Generate are
comment is incorrect

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:185: client_identity_ =
rtc::SSLIdentity::Generate("client", rtc::KT_DEFAULT);
KT_DEFAULT isn't appropriate for this test - we need to test each permutation of
{RSA, ECDSA} -> {RSA, ECDSA} for client and server.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:702: return;  // FIXME
???

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:978: client_cipher ==
rtc::SSLStreamAdapter::GetDefaultSslCipher(
GetDefaultSslCipher needs to take KeyType as a parameter.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:996: ASSERT_TRUE(client_cipher ==
"TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA" ||
GetDefaultSslCipher needs to take KeyType as a parameter.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:1014: ASSERT_TRUE(client_cipher ==
"TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA" ||
See above

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:1032: ASSERT_TRUE(client_cipher ==
"TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA" ||
See above

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:617:
TEST_F(DtlsTransportChannelTest, TestTransferDtlsRSA) {
These all seem like they should be KT_DEFAULT (and no change to the test name).
The actual testing of ECDSA/RSA operation is done by the lower-level tests in
sslidentity/adapter_unittest.

[joachim, 2015-06-30 20:15:24]

In general it would be nice to be able to pass the curve as a parameter instead
of having it hardcoded somewhere inside the sslstreamadapter implementations,
but that is probably out of scope for this CL.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:978: client_cipher ==
rtc::SSLStreamAdapter::GetDefaultSslCipher(
On 2015/06/26 19:16:02, juberti1 wrote:
> GetDefaultSslCipher needs to take KeyType as a parameter.

Yes, please don't hardcode cipher names in the tests but let the
SSLStreamAdapter decice which cipher to use for the protocol-version/key-type
combination.

[juberti1, 2015-07-01 02:34:08]

On 2015/06/30 at 20:15:24, jbauch wrote:
> In general it would be nice to be able to pass the curve as a parameter
instead of having it hardcoded somewhere inside the sslstreamadapter
implementations, but that is probably out of scope for this CL.

Agree - let's not increase scope in this CL.

[torbjorng (webrtc), 2015-07-02 12:35:08]

This should address all issues brought up.

The OpenSSL PFS issue with ECDSA seems to be gone.
Unfortunately, I am not sure which change made the
difference and this bothers me.

A preexisting issue is that we tend to use AES-256 with
BoringSSL when using RSA.  We use AES-128 with NSS.  The
same pattern is followed with ECDSA (as the logic behind
this is from the SSL_CTX_set_cipher_list string used in
opensslstreamadapter.cc).  Considering that
GetDefaultSslCipher then agrees that AES-256 is used,
it is not clear if the use of AES-256 is intentional or
not.  (Unless we move to 256-bit security strength for the
key exchange/certs, using anything stronger than AES-128
seems a bit silly.)

Please take another look.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/nssstreamadap...
File webrtc/base/nssstreamadapter.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/nssstreamadap...
webrtc/base/nssstreamadapter.cc:73: TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,
On 2015/06/26 19:16:01, juberti1 wrote:
> this should be ECDHE

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/nssstreamadap...
webrtc/base/nssstreamadapter.cc:74: TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA
On 2015/06/26 19:16:01, juberti1 wrote:
> we don't want the AES-256 variant. this should be
>  TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/ssladapter_un...
File webrtc/base/ssladapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/ssladapter_un...
webrtc/base/ssladapter_unittest.cc:401: TEST_F(SSLAdapterTestDTLS_RSA,
TestDTLSConnect) {
On 2015/06/26 19:16:01, juberti1 wrote:
> I think we need to add DTLS with ECDSA tests here.

Oops.  (I suppose this could use TEST_P here for a slight code size
reduction and to simplify future key types.)

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity.cc
File webrtc/base/sslidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity.c...
webrtc/base/sslidentity.cc:158: KeyType key_type) {
On 2015/06/26 19:16:01, juberti1 wrote:
> I think it probably makes sense to put KeyType into SSLIdentityParams rather
> than having it as a separate field.

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity_u...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:49: // Confirmed to work with KT_RSA and
KT_ECDSA.
On 2015/06/26 19:16:01, juberti1 wrote:
> This needs to specifically test both KT_RSA and KT_ECDSA behavior - using
> KT_DEFAULT isn't correct in this particular test.

OK.  I have fixed this and cleaned things up to avoid making the
code unreadable.

Added a hardwired EC key/cert in PEM format too.  Unfortunately, I
couldn't find any NSS function for reading it (see added code comments),
so this is used only for BoringSSL.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:185: client_identity_ =
rtc::SSLIdentity::Generate("client", rtc::KT_DEFAULT);
On 2015/06/26 19:16:02, juberti1 wrote:
> KT_DEFAULT isn't appropriate for this test - we need to test each permutation
of
> {RSA, ECDSA} -> {RSA, ECDSA} for client and server.

Makes sense. I enable testing using TEST_P for all most tests in this file.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:702: return;  // FIXME
On 2015/06/26 19:16:02, juberti1 wrote:
> ???

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:996: ASSERT_TRUE(client_cipher ==
"TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA" ||
On 2015/06/26 19:16:02, juberti1 wrote:
> GetDefaultSslCipher needs to take KeyType as a parameter.

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:1014: ASSERT_TRUE(client_cipher ==
"TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA" ||
On 2015/06/26 19:16:02, juberti1 wrote:
> See above

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:1032: ASSERT_TRUE(client_cipher ==
"TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA" ||
On 2015/06/26 19:16:02, juberti1 wrote:
> See above

Done.

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:617:
TEST_F(DtlsTransportChannelTest, TestTransferDtlsRSA) {
On 2015/06/26 19:16:02, juberti1 wrote:
> These all seem like they should be KT_DEFAULT (and no change to the test
name).
> The actual testing of ECDSA/RSA operation is done by the lower-level tests in
> sslidentity/adapter_unittest.

Done.

[joachim, 2015-07-02 22:44:01]

On 2015/07/02 12:35:08, torbjorng1 wrote:
> A preexisting issue is that we tend to use AES-256 with
> BoringSSL when using RSA.  We use AES-128 with NSS.  The
> same pattern is followed with ECDSA (as the logic behind
> this is from the SSL_CTX_set_cipher_list string used in
> opensslstreamadapter.cc).  Considering that
> GetDefaultSslCipher then agrees that AES-256 is used,
> it is not clear if the use of AES-256 is intentional or
> not.  (Unless we move to 256-bit security strength for the
> key exchange/certs, using anything stronger than AES-128
> seems a bit silly.)

davidben@ commented on the ordering of cipher suites in OpenSSL/BoringSSL here
(comment 8 and following):
https://webrtc-codereview.appspot.com/50989004/#msg8

Quote: "Upstream OpenSSL's default cipher order is... kind of dumb. I think
it'll actually prefer AES_256_CBC because they unconditionally strength sort."

[juberti1, 2015-07-03 03:15:19]

Overall this looks really good - the use of TEST_P is a good idea.

A few notes, mainly in sslidentity_unittest. Nothing major.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/nssstreamadap...
File webrtc/base/nssstreamadapter.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/nssstreamadap...
webrtc/base/nssstreamadapter.cc:1121: return nullptr;
nullptr is not a valid string. You can return std::string() instead.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:1166: return nullptr;
Use std::string() instead

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:91: typedef unsigned char digest_type[64]; 
// 64 is the largest hash used here.
proper style for this would be DigestType

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:100: memset(digest[0], 0, expected_len);
This seems like a lot of redundant code. Suggest extracting a helper routine
that calls ComputeDigest twice on a supplied cert and verifies it gets the right
length and same result. Then you can call that routine 5x from this function.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:132: // Sanity check that all four digests
are unique.  This could theoretically
This seems like it might not be all that valuable (although the chance of a
failure is remarkably low).

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:142: if (expected_digest) {
This is only used in one case, so suggest factoring this out into its own
function.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:155: rtc::scoped_ptr<SSLIdentity>
identityrsa1_;
identiy_rsa1_, etc would be more readable

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter.h:171: KeyType key_type = KT_DEFAULT);
Default parameters are frowned upon; please add a TODO here to clean up whatever
tests require it.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:997: rtc::SSL_PROTOCOL_DTLS_10,
::testing::get<1>(GetParam())),
Isn't <1> the server_key_type? You are using it when comparing the
client_cipher, which is confusing (here and below)/

[torbjorng (webrtc), 2015-07-06 10:11:55]

Thanks for the feedback!  This CL update addresses all the issues brought up.

Please take another look.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/nssstreamadap...
File webrtc/base/nssstreamadapter.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/nssstreamadap...
webrtc/base/nssstreamadapter.cc:1121: return nullptr;
On 2015/07/03 03:15:18, juberti1 wrote:
> nullptr is not a valid string. You can return std::string() instead.

Done.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:1166: return nullptr;
On 2015/07/03 03:15:18, juberti1 wrote:
> Use std::string() instead

Done.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:91: typedef unsigned char digest_type[64]; 
// 64 is the largest hash used here.
On 2015/07/03 03:15:18, juberti1 wrote:
> proper style for this would be DigestType

Done.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:100: memset(digest[0], 0, expected_len);
On 2015/07/03 03:15:18, juberti1 wrote:
> This seems like a lot of redundant code. Suggest extracting a helper routine
> that calls ComputeDigest twice on a supplied cert and verifies it gets the
right
> length and same result. Then you can call that routine 5x from this function.

Done.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:132: // Sanity check that all four digests
are unique.  This could theoretically
On 2015/07/03 03:15:18, juberti1 wrote:
> This seems like it might not be all that valuable (although the chance of a
> failure is remarkably low).

I inherited this check from the old code, I only generalised it.

It only catches seriously misbehaved functions. A real collision will
only happen if random generation is badly broken so that two keys
actually come out the same.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:142: if (expected_digest) {
On 2015/07/03 03:15:18, juberti1 wrote:
> This is only used in one case, so suggest factoring this out into its own
> function.

Done.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslidentity_u...
webrtc/base/sslidentity_unittest.cc:155: rtc::scoped_ptr<SSLIdentity>
identityrsa1_;
On 2015/07/03 03:15:18, juberti1 wrote:
> identiy_rsa1_, etc would be more readable

Done.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter.h:171: KeyType key_type = KT_DEFAULT);
On 2015/07/03 03:15:19, juberti1 wrote:
> Default parameters are frowned upon; please add a TODO here to clean up
whatever
> tests require it.

Done.

Incidentally, the ssl unit tests use default params in several other places
since before.

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:997: rtc::SSL_PROTOCOL_DTLS_10,
::testing::get<1>(GetParam())),
On 2015/07/03 03:15:19, juberti1 wrote:
> Isn't <1> the server_key_type? You are using it when comparing the
> client_cipher, which is confusing (here and below)/

Indeed confusing.  Now we compare to server_cipher instead
(which equals client_cipher).

[hbos, 2015-07-07 13:04:36]

lgtm

In terms of general code quality. (The crypto stuff is black magic to me and I
don't have an opinion about that or most of the testing.)

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/opensslident...
File webrtc/base/opensslidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/opensslident...
webrtc/base/opensslidentity.cc:75: }
nit: Assuming this is analogously to the RSA case and makes sense, add comment
"// ownership of ec_key struct was assigned, don't free it."?

[juberti1, 2015-07-21 22:54:13]

lgtm with changes below

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:42: : identity_rsa1_(),
These don't need to be initialized.

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:89: typedef unsigned char DigestType[64]; 
// 64 is the largest hash used here.
You can use MessageDigest::kMaxSize instead of 64 here

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:115: void TestDigest(const std::string&
algorithm, size_t expected_len) {
I would call this TestDigestForGeneratedCerts, or something like that

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:118: ASSERT_TRUE(expected_len <= 64);
Suggest using <= sizeof(digest[0]), to avoid accidental mistakes

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:137: void TestDigestExample(const
std::string& algorithm,
I would call this TestDigestForFixedCert

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:144: ASSERT_TRUE(expected_len <= 64);
<= sizeof(digest)

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:162: TestDigestExample(rtc::DIGEST_SHA_1,
20, kTestCertSha1);
I would have a call to both TestDigestForGeneratedCert and
TestDigestForFixedCert here.
You could also do the same for the other hash functions; here are the hashes for
kTestCert:
SHA224(a.bin)= d4cec6cf28cbe9773836cfb13b4ad7bdae242108cf6a440d3f942a5b
SHA256(a.bin)= 416bb49347797724770b8b2ea62be0f90aed1f31a6f75ca15ac4b0a2a478b976
SHA384(a.bin)=
42319a791dd608bf3bba36d8374a9a75d3256e2892be06b7c5a083e386b103fc6447d6d8aad9366004ccbe7d6ae83449
SHA512(a.bin)=
511dec023d5145d3d81da49d43c9ee326f4f37eeab3f25df72fc611ad592ff6b287158b3e18a18cf61330e14c304aa07f6a5dadc42422235ce26584a336dbcb6

[juberti1, 2015-08-13 20:47:38]

On 2015/07/21 22:54:13, juberti1 wrote:
> lgtm with changes below
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> File webrtc/base/sslidentity_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:42: : identity_rsa1_(),
> These don't need to be initialized.
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:89: typedef unsigned char DigestType[64]; 
> // 64 is the largest hash used here.
> You can use MessageDigest::kMaxSize instead of 64 here
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:115: void TestDigest(const std::string&
> algorithm, size_t expected_len) {
> I would call this TestDigestForGeneratedCerts, or something like that
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:118: ASSERT_TRUE(expected_len <= 64);
> Suggest using <= sizeof(digest[0]), to avoid accidental mistakes
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:137: void TestDigestExample(const
> std::string& algorithm,
> I would call this TestDigestForFixedCert
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:144: ASSERT_TRUE(expected_len <= 64);
> <= sizeof(digest)
> 
>
https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
> webrtc/base/sslidentity_unittest.cc:162: TestDigestExample(rtc::DIGEST_SHA_1,
> 20, kTestCertSha1);
> I would have a call to both TestDigestForGeneratedCert and
> TestDigestForFixedCert here.
> You could also do the same for the other hash functions; here are the hashes
for
> kTestCert:
> SHA224(a.bin)= d4cec6cf28cbe9773836cfb13b4ad7bdae242108cf6a440d3f942a5b
> SHA256(a.bin)=
416bb49347797724770b8b2ea62be0f90aed1f31a6f75ca15ac4b0a2a478b976
> SHA384(a.bin)=
>
42319a791dd608bf3bba36d8374a9a75d3256e2892be06b7c5a083e386b103fc6447d6d8aad9366004ccbe7d6ae83449
> SHA512(a.bin)=
>
511dec023d5145d3d81da49d43c9ee326f4f37eeab3f25df72fc611ad592ff6b287158b3e18a18cf61330e14c304aa07f6a5dadc42422235ce26584a336dbcb6

What are the next steps here?

[torbjorng (webrtc), 2015-08-17 11:28:24]

Patchset #8 (id:140001) has been deleted

[torbjorng (webrtc), 2015-08-17 12:09:17]

Committed patchset #8 (id:160001) manually as
b6d4ec418504fd947c6f96829c73180e9487e203 (presubmit successful).

[torbjorng (webrtc), 2015-08-17 12:12:45]

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/opensslident...
File webrtc/base/opensslidentity.cc (right):

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/opensslident...
webrtc/base/opensslidentity.cc:75: }
On 2015/07/07 13:04:36, hbos wrote:
> nit: Assuming this is analogously to the RSA case and makes sense, add comment
> "// ownership of ec_key struct was assigned, don't free it."?

Good catch! (I added a comment.)

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:42: : identity_rsa1_(),
On 2015/07/21 22:54:13, juberti1 wrote:
> These don't need to be initialized.

Done.

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:89: typedef unsigned char DigestType[64]; 
// 64 is the largest hash used here.
On 2015/07/21 22:54:13, juberti1 wrote:
> You can use MessageDigest::kMaxSize instead of 64 here

Done.

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:115: void TestDigest(const std::string&
algorithm, size_t expected_len) {
On 2015/07/21 22:54:13, juberti1 wrote:
> I would call this TestDigestForGeneratedCerts, or something like that

Done.

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:118: ASSERT_TRUE(expected_len <= 64);
On 2015/07/21 22:54:13, juberti1 wrote:
> Suggest using <= sizeof(digest[0]), to avoid accidental mistakes

Done, except using sizeof(DigestType).

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:137: void TestDigestExample(const
std::string& algorithm,
On 2015/07/21 22:54:13, juberti1 wrote:
> I would call this TestDigestForFixedCert

Done.

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:144: ASSERT_TRUE(expected_len <= 64);
On 2015/07/21 22:54:13, juberti1 wrote:
> <= sizeof(digest)

Done.

https://codereview.webrtc.org/1189583002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:162: TestDigestExample(rtc::DIGEST_SHA_1,
20, kTestCertSha1);
On 2015/07/21 22:54:13, juberti1 wrote:
> I would have a call to both TestDigestForGeneratedCert and
> TestDigestForFixedCert here.

Using more hashes checking fixed data makes sense. I've completed the set using
your hashes (thanks!).

I'm afraid don't understand what TestDigestForGeneratedCert would do here.
Adding test_cert_ checking to TestDigestForGeneratedCert could be done.
Comparing hashes with identities with hashes for a cert is somewhat odd, though.

The (pre-existing) code of this test could perhaps take some cleanups. Adding
fixed RSA/EC identities (as opposed to fixed certs) in
TestDigestForGeneratedCert, makes sense.  Using the identities and certs from
the end of this file for that purpose and let them replace kTestCertificate also
makes some sense. IMHO, that's another CL.

[Nico, 2015-08-17 19:22:30]

This doesn't build:

FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/webrtc/base/rtc_base.ssladapter.armv7.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -DDISABLE_NACL -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=242792-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_CONFIGURATION_POLICY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA
-DFIELDTRIAL_TESTING_ENABLED -DDISABLE_FTP_SUPPORT=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DWEBRTC_RESTRICT_LOGGING -DEXPAT_RELATIVE_PATH
-DWEBRTC_CHROMIUM_BUILD -DLOGGING_INSIDE_WEBRTC -DWEBRTC_POSIX -DWEBRTC_MAC
-DWEBRTC_IOS -DFEATURE_ENABLE_SSL -DLOGGING=1 -DNO_MAIN_THREAD_WRAPPING
-DSSL_USE_NSS -DHAVE_NSS_SSL_H -DSSL_USE_NSS_RNG -DCARBON_DEPRECATED=YES
-DNO_NSPR_10_SUPPORT -DNSPR_STATIC -DNSS_STATIC -DNSS_USE_STATIC_LIBS
-DUSE_UTIL_DIRECTLY -DUSE_LIBPCI=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-DNS_BLOCK_ASSERTIONS=1 -D_FORTIFY_SOURCE=2 -Igen -I../..
-I../../third_party/webrtc/overrides -I../../third_party
-I../../third_party/third_party/jsoncpp/overrides/include
-I../../third_party/third_party/jsoncpp/source/include
-I../../third_party/boringssl/src/include -I../../net/third_party/nss/ssl
-I../../third_party/nss/nspr/pr/include -I../../third_party/nss/nspr/lib/ds
-I../../third_party/nss/nspr/lib/libc/include
-I../../third_party/nss/nss/lib/base -I../../third_party/nss/nss/lib/certdb
-I../../third_party/nss/nss/lib/certhigh
-I../../third_party/nss/nss/lib/cryptohi -I../../third_party/nss/nss/lib/dev
-I../../third_party/nss/nss/lib/freebl
-I../../third_party/nss/nss/lib/freebl/ecl -I../../third_party/nss/nss/lib/nss
-I../../third_party/nss/nss/lib/pk11wrap -I../../third_party/nss/nss/lib/pkcs7
-I../../third_party/nss/nss/lib/pki -I../../third_party/nss/nss/lib/smime
-I../../third_party/nss/nss/lib/softoken -I../../third_party/nss/nss/lib/util
-I../../third_party/nss/nss/lib/ckfw/builtins -isysroot
/Applications/Xcode6.3.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS8.3.sdk
-Os -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-miphoneos-version-min=7.0 -arch armv7 -Wall -Wendif-labels -Wextra
-Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wheader-hygiene -Wno-char-subscripts
-Wno-unneeded-internal-declaration -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register
-Wno-inconsistent-missing-override -Wno-shift-negative-value -std=c++11
-stdlib=libc++ -fno-rtti -fno-exceptions -fvisibility-inlines-hidden
-fno-threadsafe-statics -Xclang -load -Xclang
/b/build/slave/ios_rel_device_ninja/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics  -c
../../third_party/webrtc/base/ssladapter.cc -o
obj/third_party/webrtc/base/rtc_base.ssladapter.armv7.o
In file included from ../../third_party/webrtc/base/ssladapter.cc:29:
In file included from ../../third_party/webrtc/base/nssstreamadapter.h:30:
../../third_party/webrtc/base/nssidentity.h:43:28: error: field 'pubkey_' will
be initialized after field 'ssl_kea_type_' [-Werror,-Wreorder]
      : privkey_(privkey), pubkey_(pubkey), ssl_kea_type_(ssl_kea_null) {}
                           ^
../../third_party/webrtc/base/nssidentity.h:47:28: error: field 'pubkey_' will
be initialized after field 'ssl_kea_type_' [-Werror,-Wreorder]
      : privkey_(privkey), pubkey_(pubkey), ssl_kea_type_(ssl_kea_type) {}