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

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

Created:
2 years ago by davidben
Modified:
2 years 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
2 years 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 ...
2 years 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 ...
2 years 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 ...
2 years 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 ...
2 years ago (2017-04-10 19:10:33 UTC) #13
svaldez
2 years 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 ...
2 years 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: > ...
2 years 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) ...
2 years 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 ...
2 years 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 > ...
2 years ago (2017-04-10 21:15:23 UTC) #21
edwardjung
LGTM, assume this error is obscure and not shown that often.
2 years 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 ...
2 years ago (2017-04-11 18:18:20 UTC) #23
davidben
+mpearson for histograms.xml
2 years 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 ...
2 years 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 ...
2 years 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
2 years ago (2017-04-12 20:49:08 UTC) #30
commit-bot: I haz the power
2 years 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