New issue
Advanced search Search tips

Issue 621780 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Delete the TLS version fallback

Project Member Reported by davidben@chromium.org, Jun 21 2016

Issue description

With master being on M53, the admin policy has expired. The version fallback may be completely deleted!

...except TLS 1.3 looks likely to require it again. (Unless the TLSWG does the smart thing and moves it to an extension.)

But I think that one we should implement at SSLConnectJob to avoid multiplying the size of the socket pools. We'll also have easier access to the SSLClientSocket so it'll be easier to get fine-grained information. We should be very particular about when we fallback (as soon as we've received the ServerHello, don't) so we don't mask bugs again. So let's delete everything but the SSLClientSocket bits and below.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 24 2016

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

commit 84936542b14d71b6c8f301b592263dcec0fb772e
Author: davidben <davidben@chromium.org>
Date: Fri Jun 24 19:04:13 2016

Unwind fallback metrics and SSLFailureState.

Part 2 of removing the TLS version fallback code.

These were of great help, but now that the fallback is dead and gone, we don't
need them anymore. (Until it's time to bring back the fallback for TLS 1.3. :(
But next time let's not require quite so much glue in net/http.)

BUG= 621780 ,621754,614110
TBR=asargent@chromium.org

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

[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/chrome/browser/extensions/api/socket/tls_socket_unittest.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/bidirectional_stream.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/bidirectional_stream.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_network_transaction.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_network_transaction.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_request.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_request.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_request_unittest.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/http/http_stream_factory_test_util.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/net.gypi
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/client_socket_handle.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/client_socket_handle.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/fuzzed_socket_factory.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/socket_test_util.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/socket_test_util.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/ssl_client_socket.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/ssl_client_socket_impl.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/net/socket/ssl_client_socket_pool.cc
[delete] https://crrev.com/014e89fdbe83fd895898331249f9bba28cc71130/net/ssl/ssl_failure_state.h
[modify] https://crrev.com/84936542b14d71b6c8f301b592263dcec0fb772e/tools/metrics/histograms/histograms.xml

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 27 2016

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

commit 4d0b082a62056eed2affe714b07e1324582f7af9
Author: davidben <davidben@chromium.org>
Date: Mon Jun 27 22:04:40 2016

Unwind the fallback admin policy knobs.

Part 1 of removing the TLS version fallback code. Later commits will unwind
other pieces.

The admin policy has expired, so there is no need to keep the code around.

BUG= 621780 

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

[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/chrome/browser/policy/configuration_policy_handler_list_factory.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/chrome/browser/prefs/command_line_pref_store.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/chrome/test/data/policy/policy_test_cases.json
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/policy/resources/policy_templates.json
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/ssl_config/ssl_config_prefs.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/ssl_config/ssl_config_prefs.h
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/ssl_config/ssl_config_service_manager_pref.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/ssl_config/ssl_config_service_manager_pref_unittest.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/ssl_config/ssl_config_switches.cc
[modify] https://crrev.com/4d0b082a62056eed2affe714b07e1324582f7af9/components/ssl_config/ssl_config_switches.h

Status: Fixed (was: Assigned)
Alas, we have deleted as much as we can now, since the fallback is to be resurrected for 1.3. But in a slightly different form this time.
Status: Started (was: Fixed)
Reopening since the fallback is not coming back. A little more code can go.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 6 2016

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

commit 471d313caec220449197a77ae369787f34fbbc47
Author: davidben <davidben@chromium.org>
Date: Thu Oct 06 15:39:36 2016

Remove the last of the TLS fallback code.

It's gone and TLS 1.3 is not going to force it back.
Remove the last of the dead code.

Note this has a small behavior change: if we load a
cached top-level resource which previously used the
fallback and wasn't revalidated since the fallback was
removed, the website_settings.cc logic will no longer
show a message about this. But that message was only
ever shown on Android and only if you went looking for the
connection info.

(There's some other stale code there like downgrading
the connection status on SSLv3, but we may wish to
drop the cache entry instead in that case. Though in
general we don't really do the full provenance tracking
so it's largely a UI thing. Notably revalidations and
range requests will clobber one set of flags with another.
Will poke at that in a follow-up.)

BUG= 621780 

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

[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/chrome/browser/ui/website_settings/website_settings.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/components/error_page/common/localized_error.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/components/pageinfo_strings.grdp
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/base/net_error_list.h
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/socket/ssl_client_socket_unittest.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/openssl_ssl_util.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/ssl_config.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/ssl_config.h
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/ssl_connection_status_flags.h
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/url_request/url_request_unittest.cc

Status: Fixed (was: Started)
Components: Internals>Network>SSL
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/471d313caec220449197a77ae369787f34fbbc47

commit 471d313caec220449197a77ae369787f34fbbc47
Author: davidben <davidben@chromium.org>
Date: Thu Oct 06 15:39:36 2016

Remove the last of the TLS fallback code.

It's gone and TLS 1.3 is not going to force it back.
Remove the last of the dead code.

Note this has a small behavior change: if we load a
cached top-level resource which previously used the
fallback and wasn't revalidated since the fallback was
removed, the website_settings.cc logic will no longer
show a message about this. But that message was only
ever shown on Android and only if you went looking for the
connection info.

(There's some other stale code there like downgrading
the connection status on SSLv3, but we may wish to
drop the cache entry instead in that case. Though in
general we don't really do the full provenance tracking
so it's largely a UI thing. Notably revalidations and
range requests will clobber one set of flags with another.
Will poke at that in a follow-up.)

BUG= 621780 

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

[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/chrome/browser/ui/website_settings/website_settings.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/components/error_page/common/localized_error.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/components/pageinfo_strings.grdp
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/base/net_error_list.h
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/socket/ssl_client_socket_unittest.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/openssl_ssl_util.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/ssl_config.cc
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/ssl_config.h
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/ssl/ssl_connection_status_flags.h
[modify] https://crrev.com/471d313caec220449197a77ae369787f34fbbc47/net/url_request/url_request_unittest.cc

Comment 9 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Sign in to add a comment