Make CertVerifier a pure virtual interface.

net/base/cert_verifier.h

Index: net/base/cert_verifier.h
diff --git a/net/base/cert_verifier.h b/net/base/cert_verifier.h
index fae7ec4586ef41682f1e73f7363e6fad9bf895e1..f4b04f15c7187dbaafd2b33b722886c61c453b9d 100644
--- a/net/base/cert_verifier.h
+++ b/net/base/cert_verifier.h
@@ -1,216 +1,87 @@
-// Copyright (c) 2012 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.
-
-#ifndef NET_BASE_CERT_VERIFIER_H_
-#define NET_BASE_CERT_VERIFIER_H_
-#pragma once
-
-#include <map>
-#include <string>
-
-#include "base/basictypes.h"
-#include "base/gtest_prod_util.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/threading/non_thread_safe.h"
-#include "net/base/cert_database.h"
-#include "net/base/cert_verify_result.h"
-#include "net/base/completion_callback.h"
-#include "net/base/expiring_cache.h"
-#include "net/base/net_export.h"
-#include "net/base/x509_cert_types.h"
-
-namespace net {
-
-class BoundNetLog;
-class CertVerifierJob;
-class CertVerifierWorker;
-class CRLSet;
-class X509Certificate;
-
-// CertVerifier represents a service for verifying certificates.
-//
-// CertVerifier can handle multiple requests at a time, so when canceling 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 NET_EXPORT CertVerifier : NON_EXPORTED_BASE(public base::NonThreadSafe),
-                                public CertDatabase::Observer {
- public:
-  // Opaque type used to cancel a request.
-  typedef void* RequestHandle;
-
-  CertVerifier();
-
-  // When the verifier is destroyed, all certificate verifications requests are
-  // canceled, and their completion callbacks will not be called.

[wtc, 2012/02/29 20:27:50]

I think this behavior is required for the destructor of
CertVerifier subclasses, so we should keep this comment,
right?

[Ryan Sleevi, 2012/02/29 21:51:34]

Yes, done.

-  virtual ~CertVerifier();
-
-  // Verifies the given certificate against the given hostname.  Returns OK if
-  // successful or an error code upon failure.
-  //
-  // The |*verify_result| structure, including the |verify_result->cert_status|
-  // bitmask, is always filled out regardless of the return value.  If the
-  // certificate has multiple errors, the corresponding status flags are set in
-  // |verify_result->cert_status|, and the error code for the most serious
-  // error is returned.
-  //
-  // |flags| is bitwise OR'd of X509Certificate::VerifyFlags.
-  // If VERIFY_REV_CHECKING_ENABLED is set in |flags|, certificate revocation
-  // checking is performed.
-  //
-  // If VERIFY_EV_CERT is set in |flags| too, EV certificate verification is
-  // performed.  If |flags| is VERIFY_EV_CERT (that is,
-  // VERIFY_REV_CHECKING_ENABLED is not set), EV certificate verification will
-  // not be performed.
-  //
-  // |crl_set| points to an optional CRLSet structure which can be used to
-  // avoid revocation checks over the network.
-  //
-  // |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.
-  //
-  // 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,
-             const std::string& hostname,
-             int flags,
-             CRLSet* crl_set,
-             CertVerifyResult* verify_result,
-             const CompletionCallback& callback,
-             RequestHandle* out_req,
-             const BoundNetLog& net_log);
-
-  // Cancels the specified request. |req| is the handle returned by Verify().
-  // After a request is canceled, its completion callback will not be called.
-  void CancelRequest(RequestHandle req);
-
- private:
-  friend class CertVerifierWorker;  // Calls HandleResult.
-  friend class CertVerifierRequest;
-  friend class CertVerifierJob;
-  FRIEND_TEST_ALL_PREFIXES(CertVerifierTest, CacheHit);
-  FRIEND_TEST_ALL_PREFIXES(CertVerifierTest, DifferentCACerts);
-  FRIEND_TEST_ALL_PREFIXES(CertVerifierTest, InflightJoin);
-  FRIEND_TEST_ALL_PREFIXES(CertVerifierTest, CancelRequest);
-  FRIEND_TEST_ALL_PREFIXES(CertVerifierTest, RequestParamsComparators);
-
-  // Input parameters of a certificate verification request.
-  struct RequestParams {
-    RequestParams(const SHA1Fingerprint& cert_fingerprint_arg,
-                  const SHA1Fingerprint& ca_fingerprint_arg,
-                  const std::string& hostname_arg,
-                  int flags_arg)
-        : cert_fingerprint(cert_fingerprint_arg),
-          ca_fingerprint(ca_fingerprint_arg),
-          hostname(hostname_arg),
-          flags(flags_arg) {}
-
-    bool operator<(const RequestParams& other) const {
-      // |flags| is compared before |cert_fingerprint|, |ca_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));
-      if (rv != 0)
-        return rv < 0;
-      rv = memcmp(ca_fingerprint.data, other.ca_fingerprint.data,
-                  sizeof(ca_fingerprint.data));
-      if (rv != 0)
-        return rv < 0;
-      return hostname < other.hostname;
-    }
-
-    SHA1Fingerprint cert_fingerprint;
-    SHA1Fingerprint ca_fingerprint;
-    std::string hostname;
-    int flags;
-  };
-
-  // CachedResult contains the result of a certificate verification.
-  struct CachedResult {
-    CachedResult();
-    ~CachedResult();
-
-    int error;  // The return value of CertVerifier::Verify.
-    CertVerifyResult result;  // The output of CertVerifier::Verify.
-  };
-
-  void HandleResult(X509Certificate* cert,
-                    const std::string& hostname,
-                    int flags,
-                    int error,
-                    const CertVerifyResult& verify_result);
-
-  // CertDatabase::Observer methods:
-  virtual void OnCertTrustChanged(const X509Certificate* cert) OVERRIDE;
-
-  // For unit testing.
-  void ClearCache() { cache_.Clear(); }
-  size_t GetCacheSize() const { return cache_.size(); }
-  uint64 cache_hits() const { return cache_hits_; }
-  uint64 requests() const { return requests_; }
-  uint64 inflight_joins() const { return inflight_joins_; }
-
-  // cache_ maps from a request to a cached result.
-  typedef ExpiringCache<RequestParams, CachedResult> CertVerifierCache;
-  CertVerifierCache cache_;
-
-  // inflight_ maps from a request to an active verification which is taking
-  // place.
-  std::map<RequestParams, CertVerifierJob*> inflight_;
-
-  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 canceled, and the completion callback will
-  // 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,
-             CRLSet* crl_set,
-             CertVerifyResult* verify_result,
-             const CompletionCallback& callback,
-             const BoundNetLog& net_log);
-
- 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_;
-
-  DISALLOW_COPY_AND_ASSIGN(SingleRequestCertVerifier);
-};
-
-}  // namespace net
-
-#endif  // NET_BASE_CERT_VERIFIER_H_
+// Copyright (c) 2012 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.
+
+#ifndef NET_BASE_CERT_VERIFIER_H_
+#define NET_BASE_CERT_VERIFIER_H_
+#pragma once
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_export.h"
+
+namespace net {
+
+class BoundNetLog;
+class CertVerifyResult;
+class CRLSet;
+class X509Certificate;
+
+// CertVerifier represents a service for verifying certificates.
+//
+// CertVerifiers can handle multiple requests at a time. 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 NET_EXPORT_PRIVATE CertVerifier {
+ public:
+  // Opaque pointer type, to return a handle to cancel outstanding requests.

[wtc, 2012/02/29 20:27:50]

Nit: delete "to return"?  It sounds like the description of
a function, but this is a type.

+  typedef void* RequestHandle;
+
+  virtual ~CertVerifier() {}
+
+  // Verifies the given certificate against the given hostname as an SSL server.
+  // Returns OK if successful or an error code upon failure.
+  //
+  // The |*verify_result| structure, including the |verify_result->cert_status|
+  // bitmask, is always filled out regardless of the return value.  If the
+  // certificate has multiple errors, the corresponding status flags are set in
+  // |verify_result->cert_status|, and the error code for the most serious
+  // error is returned.
+  //
+  // |flags| is bitwise OR'd of X509Certificate::VerifyFlags.
+  // If VERIFY_REV_CHECKING_ENABLED is set in |flags|, certificate revocation
+  // checking is performed.
+  //
+  // If VERIFY_EV_CERT is set in |flags| too, EV certificate verification is
+  // performed.  If |flags| is VERIFY_EV_CERT (that is,
+  // VERIFY_REV_CHECKING_ENABLED is not set), EV certificate verification will
+  // not be performed.
+  //
+  // |crl_set| points to an optional CRLSet structure which can be used to
+  // avoid revocation checks over the network.
+  //
+  // |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.
+  //
+  // 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.
+  //
+  // TODO(rsleevi): Move CRLSet* out of the CertVerifier signature.

[wtc, 2012/02/29 20:27:50]

CertVerifier is also responsible for building the certification
path, but CRLSet is a certificate revocation checker.

If CRLSet is used by a separate CertVerifier, will that
CertVerifier have to repeat the certification path building?

[Ryan Sleevi, 2012/02/29 21:51:34]

No, that's something that I would want to avoid.

I think of every CertVerifier wanting two inputs - |cert| and the previous/input
|verify_result|. |cert| always represents the server chain, |verify_result| is
the constructed chain (from the previous verifier).

A future CL would seek to clarify and formalize this in the API. But since "path
building" is so directly tied to "path verification", it doesn't make sense to
have every CertVerifier repeat the same logic.

+  virtual int Verify(X509Certificate* cert,
+                     const std::string& hostname,
+                     int flags,
+                     CRLSet* crl_set,
+                     CertVerifyResult* verify_result,
+                     const CompletionCallback& callback,
+                     RequestHandle* out_req,
+                     const BoundNetLog& net_log) = 0;
+
+  // Cancels the specified request. |req| is the handle returned by Verify().
+  // After a request is canceled, its completion callback will not be called.
+  virtual void CancelRequest(RequestHandle req) = 0;
+
+ protected:
+  CertVerifier() {}

[wtc, 2012/02/29 20:27:50]

Is it necessary to define the default constructor?

[Ryan Sleevi, 2012/02/29 21:51:34]

Since there are pure virtual methods, no. Removed.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CertVerifier);
+};
+
+}  // namespace net
+
+#endif  // NET_BASE_CERT_VERIFIER_H_