New issue
Advanced search Search tips

Issue 639421 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 618035



Sign in to add a comment

Replace key_exchange_info with key_exchange_group

Project Member Reported by davidben@chromium.org, Aug 19 2016

Issue description

key_exchange_info was a mistake and makes TLS 1.3 rather messy to route the key exchange out of. The value cannot be interpreted without first querying the cipher suite due to it having different semantics on all sides. Most consumers actually want the ECDHE curve and it's not reasonable for every consumer to know:

- First you look up the cipher suite.
- Now you check if it's ECDHE. If yes, you definitely have an ECDHE curve.
- Otherwise, check if it's a TLS 1.3 AEAD/PRF-only cipher suite. If yes, the value is EITHER an ECDHE curve OR zero if ECDHE wasn't used (pure PSK resumption).
- Otherwise, it is not safe to interpret the value because it's some garbage from legacy ciphers.

We've largely purged it from BoringSSL now but for lingering serialization quirks we're now stuck with. We should to purge it from Chromium too, otherwise every layer's version of net::SSLInfo needs to expose that same check to callers.

The plain RSA branch of it is gone now as the consuming UMA is gone. The DHE branch is only used by a UMA value. Once the DHE removal has stuck we can definitely take that out. Layers beyond net::SSLStatus will need to be converted to ecdhe_curve now or we can't show TLS 1.3's details in DevTools sanely.
 
Summary: Replace key_exchange_info with key_exchange_group (was: Replace key_exchange_info with ecdhe_curve)
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/960cd60330949cbfc0af26e25aa5d061cdec534c

commit 960cd60330949cbfc0af26e25aa5d061cdec534c
Author: davidben <davidben@chromium.org>
Date: Fri Sep 09 07:38:00 2016

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 

Review-Url: https://codereview.chromium.org/2272603003
Cr-Commit-Position: refs/heads/master@{#417527}

[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/chrome/app/generated_resources.grd
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/chrome/browser/ssl/chrome_security_state_model_client.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/chrome/browser/ssl/chrome_security_state_model_client_unittest.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/components/security_state/security_state_model.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/components/security_state/security_state_model.h
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/content/common/resource_messages.h
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/content/public/common/resource_response_info.h
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/content/public/common/ssl_status.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/content/public/common/ssl_status.h
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/net/ssl/ssl_info.cc
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/net/ssl/ssl_info.h
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/Source/core/inspector/browser_protocol.json
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/Source/platform/network/ResourceResponse.cpp
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/Source/platform/network/ResourceResponse.h
[modify] https://crrev.com/960cd60330949cbfc0af26e25aa5d061cdec534c/third_party/WebKit/public/platform/WebURLResponse.h

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a

commit 3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a
Author: davidben <davidben@chromium.org>
Date: Tue Sep 20 14:31:06 2016

Replace key_exchange_info with key_exchange_group.

Narrow to key_exchange_group in net::SSLInfo. The one place where
key_exchange_info remains is serialization code which sadly must filter
out garbage values.

This removes the DHE group size histogram which was the last consumer of
the non-ECDHE values. With DHE behind a fallback and now removed, there
is no need for this histogram.

BUG= 639421 

Review-Url: https://codereview.chromium.org/2349713004
Cr-Commit-Position: refs/heads/master@{#419753}

[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/content/public/browser/ssl_status.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/http/http_response_info.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/http/http_response_info_unittest.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/socket/ssl_client_socket_pool.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/ssl/ssl_cipher_suite_names.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/ssl/ssl_cipher_suite_names.h
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/ssl/ssl_info.cc
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/net/ssl/ssl_info.h
[modify] https://crrev.com/3b00e401f7b0b6ee5ed9df06acf7d3c46b12966a/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Assigned)

Sign in to add a comment