Do not roll back to SSL 3.0 for Google properties.

net/http/http_network_transaction_ssl_unittest.cc

Index: net/http/http_network_transaction_ssl_unittest.cc
diff --git a/net/http/http_network_transaction_ssl_unittest.cc b/net/http/http_network_transaction_ssl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8033910658a305670abd7c3a0271018301abbceb
--- /dev/null
+++ b/net/http/http_network_transaction_ssl_unittest.cc
@@ -0,0 +1,198 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/base/net_util.h"
+#include "net/base/request_priority.h"
+#include "net/http/http_network_session.h"
+#include "net/http/http_network_transaction.h"
+#include "net/http/http_request_info.h"
+#include "net/socket/socket_test_util.h"
+#include "net/spdy/spdy_test_util_spdy3.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+class HttpNetworkTransactionSSLTest : public testing::Test {
+ public:
+  HttpRequestInfo* GetRequestInfo(std::string url) {
+    HttpRequestInfo* request_info = new HttpRequestInfo;
+    request_info->url = GURL(url);
+    request_info->method = "GET";
+    return request_info;
+  }
+};
+
+TEST_F(HttpNetworkTransactionSSLTest, Google_SSLVersionMinPreloadedEnabled) {
+  net::test_spdy3::SpdySessionDependencies session_deps;

[thaidn_google, 2013/04/16 00:39:12]

Please let me know if you think I shouldn't reuse this class.

[Ryan Sleevi, 2013/04/16 19:55:26]

You shouldn't re-use this class :)

You could use something similar to
http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_layer_u...
for these bits, AIUI.

[thaidn_google, 2013/04/17 00:46:17]

Done.

+
+  // |ssl_data1| contains the data for the first SSL handshake that will

[Ryan Sleevi, 2013/04/16 19:55:26]

comment nit: you intermix comments of "first SSL handshake" (when talking about
TLS) and "first handshake" (when talking about SSL3)

I would suggest consistently saying "first handshake", and then making sure the
comment explains *which* version you expect to be negotiated.

Further, it seems like the # of handshakes is coupled to the TLS versions you're
using (eg: TLS 1.1 -> TLS 1.0 -> SSL3), which will then create issues if/when we
change the default to include TLS 1.2 (since now you have an ADDITIONAL set of
fallbacks)

Seems like this test harness should explicitly configure its supported TLS
version to 1.0, so that there is only ever one fallback - to SSL3.

+  // return |net::ERR_SSL_PROTOCOL_ERROR| to trigger the fallback logic in
+  // |HttpNetworkTransaction::HandleSSLHandshakeError|.
+  SSLSocketDataProvider ssl_data1(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data1);
+
+  // |ssl_data2| contains the data for the second SSL handshake. When a
+  // connection to a server fails during an SSL handshake,
+  // HttpNetworkTransaction will attempt to fallback to TLSv1 if the previous
+  // connection was attempted with TLSv1.1. This is transparent to the caller
+  // of the HttpNetworkTransaction.
+  SSLSocketDataProvider ssl_data2(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data2);
+  net::StaticSocketDataProvider data2(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data2);
+
+  // Note: usually there should be a third handshake when
+  // |HttpNetworkTransaction| attempts to fallback to SSLv3 if the previous
+  // connection was attempted with TLSv1. There are only two handshakes here
+  // because SSLv3 fallback on Google's properties is disabled in this test.
+
+  scoped_refptr<HttpNetworkSession> session(
+      net::test_spdy3::SpdySessionDependencies::SpdyCreateSession(
+          &session_deps));
+  scoped_ptr<HttpNetworkTransaction> trans(
+        new HttpNetworkTransaction(DEFAULT_PRIORITY, session));
+  TestCompletionCallback callback;
+
+  // This will consume |ssl_data1| and |ssl_data2|.
+  int rv = trans->Start(GetRequestInfo("https://www.google.com/"),
+                         callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  // Complete the SSL handshake.
+  rv = callback.WaitForResult();

[Ryan Sleevi, 2013/04/16 19:55:26]

We're trying to phase out this pattern in existing code.

You can either do:

int rv = callback.GetResult(trans->Start(...)) or

int rv = trans->Start();
rv = callback.GetResult(rv);

The former is preferable.

The point of this is that the assertion that rv == ERR_IO_PENDING isn't strictly
a requirement of the code under test. For example, you could just as well change
line 38/48 to be SYNCHRONOUS completions and the test (should) still function
correctly.

[thaidn_google, 2013/04/17 00:46:17]

Done.

+  EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);

[Ryan Sleevi, 2013/04/16 19:55:26]

You're in net (line 18), so you don't need the net:: prefix anywhere in this
file.

[thaidn_google, 2013/04/17 00:46:17]

Done.

+
+  // |version_max| never fallbacks to SSLv3 for Google's properties.
+  EXPECT_EQ(SSL_PROTOCOL_VERSION_TLS1,
+            trans->server_ssl_config_for_testing().version_max);
+}
+
+TEST_F(HttpNetworkTransactionSSLTest, Google_SSLVersionMinPreloadedDisabled) {
+  net::test_spdy3::SpdySessionDependencies session_deps;
+
+  // |ssl_data1| contains the data for the first SSL handshake that will
+  // return |net::ERR_SSL_PROTOCOL_ERROR| to trigger the fallback logic in
+  // |HttpNetworkTransaction::HandleSSLHandshakeError|.
+  SSLSocketDataProvider ssl_data1(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data1);
+
+  // |ssl_data2| contains the data for the second SSL handshake. When a
+  // connection to a server fails during an SSL handshake,
+  // HttpNetworkTransaction will attempt to fallback to TLSv1 if the previous
+  // connection was attempted with TLSv1.1. This is transparent to the caller
+  // of the HttpNetworkTransaction.
+  SSLSocketDataProvider ssl_data2(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data2);
+  net::StaticSocketDataProvider data2(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data2);
+
+  // ssl_data3 contains the data for the third SSL handshake. When a
+  // connection to a server fails during an SSL handshake,
+  // |HttpNetworkTransaction| will attempt to fallback to SSLv3 if the previous
+  // connection was attempted with TLSv1. This is transparent to the caller
+  // of the |HttpNetworkTransaction|.
+  SSLSocketDataProvider ssl_data3(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data3);
+  net::StaticSocketDataProvider data3(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data3);
+
+  // Note: if ssl_data3 is omitted (like in
+  // |Google_SSLVersionMinPreloadedEnabled|) then this test will crash, because
+  // Chrome will attempt to handshake 3 times, but there are just 2 handshake
+  // data.
+
+  scoped_refptr<HttpNetworkSession> session(
+      net::test_spdy3::SpdySessionDependencies::SpdyCreateSession(
+          &session_deps));
+  scoped_ptr<HttpNetworkTransaction> trans(
+        new HttpNetworkTransaction(DEFAULT_PRIORITY, session));
+  TestCompletionCallback callback;
+
+  SSLConfig& ssl_config = trans->server_ssl_config_for_testing();
+  ssl_config.ssl_version_min_preloaded_disabled = true;
+
+  // This will consume |ssl_data1|, |ssl_data2| and |ssl_data3|.
+  int rv = trans->Start(GetRequestInfo("https://www.google.com/"),
+                         callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  // Complete the SSL handshake.
+  rv = callback.WaitForResult();
+  EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // |version_max| fallbacks to SSLv3 for Google's properties when
+  // |ssl_version_min_preloaded_disabled| is true.
+  EXPECT_EQ(SSL_PROTOCOL_VERSION_SSL3,
+            ssl_config.version_max);
+}
+
+TEST_F(HttpNetworkTransactionSSLTest, PayPal_SSLVersionMinPreloadedDisabled) {
+  net::test_spdy3::SpdySessionDependencies session_deps;
+
+  // |ssl_data1| contains the data for the first SSL handshake that will
+  // return |net::ERR_SSL_PROTOCOL_ERROR| to trigger the fallback logic in
+  // |HttpNetworkTransaction::HandleSSLHandshakeError|.
+  SSLSocketDataProvider ssl_data1(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data1);
+
+  // |ssl_data2| contains the data for the second SSL handshake. When a
+  // connection to a server fails during an SSL handshake,
+  // HttpNetworkTransaction will attempt to fallback to TLSv1 if the previous
+  // connection was attempted with TLSv1.1. This is transparent to the caller
+  // of the HttpNetworkTransaction.
+  SSLSocketDataProvider ssl_data2(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data2);
+  net::StaticSocketDataProvider data2(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data2);
+
+  // ssl_data3 contains the data for the third SSL handshake. When a
+  // connection to a server fails during an SSL handshake,
+  // HttpNetworkTransaction will attempt to fallback to SSLv3 if the previous
+  // connection was attempted with TLSv1. This is transparent to the caller
+  // of the HttpNetworkTransaction. Because this test failure is due to
+  // requiring a client certificate, this fallback handshake should also
+  // fail.

[Ryan Sleevi, 2013/04/16 19:55:26]

This comment seems entirely incorrect (and thus creates issues for the comment
on 171-173).

Where is it ever set that the failure is due to client certs? You're just
flagging a protocol error.

[thaidn_google, 2013/04/17 00:46:17]

Done.

+  SSLSocketDataProvider ssl_data3(ASYNC, net::ERR_SSL_PROTOCOL_ERROR);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data3);
+  net::StaticSocketDataProvider data3(NULL, 0, NULL, 0);
+  session_deps.socket_factory->AddSocketDataProvider(&data3);
+
+  // Note: if ssl_data3 is omitted (like in
+  // |Google_SSLVersionMinPreloadedEnabled|) then this test will crash, because
+  // Chrome will attempt to handshake 3 times, but there are just 2 handshake

[Ryan Sleevi, 2013/04/16 19:55:26]

nit: s/Chrome/Chromium/

[thaidn_google, 2013/04/17 00:46:17]

Done.

+  // data.
+
+  scoped_refptr<HttpNetworkSession> session(
+      net::test_spdy3::SpdySessionDependencies::SpdyCreateSession(
+          &session_deps));
+  scoped_ptr<HttpNetworkTransaction> trans(
+        new HttpNetworkTransaction(DEFAULT_PRIORITY, session));

[Ryan Sleevi, 2013/04/16 19:55:26]

style: indentation

[thaidn_google, 2013/04/17 00:46:17]

Done.

+  TestCompletionCallback callback;
+
+  // This will consume |ssl_data1|, |ssl_data2| and |ssl_data3|.
+  int rv = trans->Start(GetRequestInfo("https://www.paypal.com/"),
+                         callback.callback(), BoundNetLog());

[Ryan Sleevi, 2013/04/16 19:55:26]

style: indentation

[thaidn_google, 2013/04/17 00:46:17]

Done.

+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  // Complete the SSL handshake.
+  rv = callback.WaitForResult();
+  EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // |version_max| fallbacks to SSLv3.
+  EXPECT_EQ(SSL_PROTOCOL_VERSION_SSL3,
+            trans->server_ssl_config_for_testing().version_max);
+}
+
+}  // namespace
+}  // namespace net
+