Cache certificate verification results in memory.

net/base/cert_verifier.h

Index: net/base/cert_verifier.h
===================================================================
--- net/base/cert_verifier.h	(revision 68922)
+++ net/base/cert_verifier.h	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2008-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -6,29 +6,69 @@
 #define NET_BASE_CERT_VERIFIER_H_
 #pragma once
 
+#include <map>
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/ref_counted.h"
+#include "base/non_thread_safe.h"
+#include "base/scoped_ptr.h"
+#include "base/time.h"
+#include "net/base/cert_verify_result.h"
 #include "net/base/completion_callback.h"
+#include "net/base/x509_cert_types.h"
 
 namespace net {
 
-class CertVerifyResult;
+class CertVerifierJob;
+class CertVerifierWorker;
 class X509Certificate;
 
-// This class represents the task of verifying a certificate.  It can only
-// verify a single certificate at a time, so if you need to verify multiple
-// certificates at the same time, you will need to allocate a CertVerifier
-// object for each certificate.
+// CachedCertVerifyResult contains the result of a certificate verification.
+class CachedCertVerifyResult {

[willchan no longer on Chromium, 2010/12/13 09:30:53]

I think this should be a struct.  Otherwise, please make the member variables
private and use accessors.

+ public:
+  CachedCertVerifyResult();
+  ~CachedCertVerifyResult();
+
+  int error;  // The return value of CertVerifier::Verify.
+  CertVerifyResult result;  // The output of CertVerifier::Verify.
+
+  // The time at which the certificate verification result expires.
+  base::Time expiry;
+
+  // Returns true if |current_time| is greater than or equal to |expiry|.
+  bool HasExpired(base::Time current_time) const;
+};
+
+// CertVerifier represents a service for verifying certificates.
 //
-class CertVerifier {
+// CertVerifier can handle multiple requests at a time, so when cancelling a
+// request the RequestHandle that was returned by Verify() needs to be
+// given.  A simpler alternative for consumers that only have 1 outstanding
+// 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 NonThreadSafe {

[willchan no longer on Chromium, 2010/12/13 09:30:53]

Have you thought about monitoring the NetworkChangeNotifier?  We should probably
be aborting the requests on network changes, right?  And do we need to flush the
cache too?

[agl, 2010/12/13 16:25:04]

I can imagine that OCSP/CRL/AIA requests get guillotined on a network change and
time out. However, the validity of a certificate shouldn't change based on the
current network.

(If you were thinking of DNS based stuff, then I think that this should be
layered under that.)

  public:
+  // Opaque type used to cancel a request.
+  typedef void* RequestHandle;
+
+  // CertVerifier must not call base::Time::Now() directly.  It must call
+  // time_service_->Now().  This allows unit tests to mock the current time.
+  class TimeService {
+   public:
+    virtual ~TimeService() {}
+
+    virtual base::Time Now() = 0;
+  };
+
   CertVerifier();
 
-  // If a completion callback is pending when the verifier is destroyed, the
-  // certificate verification is cancelled, and the completion callback will
-  // not be called.
+  // Used by unit tests to mock the current time.  Takes ownership of
+  // |time_service|.
+  explicit CertVerifier(TimeService* time_service);
+
+  // When the verifier is destroyed, all certificate verifications requests are
+  // cancelled, and their completion callbacks will not be called.
   ~CertVerifier();
 
   // Verifies the given certificate against the given hostname.  Returns OK if
@@ -49,23 +89,128 @@
   // VERIFY_REV_CHECKING_ENABLED is not set), EV certificate verification will
   // not be performed.
   //
-  // When callback is null, the operation completes synchronously.
-  //
-  // When callback is non-null, ERR_IO_PENDING is returned if the operation
+  // |callback| must not be null.  ERR_IO_PENDING is returned if the operation
   // could not be completed synchronously, in which case the result code will
   // be passed to the callback when available.
   //
-  int Verify(X509Certificate* cert, const std::string& hostname,
-             int flags, CertVerifyResult* verify_result,
-             CompletionCallback* callback);
+  // If |out_req| is non-NULL, then |*out_req| will be filled with a handle to
+  // the async request. This handle is not valid after the request has
+  // completed.
+  int Verify(X509Certificate* cert,

[willchan no longer on Chromium, 2010/12/13 09:30:53]

I don't know enough about this area, but it seems to be that if we're verifying
a cert, then we shouldn't be modifying the cert, so |cert| should be a const
X509Certificate&.  Just a thought for you to consider.  I don't know it well
enough to say if this is possible.  Just sounds like verification of a cert
should be a const operation for the cert.

+             const std::string& hostname,
+             int flags,
+             CertVerifyResult* verify_result,
+             CompletionCallback* callback,
+             RequestHandle* out_req);

[willchan no longer on Chromium, 2010/12/13 09:30:53]

I think you should be passing in a BoundNetLog for logging purposes now.  And
you should add a bunch of new logging events so you can see in
about:net-internals when CertVerifier joins requests or has cache hits or what
not.  Because if caching is broken in some way and someone reports a bug, you'll
want to be able to tell from about:net-internals that it's a cache issue.

Btw, now that you're caching, do we need to do something special wrt load flags?
 Eric can speak better to this.  We should be consistent with the HostResolver. 
I coulda sworn that it did something to force a lookup when a load flag was set,
but I don't see anything like that now.  I just want to comment about that here
so the right people are considering this issue.

 
+  // Cancels the specified request. |req| is the handle returned by Verify().
+  // After a request is cancelled, its completion callback will not be called.
+  void CancelRequest(RequestHandle req);
+
+  // Clears the verification result cache.
+  void ClearCache();
+
+  int GetCacheSize() const;
+
+  uint64 requests() const { return requests_; }
+  uint64 cache_hits() const { return cache_hits_; }
+  uint64 inflight_joins() const { return inflight_joins_; }
+
  private:
-  class Request;
-  friend class Request;
-  scoped_refptr<Request> request_;
+  friend class CertVerifierWorker;  // Calls HandleResult.
+
+  // Input parameters of a certificate verification request.
+  struct RequestParams {
+    bool operator==(const RequestParams& other) const {
+      // |flags| is compared before |cert_fingerprint| and |hostname| under
+      // assumption that integer comparisons are faster than memory and string
+      // comparisons.
+      return (flags == other.flags &&
+              memcmp(cert_fingerprint.data, other.cert_fingerprint.data,
+                     sizeof(cert_fingerprint.data)) == 0 &&

[willchan no longer on Chromium, 2010/12/13 09:30:53]

arraysize

+              hostname == other.hostname);
+    }
+
+    bool operator<(const RequestParams& other) const {
+      // |flags| is compared before |cert_fingerprint| and |hostname| under
+      // assumption that integer comparisons are faster than memory and string
+      // comparisons.
+      if (flags != other.flags)
+        return flags < other.flags;
+      int rv = memcmp(cert_fingerprint.data, other.cert_fingerprint.data,
+                      sizeof(cert_fingerprint.data));

[willchan no longer on Chromium, 2010/12/13 09:30:53]

arraysize

+      if (rv != 0)
+        return rv < 0;
+      return hostname < other.hostname;
+    }
+
+    SHA1Fingerprint cert_fingerprint;
+    std::string hostname;
+    int flags;
+  };
+
+  void HandleResult(X509Certificate* cert,
+                    const std::string& hostname,
+                    int flags,
+                    int error,
+                    const CertVerifyResult& verify_result);
+
+  // 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_;
+
+  // inflight_ maps from a request to an active verification which is taking
+  // place.
+  std::map<RequestParams, CertVerifierJob*> inflight_;
+
+  scoped_ptr<TimeService> time_service_;
+
+  uint64 requests_;
+  uint64 cache_hits_;
+  uint64 inflight_joins_;
+
   DISALLOW_COPY_AND_ASSIGN(CertVerifier);
 };
 
+// This class represents the task of verifying a certificate.  It wraps
+// CertVerifier to verify only a single certificate at a time and cancels this
+// request when going out of scope.
+class SingleRequestCertVerifier {
+ public:
+  // |cert_verifier| must remain valid for the lifetime of |this|.
+  explicit SingleRequestCertVerifier(CertVerifier* cert_verifier);
+
+  // If a completion callback is pending when the verifier is destroyed, the
+  // certificate verification is cancelled, and the completion callback will

[willchan no longer on Chromium, 2010/12/13 09:30:53]

Cancelled is the British spelling.  American English uses canceled.  Down with
the Brits! :P

+  // not be called.
+  ~SingleRequestCertVerifier();
+
+  // Verifies the given certificate, filling out the |verify_result| object
+  // upon success. See CertVerifier::Verify() for details.
+  int Verify(X509Certificate* cert,
+             const std::string& hostname,
+             int flags,
+             CertVerifyResult* verify_result,
+             CompletionCallback* callback);
+
+ private:
+  // Callback for when the request to |cert_verifier_| completes, so we
+  // dispatch to the user's callback.
+  void OnVerifyCompletion(int result);
+
+  // The actual certificate verifier that will handle the request.
+  CertVerifier* const cert_verifier_;
+
+  // The current request (if any).
+  CertVerifier::RequestHandle cur_request_;
+  CompletionCallback* cur_request_callback_;
+
+  // Completion callback for when request to |cert_verifier_| completes.
+  net::CompletionCallbackImpl<SingleRequestCertVerifier> callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SingleRequestCertVerifier);
+};
+
 }  // namespace net
 
 #endif  // NET_BASE_CERT_VERIFIER_H_