Update with new default boringssl no-aes cipher suites. Re-enable tests.

Update with new default boringssl no-aes cipher suites. Re-enable tests.

This undoes https://codereview.webrtc.org/1533253002 (except the DEPS part).

BUG=webrtc:5381
R=davidben@webrtc.org, henrika@webrtc.org

Committed: https://crrev.com/31c8d2eac5aec977f584ab0ae5a1d457d674f101
Cr-Commit-Position: refs/heads/master@{#11250}

[torbjorng (webrtc), 2015-12-28 14:30:44]

The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-28 14:30:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550773002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550773002/1

[henrika_webrtc, 2015-12-28 15:17:57]

LGTM % (the fact that I don't know these parts)

[torbjorng (webrtc), 2015-12-28 15:20:50]

Description was changed from

==========
Update with new default boringssl no-aes cipher suites. Re-enable tests.

BUG=webrtc:5381
==========

to

==========
Update with new default boringssl no-aes cipher suites. Re-enable tests.

This undoes https://codereview.webrtc.org/1533253002 (except the DEPS part).

BUG=webrtc:5381
==========

[torbjorng (webrtc), 2015-12-28 15:20:51]

torbjorng@webrtc.org changed reviewers:
+ davidben@webrtc.org, henrika@webrtc.org

[commit-bot: I haz the power, 2015-12-28 15:25:54]

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

[commit-bot: I haz the power, 2015-12-28 15:25:54]

Dry run: This issue passed the CQ dry run.

[davidben_webrtc, 2016-01-06 04:04:45]

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

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:164: #ifdef
TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
Do you actually need this? Surely by now DEPS rolls should be such that you can
assume the new one?

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:168:
static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256);
So, this is fine, but it seems poor that you'd need to be so over-fit to
BoringSSL's defaults. Why even have the test at all? (There's some chance we'll
have a last-minute change to this too since the IETF's been mumbling about
switching the PRF from SHA256 to SHA384 or something.)

[torbjorng (webrtc), 2016-01-13 13:16:47]

OK, I took out the backwards compatibility checks, assuming everyone is at the
bundled boringssl version.

I agree the check that we know the defaults of boringssl are a bit strange.

PTAL, davidben.

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

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:164: #ifdef
TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
On 2016/01/06 04:04:44, davidben_webrtc wrote:
> Do you actually need this? Surely by now DEPS rolls should be such that you
can
> assume the new one?

OK. I added this to for the benefit of third parties I thought may pick up an
installed version of boringssl.  Reverted back now.

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:168:
static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256);
On 2016/01/06 04:04:44, davidben_webrtc wrote:
> So, this is fine, but it seems poor that you'd need to be so over-fit to
> BoringSSL's defaults. Why even have the test at all? (There's some chance
we'll
> have a last-minute change to this too since the IETF's been mumbling about
> switching the PRF from SHA256 to SHA384 or something.)

We might want to clean this up at some point, as well as other parts of the
tests.

[davidben_webrtc, 2016-01-13 20:42:53]

lgtm

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

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:164: #ifdef
TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
On 2016/01/13 13:16:47, torbjorng (webrtc) wrote:
> On 2016/01/06 04:04:44, davidben_webrtc wrote:
> > Do you actually need this? Surely by now DEPS rolls should be such that you
> can
> > assume the new one?
> 
> OK. I added this to for the benefit of third parties I thought may pick up an
> installed version of boringssl.  Reverted back now.

Pre-installed versions of BoringSSL are explicitly not supported. The README
should be pretty clear on this. It should only be used pinned at particular
versions because we don't have any hard promises of ABI/API stability. Just a
soft "if every consumer has to change everything all the time, that's kind of
annoying" API stability.

(Although that soft "stability" is, of course, contingent on consumers not going
out of their way to make changing BoringSSL versions hard, as your test seems to
be doing. ;-) )

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:168:
static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256);
On 2016/01/13 13:16:47, torbjorng (webrtc) wrote:
> On 2016/01/06 04:04:44, davidben_webrtc wrote:
> > So, this is fine, but it seems poor that you'd need to be so over-fit to
> > BoringSSL's defaults. Why even have the test at all? (There's some chance
> we'll
> > have a last-minute change to this too since the IETF's been mumbling about
> > switching the PRF from SHA256 to SHA384 or something.)
> 
> We might want to clean this up at some point, as well as other parts of the
> tests.

Yeah, I would definitely ask that you change this, though I'm fine with this CL
as-is and then you unravel that check as a follow-up. Otherwise we'll probably
play this game again and again in the future.

[torbjorng (webrtc), 2016-01-14 12:05:08]

The CQ bit was checked by torbjorng@webrtc.org

[torbjorng (webrtc), 2016-01-14 12:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1550773002/#ps40001 (title:
"Revert to patchset #1")

[commit-bot: I haz the power, 2016-01-14 12:05:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550773002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550773002/40001

[commit-bot: I haz the power, 2016-01-14 14:05:41]

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

[commit-bot: I haz the power, 2016-01-14 14:05:42]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[torbjorng (webrtc), 2016-01-14 14:18:07]

Description was changed from

==========
Update with new default boringssl no-aes cipher suites. Re-enable tests.

This undoes https://codereview.webrtc.org/1533253002 (except the DEPS part).

BUG=webrtc:5381
==========

to

==========
Update with new default boringssl no-aes cipher suites. Re-enable tests.

This undoes https://codereview.webrtc.org/1533253002 (except the DEPS part).

BUG=webrtc:5381
R=davidben@webrtc.org, henrika@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/31c8d2eac5aec977f584ab0ae...
==========

[commit-bot: I haz the power, 2016-01-14 14:18:08]

Description was changed from

==========
Update with new default boringssl no-aes cipher suites. Re-enable tests.

This undoes https://codereview.webrtc.org/1533253002 (except the DEPS part).

BUG=webrtc:5381
R=davidben@webrtc.org, henrika@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/31c8d2eac5aec977f584ab0ae...
==========

to

==========
Update with new default boringssl no-aes cipher suites. Re-enable tests.

This undoes https://codereview.webrtc.org/1533253002 (except the DEPS part).

BUG=webrtc:5381
R=davidben@webrtc.org, henrika@webrtc.org

Committed: https://crrev.com/31c8d2eac5aec977f584ab0ae5a1d457d674f101
Cr-Commit-Position: refs/heads/master@{#11250}
==========

[torbjorng (webrtc), 2016-01-14 14:18:09]

Committed patchset #3 (id:40001) manually as
31c8d2eac5aec977f584ab0ae5a1d457d674f101 (presubmit successful).

[commit-bot: I haz the power, 2016-01-14 14:18:09]

Patchset 3 (id:??) landed as
https://crrev.com/31c8d2eac5aec977f584ab0ae5a1d457d674f101
Cr-Commit-Position: refs/heads/master@{#11250}

[torbjorng (webrtc), 2016-01-14 14:27:09]

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

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:164: #ifdef
TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
On 2016/01/13 20:42:53, davidben_webrtc wrote:
> On 2016/01/13 13:16:47, torbjorng (webrtc) wrote:
> > On 2016/01/06 04:04:44, davidben_webrtc wrote:
> > > Do you actually need this? Surely by now DEPS rolls should be such that
you
> > can
> > > assume the new one?
> > 
> > OK. I added this to for the benefit of third parties I thought may pick up
an
> > installed version of boringssl.  Reverted back now.
> 
> Pre-installed versions of BoringSSL are explicitly not supported. The README
> should be pretty clear on this. It should only be used pinned at particular
> versions because we don't have any hard promises of ABI/API stability. Just a
> soft "if every consumer has to change everything all the time, that's kind of
> annoying" API stability.
> 
> (Although that soft "stability" is, of course, contingent on consumers not
going
> out of their way to make changing BoringSSL versions hard, as your test seems
to
> be doing. ;-) )

:-)

I cannot take credit for those tests, unfortunately/fortunately.

https://codereview.webrtc.org/1550773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:168:
static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256);
On 2016/01/13 20:42:53, davidben_webrtc wrote:
> On 2016/01/13 13:16:47, torbjorng (webrtc) wrote:
> > On 2016/01/06 04:04:44, davidben_webrtc wrote:
> > > So, this is fine, but it seems poor that you'd need to be so over-fit to
> > > BoringSSL's defaults. Why even have the test at all? (There's some chance
> > we'll
> > > have a last-minute change to this too since the IETF's been mumbling about
> > > switching the PRF from SHA256 to SHA384 or something.)
> > 
> > We might want to clean this up at some point, as well as other parts of the
> > tests.
> 
> Yeah, I would definitely ask that you change this, though I'm fine with this
CL
> as-is and then you unravel that check as a follow-up. Otherwise we'll probably
> play this game again and again in the future.

I suppose somebody added these tests as a test of sanity, and while it has the
now observed disadvantage, sanity checks have their obvious advantages. A better
test would be "did we get a reasonably strong cipher suite?" as default.

[sprang_webrtc, 2016-01-14 17:13:53]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/1586183002/ by sprang@webrtc.org.

The reason for reverting is: We're getting boringssl version conflicts.
Reverting for now..