Work around our not caching the intermediate CA

certificates by passing the source of each OSCertHandle to
CreateFromHandle and the X509Certificate constructor.  If
the OSCertHandle comes from the network layer, we know it
has a complete certificate chain and therefore prefer it to
an OSCertHandle that comes from the HTTP cache, which
doesn't have the intermediate CA certificates.  A
certificate from the network layer can kick out a
certificate from the HTTP cache in our certificate cache.
This workaround seems good enough to fix all the known
symptoms of not caching the intermediate CA certificates.

Move the common code in x509_certificate_<os>.cc to
x509_certificate.cc.

R=eroman
BUG=3154,7065
Review URL: http://codereview.chromium.org/18836

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@8864 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 301 insertions, 256 deletions

diff --git a/net/base/ssl_client_socket_mac.cc b/net/base/ssl_client_socket_mac.cc
index f4b636e..24d33e5 100644
--- a/net/base/ssl_client_socket_mac.cc
+++ b/net/base/ssl_client_socket_mac.cc
@@ -380,7 +380,8 @@ void SSLClientSocketMac::GetSSLInfo(SSLInfo* ssl_info) {
         static_cast<SecCertificateRef>(
           const_cast<void*>(CFArrayGetValueAtIndex(certs, 0)));
     CFRetain(client_cert);
-    ssl_info->cert = X509Certificate::CreateFromHandle(client_cert);
+    ssl_info->cert = X509Certificate::CreateFromHandle(
+        client_cert, X509Certificate::SOURCE_FROM_NETWORK);
     CFRelease(certs);
   }
   
diff --git a/net/base/ssl_client_socket_win.cc b/net/base/ssl_client_socket_win.cc
index beeaba9..1f4c314 100644
--- a/net/base/ssl_client_socket_win.cc
+++ b/net/base/ssl_client_socket_win.cc
@@ -359,7 +359,8 @@ void SSLClientSocketWin::GetSSLInfo(SSLInfo* ssl_info) {
   if (status == SEC_E_OK) {
     DCHECK(server_cert_);
     PCCERT_CONTEXT dup_cert = CertDuplicateCertificateContext(server_cert_);
-    ssl_info->cert = X509Certificate::CreateFromHandle(dup_cert);
+    ssl_info->cert = X509Certificate::CreateFromHandle(
+        dup_cert, X509Certificate::SOURCE_FROM_NETWORK);
   }
   SecPkgContext_ConnectionInfo connection_info;
   status = QueryContextAttributes(&ctxt_,
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index d62f963..443ef81 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -4,6 +4,7 @@
 
 #include "net/base/x509_certificate.h"
 
+#include "base/histogram.h"
 #include "base/logging.h"
 
 namespace net {
@@ -122,5 +123,66 @@ void X509Certificate::Policy::Deny(X509Certificate* cert) {
   denied_.insert(cert->fingerprint());
 }
 
+// static
+X509Certificate* X509Certificate::CreateFromHandle(OSCertHandle cert_handle,
+                                                   Source source) {
+  DCHECK(cert_handle);
+  DCHECK(source != SOURCE_UNUSED);
+
+  // Check if we already have this certificate in memory.
+  X509Certificate::Cache* cache = X509Certificate::Cache::GetInstance();
+  X509Certificate* cached_cert =
+      cache->Find(CalculateFingerprint(cert_handle));
+  if (cached_cert) {
+    DCHECK(cached_cert->source_ != SOURCE_UNUSED);
+    if (cached_cert->source_ >= source) {
+      // We've found a certificate with the same fingerprint in our cache.  We
+      // own the |cert_handle|, which makes it our job to free it.
+      FreeOSCertHandle(cert_handle);
+      DHISTOGRAM_COUNTS(L"X509CertificateReuseCount", 1);
+      return cached_cert;
+    }
+    // Kick out the old certificate from our cache.  The new one is better.
+    cache->Remove(cached_cert);
+  }
+  // Otherwise, allocate a new object.
+  return new X509Certificate(cert_handle, source);
+}
+
+// static
+X509Certificate* X509Certificate::CreateFromBytes(const char* data,
+                                                  int length) {
+  OSCertHandle cert_handle = CreateOSCertHandleFromBytes(data, length);
+  if (!cert_handle)
+    return NULL;
+
+  return CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT);
+}
+
+X509Certificate::X509Certificate(OSCertHandle cert_handle, Source source)
+    : cert_handle_(cert_handle), source_(source) {
+  Initialize();
+}
+
+X509Certificate::X509Certificate(const std::string& subject,
+                                 const std::string& issuer,
+                                 base::Time start_date,
+                                 base::Time expiration_date)
+    : subject_(subject),
+      issuer_(issuer),
+      valid_start_(start_date),
+      valid_expiry_(expiration_date),
+      cert_handle_(NULL),
+      source_(SOURCE_UNUSED) {
+  memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
+}
+
+X509Certificate::~X509Certificate() {
+  // We might not be in the cache, but it is safe to remove ourselves anyway.
+  X509Certificate::Cache::GetInstance()->Remove(this);
+  if (cert_handle_)
+    FreeOSCertHandle(cert_handle_);
+}
+
 }  // namespace net
 
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index 4e41570..9127cc6 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -13,6 +13,7 @@
 #include "base/ref_counted.h"
 #include "base/singleton.h"
 #include "base/time.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
@@ -112,10 +113,25 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
     std::set<Fingerprint, FingerprintLessThan> denied_;
   };
 
+  // Where the certificate comes from.  The enumeration constants are
+  // listed in increasing order of preference.
+  enum Source {
+    SOURCE_UNUSED = 0,            // The source_ member is not used.
+    SOURCE_LONE_CERT_IMPORT = 1,  // From importing a certificate without
+                                  // its intermediate CA certificates.
+    SOURCE_FROM_NETWORK = 2,      // From the network.
+  };
+
   // Create an X509Certificate from a handle to the certificate object
   // in the underlying crypto library. This is a transfer of ownership;
   // X509Certificate will properly dispose of |cert_handle| for you.
-  static X509Certificate* CreateFromHandle(OSCertHandle cert_handle);
+  // |source| specifies where |cert_handle| comes from.  Given two
+  // certificate handles for the same certificate, our certificate cache
+  // prefers the handle from the network because our HTTP cache isn't
+  // caching the corresponding intermediate CA certificates yet
+  // (http://crbug.com/7065).
+  static X509Certificate* CreateFromHandle(OSCertHandle cert_handle,
+                                           Source source);
 
   // Create an X509Certificate from the BER-encoded representation.
   // Returns NULL on failure.
@@ -130,7 +146,7 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
 
   // Creates a X509Certificate from the ground up.  Used by tests that simulate
   // SSL connections.
-  X509Certificate(std::string subject, std::string issuer,
+  X509Certificate(const std::string& subject, const std::string& issuer,
                   base::Time start_date, base::Time expiration_date);
 
   // Appends a representation of this object to the given pickle.
@@ -172,6 +188,9 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   OSCertHandle os_cert_handle() const { return cert_handle_; }
 
  private:
+  friend class base::RefCountedThreadSafe<X509Certificate>;
+  FRIEND_TEST(X509CertificateTest, Cache);
+
   // A cache of X509Certificate objects.
   class Cache {
    public:
@@ -200,14 +219,25 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
 
   // Construct an X509Certificate from a handle to the certificate object
   // in the underlying crypto library.
-  explicit X509Certificate(OSCertHandle cert_handle);
+  X509Certificate(OSCertHandle cert_handle, Source source);
 
-  friend class base::RefCountedThreadSafe<X509Certificate>;
   ~X509Certificate();
 
   // Common object initialization code.  Called by the constructors only.
   void Initialize();
 
+  // Creates an OS certificate handle from the BER-encoded representation.
+  // Returns NULL on failure.
+  static OSCertHandle CreateOSCertHandleFromBytes(const char* data,
+                                                  int length);
+
+  // Frees an OS certificate handle.
+  static void FreeOSCertHandle(OSCertHandle cert_handle);
+
+  // Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
+  // (all zero) fingerprint on failure.
+  static Fingerprint CalculateFingerprint(OSCertHandle cert_handle);
+
   // The subject of the certificate.
   Principal subject_;
 
@@ -226,6 +256,9 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   // A handle to the certificate object in the underlying crypto library.
   OSCertHandle cert_handle_;
 
+  // Where the certificate comes from.
+  Source source_;
+
   DISALLOW_COPY_AND_ASSIGN(X509Certificate);
 };
 
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 11191bd..ac60645 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -7,7 +7,6 @@
 #include <CommonCrypto/CommonDigest.h>
 #include <time.h>
 
-#include "base/histogram.h"
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "net/base/cert_status_flags.h"
@@ -19,26 +18,6 @@ namespace net {
 
 namespace {
 
-// Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
-// (all zero) fingerprint on failure.
-X509Certificate::Fingerprint CalculateFingerprint(
-    X509Certificate::OSCertHandle cert) {
-  X509Certificate::Fingerprint sha1;
-  memset(sha1.data, 0, sizeof(sha1.data));
-  
-  CSSM_DATA cert_data;
-  OSStatus status = SecCertificateGetData(cert, &cert_data);
-  if (status)
-    return sha1;
-  
-  DCHECK(NULL != cert_data.Data);
-  DCHECK(0 != cert_data.Length);
-  
-  CC_SHA1(cert_data.Data, cert_data.Length, sha1.data);
-
-  return sha1;
-}
-
 inline bool CSSMOIDEqual(const CSSM_OID* oid1, const CSSM_OID* oid2) {
   return oid1->Length == oid2->Length &&
       (memcmp(oid1->Data, oid2->Data, oid1->Length) == 0);
@@ -242,42 +221,6 @@ void X509Certificate::Initialize() {
 }
 
 // static
-X509Certificate* X509Certificate::CreateFromHandle(OSCertHandle cert_handle) {
-  DCHECK(cert_handle);
-
-  // Check if we already have this certificate in memory.
-  X509Certificate::Cache* cache = X509Certificate::Cache::GetInstance();
-  X509Certificate* cert = cache->Find(CalculateFingerprint(cert_handle));
-  if (cert) {
-    // We've found a certificate with the same fingerprint in our cache.  We own
-    // the |cert_handle|, which makes it our job to free it.
-    CFRelease(cert_handle);
-    DHISTOGRAM_COUNTS(L"X509CertificateReuseCount", 1);
-    return cert;
-  }
-  // Otherwise, allocate a new object.
-  return new X509Certificate(cert_handle);
-}
-
-// static
-X509Certificate* X509Certificate::CreateFromBytes(const char* data,
-                                                  int length) {
-  CSSM_DATA cert_data;
-  cert_data.Data = const_cast<uint8*>(reinterpret_cast<const uint8*>(data));
-  cert_data.Length = length;
-
-  OSCertHandle cert_handle = NULL;
-  OSStatus status = SecCertificateCreateFromData(&cert_data,
-                                                 CSSM_CERT_X_509v3,
-                                                 CSSM_CERT_ENCODING_BER,
-                                                 &cert_handle);
-  if (status)
-    return NULL;
-
-  return CreateFromHandle(cert_handle);
-}
-
-// static
 X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
                                                    void** pickle_iter) {
   const char* data;
@@ -288,21 +231,6 @@ X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
   return CreateFromBytes(data, length);
 }
 
-X509Certificate::X509Certificate(OSCertHandle cert_handle)
-    : cert_handle_(cert_handle) {
-  Initialize();
-}
-
-X509Certificate::X509Certificate(std::string subject, std::string issuer,
-                                 Time start_date, Time expiration_date)
-    : subject_(subject),
-      issuer_(issuer),
-      valid_start_(start_date),
-      valid_expiry_(expiration_date),
-      cert_handle_(NULL) {
-  memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
-}
-
 void X509Certificate::Persist(Pickle* pickle) {
   CSSM_DATA cert_data;
   OSStatus status = SecCertificateGetData(cert_handle_, &cert_data);
@@ -314,13 +242,6 @@ void X509Certificate::Persist(Pickle* pickle) {
   pickle->WriteData(reinterpret_cast<char*>(cert_data.Data), cert_data.Length);
 }
 
-X509Certificate::~X509Certificate() {
-  // We might not be in the cache, but it is safe to remove ourselves anyway.
-  X509Certificate::Cache::GetInstance()->Remove(this);
-  if (cert_handle_)
-    CFRelease(cert_handle_);
-}
-
 void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
   dns_names->clear();
   
@@ -345,4 +266,46 @@ bool X509Certificate::IsEV(int cert_status) const {
   return false;
 }
 
+// static
+X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
+    const char* data, int length) {
+  CSSM_DATA cert_data;
+  cert_data.Data = const_cast<uint8*>(reinterpret_cast<const uint8*>(data));
+  cert_data.Length = length;
+
+  OSCertHandle cert_handle = NULL;
+  OSStatus status = SecCertificateCreateFromData(&cert_data,
+                                                 CSSM_CERT_X_509v3,
+                                                 CSSM_CERT_ENCODING_BER,
+                                                 &cert_handle);
+  if (status)
+    return NULL;
+
+  return cert_handle;
+}
+
+// static
+void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
+  CFRelease(cert_handle);
+}
+
+// static
+X509Certificate::Fingerprint X509Certificate::CalculateFingerprint(
+    OSCertHandle cert) {
+  Fingerprint sha1;
+  memset(sha1.data, 0, sizeof(sha1.data));
+
+  CSSM_DATA cert_data;
+  OSStatus status = SecCertificateGetData(cert, &cert_data);
+  if (status)
+    return sha1;
+
+  DCHECK(NULL != cert_data.Data);
+  DCHECK(0 != cert_data.Length);
+
+  CC_SHA1(cert_data.Data, cert_data.Length, sha1.data);
+
+  return sha1;
+}
+
 }  // namespace net
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index ef4b8cd..2ca1255 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -13,30 +13,12 @@
 #include <sechash.h>
 #undef Lock
 
-#include "base/histogram.h"
 #include "base/logging.h"
 #include "base/time.h"
 #include "base/nss_init.h"
 
 namespace net {
 
-// Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
-// (all zero) fingerprint on failure.
-X509Certificate::Fingerprint CalculateFingerprint(
-    X509Certificate::OSCertHandle cert) {
-  X509Certificate::Fingerprint sha1;
-  memset(sha1.data, 0, sizeof(sha1.data));
-  
-  DCHECK(NULL != cert->derCert.data);
-  DCHECK(0 != cert->derCert.len);
-  
-  SECStatus rv = HASH_HashBuf(HASH_AlgSHA1, sha1.data, 
-                              cert->derCert.data, cert->derCert.len);
-  DCHECK(rv == SECSuccess);
-
-  return sha1;
-}
-
 namespace {
 
 // TODO(port): Implement this more simply, and put it in the right place
@@ -57,11 +39,11 @@ base::Time PRTimeToBaseTime(PRTime prtime) {
   return base::Time::FromUTCExploded(exploded);
 }
 
-void ParsePrincipal(SECItem *der_name,
+void ParsePrincipal(SECItem* der_name,
                     X509Certificate::Principal* principal) {
 
   CERTName name;
-  PRArenaPool *arena = NULL;
+  PRArenaPool* arena = NULL;
 
   arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
   DCHECK(arena != NULL);
@@ -110,7 +92,7 @@ void ParsePrincipal(SECItem *der_name,
           SECItem* decode_item = CERT_DecodeAVAValue(&avas[pair]->value);
           if (!decode_item)
             break;
-          std::string value(reinterpret_cast<char *>(decode_item->data), 
+          std::string value(reinterpret_cast<char*>(decode_item->data), 
                             decode_item->len);
           values[oid]->push_back(value);
           SECITEM_FreeItem(decode_item, PR_TRUE);
@@ -165,9 +147,9 @@ void GetCertSubjectAltNamesOfType(X509Certificate::OSCertHandle cert_handle,
            name->type == certDNSName ||
            name->type == certURI);
     if (name->type == name_type) {
-      unsigned char *p = name->name.other.data;
+      unsigned char* p = name->name.other.data;
       int len = name->name.other.len;
-      std::string value = std::string(reinterpret_cast<char *>(p), len);
+      std::string value = std::string(reinterpret_cast<char*>(p), len);
       result->push_back(value);
     }
     name = CERT_GetNextGeneralName(name);
@@ -193,63 +175,16 @@ void X509Certificate::Initialize() {
 }
 
 // static
-X509Certificate* X509Certificate::CreateFromHandle(OSCertHandle cert_handle) {
-  DCHECK(cert_handle);
-
-  // Check if we already have this certificate in memory.
-  X509Certificate::Cache* cache = X509Certificate::Cache::GetInstance();
-  X509Certificate* cert = cache->Find(CalculateFingerprint(cert_handle));
-  if (cert) {
-    // We've found a certificate with the same fingerprint in our cache.  We own
-    // the |cert_handle|, which makes it our job to free it.
-    CERT_DestroyCertificate(cert_handle);
-    DHISTOGRAM_COUNTS(L"X509CertificateReuseCount", 1);
-    return cert;
-  }
-  // Otherwise, allocate a new object.
-  return new X509Certificate(cert_handle);
-}
-
-// static
-X509Certificate* X509Certificate::CreateFromBytes(const char* data,
-                                                  int length) {
-  base::EnsureNSSInit();
-
-  SECItem der_cert;
-  der_cert.data = reinterpret_cast<unsigned char *>(const_cast<char *>(data));
-  der_cert.len = length;
-  OSCertHandle cert_handle = 
-      CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &der_cert, 
-                              NULL, PR_FALSE, PR_TRUE);
-  if (!cert_handle)
-    return NULL;
-
-  return CreateFromHandle(cert_handle);
-}
-
-// static
 X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
                                                    void** pickle_iter) {
   NOTIMPLEMENTED();
   return NULL;
 }
 
-X509Certificate::X509Certificate(OSCertHandle cert_handle)
-    : cert_handle_(cert_handle) {
-  Initialize();
-}
-
 void X509Certificate::Persist(Pickle* pickle) {
   NOTIMPLEMENTED();
 }
 
-X509Certificate::~X509Certificate() {
-  // We might not be in the cache, but it is safe to remove ourselves anyway.
-  X509Certificate::Cache::GetInstance()->Remove(this);
-  if (cert_handle_)
-    CERT_DestroyCertificate(cert_handle_);
-}
-
 void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
   dns_names->clear();
 
@@ -263,6 +198,39 @@ void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
   if (dns_names->empty())
     dns_names->push_back(subject_.common_name);
 }
+
+// static
+X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
+    const char* data, int length) {
+  base::EnsureNSSInit();
+
+  SECItem der_cert;
+  der_cert.data = reinterpret_cast<unsigned char*>(const_cast<char*>(data));
+  der_cert.len = length;
+  return CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &der_cert,
+                                 NULL, PR_FALSE, PR_TRUE);
+}
+
+// static
+void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
+  CERT_DestroyCertificate(cert_handle);
+}
+
+// static
+X509Certificate::Fingerprint X509Certificate::CalculateFingerprint(
+    OSCertHandle cert) {
+  Fingerprint sha1;
+  memset(sha1.data, 0, sizeof(sha1.data));
+
+  DCHECK(NULL != cert->derCert.data);
+  DCHECK(0 != cert->derCert.len);
+
+  SECStatus rv = HASH_HashBuf(HASH_AlgSHA1, sha1.data,
+                              cert->derCert.data, cert->derCert.len);
+  DCHECK(rv == SECSuccess);
+
+  return sha1;
+}
   
 }  // namespace net
 
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 499b914..bf98b5c 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -18,11 +18,8 @@ using base::Time;
 
 namespace {
 
-class X509CertificateTest : public testing::Test {
-};
-
 // Certificates for test data. They're obtained with:
-// 
+//
 // $ openssl s_client -connect [host]:443 -showcerts
 // $ openssl x509 -inform PEM -outform DER > /tmp/host.der
 // $ xxd -i /tmp/host.der
@@ -332,13 +329,13 @@ unsigned char thawte_fingerprint[] = {
 
 }  // namespace
 
-using net::X509Certificate;
+namespace net {
 
 TEST(X509CertificateTest, GoogleCertParsing) {
   scoped_refptr<X509Certificate> google_cert = X509Certificate::CreateFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  
-  ASSERT_NE(static_cast<X509Certificate *>(NULL), google_cert);
+
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), google_cert);
 
   const X509Certificate::Principal& subject = google_cert->subject();
   EXPECT_EQ("www.google.com", subject.common_name);
@@ -350,7 +347,7 @@ TEST(X509CertificateTest, GoogleCertParsing) {
   EXPECT_EQ("Google Inc", subject.organization_names[0]);
   EXPECT_EQ(0U, subject.organization_unit_names.size());
   EXPECT_EQ(0U, subject.domain_components.size());
-  
+
   const X509Certificate::Principal& issuer = google_cert->issuer();
   EXPECT_EQ("Thawte SGC CA", issuer.common_name);
   EXPECT_EQ("", issuer.locality_name);
@@ -361,14 +358,14 @@ TEST(X509CertificateTest, GoogleCertParsing) {
   EXPECT_EQ("Thawte Consulting (Pty) Ltd.", issuer.organization_names[0]);
   EXPECT_EQ(0U, issuer.organization_unit_names.size());
   EXPECT_EQ(0U, issuer.domain_components.size());
-  
+
   // Use DoubleT because its epoch is the same on all platforms
   const Time& valid_start = google_cert->valid_start();
   EXPECT_EQ(1209747775, valid_start.ToDoubleT());
-  
+
   const Time& valid_expiry = google_cert->valid_expiry();
   EXPECT_EQ(1241283775, valid_expiry.ToDoubleT());
-  
+
   const X509Certificate::Fingerprint& fingerprint = google_cert->fingerprint();
   for (size_t i = 0; i < 20; ++i)
     EXPECT_EQ(google_fingerprint[i], fingerprint.data[i]);
@@ -377,7 +374,7 @@ TEST(X509CertificateTest, GoogleCertParsing) {
   google_cert->GetDNSNames(&dns_names);
   EXPECT_EQ(1U, dns_names.size());
   EXPECT_EQ("www.google.com", dns_names[0]);
-  
+
 #if ALLOW_EXTERNAL_ACCESS && defined(OS_WIN)
   // TODO(avi): turn this on for the Mac once EV checking is implemented.
   EXPECT_EQ(false, google_cert->IsEV(net::CERT_STATUS_REV_CHECKING_ENABLED));
@@ -387,8 +384,8 @@ TEST(X509CertificateTest, GoogleCertParsing) {
 TEST(X509CertificateTest, WebkitCertParsing) {
   scoped_refptr<X509Certificate> webkit_cert = X509Certificate::CreateFromBytes(
       reinterpret_cast<const char*>(webkit_der), sizeof(webkit_der));
-  
-  ASSERT_NE(static_cast<X509Certificate *>(NULL), webkit_cert);
+
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), webkit_cert);
 
   const X509Certificate::Principal& subject = webkit_cert->subject();
   EXPECT_EQ("Cupertino", subject.locality_name);
@@ -400,7 +397,7 @@ TEST(X509CertificateTest, WebkitCertParsing) {
   EXPECT_EQ(1U, subject.organization_unit_names.size());
   EXPECT_EQ("Mac OS Forge", subject.organization_unit_names[0]);
   EXPECT_EQ(0U, subject.domain_components.size());
-  
+
   const X509Certificate::Principal& issuer = webkit_cert->issuer();
   EXPECT_EQ("Go Daddy Secure Certification Authority", issuer.common_name);
   EXPECT_EQ("Scottsdale", issuer.locality_name);
@@ -413,14 +410,14 @@ TEST(X509CertificateTest, WebkitCertParsing) {
   EXPECT_EQ("http://certificates.godaddy.com/repository",
       issuer.organization_unit_names[0]);
   EXPECT_EQ(0U, issuer.domain_components.size());
-  
+
   // Use DoubleT because its epoch is the same on all platforms
   const Time& valid_start = webkit_cert->valid_start();
   EXPECT_EQ(1205883319, valid_start.ToDoubleT());
-  
+
   const Time& valid_expiry = webkit_cert->valid_expiry();
   EXPECT_EQ(1300491319, valid_expiry.ToDoubleT());
-  
+
   const X509Certificate::Fingerprint& fingerprint = webkit_cert->fingerprint();
   for (size_t i = 0; i < 20; ++i)
     EXPECT_EQ(webkit_fingerprint[i], fingerprint.data[i]);
@@ -430,7 +427,7 @@ TEST(X509CertificateTest, WebkitCertParsing) {
   EXPECT_EQ(2U, dns_names.size());
   EXPECT_EQ("*.webkit.org", dns_names[0]);
   EXPECT_EQ("webkit.org", dns_names[1]);
-  
+
 #if ALLOW_EXTERNAL_ACCESS && defined(OS_WIN)
   EXPECT_EQ(false, webkit_cert->IsEV(net::CERT_STATUS_REV_CHECKING_ENABLED));
 #endif
@@ -439,8 +436,8 @@ TEST(X509CertificateTest, WebkitCertParsing) {
 TEST(X509CertificateTest, ThawteCertParsing) {
   scoped_refptr<X509Certificate> thawte_cert = X509Certificate::CreateFromBytes(
       reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
-  
-  ASSERT_NE(static_cast<X509Certificate *>(NULL), thawte_cert);
+
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), thawte_cert);
 
   const X509Certificate::Principal& subject = thawte_cert->subject();
   EXPECT_EQ("www.thawte.com", subject.common_name);
@@ -452,7 +449,7 @@ TEST(X509CertificateTest, ThawteCertParsing) {
   EXPECT_EQ("Thawte Inc", subject.organization_names[0]);
   EXPECT_EQ(0U, subject.organization_unit_names.size());
   EXPECT_EQ(0U, subject.domain_components.size());
-  
+
   const X509Certificate::Principal& issuer = thawte_cert->issuer();
   EXPECT_EQ("thawte Extended Validation SSL CA", issuer.common_name);
   EXPECT_EQ("", issuer.locality_name);
@@ -465,14 +462,14 @@ TEST(X509CertificateTest, ThawteCertParsing) {
   EXPECT_EQ("Terms of use at https://www.thawte.com/cps (c)06",
             issuer.organization_unit_names[0]);
   EXPECT_EQ(0U, issuer.domain_components.size());
-  
+
   // Use DoubleT because its epoch is the same on all platforms
   const Time& valid_start = thawte_cert->valid_start();
   EXPECT_EQ(1169078400, valid_start.ToDoubleT());
-  
+
   const Time& valid_expiry = thawte_cert->valid_expiry();
   EXPECT_EQ(1232236799, valid_expiry.ToDoubleT());
-  
+
   const X509Certificate::Fingerprint& fingerprint = thawte_cert->fingerprint();
   for (size_t i = 0; i < 20; ++i)
     EXPECT_EQ(thawte_fingerprint[i], fingerprint.data[i]);
@@ -481,7 +478,7 @@ TEST(X509CertificateTest, ThawteCertParsing) {
   thawte_cert->GetDNSNames(&dns_names);
   EXPECT_EQ(1U, dns_names.size());
   EXPECT_EQ("www.thawte.com", dns_names[0]);
-  
+
 #if ALLOW_EXTERNAL_ACCESS && defined(OS_WIN)
   // EV cert verification requires revocation checking.
   EXPECT_EQ(true, thawte_cert->IsEV(net::CERT_STATUS_REV_CHECKING_ENABLED));
@@ -490,3 +487,57 @@ TEST(X509CertificateTest, ThawteCertParsing) {
   EXPECT_EQ(false, thawte_cert->IsEV(0));
 #endif
 }
+
+// Tests X509Certificate::Cache via X509Certificate::CreateFromHandle.  We
+// call X509Certificate::CreateFromHandle several times and observe whether
+// it returns a cached or new X509Certificate object.
+//
+// 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.
+TEST(X509CertificateTest, Cache) {
+  X509Certificate::OSCertHandle google_cert_handle;
+
+  // Add a certificate from the source SOURCE_LONE_CERT_IMPORT to our
+  // 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);
+
+  // Add a certificate from the same source (SOURCE_LONE_CERT_IMPORT).  This
+  // should return the cached certificate (cert1).
+  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);
+
+  EXPECT_EQ(cert1, cert2);
+
+  // Add a certificate from the network.  This should kick out the original
+  // cached certificate (cert1) and return a new certificate.
+  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(google_der), sizeof(google_der));
+  scoped_refptr<X509Certificate> cert3 = X509Certificate::CreateFromHandle(
+      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK);
+
+  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);
+
+  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);
+
+  EXPECT_EQ(cert3, cert5);
+}
+
+}  // namespace net
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index 4e7c4ba..57ce947 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -4,7 +4,6 @@
 
 #include "net/base/x509_certificate.h"
 
-#include "base/histogram.h"
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "base/string_tokenizer.h"
@@ -21,23 +20,6 @@ namespace net {
 
 namespace {
 
-// Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
-// (all zero) fingerprint on failure.
-X509Certificate::Fingerprint CalculateFingerprint(PCCERT_CONTEXT cert) {
-  DCHECK(NULL != cert->pbCertEncoded);
-  DCHECK(0 != cert->cbCertEncoded);
-
-  BOOL rv;
-  X509Certificate::Fingerprint sha1;
-  DWORD sha1_size = sizeof(sha1.data);
-  rv = CryptHashCertificate(NULL, CALG_SHA1, 0, cert->pbCertEncoded,
-                            cert->cbCertEncoded, sha1.data, &sha1_size);
-  DCHECK(rv && sha1_size == sizeof(sha1.data));
-  if (!rv)
-    memset(sha1.data, 0, sizeof(sha1.data));
-  return sha1;
-}
-
 // Wrappers of malloc and free for CRYPT_DECODE_PARA, which requires the
 // WINAPI calling convention.
 void* WINAPI MyCryptAlloc(size_t size) {
@@ -255,39 +237,6 @@ void X509Certificate::Initialize() {
 }
 
 // static
-X509Certificate* X509Certificate::CreateFromHandle(OSCertHandle cert_handle) {
-  DCHECK(cert_handle);
-
-  // Check if we already have this certificate in memory.
-  X509Certificate::Cache* cache = X509Certificate::Cache::GetInstance();
-  X509Certificate* cert = cache->Find(CalculateFingerprint(cert_handle));
-  if (cert) {
-    // We've found a certificate with the same fingerprint in our cache.  We own
-    // the |cert_handle|, which makes it our job to free it.
-    CertFreeCertificateContext(cert_handle);
-    DHISTOGRAM_COUNTS(L"X509CertificateReuseCount", 1);
-    return cert;
-  }
-  // Otherwise, allocate a new object.
-  return new X509Certificate(cert_handle);
-}
-
-// static
-X509Certificate* X509Certificate::CreateFromBytes(const char* data,
-                                                  int length) {
-  OSCertHandle cert_handle = NULL;
-  if (!CertAddEncodedCertificateToStore(
-      NULL,  // the cert won't be persisted in any cert store
-      X509_ASN_ENCODING,
-      reinterpret_cast<const BYTE*>(data), length,
-      CERT_STORE_ADD_USE_EXISTING,
-      &cert_handle))
-    return NULL;
-
-  return CreateFromHandle(cert_handle);
-}
-
-// static
 X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
                                                    void** pickle_iter) {
   const char* data;
@@ -303,22 +252,7 @@ X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
       NULL, reinterpret_cast<const void **>(&cert_handle)))
     return NULL;
 
-  return CreateFromHandle(cert_handle);
-}
-
-X509Certificate::X509Certificate(OSCertHandle cert_handle)
-    : cert_handle_(cert_handle) {
-  Initialize();
-}
-
-X509Certificate::X509Certificate(std::string subject, std::string issuer,
-                                 Time start_date, Time expiration_date)
-    : subject_(subject),
-      issuer_(issuer),
-      valid_start_(start_date),
-      valid_expiry_(expiration_date),
-      cert_handle_(NULL) {
-  memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
+  return CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT);
 }
 
 void X509Certificate::Persist(Pickle* pickle) {
@@ -337,13 +271,6 @@ void X509Certificate::Persist(Pickle* pickle) {
   pickle->TrimWriteData(length);
 }
 
-X509Certificate::~X509Certificate() {
-  // We might not be in the cache, but it is safe to remove ourselves anyway.
-  X509Certificate::Cache::GetInstance()->Remove(this);
-  if (cert_handle_)
-    CertFreeCertificateContext(cert_handle_);
-}
-
 void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
   dns_names->clear();
   scoped_ptr_malloc<CERT_ALT_NAME_INFO> alt_name_info;
@@ -406,7 +333,7 @@ bool X509Certificate::IsEV(int cert_status) const {
 
   // Look up the EV policy OID of the root CA.
   PCCERT_CONTEXT root_cert = element[num_elements - 1]->pCertContext;
-  X509Certificate::Fingerprint fingerprint = CalculateFingerprint(root_cert);
+  Fingerprint fingerprint = CalculateFingerprint(root_cert);
   std::string ev_policy_oid;
   if (!metadata->GetPolicyOID(fingerprint, &ev_policy_oid))
     return false;
@@ -422,5 +349,42 @@ bool X509Certificate::IsEV(int cert_status) const {
   return ContainsPolicy(policies_info.get(), ev_policy_oid.c_str());
 }
 
+// static
+X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
+    const char* data, int length) {
+  OSCertHandle cert_handle = NULL;
+  if (!CertAddEncodedCertificateToStore(
+      NULL,  // the cert won't be persisted in any cert store
+      X509_ASN_ENCODING,
+      reinterpret_cast<const BYTE*>(data), length,
+      CERT_STORE_ADD_USE_EXISTING,
+      &cert_handle))
+    return NULL;
+
+  return cert_handle;
+}
+
+// static
+void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
+  CertFreeCertificateContext(cert_handle);
+}
+
+// static
+X509Certificate::Fingerprint X509Certificate::CalculateFingerprint(
+    OSCertHandle cert) {
+  DCHECK(NULL != cert->pbCertEncoded);
+  DCHECK(0 != cert->cbCertEncoded);
+
+  BOOL rv;
+  Fingerprint sha1;
+  DWORD sha1_size = sizeof(sha1.data);
+  rv = CryptHashCertificate(NULL, CALG_SHA1, 0, cert->pbCertEncoded,
+                            cert->cbCertEncoded, sha1.data, &sha1_size);
+  DCHECK(rv && sha1_size == sizeof(sha1.data));
+  if (!rv)
+    memset(sha1.data, 0, sizeof(sha1.data));
+  return sha1;
+}
+
 }  // namespace net
 
diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index fe0a130..accd7da 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -855,6 +855,8 @@ void HttpCache::Transaction::OnNetworkInfoAvailable(int result) {
       if (mode_ == READ_WRITE) {
         if (new_response->headers->response_code() == 304) {
           // Update cached response based on headers in new_response
+          // TODO(wtc): should we update cached certificate
+          // (response_.ssl_info), too?
           response_.headers->Update(*new_response->headers);
           if (response_.headers->HasHeaderValue("cache-control", "no-store")) {
             cache_->DoomEntry(cache_key_);