Make OpenSSL UpdateServerCert() OS independent.

net/socket/ssl_client_socket_unittest.cc

 // 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 "net/socket/ssl_client_socket.h"
 
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "net/base/address_list.h"
 #include "net/base/io_buffer.h"
@@ skipping 506 matching lines @@
     return socket_factory_->CreateSSLClientSocket(
         connection.Pass(), host_and_port, ssl_config, context_);
   }
 
   ClientSocketFactory* socket_factory_;
   scoped_ptr<MockCertVerifier> cert_verifier_;
   scoped_ptr<TransportSecurityState> transport_security_state_;
   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
 // log is an SSL connect end event. The NSS sockets will cork in an attempt to
 // merge the first application data record with the Finished message when false
 // starting. However, in order to avoid the server timing out the handshake,
 // they'll give up waiting for application data and send the Finished after a
 // timeout. This means that an SSL connect end event may appear as a socket
 // write.
 static bool LogContainsSSLConnectEndEvent(

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

Nit: you can remove "static" because this function is in the anonymous
namespace.

     const CapturingNetLog::CapturedEntryList& log,
     int i) {
   return LogContainsEndEvent(log, i, NetLog::TYPE_SSL_CONNECT) ||
          LogContainsEvent(
              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,
                                 base::FilePath());
   ASSERT_TRUE(test_server.Start());
 
   AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
 
   TestCompletionCallback callback;
@@ skipping 1147 matching lines @@
   EXPECT_EQ(rv, OK);
   EXPECT_NE(memcmp(client_out1, client_out2, kKeyingMaterialSize), 0);
 }
 
 // Verifies that SSLClientSocket::ClearSessionCache can be called without
 // explicit NSS initialization.
 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.)
 //
 // CERT_CHAIN_WRONG_ROOT is redundant-server-chain.pem. It contains A
 // (end-entity) -> B -> C, and C is signed by D. redundant-validated-chain.pem
 // contains a chain of A -> B -> C2, where C2 is the same public key as C, but
 // a self-signed root. Such a situation can occur when a new root (C2) is
 // cross-certified by an old root (D) and has two different versions of its
 // floating around. Servers may supply C2 as an intermediate, but the
@@ skipping 78 matching lines @@
                                             certs[0]->os_cert_handle()));
   EXPECT_TRUE(X509Certificate::IsSameOSCert(intermediates[0],
                                             certs[1]->os_cert_handle()));
   EXPECT_TRUE(X509Certificate::IsSameOSCert(intermediates[1],
                                             certs[2]->os_cert_handle()));
 
   sock->Disconnect();
   EXPECT_FALSE(sock->IsConnected());
 }
 
 // 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;
   scoped_refptr<SSLCertRequestInfo> request_info = GetCertRequest(ssl_options);
   ASSERT_TRUE(request_info.get());
   EXPECT_EQ(0u, request_info->cert_authorities.size());
 }
 
 TEST_F(SSLClientSocketCertRequestInfoTest, TwoAuthorities) {
   const base::FilePath::CharType kThawteFile[] =
@@ skipping 31 matching lines @@
   scoped_refptr<SSLCertRequestInfo> request_info = GetCertRequest(ssl_options);
   ASSERT_TRUE(request_info.get());
   ASSERT_EQ(2u, request_info->cert_authorities.size());
   EXPECT_EQ(std::string(reinterpret_cast<const char*>(kThawteDN), kThawteLen),
             request_info->cert_authorities[0]);
   EXPECT_EQ(
       std::string(reinterpret_cast<const char*>(kDiginotarDN), kDiginotarLen),
       request_info->cert_authorities[1]);
 }
 
-}  // namespace
-
 TEST_F(SSLClientSocketTest, ConnectSignedCertTimestampsEnabledTLSExtension) {
   SpawnedTestServer::SSLOptions ssl_options;
   ssl_options.signed_cert_timestamps_tls_ext = "test";
 
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 ssl_options,
                                 base::FilePath());
   ASSERT_TRUE(test_server.Start());
 
   AddressList addr;
@@ skipping 139 matching lines @@
   log.GetEntries(&entries);
   EXPECT_TRUE(LogContainsSSLConnectEndEvent(entries, -1));
 
   EXPECT_FALSE(sock->signed_cert_timestamps_received_);
 
   sock->Disconnect();
   EXPECT_FALSE(sock->IsConnected());
 }
 
 }  // namespace net