Remember if a user declines to provide a server with a client certificate

When an SSL/TLS server requests a client certificate, the user may opt to not send any certificate. This will work if the server does not require a client certificate to be sent in order to load the resource - for example, to fall-back to forms-based authentication or when client certificates are configured as optional.

If the user declines to provide a client certificate, remember that choice in the SSLClientAuthCache so that they will not be repeatedly prompted to select a certificate (declining each time) for every sub-resource that loads, similar to how the SSLClientAuthCache remembers an explicit certificate selected by a user.

If the server requires a client certificate, it is expected that it will abort the TLS handshake with an appropriate TLS error code. When the server aborts and the error code is processed, the existing cached selection will be removed, and the user will be re-prompted to select a certificate on their next connection to that server.

If the server requires a client certificate to continue, but does not use the TLS stack to indicate that requirement - for example, to return a "friendlier" error page or an HTTP error code like 403, the selection will not be evicted from the cache, and the user must restart the browser before they will be prompted again for a certificate from that server. This is the same lifetime and behaviour as would happen if the user actively selected a certificate, rather than declined to provide one.

BUG=56177
TEST=SSLClientAuthCacheTest.LookupNullPreference . Also see the Apache configuration in http://crbug.com/56177. Access a site that requests, but does not require, client authentication, which will attempt to load multiple sub-resources, and which does not permit HTTP KeepAlives.

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

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

4 files changed, 131 insertions, 44 deletions

diff --git a/net/base/ssl_client_auth_cache.cc b/net/base/ssl_client_auth_cache.cc
index d2f47cc..355073f 100644
--- a/net/base/ssl_client_auth_cache.cc
+++ b/net/base/ssl_client_auth_cache.cc
@@ -4,15 +4,26 @@
 
 #include "net/base/ssl_client_auth_cache.h"
 
+#include "base/logging.h"
+#include "net/base/x509_certificate.h"
+
 namespace net {
 
 SSLClientAuthCache::SSLClientAuthCache() {}
 
 SSLClientAuthCache::~SSLClientAuthCache() {}
 
-X509Certificate* SSLClientAuthCache::Lookup(const std::string& server) {
+bool SSLClientAuthCache::Lookup(
+    const std::string& server,
+    scoped_refptr<X509Certificate>* certificate) {
+  DCHECK(certificate);
+
   AuthCacheMap::iterator iter = cache_.find(server);
-  return (iter == cache_.end()) ? NULL : iter->second;
+  if (iter == cache_.end())
+    return false;
+
+  *certificate = iter->second;
+  return true;
 }
 
 void SSLClientAuthCache::Add(const std::string& server,
diff --git a/net/base/ssl_client_auth_cache.h b/net/base/ssl_client_auth_cache.h
index 023480b..2b276a2 100644
--- a/net/base/ssl_client_auth_cache.h
+++ b/net/base/ssl_client_auth_cache.h
@@ -10,10 +10,11 @@
 #include <map>
 
 #include "base/ref_counted.h"
-#include "net/base/x509_certificate.h"
 
 namespace net {
 
+class X509Certificate;
+
 // The SSLClientAuthCache class is a simple cache structure to store SSL
 // client certificates. Provides lookup, insertion, and deletion of entries.
 // The parameter for doing lookups, insertions, and deletions is the server's
@@ -26,13 +27,18 @@ class SSLClientAuthCache {
   SSLClientAuthCache();
   ~SSLClientAuthCache();
 
-  // Check if we have a client certificate for SSL server at |server|.
-  // Returns the client certificate (if found) or NULL (if not found).
-  X509Certificate* Lookup(const std::string& server);
+  // Checks for a client certificate preference for SSL server at |server|.
+  // Returns true if a preference is found, and sets |*certificate| to the
+  // desired client certificate. The desired certificate may be NULL, which
+  // indicates a preference to not send any certificate to |server|.
+  // If a certificate preference is not found, returns false.
+  bool Lookup(const std::string& server,
+              scoped_refptr<X509Certificate>* certificate);
 
   // Add a client certificate for |server| to the cache. If there is already
-  // a client certificate for |server|, it will be overwritten. Both parameters
-  // are IN only.
+  // a client certificate for |server|, it will be overwritten. A NULL
+  // |client_cert| indicates a preference that no client certificate should
+  // be sent to |server|.
   void Add(const std::string& server, X509Certificate* client_cert);
 
   // Remove the client certificate for |server| from the cache, if one exists.
diff --git a/net/base/ssl_client_auth_cache_unittest.cc b/net/base/ssl_client_auth_cache_unittest.cc
index 85b3d5e..f528d58 100644
--- a/net/base/ssl_client_auth_cache_unittest.cc
+++ b/net/base/ssl_client_auth_cache_unittest.cc
@@ -5,6 +5,7 @@
 #include "net/base/ssl_client_auth_cache.h"
 
 #include "base/time.h"
+#include "net/base/x509_certificate.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -27,32 +28,50 @@ TEST(SSLClientAuthCacheTest, LookupAddRemove) {
   scoped_refptr<X509Certificate> cert3(
     new X509Certificate("foo3", "CA", start_date, expiration_date));
 
+  scoped_refptr<X509Certificate> cached_cert;
   // Lookup non-existent client certificate.
-  EXPECT_TRUE(cache.Lookup(server1) == NULL);
+  cached_cert = NULL;
+  EXPECT_FALSE(cache.Lookup(server1, &cached_cert));
 
   // Add client certificate for server1.
-  cache.Add(server1, cert1.get());
-  EXPECT_EQ(cert1.get(), cache.Lookup(server1));
+  cache.Add(server1, cert1);
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(cert1, cached_cert);
 
   // Add client certificate for server2.
-  cache.Add(server2, cert2.get());
-  EXPECT_EQ(cert1.get(), cache.Lookup(server1));
-  EXPECT_EQ(cert2.get(), cache.Lookup(server2));
+  cache.Add(server2, cert2);
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(cert1, cached_cert.get());
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server2, &cached_cert));
+  EXPECT_EQ(cert2, cached_cert);
 
   // Overwrite the client certificate for server1.
-  cache.Add(server1, cert3.get());
-  EXPECT_EQ(cert3.get(), cache.Lookup(server1));
-  EXPECT_EQ(cert2.get(), cache.Lookup(server2));
+  cache.Add(server1, cert3);
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(cert3, cached_cert);
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server2, &cached_cert));
+  EXPECT_EQ(cert2, cached_cert);
 
   // Remove client certificate of server1.
   cache.Remove(server1);
-  EXPECT_TRUE(cache.Lookup(server1) == NULL);
-  EXPECT_EQ(cert2.get(), cache.Lookup(server2));
+  cached_cert = NULL;
+  EXPECT_FALSE(cache.Lookup(server1, &cached_cert));
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server2, &cached_cert));
+  EXPECT_EQ(cert2, cached_cert);
 
   // Remove non-existent client certificate.
   cache.Remove(server1);
-  EXPECT_TRUE(cache.Lookup(server1) == NULL);
-  EXPECT_EQ(cert2.get(), cache.Lookup(server2));
+  cached_cert = NULL;
+  EXPECT_FALSE(cache.Lookup(server1, &cached_cert));
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server2, &cached_cert));
+  EXPECT_EQ(cert2, cached_cert);
 }
 
 // Check that if the server differs only by port number, it is considered
@@ -74,8 +93,48 @@ TEST(SSLClientAuthCacheTest, LookupWithPort) {
   cache.Add(server1, cert1.get());
   cache.Add(server2, cert2.get());
 
-  EXPECT_EQ(cert1.get(), cache.Lookup(server1));
-  EXPECT_EQ(cert2.get(), cache.Lookup(server2));
+  scoped_refptr<X509Certificate> cached_cert;
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(cert1.get(), cached_cert);
+  EXPECT_TRUE(cache.Lookup(server2, &cached_cert));
+  EXPECT_EQ(cert2.get(), cached_cert);
+}
+
+// Check that the a NULL certificate, indicating the user has declined to send
+// a certificate, is properly cached.
+TEST(SSLClientAuthCacheTest, LookupNullPreference) {
+  SSLClientAuthCache cache;
+  base::Time start_date = base::Time::Now();
+  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
+
+  std::string server1("foo:443");
+  scoped_refptr<X509Certificate> cert1(
+      new X509Certificate("foo", "CA", start_date, expiration_date));
+
+  cache.Add(server1, NULL);
+
+  scoped_refptr<X509Certificate> cached_cert(cert1);
+  // Make sure that |cached_cert| is updated to NULL, indicating the user
+  // declined to send a certificate to |server1|.
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(NULL, cached_cert.get());
+
+  // Remove the existing cached certificate.
+  cache.Remove(server1);
+  cached_cert = NULL;
+  EXPECT_FALSE(cache.Lookup(server1, &cached_cert));
+
+  // Add a new preference for a specific certificate.
+  cache.Add(server1, cert1);
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(cert1, cached_cert);
+
+  // Replace the specific preference with a NULL certificate.
+  cache.Add(server1, NULL);
+  cached_cert = NULL;
+  EXPECT_TRUE(cache.Lookup(server1, &cached_cert));
+  EXPECT_EQ(NULL, cached_cert.get());
 }
 
 }  // namespace net
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 3e1d8d1..c84dfa4 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -96,7 +96,6 @@ HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session)
   session->ssl_config_service()->GetSSLConfig(&ssl_config_);
   if (session->http_stream_factory()->next_protos())
     ssl_config_.next_protos = *session->http_stream_factory()->next_protos();
-
 }
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
@@ -171,10 +170,8 @@ int HttpNetworkTransaction::RestartWithCertificate(
   DCHECK_EQ(STATE_NONE, next_state_);
 
   ssl_config_.client_cert = client_cert;
-  if (client_cert) {
-    session_->ssl_client_auth_cache()->Add(
-        response_.cert_request_info->host_and_port, client_cert);
-  }
+  session_->ssl_client_auth_cache()->Add(
+      response_.cert_request_info->host_and_port, client_cert);
   ssl_config_.send_client_cert = true;
   // Reset the other member variables.
   // Note: this is necessary only with SSL renegotiation.
@@ -971,29 +968,43 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) {
   // handshake.
   stream_request_.reset();
 
-  // If the user selected one of the certificate in client_certs for this
-  // server before, use it automatically.
-  X509Certificate* client_cert = session_->ssl_client_auth_cache()->Lookup(
-      response_.cert_request_info->host_and_port);
+  // If the user selected one of the certificates in client_certs or declined
+  // to provide one for this server before, use the past decision
+  // automatically.
+  scoped_refptr<X509Certificate> client_cert;
+  bool found_cached_cert = session_->ssl_client_auth_cache()->Lookup(
+      response_.cert_request_info->host_and_port, &client_cert);
+  if (!found_cached_cert)
+    return error;
+
+  // Check that the certificate selected is still a certificate the server
+  // is likely to accept, based on the criteria supplied in the
+  // CertificateRequest message.
   if (client_cert) {
     const std::vector<scoped_refptr<X509Certificate> >& client_certs =
         response_.cert_request_info->client_certs;
+    bool cert_still_valid = false;
     for (size_t i = 0; i < client_certs.size(); ++i) {
-      if (client_cert->fingerprint().Equals(client_certs[i]->fingerprint())) {
-        // TODO(davidben): Add a unit test which covers this path; we need to be
-        // able to send a legitimate certificate and also bypass/clear the
-        // SSL session cache.
-        ssl_config_.client_cert = client_cert;
-        ssl_config_.send_client_cert = true;
-        next_state_ = STATE_CREATE_STREAM;
-        // Reset the other member variables.
-        // Note: this is necessary only with SSL renegotiation.
-        ResetStateForRestart();
-        return OK;
+      if (client_cert->Equals(client_certs[i])) {
+        cert_still_valid = true;
+        break;
       }
     }
+
+    if (!cert_still_valid)
+      return error;
   }
-  return error;
+
+  // TODO(davidben): Add a unit test which covers this path; we need to be
+  // able to send a legitimate certificate and also bypass/clear the
+  // SSL session cache.
+  ssl_config_.client_cert = client_cert;
+  ssl_config_.send_client_cert = true;
+  next_state_ = STATE_CREATE_STREAM;
+  // Reset the other member variables.
+  // Note: this is necessary only with SSL renegotiation.
+  ResetStateForRestart();
+  return OK;
 }
 
 // This method determines whether it is safe to resend the request after an