Remove DHE.

Remove DHE.

Connections to sites which require DHE will, for the moment,
return ERR_SSL_OBSOLETE_CIPHER. As always, there is an admin policy
to temporarily re-enable and field trial to back out of it if
needed.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.

Committed: https://crrev.com/b4c25b632a7078d2c3346a37b51034bb853b24b3
Cr-Commit-Position: refs/heads/master@{#401793}

[davidben, 2016-06-14 22:07:47]

Description was changed from

==========
Remove DHE.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.
==========

to

==========
Remove DHE.

Connections to sites which require DHE will, for the moment,
return ERR_SSL_OBSOLETE_CIPHER. As always, there is an admin policy
to temporarily re-enable and field trial to back out of it if
needed.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.
==========

[davidben, 2016-06-14 22:08:35]

davidben@chromium.org changed reviewers:
+ edwardjung@chromium.org, rsleevi@chromium.org, tnagel@chromium.org

[davidben, 2016-06-14 22:08:36]

I'm going to need more OWNERS in the second iteration, but since I'm still
waiting on the third LGTM in the Intent thread, you three are probably a good
first pass:

rsleevi: net
tnagel: policy-related files
edwardjung: error page UI question below

https://codereview.chromium.org/2056343006/diff/40001/components/error_page/c...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2056343006/diff/40001/components/error_page/c...
components/error_page/common/localized_error.cc:374:
IDS_ERRORPAGES_DETAILS_SSL_VERSION_OR_CIPHER_MISMATCH,
edwardjung: I intentionally just reused the SSL_VERSION_OR_CIPHER_MISMATCH
strings since I gather you all don't like having much error-specific strings
with all the detailed IDS_ERRORPAGES_DETAILS_* strings no longer being used.

Let me know if you'd rather we have a different error string here.
SUGGEST_UNSUPPORTED_CIPHER is accurate, but we could say something more specific
if we wanted.

[davidben, 2016-06-14 22:09:01]

On 2016/06/14 22:08:36, davidben wrote:
> I'm going to need more OWNERS in the second iteration, but since I'm still
> waiting on the third LGTM in the Intent thread, you three are probably a good
> first pass:
> 
> rsleevi: net

Sorry, net and components/ssl_config.

[Ryan Sleevi, 2016-06-15 01:23:25]

//net & //components LGTM % nit

https://codereview.chromium.org/2056343006/diff/40001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/2056343006/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref_unittest.cc:205:
base::FeatureList::ClearInstanceForTesting();
Doesn't the end of this test leave DHE enabled? Why not toggle it via the Prefs?


Caveat: I'm not a fan of FeatureList, and from looking at
ClearInstanceForTesting() call sites, I feel my hate is a bit justified. Most
consumers - other than a few, such as ScopedSubresourceFilterFeatureToggle,
TestBlinkWebUnitTestSupport, and VariationsSeedProcessorTest - end up leaving
the feature test enabled, and are relying on the side-effect that flaky tests
will be retried in new processes.

I'm inclined to suggest a test-level scoper (like ScopedSubresource...) to
handle this. You could get away with just

ScopedClosureRunner
cleanup(base::Bind(&base::FeatureList::ClearInstanceForTesting));

at the top, followed by line 205.

[davidben, 2016-06-15 19:21:21]

https://codereview.chromium.org/2056343006/diff/40001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/2056343006/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref_unittest.cc:205:
base::FeatureList::ClearInstanceForTesting();
On 2016/06/15 01:23:25, Ryan Sleevi wrote:
> Doesn't the end of this test leave DHE enabled? Why not toggle it via the
Prefs?

The whole point of this test is to make sure the FeatureList plumbing works.

> Caveat: I'm not a fan of FeatureList, and from looking at
> ClearInstanceForTesting() call sites, I feel my hate is a bit justified. 

Metrics folks asked that I use FeatureList the last time this came up. This is
just the previous version of this test. But I agree this mess of globals is a
problem.

> Most
> consumers - other than a few, such as ScopedSubresourceFilterFeatureToggle,
> TestBlinkWebUnitTestSupport, and VariationsSeedProcessorTest - end up leaving
> the feature test enabled, and are relying on the side-effect that flaky tests
> will be retried in new processes.
> 
> I'm inclined to suggest a test-level scoper (like ScopedSubresource...) to
> handle this. You could get away with just
> 
> ScopedClosureRunner
> cleanup(base::Bind(&base::FeatureList::ClearInstanceForTesting));
> 
> at the top, followed by line 205.

That doesn't work because base::FeatureList does not start out cleared. Tests
which call FeatureList::IsEnabled will crash instead. Unfortunately, asvitkine
is OOO for a while. I'm thinking we'd need a base::ScopedFeatureListInstance
that takes ownership of the current FeatureList, replace with a new one, and
then swaps it back afterwards.

That's enough of a mess that I'd rather deal with it asynchronously.

[davidben, 2016-06-15 19:49:18]

https://codereview.chromium.org/2056343006/diff/40001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/2056343006/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref_unittest.cc:205:
base::FeatureList::ClearInstanceForTesting();
On 2016/06/15 19:21:20, davidben wrote:
> On 2016/06/15 01:23:25, Ryan Sleevi wrote:
> > Doesn't the end of this test leave DHE enabled? Why not toggle it via the
> Prefs?
> 
> The whole point of this test is to make sure the FeatureList plumbing works.
> 
> > Caveat: I'm not a fan of FeatureList, and from looking at
> > ClearInstanceForTesting() call sites, I feel my hate is a bit justified. 
> 
> Metrics folks asked that I use FeatureList the last time this came up. This is
> just the previous version of this test. But I agree this mess of globals is a
> problem.
> 
> > Most
> > consumers - other than a few, such as ScopedSubresourceFilterFeatureToggle,
> > TestBlinkWebUnitTestSupport, and VariationsSeedProcessorTest - end up
leaving
> > the feature test enabled, and are relying on the side-effect that flaky
tests
> > will be retried in new processes.
> > 
> > I'm inclined to suggest a test-level scoper (like ScopedSubresource...) to
> > handle this. You could get away with just
> > 
> > ScopedClosureRunner
> > cleanup(base::Bind(&base::FeatureList::ClearInstanceForTesting));
> > 
> > at the top, followed by line 205.
> 
> That doesn't work because base::FeatureList does not start out cleared. Tests
> which call FeatureList::IsEnabled will crash instead. Unfortunately, asvitkine
> is OOO for a while. I'm thinking we'd need a base::ScopedFeatureListInstance
> that takes ownership of the current FeatureList, replace with a new one, and
> then swaps it back afterwards.
> 
> That's enough of a mess that I'd rather deal with it asynchronously.

I started trying to put this together, but there's enough mess around
initializing things that this probably should be done by base::FeatureList
folks.

In the meantime, I suggest we either leave it as-is, or I end the test with
base::FeatureList::SetInstance(base::WrapUnique(new FeatureList)) in hopes that
it's *probably* not messing with too much state.

[Ryan Sleevi, 2016-06-15 20:14:36]

LGTM - but definitely should file a bug about base::FeatureList because that
seems like a real API anti-pattern that needs to be fixed.

[Ryan Sleevi, 2016-06-15 20:14:37]

LGTM - but definitely should file a bug about base::FeatureList because that
seems like a real API anti-pattern that needs to be fixed.

[edwardjung, 2016-06-16 09:59:02]

> IDS_ERRORPAGES_DETAILS_SSL_VERSION_OR_CIPHER_MISMATCH,
> edwardjung: I intentionally just reused the SSL_VERSION_OR_CIPHER_MISMATCH
> strings since I gather you all don't like having much error-specific strings
> with all the detailed IDS_ERRORPAGES_DETAILS_* strings no longer being used.
> 
> Let me know if you'd rather we have a different error string here.
> SUGGEST_UNSUPPORTED_CIPHER is accurate, but we could say something more
specific
> if we wanted.

LGTM for string reuse. I don't think we need specifics here.

[mmenke, 2016-06-16 15:37:14]

On 2016/06/16 09:59:02, edwardjung wrote:
> > IDS_ERRORPAGES_DETAILS_SSL_VERSION_OR_CIPHER_MISMATCH,
> > edwardjung: I intentionally just reused the SSL_VERSION_OR_CIPHER_MISMATCH
> > strings since I gather you all don't like having much error-specific strings
> > with all the detailed IDS_ERRORPAGES_DETAILS_* strings no longer being used.
> > 
> > Let me know if you'd rather we have a different error string here.
> > SUGGEST_UNSUPPORTED_CIPHER is accurate, but we could say something more
> specific
> > if we wanted.
> 
> LGTM for string reuse. I don't think we need specifics here.

localized_error LGTM, rubberstamp.

[davidben, 2016-06-17 20:01:58]

tnagel: friendly ping

[davidben, 2016-06-17 20:12:15]

davidben@chromium.org changed reviewers:
+ mpearson@chromium.org

[davidben, 2016-06-17 20:12:16]

Also +mpearson for histograms OWNERS

[Mark P, 2016-06-20 20:21:25]

histograms.xml lgtm

[Thiemo Nagel, 2016-06-21 12:22:42]

policy lgtm (sorry for the slow review, I was travelling last week)

[Thiemo Nagel, 2016-06-21 12:23:23]

https://codereview.chromium.org/2056343006/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2056343006/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:7966: If the policy is not
set, or is set to false, then DHE cipher suites in TLS will not be enabled.
Otherwise it may be set to true to retain compatibility with an outdated server.
This is a stopgap measure and the server should be reconfigured.''',
Nit: Present tense is better style in my opinion.
Nit: "Otherwise ..." should mirror the "false" wording and spell out explicitly
what happens in the "true" case, i.e. DHE being enabled.
Nit: It would be nice to point to docs explaining why DHE should not be used
anymore.

[davidben, 2016-06-22 04:18:20]

https://codereview.chromium.org/2056343006/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2056343006/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:7966: If the policy is not
set, or is set to false, then DHE cipher suites in TLS will not be enabled.
Otherwise it may be set to true to retain compatibility with an outdated server.
This is a stopgap measure and the server should be reconfigured.''',
On 2016/06/21 12:23:23, Thiemo Nagel wrote:
> Nit: Present tense is better style in my opinion.

Which part? Do you mean "If the policy [...], then DHE cipher suites in TLS are
not enabled"? That reads a little off to me.

> Nit: "Otherwise ..." should mirror the "false" wording and spell out
explicitly
> what happens in the "true" case, i.e. DHE being enabled.

Done.

> Nit: It would be nice to point to docs explaining why DHE should not be used
> anymore.

Hrm. So, I added a sentence about what servers should do instead. Why DHE
shouldn't be used is... complicated. There's some info here:
https://www.chromestatus.com/feature/5128908798164992
https://groups.google.com/a/chromium.org/d/msg/blink-dev/AAdv838-koo/bJv17voI...
https://groups.google.com/a/chromium.org/d/msg/blink-dev/ShRaCsYx4lk/46rD81As...

The only detailed info is in the Intent threads. I feel a little silly linking
to blink-dev in here though. Thoughts? I imagine later there will be something
on https://developers.google.com/web/updates/, but that text gets written after
branch point, not before.

[davidben, 2016-06-23 22:02:21]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org

[davidben, 2016-06-23 22:05:34]

https://codereview.chromium.org/2056343006/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2056343006/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:7966: If the policy is not
set, or is set to false, then DHE cipher suites in TLS will not be enabled.
Otherwise it may be set to true to retain compatibility with an outdated server.
This is a stopgap measure and the server should be reconfigured.''',
On 2016/06/22 04:18:19, davidben wrote:
> On 2016/06/21 12:23:23, Thiemo Nagel wrote:
> > Nit: Present tense is better style in my opinion.
> 
> Which part? Do you mean "If the policy [...], then DHE cipher suites in TLS
are
> not enabled"? That reads a little off to me.
> 
> > Nit: "Otherwise ..." should mirror the "false" wording and spell out
> explicitly
> > what happens in the "true" case, i.e. DHE being enabled.
> 
> Done.
> 
> > Nit: It would be nice to point to docs explaining why DHE should not be used
> > anymore.
> 
> Hrm. So, I added a sentence about what servers should do instead. Why DHE
> shouldn't be used is... complicated. There's some info here:
> https://www.chromestatus.com/feature/5128908798164992
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/AAdv838-koo/bJv17voI...
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/ShRaCsYx4lk/46rD81As...
> 
> The only detailed info is in the Intent threads. I feel a little silly linking
> to blink-dev in here though. Thoughts? I imagine later there will be something
> on https://developers.google.com/web/updates/, but that text gets written
after
> branch point, not before.

(Urf, apparently you're OOO for a bit. I'll go ahead and land this now to get
the ball rolling. If you want me to tweak the text further when you get back,
let me know and I'll upload a follow-up.)

[davidben, 2016-06-23 22:05:39]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-06-23 22:05:39]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
edwardjung@chromium.org, mpearson@chromium.org, rsleevi@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2056343006/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-23 22:06:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056343006/80001

[commit-bot: I haz the power, 2016-06-23 22:38:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 22:38:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[davidben, 2016-06-23 23:22:49]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-06-23 23:22:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
edwardjung@chromium.org, mpearson@chromium.org, rsleevi@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2056343006/#ps100001
(title: "actually connect up the feature flag")

[commit-bot: I haz the power, 2016-06-23 23:23:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056343006/100001

[commit-bot: I haz the power, 2016-06-24 02:39:40]

Description was changed from

==========
Remove DHE.

Connections to sites which require DHE will, for the moment,
return ERR_SSL_OBSOLETE_CIPHER. As always, there is an admin policy
to temporarily re-enable and field trial to back out of it if
needed.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.
==========

to

==========
Remove DHE.

Connections to sites which require DHE will, for the moment,
return ERR_SSL_OBSOLETE_CIPHER. As always, there is an admin policy
to temporarily re-enable and field trial to back out of it if
needed.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.
==========

[commit-bot: I haz the power, 2016-06-24 02:39:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-24 02:41:09]

Description was changed from

==========
Remove DHE.

Connections to sites which require DHE will, for the moment,
return ERR_SSL_OBSOLETE_CIPHER. As always, there is an admin policy
to temporarily re-enable and field trial to back out of it if
needed.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.
==========

to

==========
Remove DHE.

Connections to sites which require DHE will, for the moment,
return ERR_SSL_OBSOLETE_CIPHER. As always, there is an admin policy
to temporarily re-enable and field trial to back out of it if
needed.

BUG=619194
TEST=https://dh1024.badssl.com fails with ERR_SSL_OBSOLETE_CIPHER.

Committed: https://crrev.com/b4c25b632a7078d2c3346a37b51034bb853b24b3
Cr-Commit-Position: refs/heads/master@{#401793}
==========

[commit-bot: I haz the power, 2016-06-24 02:41:10]

Patchset 6 (id:??) landed as
https://crrev.com/b4c25b632a7078d2c3346a37b51034bb853b24b3
Cr-Commit-Position: refs/heads/master@{#401793}