Move RC4 behind a fallback.

Move RC4 behind a fallback.

This is to start gathering better metrics on which servers require RC4. Many
servers which prefer RC4 will still accept another cipher suite. Moving it
behind a fallback will make our cipher suite metrics reflect roughly servers
which require it and not only those which prefer it.

Note that this sort of fallback provides NO security benefit. If a server
prefers a non-RC4 cipher, they already securely negotiate it whether or not the
client offers RC4. If a server prefers RC4, this kind of fallback does nothing
to prevent an attacker from switching the connection to RC4. This is purely for
measurement.

BUG=375342
Committed: https://crrev.com/a4c9d060fa6e74b6513b198064cfe7b06482d404
Cr-Commit-Position: refs/heads/master@{#323837}

[davidben, 2015-04-01 22:22:58]

davidben@chromium.org changed reviewers:
+ agl@chromium.org, rsleevi@chromium.org

[davidben, 2015-04-01 22:22:59]

Just doing the fallback on this iteration, no UI degradation, so we can get UMA
metrics to match the Alexa probing.

https://codereview.chromium.org/1052743003/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1052743003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:1258: error == ERR_CONNECTION_CLOSED ||
error == ERR_CONNECTION_RESET)) {
This matches the set of error codes Mozilla uses. fonts.com does a reset
(happened to pick one that did at random).

Both BoringSSL and NSS ports should now map a handshake_failure in response to a
ClientHello to ERR_SSL_VERSION_OR_CIPHER_MISMATCH, so there shouldn't be any
need to toss ERR_SSL_PROTOCOL_ERROR into the mix.

https://codereview.chromium.org/1052743003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1052743003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:155: prefix += "deprecated/";
NOTE: This, like with the fallback, means that we'll send RC4-less ClientHellos
to RC4-requiring servers once on every request attempt. Only when that fails
will we try the other group and reuse any existing socket.

The alternative (not including this line) is worse as we'll get very strange
mixing due to late-binding of connection attempts.

[Ryan Sleevi, 2015-04-02 06:32:06]

LGTM % test nits

https://codereview.chromium.org/1052743003/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1052743003/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2937: // However, a normal connection
needs a full handshake.
But does a normal connection prime a deprecated cache? 

[I know the answer is no, but the test doesn't confirm that side; for example,
this could have broken resumption for all normal handshakes]

https://codereview.chromium.org/1052743003/diff/40001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1052743003/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7149: {
Is this scope necessary? Seems like a normal scoping / order would tear
everything down correctly.

[davidben, 2015-04-03 19:58:25]

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

[davidben, 2015-04-03 19:58:26]

+isherman for histograms.xml

https://codereview.chromium.org/1052743003/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1052743003/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2937: // However, a normal connection
needs a full handshake.
On 2015/04/02 06:32:06, Ryan Sleevi wrote:
> But does a normal connection prime a deprecated cache? 
> 
> [I know the answer is no, but the test doesn't confirm that side; for example,
> this could have broken resumption for all normal handshakes]

Done. (Though I can't imagine an implementation that could possibly have that
bug. :-) Not sure I see how it could have broken resumption for all normal
handshakes. It could have... broken a spurious fallback connection... when
connecting to servers that didn't need the fallback? If anything, we actually
want the mixing in that direction.)

https://codereview.chromium.org/1052743003/diff/40001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1052743003/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7149: {
On 2015/04/02 06:32:06, Ryan Sleevi wrote:
> Is this scope necessary? Seems like a normal scoping / order would tear
> everything down correctly.

Done. (Copy-paste from elsewhere.)

[Ilya Sherman, 2015-04-03 20:57:58]

https://codereview.chromium.org/1052743003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1052743003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46106: +  <int value="1"
label="Deprecated ciphers enabled"/>
Hmm, this doesn't quite seem to match the histogram description, which says that
deprecated ciphers were *used*.  Could you please clarify whether they are being
used or simply enabled/disabled?

[davidben, 2015-04-03 21:03:43]

https://codereview.chromium.org/1052743003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1052743003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46106: +  <int value="1"
label="Deprecated ciphers enabled"/>
On 2015/04/03 20:57:58, Ilya Sherman wrote:
> Hmm, this doesn't quite seem to match the histogram description, which says
that
> deprecated ciphers were *used*.  Could you please clarify whether they are
being
> used or simply enabled/disabled?

It says that the deprecated cipher suite *fallback* was used. The fallback just
enables deprecated ciphers; it's the server's choice whether they get used.
(Though it'll presumably use them, if it needed the fallback at all.)

[agl, 2015-04-03 21:06:17]

LGTM

https://codereview.chromium.org/1052743003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1052743003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:155: prefix += "deprecated/";
"deprecated" is a rather generic name. I'm not sure that someone looking at the
socket pools would figure it out. "deprecatedciphers/", or "weakciphers/" might
be more descriptive.

https://codereview.chromium.org/1052743003/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1052743003/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3360: peer_id += "deprecated";
(The scope is much smaller here so this is probably fine.)

[davidben, 2015-04-03 21:10:00]

https://codereview.chromium.org/1052743003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1052743003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:155: prefix += "deprecated/";
On 2015/04/03 21:06:17, agl wrote:
> "deprecated" is a rather generic name. I'm not sure that someone looking at
the
> socket pools would figure it out. "deprecatedciphers/", or "weakciphers/"
might
> be more descriptive.

Done. (Went with deprecatedciphers. weakciphers implies that the ciphers that
don't match the bucket aren't weak, but we aren't deprecating all weak ciphers
in one swoop.)

[Ilya Sherman, 2015-04-03 21:19:58]

LGTM

[davidben, 2015-04-03 21:23:00]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-03 21:23:01]

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

[commit-bot: I haz the power, 2015-04-03 21:24:00]

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

[commit-bot: I haz the power, 2015-04-03 22:34:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-03 22:35:33]

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