Refactor X509Certificate caching to cache the OS handle, rather than the X509Certificate

net/base/x509_certificate_unittest.cc

 // Copyright (c) 2011 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 "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/string_number_conversions.h"
@@ skipping 457 matching lines @@
   ASSERT_NE(static_cast<X509Certificate*>(NULL), intermediate_cert);
 
   FilePath root_cert_path = certs_dir.AppendASCII("dod_root_ca_2_cert.der");
   TestRootCerts* root_certs = TestRootCerts::GetInstance();
   ASSERT_TRUE(root_certs->AddFromFile(root_cert_path));
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
   CertVerifyResult verify_result;
   int error = cert_chain->Verify("www.us.army.mil", flags, &verify_result);
   EXPECT_EQ(OK, error);
   EXPECT_EQ(0, verify_result.cert_status);
   root_certs->Clear();
 }
 
@@ skipping 14 matching lines @@
   ASSERT_NE(static_cast<X509Certificate*>(NULL), server_cert);
 
   scoped_refptr<X509Certificate> intermediate_cert =
       ImportCertFromFile(certs_dir, "globalsign_ev_sha256_ca_cert.pem");
   ASSERT_NE(static_cast<X509Certificate*>(NULL), intermediate_cert);
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   CertVerifyResult verify_result;
   int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
               X509Certificate::VERIFY_EV_CERT;
   int error = cert_chain->Verify("2029.globalsign.com", flags, &verify_result);
   if (error == OK)
     EXPECT_NE(0, verify_result.cert_status & CERT_STATUS_IS_EV);
   else
     EXPECT_EQ(ERR_CERT_DATE_INVALID, error);
 }
 
 TEST(X509CertificateTest, TestKnownRoot) {
   FilePath certs_dir = GetTestCertsDirectory();
   scoped_refptr<X509Certificate> cert =
       ImportCertFromFile(certs_dir, "nist.der");
   ASSERT_NE(static_cast<X509Certificate*>(NULL), cert);
 
   // This intermediate is only needed for old Linux machines. Modern NSS
   // includes it as a root already.
   scoped_refptr<X509Certificate> intermediate_cert =
       ImportCertFromFile(certs_dir, "nist_intermediate.der");
   ASSERT_NE(static_cast<X509Certificate*>(NULL), intermediate_cert);
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
   CertVerifyResult verify_result;
   // This is going to blow up in Feb 2012. Sorry! Disable and file a bug
   // against agl. Also see PublicKeyHashes in this file.
   int error = cert_chain->Verify("www.nist.gov", flags, &verify_result);
   EXPECT_EQ(OK, error);
   EXPECT_EQ(0, verify_result.cert_status);
   EXPECT_TRUE(verify_result.is_issued_by_known_root);
@@ skipping 55 matching lines @@
   scoped_refptr<X509Certificate> intermediate_cert =
       ImportCertFromFile(certs_dir, "nist_intermediate.der");
   ASSERT_NE(static_cast<X509Certificate*>(NULL), intermediate_cert);
 
   TestRootCerts::GetInstance()->Add(intermediate_cert.get());
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
   CertVerifyResult verify_result;
 
   int error = cert_chain->Verify("www.nist.gov", flags, &verify_result);
   EXPECT_EQ(OK, error);
   EXPECT_EQ(0, verify_result.cert_status);
   ASSERT_LE(2u, verify_result.public_key_hashes.size());
   EXPECT_EQ(HexEncode(nistSPKIHash, base::SHA1_LENGTH),
@@ skipping 27 matching lines @@
   EXPECT_NE(0, verify_result.cert_status & CERT_STATUS_INVALID);
 #endif
   // TODO(wtc): fix http://crbug.com/75520 to get all the certificate errors
   // from NSS.
 #if !defined(USE_NSS)
   // The certificate is issued by an unknown CA.
   EXPECT_NE(0, verify_result.cert_status & CERT_STATUS_AUTHORITY_INVALID);
 #endif
 }
 
-// Tests X509Certificate::Cache via X509Certificate::CreateFromHandle.  We
+// Tests X509CertificateCache via X509Certificate::CreateFromHandle.  We
 // call X509Certificate::CreateFromHandle several times and observe whether
 // it returns a cached or new X509Certificate object.

[wtc, 2011/07/17 01:55:32]

The second sentence of this paragraph should be removed or
updated.

 //
 // All the OS certificate handles in this test are actually from the same
 // source (the bytes of a lone certificate), but we pretend that some of them
 // come from the network.

[wtc, 2011/07/17 01:55:32]

Delete this paragraph.

 TEST(X509CertificateTest, Cache) {
   X509Certificate::OSCertHandle google_cert_handle;
+  X509Certificate::OSCertHandle thawte_cert_handle;
 
-  // Add a certificate from the source SOURCE_LONE_CERT_IMPORT to our
-  // certificate cache.
+  // Add a single certificate to the certificate cache.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert1(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, X509Certificate::OSCertHandles()));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
 
-  // Add a certificate from the same source (SOURCE_LONE_CERT_IMPORT).  This
-  // should return the cached certificate (cert1).
+  // Add the same certificate, but as a new handle.

[wtc, 2011/07/17 01:55:32]

Note: NSS should return the same handle here.

   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert2(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, X509Certificate::OSCertHandles()));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
 
-  EXPECT_EQ(cert1, cert2);
+  // A new X509Certificate should be returned.
+  EXPECT_NE(cert1.get(), cert2.get());
+  // But both instances should share the underlying OS certificate handle.
+  EXPECT_EQ(cert1->os_cert_handle(), cert2->os_cert_handle());
+  EXPECT_TRUE(cert1->HasIntermediateCertificates(
+      cert2->GetIntermediateCertificates()));

[wtc, 2011/07/17 01:55:32]

Since cert1 and cert2 don't have intermediate CA certificates,
this check (lines 691-692) is not effective.

Note: HasIntermediateCertificates uses IsSameOSCert rather
than OS cert handle value equality, so that weakens the
check for the new X509CertificateCache even if cert1 and
cert2 have intermediate CA certificates.

 
-  // Add a certificate from the network.  This should kick out the original
-  // cached certificate (cert1) and return a new certificate.
+  // Add the same certificate, but this time with an intermediate. This
+  // should result in the intermediate being cached. Note that this is not
+  // a legitimate chain, but is suitable for testing.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
+  thawte_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(thawte_cert_handle);
   scoped_refptr<X509Certificate> cert3(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, intermediates));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
+  X509Certificate::FreeOSCertHandle(thawte_cert_handle);
 
-  EXPECT_NE(cert1, cert3);
-
-  // Add one certificate from each source.  Both should return the new cached
-  // certificate (cert3).
-  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
-      reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  scoped_refptr<X509Certificate> cert4(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
-  X509Certificate::FreeOSCertHandle(google_cert_handle);
-
-  EXPECT_EQ(cert3, cert4);
-
-  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
-      reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  scoped_refptr<X509Certificate> cert5(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
-  X509Certificate::FreeOSCertHandle(google_cert_handle);
-
-  EXPECT_EQ(cert3, cert5);
+  // Test that the new certificate, even with intermediates, results in the
+  // same underlying handle being used.
+  EXPECT_EQ(cert1->os_cert_handle(), cert3->os_cert_handle());
+  // Though they use the same OS handle, the intermediates should be different.
+  EXPECT_FALSE(cert1->HasIntermediateCertificates(
+      cert3->GetIntermediateCertificates()));
 }
 
 TEST(X509CertificateTest, Pickle) {
   X509Certificate::OSCertHandle google_cert_handle =
       X509Certificate::CreateOSCertHandleFromBytes(
           reinterpret_cast<const char*>(google_der), sizeof(google_der));
   X509Certificate::OSCertHandle thawte_cert_handle =
       X509Certificate::CreateOSCertHandleFromBytes(
           reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(thawte_cert_handle);
-  // Faking SOURCE_LONE_CERT_IMPORT so that when the pickled certificate is
-  // read, it successfully evicts |cert| from the X509Certificate::Cache.
-  // This will be fixed when http://crbug.com/49377 is fixed.
   scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
-      google_cert_handle,
-      X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      intermediates);
+      google_cert_handle, intermediates);
   ASSERT_NE(static_cast<X509Certificate*>(NULL), cert.get());
 
   X509Certificate::FreeOSCertHandle(google_cert_handle);
   X509Certificate::FreeOSCertHandle(thawte_cert_handle);
 
   Pickle pickle;
   cert->Persist(&pickle);
 
   void* iter = NULL;
   scoped_refptr<X509Certificate> cert_from_pickle =
       X509Certificate::CreateFromPickle(
           pickle, &iter, X509Certificate::PICKLETYPE_CERTIFICATE_CHAIN);
   ASSERT_NE(static_cast<X509Certificate*>(NULL), cert_from_pickle);
   EXPECT_NE(cert.get(), cert_from_pickle.get());

[wtc, 2011/07/17 01:55:32]

Consider removing this check, irrelevant in the new code. 
This check was linked to the "Faking SOURCE_LONE_CERT_IMPORT
..." comment in the old code.

   EXPECT_TRUE(X509Certificate::IsSameOSCert(
       cert->os_cert_handle(), cert_from_pickle->os_cert_handle()));
   EXPECT_TRUE(cert->HasIntermediateCertificates(
       cert_from_pickle->GetIntermediateCertificates()));
 }
 
 TEST(X509CertificateTest, Policy) {
   scoped_refptr<X509Certificate> google_cert(X509Certificate::CreateFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der)));
 
@@ skipping 22 matching lines @@
   EXPECT_TRUE(policy.HasDeniedCert());
 
   policy.Allow(webkit_cert.get());
 
   EXPECT_EQ(policy.Check(google_cert.get()), CertPolicy::DENIED);
   EXPECT_EQ(policy.Check(webkit_cert.get()), CertPolicy::ALLOWED);
   EXPECT_TRUE(policy.HasAllowedCert());
   EXPECT_TRUE(policy.HasDeniedCert());
 }
 
-#if defined(OS_MACOSX) || defined(OS_WIN)
 TEST(X509CertificateTest, IntermediateCertificates) {
   scoped_refptr<X509Certificate> webkit_cert(
       X509Certificate::CreateFromBytes(
           reinterpret_cast<const char*>(webkit_der), sizeof(webkit_der)));
 
   scoped_refptr<X509Certificate> thawte_cert(
       X509Certificate::CreateFromBytes(
           reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der)));
 
   scoped_refptr<X509Certificate> paypal_cert(
       X509Certificate::CreateFromBytes(
           reinterpret_cast<const char*>(paypal_null_der),
           sizeof(paypal_null_der)));
 
   X509Certificate::OSCertHandle google_handle;
   // Create object with no intermediates:
   google_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   X509Certificate::OSCertHandles intermediates1;
   scoped_refptr<X509Certificate> cert1;
-  cert1 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates1);
+  cert1 = X509Certificate::CreateFromHandle(google_handle, intermediates1);
   EXPECT_TRUE(cert1->HasIntermediateCertificates(intermediates1));
   EXPECT_FALSE(cert1->HasIntermediateCertificate(
       webkit_cert->os_cert_handle()));
 
   // Create object with 2 intermediates:
   X509Certificate::OSCertHandles intermediates2;
   intermediates2.push_back(webkit_cert->os_cert_handle());
   intermediates2.push_back(thawte_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert2;
-  cert2 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates2);
+  cert2 = X509Certificate::CreateFromHandle(google_handle, intermediates2);
 
   // The cache should have stored cert2 'cause it has more intermediates:
   EXPECT_NE(cert1, cert2);

[wtc, 2011/07/17 01:55:32]

Delete this comment and check, just like you deleted the
comment and check on lines 854-855 of the old code.

Perhaps this entire unit test should be deleted.

 
   // Verify it has all the intermediates:
   EXPECT_TRUE(cert2->HasIntermediateCertificate(
       webkit_cert->os_cert_handle()));
   EXPECT_TRUE(cert2->HasIntermediateCertificate(
       thawte_cert->os_cert_handle()));
   EXPECT_FALSE(cert2->HasIntermediateCertificate(
       paypal_cert->os_cert_handle()));
 
-  // Create object with 1 intermediate:
-  X509Certificate::OSCertHandles intermediates3;
-  intermediates2.push_back(thawte_cert->os_cert_handle());
-  scoped_refptr<X509Certificate> cert3;
-  cert3 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates3);
-
-  // The cache should have returned cert2 'cause it has more intermediates:
-  EXPECT_EQ(cert3, cert2);
-
   // Cleanup
   X509Certificate::FreeOSCertHandle(google_handle);
 }
-#endif
 
 #if defined(OS_MACOSX)
 TEST(X509CertificateTest, IsIssuedBy) {
   FilePath certs_dir = GetTestCertsDirectory();
 
   // Test a client certificate from MIT.
   scoped_refptr<X509Certificate> mit_davidben_cert(
       ImportCertFromFile(certs_dir, "mit.davidben.der"));
   ASSERT_NE(static_cast<X509Certificate*>(NULL), mit_davidben_cert);
 
@@ skipping 297 matching lines @@
   EXPECT_EQ(test_data.expected,
             X509Certificate::VerifyHostname(test_data.hostname, cert_names))
       << "Host [" << test_data.hostname
       << "], cert name [" << test_data.cert_names << "]";
 }
 
 INSTANTIATE_TEST_CASE_P(, X509CertificateNameVerifyTest,
                         testing::ValuesIn(kNameVerifyTestData));
 
 }  // namespace net