SSL fixes for sites with buggy DEFLATE support.

SSL fixes for sites with buggy DEFLATE support.

1) Keep a global set of known buggy hosts in memory.

   Previously we would fallback to SSLv3 with each connection. While I
   quite like the idea of making these buggy sites slow, it doesn't
   really help users and it feels silly.

2) Catch SSL protocol errors while reading the initial data from the
   server and fallback so SSLv3.

   Since we added False Start, servers which believe that they support
   DEFLATE compression but fail at it are not detected as a handshake
   error. Rather they cause an error when reading the first byte of
   application level data.

BUG=41591
TEST=Navigate to https://ws.sso.post.ch/ without an SSL error

[agl, 2010-04-16 15:57:35]

I'll run this by wtc also but he's away today and you might notice something
terrible that I've missed about this. (I'm a little unsure about restarting the
request after the HttpStream has been created.)


Cheers

[willchan no longer on Chromium, 2010-04-16 20:32:04]

http://codereview.chromium.org/1585041/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1585041/diff/1/2#newcode708
net/http/http_network_transaction.cc:708: if
(g_ssl_broken_servers->count(GetHostAndPort(request_->url))) {
My personal pref is to use ContainsKey() [base/stl_util-inl.h] here, since I
think people read that more easily than count().

http://codereview.chromium.org/1585041/diff/1/2#newcode1031
net/http/http_network_transaction.cc:1031: } else if (result ==
ERR_SSL_PROTOCOL_ERROR && ssl_config_.tls1_enabled) {
I'm wondering if we should add a net error code for NSS error
SSL_ERROR_DECOMPRESSION_FAILURE_ALERT and match on that instead.  I'm fine with
this as is though.

I think you can add a test for this by having the SocketDataProvider return an
ERR_SSL_PROTOCOL_ERROR in the first MockRead.

[wtc, 2010-04-19 22:24:04]

LGTM.  You're right, I wasn't interested in optimizing the
performance of talking to TLS-intolerant servers.  Firefox
has the equivalent of the g_ssl_broken_servers set that you
added.

http://codereview.chromium.org/1585041/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1585041/diff/1/2#newcode51
net/http/http_network_transaction.cc:51: std::set<std::string>*
g_ssl_broken_servers = NULL;
Could you document what the std::string is?  I can guess it
contains the server's hostname.  Does it include the port
number?

I suggest naming this variable g_tls_intolerant_servers to
make it clear what's broken about these servers.

http://codereview.chromium.org/1585041/diff/1/2#newcode709
net/http/http_network_transaction.cc:709: LOG(WARNING) << "Falling back to SSLv3
because host is buggy: "
Nit: buggy => TLS intolerant

It may be better to move this code to DoSSLConnect(),
which is the state that actually uses ssl_config_.

http://codereview.chromium.org/1585041/diff/1/2#newcode1031
net/http/http_network_transaction.cc:1031: } else if (result ==
ERR_SSL_PROTOCOL_ERROR && ssl_config_.tls1_enabled) {
I agree with willchan's suggestion of adding a new net error
code that corresponds to NSS's
SSL_ERROR_DECOMPRESSION_FAILURE_ALERT.

http://codereview.chromium.org/1585041/diff/1/2#newcode1549
net/http/http_network_transaction.cc:1549: connection_->socket()->Disconnect();
Let's replace these three lines (lines 1549-1551) with a
ResetConnectionAndRequestForResend() call.
ResetConnectionAndRequestForResend() is only slightly less
efficient (has a request_headers_.clear() call that we don't
need here).

[agl, 2010-04-20 18:27:06]

PTAL.

http://codereview.chromium.org/1585041/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1585041/diff/1/2#newcode51
net/http/http_network_transaction.cc:51: std::set<std::string>*
g_ssl_broken_servers = NULL;
On 2010/04/19 22:24:05, wtc wrote:
> Could you document what the std::string is?  I can guess it
> contains the server's hostname.  Does it include the port
> number?
> 
> I suggest naming this variable g_tls_intolerant_servers to
> make it clear what's broken about these servers.

Done.

http://codereview.chromium.org/1585041/diff/1/2#newcode708
net/http/http_network_transaction.cc:708: if
(g_ssl_broken_servers->count(GetHostAndPort(request_->url))) {
On 2010/04/16 20:32:04, willchan wrote:
> My personal pref is to use ContainsKey() [base/stl_util-inl.h] here, since I
> think people read that more easily than count().

Done.

http://codereview.chromium.org/1585041/diff/1/2#newcode709
net/http/http_network_transaction.cc:709: LOG(WARNING) << "Falling back to SSLv3
because host is buggy: "
On 2010/04/19 22:24:05, wtc wrote:
> Nit: buggy => TLS intolerant
> 
> It may be better to move this code to DoSSLConnect(),
> which is the state that actually uses ssl_config_.

Done.

http://codereview.chromium.org/1585041/diff/1/2#newcode1031
net/http/http_network_transaction.cc:1031: } else if (result ==
ERR_SSL_PROTOCOL_ERROR && ssl_config_.tls1_enabled) {
On 2010/04/19 22:24:05, wtc wrote:
> I agree with willchan's suggestion of adding a new net error
> code that corresponds to NSS's
> SSL_ERROR_DECOMPRESSION_FAILURE_ALERT.

Done.

http://codereview.chromium.org/1585041/diff/1/2#newcode1549
net/http/http_network_transaction.cc:1549: connection_->socket()->Disconnect();
On 2010/04/19 22:24:05, wtc wrote:
> Let's replace these three lines (lines 1549-1551) with a
> ResetConnectionAndRequestForResend() call.
> ResetConnectionAndRequestForResend() is only slightly less
> efficient (has a request_headers_.clear() call that we don't
> need here).

Done.

[wtc, 2010-04-20 19:15:19]

LGTM.

http://codereview.chromium.org/1585041/diff/10001/11001
File net/base/net_error_list.h (right):

http://codereview.chromium.org/1585041/diff/10001/11001#newcode150
net/base/net_error_list.h:150: // An SSL peer sent us a fatal decompression
failure alert. This typically
Nit: decompression failure => decompression_failure
with the underscore.

http://codereview.chromium.org/1585041/diff/10001/11002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1585041/diff/10001/11002#newcode1552
net/http/http_network_transaction.cc:1552: case
ERR_SSL_VERSION_OR_CIPHER_MISMATCH:
You also need to list ERR_SSL_DECOMPRESSION_FAILURE_ALERT
here, to handle the case where TLS false start is disabled.

[agl, 2010-04-20 19:35:45]

PTAL.

Hopefully the last round. I added some unittests and verified that they caught
the error that you noticed below.

http://codereview.chromium.org/1585041/diff/10001/11001
File net/base/net_error_list.h (right):

http://codereview.chromium.org/1585041/diff/10001/11001#newcode150
net/base/net_error_list.h:150: // An SSL peer sent us a fatal decompression
failure alert. This typically
On 2010/04/20 19:15:19, wtc wrote:
> Nit: decompression failure => decompression_failure
> with the underscore.

Done.

http://codereview.chromium.org/1585041/diff/10001/11002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1585041/diff/10001/11002#newcode1552
net/http/http_network_transaction.cc:1552: case
ERR_SSL_VERSION_OR_CIPHER_MISMATCH:
On 2010/04/20 19:15:19, wtc wrote:
> You also need to list ERR_SSL_DECOMPRESSION_FAILURE_ALERT
> here, to handle the case where TLS false start is disabled.

Good catch, thanks!

[wtc, 2010-04-20 21:31:07]

LGTM.

http://codereview.chromium.org/1585041/diff/16001/17003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1585041/diff/16001/17003#newcode4889
net/http/http_network_transaction_unittest.cc:4889: struct
TLSCompressionFailureSocketDataProvider : public SocketDataProvider {
Why is this class defined as a 'struct'?

Nit: this class name should say DecompressionFailure.  I
guess that would make the class name too long?