Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1682623002:
Disable the TLS version fallback. (Closed)
DescriptionDisable the TLS version fallback.
This sets the default minimum TLS fallback version to TLS 1.2. The code is
retained for now to support a resurrected SSLVersionFallbackMin admin policy.
The policy is set to expire in Chrome 53, matching the timeline for the
previous fallback removal. As an escape hatch (but I don't expect to need it),
it's also connected to a field trial.
This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely
gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION
hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code
to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of
showing the error code. This will decrease the amount of time it takes to show
an error page in some cases.
The ssl_version_fallback_min toggle is also tweaked to reject all values below
TLS 1.1, so that the resurrected admin policy cannot be used to set the value
at TLS 1.0 again. (Though it would be moot due to the above change.)
We'll also want to add a link to some to-be-written Help Center article on the
error page, but that'll be done separately after chatting with UI folks.
BUG=536200, 583787
Committed: https://crrev.com/3b26751ff0ac3ca5d1377616b55d0284673dc232
Cr-Commit-Position: refs/heads/master@{#377352}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : fix policy_browsertest #
Total comments: 7
Patch Set 5 : comment #Patch Set 6 : base::FeatureList #
Total comments: 6
Patch Set 7 : comments #
Total comments: 4
Patch Set 8 : atwilson comments #Messages
Total messages: 33 (8 generated)
davidben@chromium.org changed reviewers: + rsleevi@chromium.org
Will add OWNERS if you're happy with this.
Description was changed from ========== Remove the TLS version fallback. This sets the default minimum TLS fallback version to TLS 1.2. The code is retained for now to support a resurrected SSLVersionFallbackMin admin policy. The policy is set to expire in Chrome 53, matching the timeline for the previous fallback removal. As an escape hatch (but I don't expect to need it), it's also connected to a field trial. This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of showing the error code. This will decrease the amount of time it takes to show an error page in some cases. The ssl_version_fallback_min toggle is also tweaked to reject all values below TLS 1.1, so that the resurrected admin policy cannot be used to set the value at TLS 1.0 again. (Though it would be moot due to the above change.) We'll also want to add a link to some to-be-written Help Center article on the error page, but that'll be done separately after chatting with UI folks. BUG=536200,583787 ========== to ========== Disable the TLS version fallback. This sets the default minimum TLS fallback version to TLS 1.2. The code is retained for now to support a resurrected SSLVersionFallbackMin admin policy. The policy is set to expire in Chrome 53, matching the timeline for the previous fallback removal. As an escape hatch (but I don't expect to need it), it's also connected to a field trial. This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of showing the error code. This will decrease the amount of time it takes to show an error page in some cases. The ssl_version_fallback_min toggle is also tweaked to reject all values below TLS 1.1, so that the resurrected admin policy cannot be used to set the value at TLS 1.0 again. (Though it would be moot due to the above change.) We'll also want to add a link to some to-be-written Help Center article on the error page, but that'll be done separately after chatting with UI folks. BUG=536200,583787 ==========
https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); I think you want to indirect this through a function; AIUI, on the first call (e.g. before Finch seeds are fetched), this simply returns "Default" - is that what you expect?
https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); On 2016/02/09 02:16:12, Ryan Sleevi wrote: > I think you want to indirect this through a function; AIUI, on the first call > (e.g. before Finch seeds are fetched), this simply returns "Default" - is that > what you expect? I believe these get called at a point where fetch state is seeded. (Otherwise the RC4 one wouldn't work.) I don't know if it's possible to tell the pref system to indirect through a function since it needs to know when the value changed. Other code does something similar: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...
https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); (I believe it's the empty string, not "Default".)
https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); On 2016/02/09 02:58:36, davidben wrote: > I believe these get called at a point where fetch state is seeded. (Otherwise > the RC4 one wouldn't work.) I don't believe the RC4 one would necessarily work correctly. That was certainly my (negative) experience with handling things like certificate expiration policies or EV whitelists. I'm not terribly keen to couple with the Finch state. Just looking through code search, this seems *wrong* to me. Perhaps check with one of the metrics/prefs owners?
https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); On 2016/02/09 03:28:43, Ryan Sleevi wrote: > On 2016/02/09 02:58:36, davidben wrote: > > I believe these get called at a point where fetch state is seeded. (Otherwise > > the RC4 one wouldn't work.) > > I don't believe the RC4 one would necessarily work correctly. That was certainly > my (negative) experience with handling things like certificate expiration > policies or EV whitelists. I'm not terribly keen to couple with the Finch state. It works. It's pretty easy to test this with --force-fieldtrials. I tested the various cases extensively before uploading, just as with RC4. That was why I did RC4 this way rather than in RegisterPrefs which would be more natural. Doing it in RegisterPrefs is the one that doesn't work. This code runs in CreateDefaultManager which is called right next to all the QUIC field trial bits in the link earlier. > Just looking through code search, this seems *wrong* to me. Perhaps check with > one of the metrics/prefs owners? Yup. They reviewed the RC4 code and I'll add them to this CL as well when getting the rest of the OWNERS in.
https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/1682623002/diff/40001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref_unittest.cc:177: // Tests that fallback beyond TLS 1.0 cannot be re-enabled. And if they force a field trial via --force-fieldtrial?
On 2016/02/09 05:11:56, davidben wrote: > It works. It's pretty easy to test this with --force-fieldtrials. I tested the > various cases extensively before uploading, just as with RC4. That was why I did > RC4 this way rather than in RegisterPrefs which would be more natural. Doing it > in RegisterPrefs is the one that doesn't work. Well, yes, --force-fieldtrials works because it does just that, it forces it. Put differently, when a new Chrome profile launches, it has no seed state. The field trials do not take effect until the next restart after this state is queried. Could this prevent such state from being queried - thus preventing a remote kill switch? I don't know - one way or the other. But that was the concern. Separately, I was concerned over the fact that it returns an empty-string (or, potentially, any server value string) into the pref, which means it is difficult to test that if a remote flag were set, that there wasn't an issue (for example, a typo). But I suppose the alternative - for example, CHECK()ing to make sure the pref-value is valid - might be equally prone to disaster. Everything else mechnically LG, although I'm a little curious what's up with the fact that it would seem "tls1" can still be forced in some places, leading to perhaps inconsistencies.
davidben@chromium.org changed reviewers: + asvitkine@chromium.org, atwilson@chromium.org
+atwilson for chrome/browser/policy and components/policy +asvitkine for field trials question below https://codereview.chromium.org/1682623002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1682623002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2733: const std::string new_value("tls1.1"); (This changed because the default is now tls1.2.) https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); +asvitkine, could you confirm that this works for configuring field trials? It gets called after ChromeBrowserMainParts::SetupMetricsAndFieldTrials and works with --force-fieldtrials. Are there other considerations here? The intent is to have an emergency off switch in case this deprecation doesn't work out.
Chatted over IM, I read the code wrong, LGTM
https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:200: // TODO(davidben): Remove this when the fallback removal has succeeded. Nit: Can you make the comment reference a crbug? https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); On 2016/02/10 22:14:29, davidben wrote: > +asvitkine, could you confirm that this works for configuring field trials? It > gets called after ChromeBrowserMainParts::SetupMetricsAndFieldTrials and works > with --force-fieldtrials. Are there other considerations here? > > The intent is to have an emergency off switch in case this deprecation doesn't > work out. Yes this should work. However, I think it would be better to use variations params here (go/variations-params) - as this relies on the name of the field trial group, which often times proves to be insufficiently flexible (i.e. can't have two groups with the same name). But if it's just an emergency kill switch that you expect to either be not set or turned on at 100% if there's an issue, then maybe this is fine. Up to you, I guess.
https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:200: // TODO(davidben): Remove this when the fallback removal has succeeded. On 2016/02/11 16:24:06, Alexei Svitkine wrote: > Nit: Can you make the comment reference a crbug? Done. https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); On 2016/02/11 16:24:06, Alexei Svitkine wrote: > On 2016/02/10 22:14:29, davidben wrote: > > +asvitkine, could you confirm that this works for configuring field trials? It > > gets called after ChromeBrowserMainParts::SetupMetricsAndFieldTrials and works > > with --force-fieldtrials. Are there other considerations here? > > > > The intent is to have an emergency off switch in case this deprecation doesn't > > work out. > > Yes this should work. However, I think it would be better to use variations > params here (go/variations-params) - as this relies on the name of the field > trial group, which often times proves to be insufficiently flexible (i.e. can't > have two groups with the same name). > > But if it's just an emergency kill switch that you expect to either be not set > or turned on at 100% if there's an issue, then maybe this is fine. Up to you, I > guess. Hrm. Are you suggesting I instead do something like. std::map<std::string, std::string> params; if (variations::GetVariationParams("SSLVersionFallbackMin", ¶ms)) { local_state->SetDefaultPrefValue( ssl_config::prefs::kSSLVersionFallbackMin, new base::StringValue(params["version"])); } I'm not sure what I'd do with that extra flexibility. It's only ever going to control this one toggle. It really may as well be a boolean, actually. This just seemed easier. Should I be using base::FeatureList instead? And, yeah, it's really just an emergency kill switch. This is mostly just insurance and so some particularly risk-averse people stop grumping at me. I think it's overkill (numbers are 10x below usual removal cutoff), but I'd previously told them I'd do it, so I probably should. :-)
https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/60001/components/ssl_config/s... components/ssl_config/ssl_config_service_manager_pref.cc:204: base::FieldTrialList::FindFullName("SSLVersionFallbackMin"))); On 2016/02/11 21:53:14, davidben wrote: > On 2016/02/11 16:24:06, Alexei Svitkine wrote: > > On 2016/02/10 22:14:29, davidben wrote: > > > +asvitkine, could you confirm that this works for configuring field trials? > It > > > gets called after ChromeBrowserMainParts::SetupMetricsAndFieldTrials and > works > > > with --force-fieldtrials. Are there other considerations here? > > > > > > The intent is to have an emergency off switch in case this deprecation > doesn't > > > work out. > > > > Yes this should work. However, I think it would be better to use variations > > params here (go/variations-params) - as this relies on the name of the field > > trial group, which often times proves to be insufficiently flexible (i.e. > can't > > have two groups with the same name). > > > > But if it's just an emergency kill switch that you expect to either be not set > > or turned on at 100% if there's an issue, then maybe this is fine. Up to you, > I > > guess. > > Hrm. Are you suggesting I instead do something like. > > std::map<std::string, std::string> params; > if (variations::GetVariationParams("SSLVersionFallbackMin", ¶ms)) { > local_state->SetDefaultPrefValue( > ssl_config::prefs::kSSLVersionFallbackMin, > new base::StringValue(params["version"])); > } > > I'm not sure what I'd do with that extra flexibility. It's only ever going to > control this one toggle. It really may as well be a boolean, actually. This just > seemed easier. Should I be using base::FeatureList instead? > > And, yeah, it's really just an emergency kill switch. This is mostly just > insurance and so some particularly risk-averse people stop grumping at me. I > think it's overkill (numbers are 10x below usual removal cutoff), but I'd > previously told them I'd do it, so I probably should. :-) If it's just a boolean, I would indeed suggest using base::FeatureList - and hardcode the actual values of the prefs in the client. FeatureList was designed to support these kinds of kill switches and does support extra flexibility by default.
Switched to base::Feature and added a test.
tnagel@chromium.org changed reviewers: + tnagel@chromium.org
https://codereview.chromium.org/1682623002/diff/100001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1682623002/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:7873: If this policy is not configured then <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> no longer performs this fallback. Note this does not disable support for older TLS versions, only whether <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will work around buggy servers which cannot negotiate versions correctly. I'd suggest: 'If this policy is not configured' --> 'If this policy is not configured or if it is set to "tls1.2"' https://codereview.chromium.org/1682623002/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:7875: Otherwise it may be set to one of the following values: "tls1.1" or "tls1.2". If compatibility with a buggy server must be maintained, this may be set to "tls1.1". This is a stopgap measure and the server should be rapidly fixed.''', In my opinion it would be cleaner to drop 'tls1.2' from that paragraph and only mention it in the previous paragraph, see comment above.
lgtm use of feature API lgtm % comment https://codereview.chromium.org/1682623002/diff/100001/components/ssl_config/... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/100001/components/ssl_config/... components/ssl_config/ssl_config_service_manager_pref.cc:92: const base::Feature kSSLVersionFallbackTLSv11 = { Nit: No =
https://codereview.chromium.org/1682623002/diff/100001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1682623002/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:7873: If this policy is not configured then <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> no longer performs this fallback. Note this does not disable support for older TLS versions, only whether <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will work around buggy servers which cannot negotiate versions correctly. On 2016/02/15 14:30:23, Thiemo Nagel wrote: > I'd suggest: 'If this policy is not configured' --> 'If this policy is not > configured or if it is set to "tls1.2"' Done. https://codereview.chromium.org/1682623002/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:7875: Otherwise it may be set to one of the following values: "tls1.1" or "tls1.2". If compatibility with a buggy server must be maintained, this may be set to "tls1.1". This is a stopgap measure and the server should be rapidly fixed.''', On 2016/02/15 14:30:23, Thiemo Nagel wrote: > In my opinion it would be cleaner to drop 'tls1.2' from that paragraph and only > mention it in the previous paragraph, see comment above. Done. That's much better, thanks! https://codereview.chromium.org/1682623002/diff/100001/components/ssl_config/... File components/ssl_config/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1682623002/diff/100001/components/ssl_config/... components/ssl_config/ssl_config_service_manager_pref.cc:92: const base::Feature kSSLVersionFallbackTLSv11 = { On 2016/02/16 16:25:04, Alexei Svitkine wrote: > Nit: No = Done.
tnagel: friendly ping
On 2016/02/22 19:05:36, davidben wrote: > tnagel: friendly ping Also friendly ping to atwilson because I need coverage on chrome/browser/policy/policy_browsertest.cc.
On 2016/02/23 21:28:01, davidben wrote: > On 2016/02/22 19:05:36, davidben wrote: > > tnagel: friendly ping > > Also friendly ping to atwilson because I need coverage on > chrome/browser/policy/policy_browsertest.cc. Thank you for updating the description. LGTM policy_templates.json
policy/* lgtm with a couple minor nits https://codereview.chromium.org/1682623002/diff/120001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1682623002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:7873: If this policy is not configured or if it set to "tls1.2" then <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> no longer performs this fallback. Note this does not disable support for older TLS versions, only whether <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will work around buggy servers which cannot negotiate versions correctly. nit: if it is set to https://codereview.chromium.org/1682623002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:7875: Otherwise, if compatibility with a buggy server must be maintained, it may be set to "tls1.1". This is a stopgap measure and the server should be rapidly fixed.''', nit: it -> this policy
https://codereview.chromium.org/1682623002/diff/120001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1682623002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:7873: If this policy is not configured or if it set to "tls1.2" then <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> no longer performs this fallback. Note this does not disable support for older TLS versions, only whether <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will work around buggy servers which cannot negotiate versions correctly. On 2016/02/24 16:02:29, Andrew T Wilson (Slow) wrote: > nit: if it is set to Done. https://codereview.chromium.org/1682623002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:7875: Otherwise, if compatibility with a buggy server must be maintained, it may be set to "tls1.1". This is a stopgap measure and the server should be rapidly fixed.''', On 2016/02/24 16:02:29, Andrew T Wilson (Slow) wrote: > nit: it -> this policy Done.
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, asvitkine@chromium.org, tnagel@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1682623002/#ps140001 (title: "atwilson comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1682623002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1682623002/140001
Message was sent while issue was closed.
Description was changed from ========== Disable the TLS version fallback. This sets the default minimum TLS fallback version to TLS 1.2. The code is retained for now to support a resurrected SSLVersionFallbackMin admin policy. The policy is set to expire in Chrome 53, matching the timeline for the previous fallback removal. As an escape hatch (but I don't expect to need it), it's also connected to a field trial. This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of showing the error code. This will decrease the amount of time it takes to show an error page in some cases. The ssl_version_fallback_min toggle is also tweaked to reject all values below TLS 1.1, so that the resurrected admin policy cannot be used to set the value at TLS 1.0 again. (Though it would be moot due to the above change.) We'll also want to add a link to some to-be-written Help Center article on the error page, but that'll be done separately after chatting with UI folks. BUG=536200,583787 ========== to ========== Disable the TLS version fallback. This sets the default minimum TLS fallback version to TLS 1.2. The code is retained for now to support a resurrected SSLVersionFallbackMin admin policy. The policy is set to expire in Chrome 53, matching the timeline for the previous fallback removal. As an escape hatch (but I don't expect to need it), it's also connected to a field trial. This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of showing the error code. This will decrease the amount of time it takes to show an error page in some cases. The ssl_version_fallback_min toggle is also tweaked to reject all values below TLS 1.1, so that the resurrected admin policy cannot be used to set the value at TLS 1.0 again. (Though it would be moot due to the above change.) We'll also want to add a link to some to-be-written Help Center article on the error page, but that'll be done separately after chatting with UI folks. BUG=536200,583787 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Disable the TLS version fallback. This sets the default minimum TLS fallback version to TLS 1.2. The code is retained for now to support a resurrected SSLVersionFallbackMin admin policy. The policy is set to expire in Chrome 53, matching the timeline for the previous fallback removal. As an escape hatch (but I don't expect to need it), it's also connected to a field trial. This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of showing the error code. This will decrease the amount of time it takes to show an error page in some cases. The ssl_version_fallback_min toggle is also tweaked to reject all values below TLS 1.1, so that the resurrected admin policy cannot be used to set the value at TLS 1.0 again. (Though it would be moot due to the above change.) We'll also want to add a link to some to-be-written Help Center article on the error page, but that'll be done separately after chatting with UI folks. BUG=536200,583787 ========== to ========== Disable the TLS version fallback. This sets the default minimum TLS fallback version to TLS 1.2. The code is retained for now to support a resurrected SSLVersionFallbackMin admin policy. The policy is set to expire in Chrome 53, matching the timeline for the previous fallback removal. As an escape hatch (but I don't expect to need it), it's also connected to a field trial. This also tweaks the fallback code. The TLS 1.0 fallback leg is now completely gone (the admin policy expired) and ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION hits have leveled off (see Net.ErrorCodesForMainFrame3), cap the fallback code to TLS 1.1. We will no longer even try TLS 1.0 ClientHellos for the purposes of showing the error code. This will decrease the amount of time it takes to show an error page in some cases. The ssl_version_fallback_min toggle is also tweaked to reject all values below TLS 1.1, so that the resurrected admin policy cannot be used to set the value at TLS 1.0 again. (Though it would be moot due to the above change.) We'll also want to add a link to some to-be-written Help Center article on the error page, but that'll be done separately after chatting with UI folks. BUG=536200,583787 Committed: https://crrev.com/3b26751ff0ac3ca5d1377616b55d0284673dc232 Cr-Commit-Position: refs/heads/master@{#377352} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/3b26751ff0ac3ca5d1377616b55d0284673dc232 Cr-Commit-Position: refs/heads/master@{#377352} |
