Fix a problem that cert trust change needs a chrome restart to be effective.

This seems to be caused by CertVerifier's verification result cache.
- Added a new OnCertTrustChanged to CertDatabase::Observer;
- For NSS cert database, SetCertTrust triggers OnCertTrustChanged;
- Clear CertVerifier's result cache when OnCertDatabaseChanged is fired;

BUG=chromium-os:7988
TEST=Verify #2 issue in chromium-os:7988 where cert trust change only takes
effect after chrome restart.

Review URL: http://codereview.chromium.org/6816035

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

12 files changed, 95 insertions, 23 deletions

diff --git a/net/base/cert_database.cc b/net/base/cert_database.cc
index 70e7a9b..b8c9aa3 100644
--- a/net/base/cert_database.cc
+++ b/net/base/cert_database.cc
@@ -46,9 +46,15 @@ void CertDatabase::RemoveObserver(Observer* observer) {
   CertDatabaseNotifier::GetInstance()->observer_list_->RemoveObserver(observer);
 }
 
-void CertDatabase::NotifyObserversOfUserCertAdded(X509Certificate* cert) {
+void CertDatabase::NotifyObserversOfUserCertAdded(const X509Certificate* cert) {
   CertDatabaseNotifier::GetInstance()->observer_list_->Notify(
       &CertDatabase::Observer::OnUserCertAdded, make_scoped_refptr(cert));
 }
 
+void CertDatabase::NotifyObserversOfCertTrustChanged(
+    const X509Certificate* cert) {
+  CertDatabaseNotifier::GetInstance()->observer_list_->Notify(
+      &CertDatabase::Observer::OnCertTrustChanged, make_scoped_refptr(cert));
+}
+
 }  // namespace net
diff --git a/net/base/cert_database.h b/net/base/cert_database.h
index 4204578..0fadb61 100644
--- a/net/base/cert_database.h
+++ b/net/base/cert_database.h
@@ -32,8 +32,9 @@ typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 class CertDatabase {
  public:
 
-  // A CertDatabase::Observer will be notified every time a new user
-  // certificate is added to the database.  Observers can register themselves
+  // A CertDatabase::Observer will be notified on certificate database changes.
+  // The change could be either a new user certificate is added or trust on
+  // a certificate is changed.  Observers can register themselves
   // via CertDatabase::AddObserver, and can un-register with
   // CertDatabase::RemoveObserver.
   class Observer {
@@ -41,7 +42,12 @@ class CertDatabase {
     virtual ~Observer() {}
 
     // Will be called when a new user certificate is added.
-    virtual void OnUserCertAdded(X509Certificate* cert) = 0;
+    // Note that |cert| could be NULL when called.
+    virtual void OnUserCertAdded(const X509Certificate* cert) {}
+
+    // Will be called when a certificate's trust is changed.
+    // Note that |cert| could be NULL when called.
+    virtual void OnCertTrustChanged(const X509Certificate* cert) {}
 
    protected:
     Observer() {}
@@ -174,8 +180,9 @@ class CertDatabase {
   static void RemoveObserver(Observer* observer);
 
  private:
-  // Broadcasts a notification to all registered observers.
-  static void NotifyObserversOfUserCertAdded(X509Certificate* cert);
+  // Broadcasts notifications to all registered observers.
+  static void NotifyObserversOfUserCertAdded(const X509Certificate* cert);
+  static void NotifyObserversOfCertTrustChanged(const X509Certificate* cert);
 
   DISALLOW_COPY_AND_ASSIGN(CertDatabase);
 };
diff --git a/net/base/cert_database_nss.cc b/net/base/cert_database_nss.cc
index c8e1e56..1cc5bfb 100644
--- a/net/base/cert_database_nss.cc
+++ b/net/base/cert_database_nss.cc
@@ -157,9 +157,13 @@ int CertDatabase::ImportFromPKCS12(
     CryptoModule* module,
     const std::string& data,
     const string16& password) {
-  return psm::nsPKCS12Blob_Import(module->os_module_handle(),
-                                  data.data(), data.size(),
-                                  password);
+  int result = psm::nsPKCS12Blob_Import(module->os_module_handle(),
+                                        data.data(), data.size(),
+                                        password);
+  if (result == net::OK)
+    CertDatabase::NotifyObserversOfUserCertAdded(NULL);
+
+  return result;
 }
 
 int CertDatabase::ExportToPKCS12(
@@ -196,7 +200,12 @@ bool CertDatabase::ImportCACerts(const CertificateList& certificates,
                                  unsigned int trust_bits,
                                  ImportCertFailureList* not_imported) {
   X509Certificate* root = FindRootInList(certificates);
-  return psm::ImportCACerts(certificates, root, trust_bits, not_imported);
+  bool success = psm::ImportCACerts(certificates, root, trust_bits,
+                                    not_imported);
+  if (success)
+    CertDatabase::NotifyObserversOfCertTrustChanged(NULL);
+
+  return success;
 }
 
 bool CertDatabase::ImportServerCert(const CertificateList& certificates,
@@ -230,9 +239,14 @@ unsigned int CertDatabase::GetCertTrust(
 bool CertDatabase::SetCertTrust(const X509Certificate* cert,
                                 CertType type,
                                 unsigned int trusted) {
-  return psm::SetCertTrust(cert, type, trusted);
+  bool success = psm::SetCertTrust(cert, type, trusted);
+  if (success)
+    CertDatabase::NotifyObserversOfCertTrustChanged(cert);
+
+  return success;
 }
 
+// TODO(xiyuan): Add an Observer method for this event.
 bool CertDatabase::DeleteCertAndKey(const X509Certificate* cert) {
   // For some reason, PK11_DeleteTokenCertAndKey only calls
   // SEC_DeletePermCertificate if the private key is found.  So, we check
diff --git a/net/base/cert_verifier.cc b/net/base/cert_verifier.cc
index a3755aa..2644c39 100644
--- a/net/base/cert_verifier.cc
+++ b/net/base/cert_verifier.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -286,6 +286,7 @@ CertVerifier::CertVerifier()
       requests_(0),
       cache_hits_(0),
       inflight_joins_(0) {
+  CertDatabase::AddObserver(this);
 }
 
 CertVerifier::CertVerifier(TimeService* time_service)
@@ -293,10 +294,13 @@ CertVerifier::CertVerifier(TimeService* time_service)
       requests_(0),
       cache_hits_(0),
       inflight_joins_(0) {
+  CertDatabase::AddObserver(this);
 }
 
 CertVerifier::~CertVerifier() {
   STLDeleteValues(&inflight_);
+
+  CertDatabase::RemoveObserver(this);
 }
 
 int CertVerifier::Verify(X509Certificate* cert,
@@ -431,6 +435,12 @@ void CertVerifier::HandleResult(X509Certificate* cert,
   delete job;
 }
 
+void CertVerifier::OnCertTrustChanged(const X509Certificate* cert) {
+  DCHECK(CalledOnValidThread());
+
+  ClearCache();
+}
+
 /////////////////////////////////////////////////////////////////////
 
 SingleRequestCertVerifier::SingleRequestCertVerifier(
@@ -494,4 +504,3 @@ void SingleRequestCertVerifier::OnVerifyCompletion(int result) {
 }  // namespace net
 
 DISABLE_RUNNABLE_METHOD_REFCOUNT(net::CertVerifierWorker);
-
diff --git a/net/base/cert_verifier.h b/net/base/cert_verifier.h
index 49491db..f2db53f 100644
--- a/net/base/cert_verifier.h
+++ b/net/base/cert_verifier.h
@@ -13,6 +13,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time.h"
+#include "net/base/cert_database.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/completion_callback.h"
 #include "net/base/x509_cert_types.h"
@@ -46,7 +47,8 @@ struct CachedCertVerifyResult {
 // request at a time is to create a SingleRequestCertVerifier wrapper around
 // CertVerifier (which will automatically cancel the single request when it
 // goes out of scope).
-class CertVerifier : public base::NonThreadSafe {
+class CertVerifier : public base::NonThreadSafe,
+                     public CertDatabase::Observer {
  public:
   // Opaque type used to cancel a request.
   typedef void* RequestHandle;
@@ -154,6 +156,9 @@ class CertVerifier : public base::NonThreadSafe {
                     int error,
                     const CertVerifyResult& verify_result);
 
+  // CertDatabase::Observer methods:
+  virtual void OnCertTrustChanged(const X509Certificate* cert);
+
   // cache_ maps from a request to a cached result. The cached result may
   // have expired and the size of |cache_| must be <= kMaxCacheEntries.
   std::map<RequestParams, CachedCertVerifyResult> cache_;
diff --git a/net/base/ssl_client_auth_cache.cc b/net/base/ssl_client_auth_cache.cc
index ecfada8..7b0e158 100644
--- a/net/base/ssl_client_auth_cache.cc
+++ b/net/base/ssl_client_auth_cache.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -41,7 +41,7 @@ void SSLClientAuthCache::Remove(const std::string& server) {
   cache_.erase(server);
 }
 
-void SSLClientAuthCache::OnUserCertAdded(X509Certificate* cert) {
+void SSLClientAuthCache::OnUserCertAdded(const X509Certificate* cert) {
   cache_.clear();
 }
 
diff --git a/net/base/ssl_client_auth_cache.h b/net/base/ssl_client_auth_cache.h
index d66d727..571461e 100644
--- a/net/base/ssl_client_auth_cache.h
+++ b/net/base/ssl_client_auth_cache.h
@@ -46,7 +46,7 @@ class SSLClientAuthCache : public CertDatabase::Observer {
   void Remove(const std::string& server);
 
   // CertDatabase::Observer methods:
-  virtual void OnUserCertAdded(X509Certificate* cert);
+  virtual void OnUserCertAdded(const X509Certificate* cert);
 
  private:
   typedef std::string AuthCacheKey;
diff --git a/net/socket/client_socket_factory.cc b/net/socket/client_socket_factory.cc
index 1f8a76c..966dc69 100644
--- a/net/socket/client_socket_factory.cc
+++ b/net/socket/client_socket_factory.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -41,7 +41,14 @@ class DefaultClientSocketFactory : public ClientSocketFactory,
     CertDatabase::RemoveObserver(this);
   }
 
-  virtual void OnUserCertAdded(X509Certificate* cert) {
+  virtual void OnUserCertAdded(const X509Certificate* cert) {
+    ClearSSLSessionCache();
+  }
+
+  virtual void OnCertTrustChanged(const X509Certificate* cert) {
+    // Per wtc, we actually only need to flush when trust is reduced.
+    // Always flush now because OnCertTrustChanged does not tell us this.
+    // See comments in ClientSocketPoolManager::OnCertTrustChanged.
     ClearSSLSessionCache();
   }
 
diff --git a/net/socket/client_socket_pool_manager.cc b/net/socket/client_socket_pool_manager.cc
index d9a2225..de29bd5 100644
--- a/net/socket/client_socket_pool_manager.cc
+++ b/net/socket/client_socket_pool_manager.cc
@@ -580,7 +580,21 @@ Value* ClientSocketPoolManager::SocketPoolInfoToValue() const {
   return list;
 }
 
-void ClientSocketPoolManager::OnUserCertAdded(X509Certificate* cert) {
+void ClientSocketPoolManager::OnUserCertAdded(const X509Certificate* cert) {
+  FlushSocketPools();
+}
+
+void ClientSocketPoolManager::OnCertTrustChanged(const X509Certificate* cert) {
+  // We should flush the socket pools if we removed trust from a
+  // cert, because a previously trusted server may have become
+  // untrusted.
+  //
+  // We should not flush the socket pools if we added trust to a
+  // cert.
+  //
+  // Since the OnCertTrustChanged method doesn't tell us what
+  // kind of trust change it is, we have to flush the socket
+  // pools to be safe.
   FlushSocketPools();
 }
 
diff --git a/net/socket/client_socket_pool_manager.h b/net/socket/client_socket_pool_manager.h
index ca580e4..54b13f7 100644
--- a/net/socket/client_socket_pool_manager.h
+++ b/net/socket/client_socket_pool_manager.h
@@ -150,7 +150,8 @@ class ClientSocketPoolManager : public base::NonThreadSafe,
   Value* SocketPoolInfoToValue() const;
 
   // CertDatabase::Observer methods:
-  virtual void OnUserCertAdded(X509Certificate* cert);
+  virtual void OnUserCertAdded(const X509Certificate* cert);
+  virtual void OnCertTrustChanged(const X509Certificate* cert);
 
  private:
   friend class HttpNetworkSessionPeer;
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index 9d14f4e..9e49d45 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -249,7 +249,15 @@ scoped_refptr<SpdySession> SpdySessionPool::GetFromAlias(
   return NULL;
 }
 
-void SpdySessionPool::OnUserCertAdded(X509Certificate* cert) {
+void SpdySessionPool::OnUserCertAdded(const X509Certificate* cert) {
+  CloseCurrentSessions();
+}
+
+void SpdySessionPool::OnCertTrustChanged(const X509Certificate* cert) {
+  // Per wtc, we actually only need to CloseCurrentSessions when trust is
+  // reduced. CloseCurrentSessions now because OnCertTrustChanged does not
+  // tell us this.
+  // See comments in ClientSocketPoolManager::OnCertTrustChanged.
   CloseCurrentSessions();
 }
 
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index 7a08861..32531d5 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -115,7 +115,8 @@ class SpdySessionPool
   static void enable_ip_pooling(bool value) { g_enable_ip_pooling = value; }
 
   // CertDatabase::Observer methods:
-  virtual void OnUserCertAdded(X509Certificate* cert);
+  virtual void OnUserCertAdded(const X509Certificate* cert);
+  virtual void OnCertTrustChanged(const X509Certificate* cert);
 
  private:
   friend class SpdySessionPoolPeer;  // For testing.