Route key_exchange_group over to DevTools.

Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented and a second change made.)

BUG=639421, 618035
Committed: https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c
Cr-Commit-Position: refs/heads/master@{#417527}

[davidben, 2016-08-23 14:57:19]

Description was changed from

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field 'Key Exchange Group'.

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key exchanges.
(https://crbug.com/639495)

TODO upload screenshots.

BUG=639421,618035
==========

to

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

TODO upload screenshots.

BUG=639421,618035
==========

[davidben, 2016-08-23 14:57:26]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 14:57:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 18:02:45]

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

[commit-bot: I haz the power, 2016-08-23 18:02:48]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[davidben, 2016-08-25 18:59:19]

Description was changed from

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

TODO upload screenshots.

BUG=639421,618035
==========

to

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented.)

BUG=639421,618035
==========

[davidben, 2016-08-25 18:59:31]

Description was changed from

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented.)

BUG=639421,618035
==========

to

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented and a second change made.)

BUG=639421,618035
==========

[davidben, 2016-08-25 19:26:53]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 19:27:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-08-25 19:28:49]

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

[davidben, 2016-08-25 19:28:50]

This is going to need all kinds of reviewers, but let's start with Lucas for the
initial review.

[commit-bot: I haz the power, 2016-08-25 20:59:56]

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

[commit-bot: I haz the power, 2016-08-25 20:59:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[davidben, 2016-08-26 22:18:25]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 22:19:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

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

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

Dry run: This issue passed the CQ dry run.

[lgarron, 2016-08-30 22:13:51]

LGTM

I had a bunch of possible nits ready (e.g. make sure to check whether
keyExchangeGroup before adding it to the table in DevTools), but all of them are
properly addressed. :-)

https://codereview.chromium.org/2272603003/diff/60001/net/ssl/ssl_info.cc
File net/ssl/ssl_info.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/net/ssl/ssl_info.cc#new...
net/ssl/ssl_info.cc:78: // key_exchange_info is sometimes the (EC)DH group ID
and sometimes a
That makes is sound like you can end up with a grinning cat emoji. :-P
How about "key_exchange_info either the (EC)DH group ID or a synthesized name
for the key exchange, depending on the version of TLS."?

[davidben, 2016-08-30 22:24:26]

https://codereview.chromium.org/2272603003/diff/60001/net/ssl/ssl_info.cc
File net/ssl/ssl_info.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/net/ssl/ssl_info.cc#new...
net/ssl/ssl_info.cc:78: // key_exchange_info is sometimes the (EC)DH group ID
and sometimes a
On 2016/08/30 22:13:51, lgarron wrote:
> That makes is sound like you can end up with a grinning cat emoji. :-P
> How about "key_exchange_info either the (EC)DH group ID or a synthesized name
> for the key exchange, depending on the version of TLS."?

Cat emoji is... basically right. For DHE ciphers, it's the number of bits in the
group. For FFDHE in TLS 1.3 which we won't implement, it's back to being the
group ID. For static RSA it was, at one point, the number of bits in the RSA key
and then I took that out because it was useless.

key_exchange_info was just a mistake. :-)

[davidben, 2016-08-30 23:00:23]

davidben@chromium.org changed reviewers:
+ estark@chromium.org, nasko@chromium.org, pfeldman@chromium.org,
rch@chromium.org, rsleevi@chromium.org

[davidben, 2016-08-30 23:00:23]

Okay, now for OWNERS-gathering.

+pfeldman: third_party/WebKit/
+estark: chrome/browser/ssl and components/security_state
+nasko: content

And then probably I should have the two Ryans sanity-check the bits of net I
changed.
+rch: net/quic
+rsleevi: net/ssl

[Ryan Hamilton, 2016-08-30 23:11:55]

net/quic/ lgtm

[Ryan Sleevi, 2016-08-30 23:19:02]

I read through the //net/ssl bit, and
https://bugs.chromium.org/p/chromium/issues/detail?id=639421 , and I just want
to make sure I'm on the same page for what your plan is

1) Introduce GetKeyExchangeGroup (this CL)
2) Update everyone to use GKEG (this CL)
3) Remove key_exchange_info [when DHE goes away for good]
4) Introduce new struct member that does the needful?


And the reason for #1 is because legacy callers/serialization (e.g. those in 3)
set key_exchange_info, and key_exchange_group is really dependent on the value
of key_exchange_info (at present), so you would need to update
key_exchange_group every time you change key_exchange_info, and that's error
prone and annoying at present. Hence why if you can get to #3, then once you've
removed the ability for people to set key_exchange_info, you can just
compute/set key_exchange_group directly.

Is that a correct restating of the concerns/timelines? If so, that LGTM, but if
I'm missing some plan (e.g. to keep GKEG around), then we should chat, because
it feels like it'd be a long-term smell, but a short-term wise refactoring.

[estark, 2016-08-30 23:23:56]

components/security_state and chrome/browser/ssl lgtm w/ a question about a test

https://codereview.chromium.org/2272603003/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:123: if
(security_info.key_exchange_group != 0) {
How do you feel about adding a test to exercise this code path? (There's a unit
test file now with the obnoxiously long name
chrome_security_state_model_client_unittest.cc, though code search doesn't seem
to have picked it up yet -- or a browser test would work too.)

I dunno, though; maybe it's silly to test that every string is produced
correctly.

[davidben, 2016-08-30 23:25:54]

On 2016/08/30 23:19:02, Ryan Sleevi (slow) wrote:
> I read through the //net/ssl bit, and
> https://bugs.chromium.org/p/chromium/issues/detail?id=639421 , and I just want
> to make sure I'm on the same page for what your plan is
> 
> 1) Introduce GetKeyExchangeGroup (this CL)
> 2) Update everyone to use GKEG (this CL)
> 3) Remove key_exchange_info [when DHE goes away for good]

(Actually the only blocker is taking away that histogram to tell us group sizes.
Perhaps I should just do that. It was semi-useful to prove that going beyond
1024-bit was indeed hopeless, but probably not useful going forwards.)

> 4) Introduce new struct member that does the needful?
> 
> 
> And the reason for #1 is because legacy callers/serialization (e.g. those in
3)
> set key_exchange_info, and key_exchange_group is really dependent on the value
> of key_exchange_info (at present), so you would need to update
> key_exchange_group every time you change key_exchange_info, and that's error
> prone and annoying at present. Hence why if you can get to #3, then once
you've
> removed the ability for people to set key_exchange_info, you can just
> compute/set key_exchange_group directly.
> 
> Is that a correct restating of the concerns/timelines? If so, that LGTM, but
if
> I'm missing some plan (e.g. to keep GKEG around), then we should chat, because
> it feels like it'd be a long-term smell, but a short-term wise refactoring.

Yeah, #1 is mostly because I didn't want to change the serialization format or
anything in this CL or have it store both of them. But that's right.
key_exchange_group is a just a subset of key_exchange_info except that it's zero
when key_exchange_info tries to stuff other values in there to make it
confusing. So when key_exchange_info is gone, GetKeyExchangeGroup will be
replaced with a key_exchange_group member.

That we've serialized key_exchange_info is a bit of a nuisance. I'm thinking
we'll want to either change the format or, in the parser, discard some
key_exchange_info values indefinitely based on the cipher suite check. Dunno if
there's a way around that one. Fundamentally making sense of a key_exchange_info
value require this increasingly complex check, so we either throw away past
values or try to continue to make sense of them.

[nasko, 2016-09-01 17:15:22]

content/ LGTM

https://codereview.chromium.org/2272603003/diff/60001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:53: std::string
SerializeSecurityInfo(const SSLStatus& ssl_status) {
The result of this is never persisted on disk, right?

[davidben, 2016-09-02 21:46:10]

Add test

[davidben, 2016-09-02 21:47:29]

Comment

[davidben, 2016-09-02 21:48:05]

https://codereview.chromium.org/2272603003/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:123: if
(security_info.key_exchange_group != 0) {
On 2016/08/30 23:23:56, estark (OOO through Sept 12) wrote:
> How do you feel about adding a test to exercise this code path? (There's a
unit
> test file now with the obnoxiously long name
> chrome_security_state_model_client_unittest.cc, though code search doesn't
seem
> to have picked it up yet -- or a browser test would work too.)
> 
> I dunno, though; maybe it's silly to test that every string is produced
> correctly.

Done. lgarron, since Emily's OOO, could you look over the test and tweak to
AddConnectionExplanation (assuming try bots are happy...)?

https://codereview.chromium.org/2272603003/diff/60001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:53: std::string
SerializeSecurityInfo(const SSLStatus& ssl_status) {
On 2016/09/01 17:15:22, nasko (slow) wrote:
> The result of this is never persisted on disk, right?

I don't think so, no. (We've done other incompatible changes before.) It's an
IPC thing.

[davidben, 2016-09-02 21:49:52]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 21:50:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lgarron, 2016-09-02 22:45:33]

https://codereview.chromium.org/2272603003/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/2272603003/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:123: if
(security_info.key_exchange_group != 0) {
On 2016/09/02 at 21:48:05, davidben wrote:
> On 2016/08/30 23:23:56, estark (OOO through Sept 12) wrote:
> > How do you feel about adding a test to exercise this code path? (There's a
unit
> > test file now with the obnoxiously long name
> > chrome_security_state_model_client_unittest.cc, though code search doesn't
seem
> > to have picked it up yet -- or a browser test would work too.)
> > 
> > I dunno, though; maybe it's silly to test that every string is produced
> > correctly.
> 
> Done. lgarron, since Emily's OOO, could you look over the test and tweak to
AddConnectionExplanation (assuming try bots are happy...)?

LGTM.

https://codereview.chromium.org/2272603003/diff/100001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2272603003/diff/100001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:185: if
(security_info.key_exchange_group != 0) {
Hmm, I specifically wanted to avoid branching in this function, to avoid the
chance of dead code paths, and to reduce code duplication with the thing being
tested. (Hence assuming/checking that the MAC is NULL instead of handling that
case.)

Does this actually get hit? If so, then that sounds fine to me.

[davidben, 2016-09-02 22:46:56]

https://codereview.chromium.org/2272603003/diff/100001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2272603003/diff/100001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:185: if
(security_info.key_exchange_group != 0) {
On 2016/09/02 22:45:33, lgarron wrote:
> Hmm, I specifically wanted to avoid branching in this function, to avoid the
> chance of dead code paths, and to reduce code duplication with the thing being
> tested. (Hence assuming/checking that the MAC is NULL instead of handling that
> case.)
> 
> Does this actually get hit? If so, then that sounds fine to me.

Yeah, it definitely gets hit. That's what prompted the unit test. We're going to
need more branching later anyway for TLS 1.3 stuff.

[commit-bot: I haz the power, 2016-09-03 00:37:07]

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

[commit-bot: I haz the power, 2016-09-03 00:37:08]

Dry run: Exceeded global retry quota

[davidben, 2016-09-06 18:06:23]

pfeldman: friendly ping

[pfeldman, 2016-09-06 19:38:24]

lgtm

[davidben, 2016-09-06 19:39:49]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-09-06 19:39:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, rch@chromium.org, nasko@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2272603003/#ps100001
(title: "Comment")

[commit-bot: I haz the power, 2016-09-06 19:40:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 21:30:19]

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

[commit-bot: I haz the power, 2016-09-06 21:30:21]

Failed to apply patch for content/child/web_url_loader_impl.cc:
While running git apply --index -3 -p1;
  error: patch failed: content/child/web_url_loader_impl.cc:256
  Falling back to three-way merge...
  Applied patch to 'content/child/web_url_loader_impl.cc' with conflicts.
  U content/child/web_url_loader_impl.cc

Patch:       content/child/web_url_loader_impl.cc
Index: content/child/web_url_loader_impl.cc
diff --git a/content/child/web_url_loader_impl.cc
b/content/child/web_url_loader_impl.cc
index
17da0e103519d484dec9df5c4376e17e589aa2f0..08cc6369d72b73f21ab958a309b92e2c6cea5e8b
100644
--- a/content/child/web_url_loader_impl.cc
+++ b/content/child/web_url_loader_impl.cc
@@ -4,6 +4,7 @@
 
 #include "content/child/web_url_loader_impl.h"
 
+#include <openssl/ssl.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ -256,6 +257,16 @@ void SetSecurityStyleAndDetails(const GURL& url,
     mac = "";
   }
 
+  const char* key_exchange_group = "";
+  if (ssl_status.key_exchange_group != 0) {
+    // Historically the field was named 'curve' rather than 'group'.
+    key_exchange_group = SSL_get_curve_name(ssl_status.key_exchange_group);
+    if (!key_exchange_group) {
+      NOTREACHED();
+      key_exchange_group = "";
+    }
+  }
+
   blink::WebURLResponse::SecurityStyle securityStyle =
       WebURLResponse::SecurityStyleUnknown;
   switch (ssl_status.security_style) {
@@ -286,8 +297,8 @@ void SetSecurityStyleAndDetails(const GURL& url,
 
   blink::WebURLResponse::WebSecurityDetails webSecurityDetails(
       WebString::fromUTF8(protocol), WebString::fromUTF8(key_exchange),
-      WebString::fromUTF8(cipher), WebString::fromUTF8(mac),
ssl_status.cert_id,
-      sct_list);
+      WebString::fromUTF8(key_exchange_group), WebString::fromUTF8(cipher),
+      WebString::fromUTF8(mac), ssl_status.cert_id, sct_list);
 
   response->setSecurityDetails(webSecurityDetails);
 }

[davidben, 2016-09-06 22:14:46]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-09-06 22:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, rsleevi@chromium.org, rch@chromium.org,
nasko@chromium.org, estark@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2272603003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-06 22:15:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 22:30:25]

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

[commit-bot: I haz the power, 2016-09-06 22:30:27]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[davidben, 2016-09-06 23:01:42]

even more plumbing / mismerge

[davidben, 2016-09-06 23:04:07]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[davidben, 2016-09-06 23:04:54]

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

[davidben, 2016-09-06 23:04:55]

+jam, mind looking over the delta between patch sets 7 and 8 to make sure this
is consistent with your recent change? Thanks!

[commit-bot: I haz the power, 2016-09-06 23:05:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 01:04:34]

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

[commit-bot: I haz the power, 2016-09-07 01:04:35]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-08 20:31:15]

lgtm, sorry I missed this earlier

[davidben, 2016-09-08 20:31:57]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-09-08 20:31:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, rsleevi@chromium.org, rch@chromium.org,
nasko@chromium.org, estark@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2272603003/#ps140001
(title: "even more plumbing / mismerge")

[commit-bot: I haz the power, 2016-09-08 20:32:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 20:35:58]

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

[commit-bot: I haz the power, 2016-09-08 20:36:00]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[davidben, 2016-09-08 20:54:39]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-09-08 20:54:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, rsleevi@chromium.org, jam@chromium.org, rch@chromium.org,
nasko@chromium.org, estark@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2272603003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-08 20:55:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 00:57:46]

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

[commit-bot: I haz the power, 2016-09-09 00:57:48]

Failed to apply patch for
chrome/browser/ssl/chrome_security_state_model_client.cc:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/browser/ssl/chrome_security_state_model_client.cc:92
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/ssl/chrome_security_state_model_client.cc'
with conflicts.
  U chrome/browser/ssl/chrome_security_state_model_client.cc

Patch:       chrome/browser/ssl/chrome_security_state_model_client.cc
Index: chrome/browser/ssl/chrome_security_state_model_client.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client.cc
b/chrome/browser/ssl/chrome_security_state_model_client.cc
index
290e004ad7268df2843e7bd77d6c173bc4cd0b26..9aea3f240663ed43be862e7794a01df853f65524
100644
--- a/chrome/browser/ssl/chrome_security_state_model_client.cc
+++ b/chrome/browser/ssl/chrome_security_state_model_client.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ssl/chrome_security_state_model_client.h"
 
+#include <openssl/ssl.h>
+
 #include <vector>
 
 #include "base/command_line.h"
@@ -92,8 +94,8 @@ void AddConnectionExplanation(
 
   // Avoid showing TLS details when we couldn't even establish a TLS connection
   // (e.g. for net errors) or if there was no real connection (some tests). We
-  // check the |cert_id| to see if there was a connection.
-  if (security_info.cert_id == 0 || security_info.connection_status == 0) {
+  // check the |connection_status| to see if there was a connection.
+  if (security_info.connection_status == 0) {
     return;
   }
 
@@ -116,6 +118,19 @@ void AddConnectionExplanation(
                     : l10n_util::GetStringFUTF16(IDS_CIPHER_WITH_MAC,
                                                  base::ASCIIToUTF16(cipher),
                                                  base::ASCIIToUTF16(mac));
+
+  // Include the key exchange group (previously known as curve) if specified.
+  //
+  // TODO(davidben): When TLS 1.3's new negotiation is implemented, omit the
+  // "key exchange" if empty and only display the group, which is the true key
+  // exchange. See https://crbug.com/639495.
+  if (security_info.key_exchange_group != 0) {
+    key_exchange_name = l10n_util::GetStringFUTF16(
+        IDS_SSL_KEY_EXCHANGE_WITH_GROUP, key_exchange_name,
+        base::ASCIIToUTF16(
+            SSL_get_curve_name(security_info.key_exchange_group)));
+  }
+
   if (security_info.obsolete_ssl_status == net::OBSOLETE_SSL_NONE) {
     security_style_explanations->secure_explanations.push_back(
         content::SecurityStyleExplanation(
@@ -360,6 +375,7 @@ void
ChromeSecurityStateModelClient::GetVisibleSecurityState(
   state->cert_id = ssl.cert_id;
   state->cert_status = ssl.cert_status;
   state->connection_status = ssl.connection_status;
+  state->key_exchange_group = ssl.key_exchange_group;
   state->security_bits = ssl.security_bits;
   state->pkp_bypassed = ssl.pkp_bypassed;
   state->sct_verify_statuses.clear();

[davidben, 2016-09-09 02:39:36]

rebase again

[davidben, 2016-09-09 02:40:15]

jam, the number of times this CL has collided with one of your patches... ;-)

[davidben, 2016-09-09 02:40:22]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-09-09 02:40:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, rsleevi@chromium.org, jam@chromium.org, rch@chromium.org,
nasko@chromium.org, estark@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2272603003/#ps180001
(title: "rebase again")

[commit-bot: I haz the power, 2016-09-09 02:40:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 07:38:20]

Description was changed from

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented and a second change made.)

BUG=639421,618035
==========

to

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented and a second change made.)

BUG=639421,618035
==========

[commit-bot: I haz the power, 2016-09-09 07:38:22]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-09 07:41:36]

Description was changed from

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented and a second change made.)

BUG=639421,618035
==========

to

==========
Route key_exchange_group over to DevTools.

This replaces key_exchange_info with key_exchange_group at all layers
but net::SSLInfo, fixing key_exchange_info's type issues. Then we route
it over to DevTools as an optional new field "Key Exchange Group".

Later work, once the TLS 1.3 draft 15 negotiation is implemented, will
make the DevTools able to handle missing key_exchange. In 1.3, what
DevTools currently calls a "Key Exchange" isn't really meaningful and
is just described by the group. (https://crbug.com/639495)

Screenshots:
https://drive.google.com/folderview?id=0Bz14lOW5Hke4VFlBVXJOY0Z0cUU&usp=sharing

(Note: the TLS 1.3 picture is a mock of how it will look when the
new cipher suite negotiation is implemented and a second change made.)

BUG=639421,618035

Committed: https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c
Cr-Commit-Position: refs/heads/master@{#417527}
==========

[commit-bot: I haz the power, 2016-09-09 07:41:38]

Patchset 10 (id:??) landed as
https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c
Cr-Commit-Position: refs/heads/master@{#417527}

[dmurph, 2016-09-09 17:36:18]

On 2016/09/09 07:41:38, commit-bot: I haz the power wrote:
> Patchset 10 (id:??) landed as
> https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c
> Cr-Commit-Position: refs/heads/master@{#417527}

I believe this is causing the MSAN bot to crash
https://bugs.chromium.org/p/chromium/issues/detail?id=645514

reverting

[dmurph, 2016-09-09 20:21:03]

On 2016/09/09 17:36:18, dmurph wrote:
> On 2016/09/09 07:41:38, commit-bot: I haz the power wrote:
> > Patchset 10 (id:??) landed as
> > https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c
> > Cr-Commit-Position: refs/heads/master@{#417527}
> 
> I believe this is causing the MSAN bot to crash
> https://bugs.chromium.org/p/chromium/issues/detail?id=645514
> 
> reverting

update: not reverting, fix is out, see bug.