Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2800853008: Add a dedicated error code for TLS 1.3 interference. (Closed)

Created:
1 year, 10 months ago by davidben
Modified:
1 year, 10 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, net-reviews_chromium.org, bnc+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a dedicated error code for TLS 1.3 interference. From the previous TLS 1.3 launch attempt, we learned that many firewall, proxy, etc., products are buggy and interfere with TLS 1.3's deployment, holding back a security and performance improvement across the web. To make diagnosing such issues easier, this CL implements a dedicated error code based on a retry probe. On SSL connection failure, if TLS 1.3 was enabled and the error code is one of a handful which, in the past, have potentially signaled version intolerance, we retry the connection with TLS 1.3 disabled. If this connection succeeds, we still reject the connection (otherwise a network attacker can break the security of the version negotiation, cf. POODLE) and return ERR_SSL_VERSION_INTERFERENCE. This error code should hopefully give an easier target for search metrics and others, as we otherwise cannot reliably classify individual errors. Unfortunately, such a probe is inherently flaky and is itself not reliable. This error could mean one of three things: 1. This is a transient network error that will be resolved when the user reloads. 2. The server is buggy and does not implement TLS version negotiation correctly. 3. The user is behind a buggy network middlebox, firewall, or proxy which is interfering with TLS 1.3. Based on server side probes, the lack of TLS 1.3 error reports until it was enabled on the server, and a protocol change in TLS 1.3 intended to avoid this, we do not believe (2) is common. (The difference between (2) and (3) is whether the servers or middleboxes are at fault here.) (1) is unavoidable. There is no way to reliably distinguish (1) and (3). We can only make (1) less and less likely by spamming the user's network with probes, which is undesirable. Accordingly, though the error string is short and easily searchable, I have left the network error page fairly non-descript, borrowing from the ERR_CONNECTION_FAILED text, but with SUGGEST_PROXY_CONFIG and friends enabled, to hint that users should, if their default reaction of mashing reload (or the auto-reload feature) doesn't work, look there. Screentshot: https://drive.google.com/open?id=0B2ImyA6KAoPULVp3V0xPVEJHQms BUG=694593, 658863 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2800853008 Cr-Commit-Position: refs/heads/master@{#464173} Committed: https://chromium.googlesource.com/chromium/src/+/095ebb57de0053925c4900bace0458f38bf5e051

Patch Set 1 #

Patch Set 2 : shrink CL #

Total comments: 14

Patch Set 3 : svaldez and mmenke comments #

Total comments: 2

Patch Set 4 : Reorder comment. #

Total comments: 2

Patch Set 5 : mpearson comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -21 lines) Patch
M components/error_page/common/localized_error.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M net/base/net_error_list.h View 1 2 3 1 chunk +14 lines, -0 lines 0 comments Download
M net/log/net_log_event_type_list.h View 1 1 chunk +9 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_impl.cc View 1 2 2 chunks +10 lines, -9 lines 0 comments Download
M net/socket/ssl_client_socket_pool.h View 1 2 2 chunks +8 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_pool.cc View 1 2 7 chunks +60 lines, -4 lines 0 comments Download
M net/socket/ssl_client_socket_unittest.cc View 1 chunk +31 lines, -0 lines 0 comments Download
M net/ssl/ssl_config.h View 1 chunk +6 lines, -0 lines 0 comments Download
M net/ssl/ssl_config.cc View 1 chunk +1 line, -0 lines 0 comments Download
M net/test/spawned_test_server/base_test_server.h View 1 chunk +2 lines, -1 line 0 comments Download
M net/tools/testserver/testserver.py View 1 chunk +1 line, -0 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 6 chunks +52 lines, -5 lines 0 comments Download
M third_party/tlslite/README.chromium View 1 chunk +1 line, -0 lines 0 comments Download
A third_party/tlslite/patches/tls13_intolerance.patch View 1 chunk +66 lines, -0 lines 0 comments Download
M third_party/tlslite/tlslite/constants.py View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/tlslite/tlslite/messages.py View 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/tlslite/tlslite/tlsconnection.py View 2 chunks +10 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 3 chunks +20 lines, -0 lines 0 comments Download

Messages

Total messages: 33 (15 generated)
davidben
1 year, 10 months ago (2017-04-10 18:21:05 UTC) #8
mmenke
I'm a bit hosed the next two weeks. Could you have someone else review the ...
1 year, 10 months ago (2017-04-10 18:39:25 UTC) #9
davidben
On 2017/04/10 18:39:25, mmenke wrote: > I'm a bit hosed the next two weeks. Could ...
1 year, 10 months ago (2017-04-10 18:53:01 UTC) #11
mmenke
https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_pool.cc File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_pool.cc#newcode333 net/socket/ssl_client_socket_pool.cc:333: ssl_config.version_interference_probe = true; It seems a little bit silly ...
1 year, 10 months ago (2017-04-10 19:08:46 UTC) #12
svaldez
https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_impl.cc File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_impl.cc#newcode1690 net/socket/ssl_client_socket_impl.cc:1690: result.push_back('1'); Change these numbers to be different if you ...
1 year, 10 months ago (2017-04-10 19:10:33 UTC) #13
svaldez
1 year, 10 months ago (2017-04-10 19:10:34 UTC) #14
davidben
https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_impl.cc File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_impl.cc#newcode1690 net/socket/ssl_client_socket_impl.cc:1690: result.push_back('1'); On 2017/04/10 19:10:32, svaldez wrote: > Change these ...
1 year, 10 months ago (2017-04-10 19:52:25 UTC) #16
mmenke
https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_pool.cc File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2800853008/diff/20001/net/socket/ssl_client_socket_pool.cc#newcode333 net/socket/ssl_client_socket_pool.cc:333: ssl_config.version_interference_probe = true; On 2017/04/10 19:52:25, davidben wrote: > ...
1 year, 10 months ago (2017-04-10 19:55:06 UTC) #18
svaldez
lgtm https://codereview.chromium.org/2800853008/diff/40001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2800853008/diff/40001/net/base/net_error_list.h#newcode395 net/base/net_error_list.h:395: // Maybe reorder this in expected order: 1) ...
1 year, 10 months ago (2017-04-10 20:40:02 UTC) #19
davidben
https://codereview.chromium.org/2800853008/diff/40001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2800853008/diff/40001/net/base/net_error_list.h#newcode395 net/base/net_error_list.h:395: // On 2017/04/10 20:40:02, svaldez wrote: > Maybe reorder ...
1 year, 10 months ago (2017-04-10 21:07:36 UTC) #20
mmenke
On 2017/04/10 21:07:36, davidben wrote: > https://codereview.chromium.org/2800853008/diff/40001/net/base/net_error_list.h > File net/base/net_error_list.h (right): > > https://codereview.chromium.org/2800853008/diff/40001/net/base/net_error_list.h#newcode395 > ...
1 year, 10 months ago (2017-04-10 21:15:23 UTC) #21
edwardjung
LGTM, assume this error is obscure and not shown that often.
1 year, 10 months ago (2017-04-11 10:04:55 UTC) #22
davidben
On 2017/04/11 10:04:55, edwardjung wrote: > LGTM, assume this error is obscure and not shown ...
1 year, 10 months ago (2017-04-11 18:18:20 UTC) #23
davidben
+mpearson for histograms.xml
1 year, 10 months ago (2017-04-11 18:19:04 UTC) #25
Mark P
histograms.xml lgtm with one minor comment Thanks for pinging me. This was easier to review ...
1 year, 10 months ago (2017-04-12 20:04:10 UTC) #26
davidben
https://codereview.chromium.org/2800853008/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2800853008/diff/60001/tools/metrics/histograms/histograms.xml#newcode38149 tools/metrics/histograms/histograms.xml:38149: + For each SSL version interference probe, what network ...
1 year, 10 months ago (2017-04-12 20:47:55 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2800853008/80001
1 year, 10 months ago (2017-04-12 20:49:08 UTC) #30
commit-bot: I haz the power
1 year, 10 months ago (2017-04-12 22:24:39 UTC) #33
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/095ebb57de0053925c4900bace04...

Powered by Google App Engine
This is Rietveld 408576698