Make OpenSSL UpdateServerCert() OS independent.

net/socket/ssl_client_socket_unittest.cc

Index: net/socket/ssl_client_socket_unittest.cc
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index af05a397798c0493cd638ac278f2e1bfd2d0a067..0fe0d55c56a9eff3946c0b15d23942dbe2c81977 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -524,6 +524,46 @@ class SSLClientSocketTest : public PlatformTest {
   SSLClientSocketContext context_;
 };
 
+class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {

[haavardm, 2014/03/03 19:11:26]

Moved this class from below into the anonymous namespace

+ protected:
+  // Creates a test server with the given SSLOptions, connects to it and returns
+  // the SSLCertRequestInfo reported by the socket.
+  scoped_refptr<SSLCertRequestInfo> GetCertRequest(
+      SpawnedTestServer::SSLOptions ssl_options) {
+    SpawnedTestServer test_server(
+        SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath());
+    if (!test_server.Start())
+      return NULL;
+
+    AddressList addr;
+    if (!test_server.GetAddressList(&addr))
+      return NULL;
+
+    TestCompletionCallback callback;
+    CapturingNetLog log;
+    scoped_ptr<StreamSocket> transport(
+        new TCPClientSocket(addr, &log, NetLog::Source()));
+    int rv = transport->Connect(callback.callback());
+    if (rv == ERR_IO_PENDING)
+      rv = callback.WaitForResult();
+    EXPECT_EQ(OK, rv);
+
+    scoped_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+        transport.Pass(), test_server.host_port_pair(), kDefaultSSLConfig));
+    EXPECT_FALSE(sock->IsConnected());
+
+    rv = sock->Connect(callback.callback());
+    if (rv == ERR_IO_PENDING)
+      rv = callback.WaitForResult();
+    scoped_refptr<SSLCertRequestInfo> request_info = new SSLCertRequestInfo();
+    sock->GetSSLCertRequestInfo(request_info.get());
+    sock->Disconnect();
+    EXPECT_FALSE(sock->IsConnected());
+
+    return request_info;
+  }
+};
+
 //-----------------------------------------------------------------------------
 
 // LogContainsSSLConnectEndEvent returns true if the given index in the given
@@ -541,6 +581,8 @@ static bool LogContainsSSLConnectEndEvent(
              log, i, NetLog::TYPE_SOCKET_BYTES_SENT, NetLog::PHASE_NONE);
 }
 
+}  // namespace

[haavardm, 2014/03/03 19:11:26]

Moved tests out of the anonymous namespace as it gave trouble for
FRIEND_TEST_ALL_PREFIXES 

+
 TEST_F(SSLClientSocketTest, Connect) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
@@ -1708,6 +1750,74 @@ TEST(SSLClientSocket, ClearSessionCache) {
   SSLClientSocket::ClearSessionCache();
 }
 
+// Test that the server certificates are properly retrieved from the underlying

[wtc, 2014/03/10 21:45:34]

Nit: I don't understand why it is important to test this explicitly.

[Ryan Sleevi, 2014/03/11 00:15:15]

because we weren't before, and we were buggy :)

+// SSL stack.
+TEST_F(SSLClientSocketTest, VerifyServerChainProperlyOrdered) {
+  // The connection does not have to be successful

[wtc, 2014/03/10 21:45:34]

Nit: missing a period (.) at the end of the sentence.

Note: Some other comments in this CL are also missing a period (.). To avoid
such nits overshadowing the more substantive issues, I won't point out this kind
of punctuation issues in the future.

+  cert_verifier_->set_default_result(ERR_CERT_INVALID);
+
+  // Set up a test server with CERT_CHAIN_WRONG_ROOT.

[wtc, 2014/03/10 21:45:34]

Nit: it seems better to send a valid certificate chain that contains the root
certificate unnecessarily. In that case, the verified cert will not include the
root certificate.

[haavardm, 2014/03/11 18:43:21]

This was to reuse the certs set in  BaseTestServer, and avoid having to add a
new chain. The test does work (I've tested that it catches the bug I've fixed).
Can it stay as is?

On 2014/03/10 21:45:34, wtc wrote:

+  // This makes the server present redundant-server-chain.pem, which contains
+  // intermediate certificates.
+  SpawnedTestServer::SSLOptions ssl_options(
+      SpawnedTestServer::SSLOptions::CERT_CHAIN_WRONG_ROOT);
+  SpawnedTestServer test_server(
+      SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+  int rv = transport->Connect(callback.callback());
+  if (rv == ERR_IO_PENDING)
+    rv = callback.WaitForResult();

[wtc, 2014/03/10 21:45:34]

Just FYI: there is a callback.GetResult() method that you can use to replace
these three lines (1775-1777, and possibly lines 1784-1787).

[haavardm, 2014/03/11 18:43:21]

Ah, right. I always forget about that one :)

On 2014/03/10 21:45:34, wtc wrote:

+  EXPECT_EQ(OK, rv);
+
+  scoped_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      transport.Pass(), test_server.host_port_pair(), kDefaultSSLConfig));
+  EXPECT_FALSE(sock->IsConnected());
+  rv = sock->Connect(callback.callback());
+
+  if (rv == ERR_IO_PENDING)
+    rv = callback.WaitForResult();
+
+  EXPECT_EQ(ERR_CERT_INVALID, rv);
+  EXPECT_TRUE(sock->IsConnected());
+
+  // When given option CERT_CHAIN_WRONG_ROOT, SpawnedTestServer will present
+  // certs from redundant-server-chain.pem.
+  CertificateList server_certs =
+      CreateCertificateListFromFile(GetTestCertsDirectory(),
+                                    "redundant-server-chain.pem",
+                                    X509Certificate::FORMAT_AUTO);
+
+  // Get the server certificate as received client side
+  const scoped_refptr<X509Certificate> server_certificate =
+      sock->GetUnverifiedServerCertificate();
+
+  // Get the intermediates as received  client side
+  const X509Certificate::OSCertHandles& server_intermediates =
+      server_certificate->GetIntermediateCertificates();
+
+  // Check that the unverified server certificate chain is properly retrieved
+  // from the underlying ssl stack.
+  ASSERT_EQ(3U, server_intermediates.size());

[wtc, 2014/03/10 21:45:34]

1. Should we also assert that server_certs.size() is 4u?

2. Nit: move this after the next EXPECT_TRUE, because the next EXPECT_TRUE
doesn't use server_intermediates. But it may be better to assert the two sizes
together. Up to you.

[haavardm, 2014/03/11 18:43:21]

Done.

+  EXPECT_TRUE(X509Certificate::IsSameOSCert(
+      server_certificate->os_cert_handle(), server_certs[0]->os_cert_handle()));
+  EXPECT_TRUE(X509Certificate::IsSameOSCert(server_intermediates[0],
+                                            server_certs[1]->os_cert_handle()));
+  EXPECT_TRUE(X509Certificate::IsSameOSCert(server_intermediates[1],
+                                            server_certs[2]->os_cert_handle()));
+  EXPECT_TRUE(X509Certificate::IsSameOSCert(server_intermediates[2],
+                                            server_certs[3]->os_cert_handle()));
+
+  sock->Disconnect();
+  EXPECT_FALSE(sock->IsConnected());
+}
+
 // This tests that SSLInfo contains a properly re-constructed certificate
 // chain. That, in turn, verifies that GetSSLInfo is giving us the chain as
 // verified, not the chain as served by the server. (They may be different.)
@@ -1807,46 +1917,6 @@ TEST_F(SSLClientSocketTest, VerifyReturnChainProperlyOrdered) {
 }
 
 // Verifies the correctness of GetSSLCertRequestInfo.

[wtc, 2014/03/10 21:45:34]

I think this comment should be moved along with the
SSLClientSocketCertRequestInfoTest class. 

[haavardm, 2014/03/11 18:43:21]

Done.

-class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
- protected:
-  // Creates a test server with the given SSLOptions, connects to it and returns
-  // the SSLCertRequestInfo reported by the socket.
-  scoped_refptr<SSLCertRequestInfo> GetCertRequest(
-      SpawnedTestServer::SSLOptions ssl_options) {
-    SpawnedTestServer test_server(
-        SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath());
-    if (!test_server.Start())
-      return NULL;
-
-    AddressList addr;
-    if (!test_server.GetAddressList(&addr))
-      return NULL;
-
-    TestCompletionCallback callback;
-    CapturingNetLog log;
-    scoped_ptr<StreamSocket> transport(
-        new TCPClientSocket(addr, &log, NetLog::Source()));
-    int rv = transport->Connect(callback.callback());
-    if (rv == ERR_IO_PENDING)
-      rv = callback.WaitForResult();
-    EXPECT_EQ(OK, rv);
-
-    scoped_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
-        transport.Pass(), test_server.host_port_pair(), kDefaultSSLConfig));
-    EXPECT_FALSE(sock->IsConnected());
-
-    rv = sock->Connect(callback.callback());
-    if (rv == ERR_IO_PENDING)
-      rv = callback.WaitForResult();
-    scoped_refptr<SSLCertRequestInfo> request_info = new SSLCertRequestInfo();
-    sock->GetSSLCertRequestInfo(request_info.get());
-    sock->Disconnect();
-    EXPECT_FALSE(sock->IsConnected());
-
-    return request_info;
-  }
-};
-
 TEST_F(SSLClientSocketCertRequestInfoTest, NoAuthorities) {
   SpawnedTestServer::SSLOptions ssl_options;
   ssl_options.request_client_certificate = true;
@@ -1898,8 +1968,6 @@ TEST_F(SSLClientSocketCertRequestInfoTest, TwoAuthorities) {
       request_info->cert_authorities[1]);
 }
 
-}  // namespace
-
 TEST_F(SSLClientSocketTest, ConnectSignedCertTimestampsEnabledTLSExtension) {
   SpawnedTestServer::SSLOptions ssl_options;
   ssl_options.signed_cert_timestamps_tls_ext = "test";