Remove RC4 by default.

Remove RC4 by default.

RC4 may still be re-enabled via the RC4Enabled administrative policy, until
sometime around September. Also control it via a field trial so we still have
an escape hatch should something catastrophic happen.

Keep the deprecated cipher suite fallback around (rename the parameter since I
got the naming convention wrong) since it's still got the IIS AES-GCM
workaround in it, and it will be used in not too long for DHE_RSA instead.

BUG=375342
TEST=Loading https://rc4.badssl.com/ fails with ERR_SSL_VERSION_OR_CIPHER_MISMATCH
     Relaunching Chrome with --force-fieldtrials=RC4Ciphers/Enabled/ makes that page succeed.
     Relaunching Chrome after setting the RC4Enabled polcy to true makes that page succeed.
     (Note: press refresh when loading the site to make sure it's not cached.)

Committed: https://crrev.com/14b1a53362ffb727e02bdf27e24e93c5f9b2d423
Cr-Commit-Position: refs/heads/master@{#357114}

[davidben, 2015-10-27 21:21:45]

Description was changed from

==========
Remove RC4 by default.

RC4 may still be re-enabled via the RC4Enabled administrative policy, until
sometime around September. Also control it via a field trial so we still have
an escape hatch should something catastrophic happen.

BUG=375342
==========

to

==========
Remove RC4 by default.

RC4 may still be re-enabled via the RC4Enabled administrative policy, until
sometime around September. Also control it via a field trial so we still have
an escape hatch should something catastrophic happen.

Keep the deprecated cipher suite fallback around (rename the parameter since I
got the naming convention wrong) since it's still got the IIS AES-GCM
workaround in it, and it will be used in not too long for DHE_RSA instead.

BUG=375342
TEST=Loading https://rc4.badssl.com/ fails with
ERR_SSL_VERSION_OR_CIPHER_MISMATCH
     Relaunching Chrome with --force-fieldtrials=RC4Ciphers/Enabled/ makes that
page succeed.
     Relaunching Chrome after setting the RC4Enabled polcy to true makes that
page succeed.
     (Note: press refresh when loading the site to make sure it's not cached.)
==========

[davidben, 2015-10-27 21:43:54]

davidben@chromium.org changed reviewers:
+ cschuet@chromium.org, isherman@chromium.org, lgarron@chromium.org,
rsleevi@chromium.org

[davidben, 2015-10-27 21:43:54]

rsleevi: Primary review.

lgarron: Enamel side of things since felt is OOO.

cschuet: */policy/*

isherman: histograms.xml and field trial code in
components/ssl_config/ssl_config_service_manager_pref.cc. The field trial is
intended as an escape hatch so we can easily back this out if there's a
catastrophe. Otherwise I hope not to have to set it. Let me know if there's
anything that needs to be done beyond just landing this code.

[davidben, 2015-10-27 21:44:44]

(Oh, also agl FYI.)

[lgarron, 2015-10-27 21:53:14]

https://codereview.chromium.org/1422293002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1422293002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9247: +        The client and server don't
support a common SSL protocol version or cipher suite. This is usually caused
when the server needs RC4 support, which has been removed.
Given the trouble cause by ambiguous strings in the past (looking at you,
SHA-1), I think it would be much better if we could state unambiguously when RC4
caused the problem.
This would probably take a separate string and a way to test if the error was
RC4. Is that possible?

[davidben, 2015-10-27 22:30:22]

https://codereview.chromium.org/1422293002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1422293002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9247: +        The client and server don't
support a common SSL protocol version or cipher suite. This is usually caused
when the server needs RC4 support, which has been removed.
On 2015/10/27 21:53:14, lgarron wrote:
> Given the trouble cause by ambiguous strings in the past (looking at you,
> SHA-1), I think it would be much better if we could state unambiguously when
RC4
> caused the problem.
> This would probably take a separate string and a way to test if the error was
> RC4. Is that possible?

The server selects parameters, so it's not possible without complicating how
connections work. That said, we did do this for the SSL 3.0 fallback for a short
while until we raised the minimum completely. (And likewise with the rest of the
fallback legs, though we're not planning on raising the minimum anytime soon.)

Ryan, what do you think? If I had to give it a dedicated error code, I'd
probably do something like:

- In the deprecated cipher fallback, continue to advertise RC4 as before and not
just the AES-GCM IIS workaround.
- After the handshake, check the resulting cipher suite again and fail with a
dedicated error if rc4_enabled and the cipher is RC4.

In future, when the error code stops showing up much, remove that logic and roll
back to how we're doing now.

[lgarron, 2015-10-27 22:38:32]

https://codereview.chromium.org/1422293002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1422293002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9247: +        The client and server don't
support a common SSL protocol version or cipher suite. This is usually caused
when the server needs RC4 support, which has been removed.
On 2015/10/27 at 22:30:22, davidben wrote:
> On 2015/10/27 21:53:14, lgarron wrote:
> > Given the trouble cause by ambiguous strings in the past (looking at you,
> > SHA-1), I think it would be much better if we could state unambiguously when
RC4
> > caused the problem.
> > This would probably take a separate string and a way to test if the error
was
> > RC4. Is that possible?
> 
> The server selects parameters, so it's not possible without complicating how
connections work. That said, we did do this for the SSL 3.0 fallback for a short
while until we raised the minimum completely. (And likewise with the rest of the
fallback legs, though we're not planning on raising the minimum anytime soon.)
> 
> Ryan, what do you think? If I had to give it a dedicated error code, I'd
probably do something like:
> 
> - In the deprecated cipher fallback, continue to advertise RC4 as before and
not just the AES-GCM IIS workaround.
> - After the handshake, check the resulting cipher suite again and fail with a
dedicated error if rc4_enabled and the cipher is RC4.
> 
> In future, when the error code stops showing up much, remove that logic and
roll back to how we're doing now.

Something like that would be great if we could do it for at least one or two
releases. :-D
(If it's a matter of taking time to code this, feel free to pull me in.)

[Ryan Sleevi, 2015-10-27 22:48:40]

On 2015/10/27 22:38:32, lgarron wrote:
> Something like that would be great if we could do it for at least one or two
> releases. :-D
> (If it's a matter of taking time to code this, feel free to pull me in.)

I don't feel like this would be a good use of the time/size tradeoff. I can
appreciate the confusion, but I don't think the precision is worth the
complexity. This also significantly complicates the ability for servers to
determine if a client if vulnerable or supports RC4, and I feel that the
messaging gained (for clients) would not outweigh the ability for servers to
quantify their own risk of compat from disabling RC4.

Put differently, I'm not a fan of that change.

[Ryan Sleevi, 2015-10-27 22:58:38]

LGTM w/ a note for whomover 'owns' finch bits :)

https://codereview.chromium.org/1422293002/diff/40001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/1422293002/diff/40001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:1804: "os": ["win", "linux",
"mac", "chromeos"],
Shouldn't this include "android"?

It looks like someone failed to update the documentation about the 'os' param -
or we have a ton of invalid 'android' policies in place.

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

https://codereview.chromium.org/1422293002/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:86: return
base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE);
Finch docs seem inconsistent here. Chromium prefers ==, generally, although both
finch101 and finch-and-flags suggest StartsWith for some reason of multiple
enabled groups (which don't seem to apply). Finch-and-flags uses a case
insensitive match, finch101 uses a case-sensitive match, and of course, equality
works on case-sensitivity.

I'm not sure what the 'right' answer is here. It feels like a "return group_name
== "Enabled"" is sufficient, but just worth highlighting the inconsistency.

[Ilya Sherman, 2015-10-28 03:12:56]

LGTM, though I'd like to take another glance if you do switch to the
base::FeatureList API.

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

https://codereview.chromium.org/1422293002/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:86: return
base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE);
On 2015/10/27 22:58:37, Ryan Sleevi wrote:
> Finch docs seem inconsistent here. Chromium prefers ==, generally, although
both
> finch101 and finch-and-flags suggest StartsWith for some reason of multiple
> enabled groups (which don't seem to apply). Finch-and-flags uses a case
> insensitive match, finch101 uses a case-sensitive match, and of course,
equality
> works on case-sensitivity.
> 
> I'm not sure what the 'right' answer is here. It feels like a "return
group_name
> == "Enabled"" is sufficient, but just worth highlighting the inconsistency.

I think either == or StartsWith is fine.  Alternately, if you'd like to be bold,
you could instead use the nascent base::FeatureList API =)

[cschuet (SLOW), 2015-10-28 05:50:17]

On 2015/10/28 03:12:56, Ilya Sherman wrote:
> LGTM, though I'd like to take another glance if you do switch to the
> base::FeatureList API.
> 
>
https://codereview.chromium.org/1422293002/diff/40001/components/ssl_config/s...
> File components/ssl_config/ssl_config_service_manager_pref.cc (right):
> 
>
https://codereview.chromium.org/1422293002/diff/40001/components/ssl_config/s...
> components/ssl_config/ssl_config_service_manager_pref.cc:86: return
> base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE);
> On 2015/10/27 22:58:37, Ryan Sleevi wrote:
> > Finch docs seem inconsistent here. Chromium prefers ==, generally, although
> both
> > finch101 and finch-and-flags suggest StartsWith for some reason of multiple
> > enabled groups (which don't seem to apply). Finch-and-flags uses a case
> > insensitive match, finch101 uses a case-sensitive match, and of course,
> equality
> > works on case-sensitivity.
> > 
> > I'm not sure what the 'right' answer is here. It feels like a "return
> group_name
> > == "Enabled"" is sufficient, but just worth highlighting the inconsistency.
> 
> I think either == or StartsWith is fine.  Alternately, if you'd like to be
bold,
> you could instead use the nascent base::FeatureList API =)

LGTM

[Thiemo Nagel, 2015-10-28 13:55:14]

LGTM policy_templates.json

[davidben, 2015-10-28 21:36:13]

lgarron/rsleevi: So... do we a conclusion as to what to do with the error
string. I think I probably agree with Ryan now since my previous proposal would
cause strange behaviors when DHE_RSA gets put into the deprecated cipher
fallback, so we'd really need a brand new fallback and that's quite a price to
pay here for something with a laundry list of caveats. Plus we'll be doing this
at the same time as everyone else.

We aren't going to be doing any other change that'll signal
ERR_SSL_VERSION_OR_CIPHER_MISMATCH at the same time, SSL 3.0 is certainly
flushed out by now (we actually could have had a dedicated SSL 3.0 error code
anyway, but it's not worth bothering at this point), so I think we should just
put RC4 in the description of ERR_SSL_VERSION_OR_CIPHER_MISMATCH.

This CL does that, but it puts it in the "details" string which apparently isn't
what's shown under "details" anymore. That's actually the summary? Lucas, is
this correct? What string would you like me to use? What about:

"A secure connection cannot be established because this site uses an unsupported
protocol or cipher suite. This is usually caused when the server needs RC4
support, which has been removed."

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

https://codereview.chromium.org/1422293002/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:86: return
base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE);
On 2015/10/28 03:12:56, Ilya Sherman wrote:
> On 2015/10/27 22:58:37, Ryan Sleevi wrote:
> > Finch docs seem inconsistent here. Chromium prefers ==, generally, although
> both
> > finch101 and finch-and-flags suggest StartsWith for some reason of multiple
> > enabled groups (which don't seem to apply). Finch-and-flags uses a case
> > insensitive match, finch101 uses a case-sensitive match, and of course,
> equality
> > works on case-sensitivity.
> > 
> > I'm not sure what the 'right' answer is here. It feels like a "return
> group_name
> > == "Enabled"" is sufficient, but just worth highlighting the inconsistency.
> 
> I think either == or StartsWith is fine.  Alternately, if you'd like to be
bold,
> you could instead use the nascent base::FeatureList API =)

I don't see any users of base::Feature in the entire codebase yet and this is
kind of a weird feature toggle, so probably you want someone else to be the
guinea pig. :-)

[davidben, 2015-10-29 19:49:06]

On 2015/10/28 21:36:13, davidben wrote:
> lgarron/rsleevi: So... do we a conclusion as to what to do with the error
> string. I think I probably agree with Ryan now since my previous proposal
would
> cause strange behaviors when DHE_RSA gets put into the deprecated cipher
> fallback, so we'd really need a brand new fallback and that's quite a price to
> pay here for something with a laundry list of caveats. Plus we'll be doing
this
> at the same time as everyone else.
> 
> We aren't going to be doing any other change that'll signal
> ERR_SSL_VERSION_OR_CIPHER_MISMATCH at the same time, SSL 3.0 is certainly
> flushed out by now (we actually could have had a dedicated SSL 3.0 error code
> anyway, but it's not worth bothering at this point), so I think we should just
> put RC4 in the description of ERR_SSL_VERSION_OR_CIPHER_MISMATCH.
> 
> This CL does that, but it puts it in the "details" string which apparently
isn't
> what's shown under "details" anymore. That's actually the summary? Lucas, is
> this correct? What string would you like me to use? What about:
> 
> "A secure connection cannot be established because this site uses an
unsupported
> protocol or cipher suite. This is usually caused when the server needs RC4
> support, which has been removed."

I've done this. It looks like this.

https://drive.google.com/file/d/0Bz14lOW5Hke4cDBNWTA3LThyVE0/view?usp=sharing
https://drive.google.com/file/d/0Bz14lOW5Hke4bjdaZnFoOXFxUTA/view?usp=sharing

[Ryan Sleevi, 2015-10-29 19:56:11]

On 2015/10/29 19:49:06, davidben wrote:
> I've done this. It looks like this.

I'm good with this; it really depends on how strongly lgarron and enamel feel.
As indicated previously, my gut is that the complexity (for Chrome) and the
implications to skewing metrics (for server operators) are more costly than the
cost to users (who may be confused by the messaging)

[davidben, 2015-10-29 20:56:06]

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

[davidben, 2015-10-29 20:56:07]

Apparently I'm missing a little OWNERS coverage. +tnagel for
components/policy/resources/policy_templates.json

[lgarron, 2015-10-29 22:23:32]

On 2015/10/29 at 19:56:11, rsleevi wrote:
> On 2015/10/29 19:49:06, davidben wrote:
> > I've done this. It looks like this.
> 
> I'm good with this; it really depends on how strongly lgarron and enamel feel.
As indicated previously, my gut is that the complexity (for Chrome) and the
implications to skewing metrics (for server operators) are more costly than the
cost to users (who may be confused by the messaging)

After talking with davidben@ yesterday, I've been convinced that the tradeoff is
not worth it.

As long as we mention RC4 as a clear probable cause, I think it's pretty good.

I don't know about the which string shows up exactly where on a net error page.
I don't think it matters much, as long as you can get to it when trying to
debug.

[Thiemo Nagel, 2015-10-30 12:32:14]

On 2015/10/29 20:56:07, davidben wrote:
> Apparently I'm missing a little OWNERS coverage. +tnagel for
> components/policy/resources/policy_templates.json

I don't think you are.  I've lgtm'ed on Wednesday already.

[davidben, 2015-10-30 15:26:06]

On 2015/10/30 12:32:14, Thiemo Nagel wrote:
> On 2015/10/29 20:56:07, davidben wrote:
> > Apparently I'm missing a little OWNERS coverage. +tnagel for
> > components/policy/resources/policy_templates.json
> 
> I don't think you are.  I've lgtm'ed on Wednesday already.

So you did. Huh. Were you not listed as a reviewer? git cl upload grumped at me,
but I wonder if it does dumb things if someone l-g-t-ms but isn't listed as
reviewer. *shrug*

[davidben, 2015-10-30 15:26:17]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-10-30 15:26:17]

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

[commit-bot: I haz the power, 2015-10-30 15:26:57]

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

[commit-bot: I haz the power, 2015-10-30 16:01:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-30 16:02:11]

Patchset 5 (id:??) landed as
https://crrev.com/14b1a53362ffb727e02bdf27e24e93c5f9b2d423
Cr-Commit-Position: refs/heads/master@{#357114}