Move caching out of MultiThreadedCertVerifier

net/cert/caching_cert_verifier.h

+// Copyright 2016 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_CERT_CACHING_CERT_VERIFIER_H_
+#define NET_CERT_CACHING_CERT_VERIFIER_H_
+
+#include <memory>
+
+#include "net/base/expiring_cache.h"
+#include "net/base/net_export.h"
+#include "net/cert/cert_database.h"
+#include "net/cert/cert_verifier.h"
+#include "net/cert/cert_verify_result.h"
+
+namespace net {
+
+class CertTrustAnchorProvider;
+
+class NET_EXPORT CachingCertVerifier : public CertVerifier,

[eroman, 2016/06/10 01:31:52]

Also, please add a comment for this class (even though the behavior is entirely
unsurprising given the name).

+                                       public CertDatabase::Observer {
+ public:
+  explicit CachingCertVerifier(std::unique_ptr<CertVerifier> verifier);
+
+  ~CachingCertVerifier() override;
+
+  // Configures a source of additional certificates that should be treated as
+  // trust anchors during verification, provided that the underlying
+  // CertVerifyProc supports additional trust beyond the default implementation.
+  // It must outlive the CachingCertVerifier.
+  void SetCertTrustAnchorProvider(

[eroman, 2016/06/10 00:08:03]

This is weird (being part of CachingCertVerifier layer). Described in more
detail in the other comment.

+      CertTrustAnchorProvider* trust_anchor_provider);
+
+  // CertVerifier implementation:
+  int Verify(const RequestParams& params,
+             CRLSet* crl_set,
+             CertVerifyResult* verify_result,
+             const CompletionCallback& callback,
+             std::unique_ptr<Request>* out_req,
+             const BoundNetLog& net_log) override;
+  bool SupportsOCSPStapling() override;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, CacheHit);
+  FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, DifferentCACerts);
+  FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, CertTrustAnchorProvider);
+
+  // CachedResult contains the result of a certificate verification.
+  struct NET_EXPORT_PRIVATE CachedResult {
+    CachedResult();
+    ~CachedResult();
+
+    int error;                // The return value of CertVerifier::Verify.
+    CertVerifyResult result;  // The output of CertVerifier::Verify.
+  };
+
+  // Rather than having a single validity point along a monotonically increasing
+  // timeline, certificate verification is based on falling within a range of
+  // the certificate's NotBefore and NotAfter and based on what the current
+  // system clock says (which may advance forwards or backwards as users correct
+  // clock skew). CacheValidityPeriod and CacheExpirationFunctor are helpers to
+  // ensure that expiration is measured both by the 'general' case (now + cache
+  // TTL) and by whether or not significant enough clock skew was introduced
+  // since the last verification.
+  struct CacheValidityPeriod {
+    explicit CacheValidityPeriod(base::Time now);
+    CacheValidityPeriod(base::Time now, base::Time expiration);
+
+    base::Time verification_time;
+    base::Time expiration_time;
+  };
+
+  struct CacheExpirationFunctor {
+    // Returns true iff |now| is within the validity period of |expiration|.
+    bool operator()(const CacheValidityPeriod& now,
+                    const CacheValidityPeriod& expiration) const;
+  };
+
+  using CertVerificationCache = ExpiringCache<RequestParams,
+                                              CachedResult,
+                                              CacheValidityPeriod,
+                                              CacheExpirationFunctor>;
+
+  // Handles completion of the request matching |params|, which started at
+  // |start_time|, completing. |verify_result| and |result| are added to the
+  // cache, and then |callback| (the original caller's callback) is invoked.
+  void OnRequestFinished(const RequestParams& params,
+                         base::Time start_time,
+                         const CompletionCallback& callback,
+                         CertVerifyResult* verify_result,
+                         int error);
+
+  // Adds |verify_result| and |error| to the cache for |params|, whose
+  // verification attempt began at |start_time|. See the implementation
+  // for more details about the necessity of |start_time|.
+  void AddResultToCache(const RequestParams& params,
+                        base::Time start_time,
+                        const CertVerifyResult& verify_result,
+                        int error);
+
+  // CertDatabase::Observer methods:
+  void OnCACertChanged(const X509Certificate* cert) override;
+
+  // For unit testing.
+  void ClearCache();
+  size_t GetCacheSize() const;
+  uint64_t cache_hits() const { return cache_hits_; }

[eroman, 2016/06/10 01:31:52]

side comment: if this is just for unit-testing, is size_t sufficient?

[Ryan Sleevi, 2016/06/10 07:51:20]

Fair point, will do.

[Ryan Sleevi, 2016/06/10 23:41:51]

Actually, after reading
https://www.chromium.org/developers/coding-style#TOC-Types again, and looking at
google internal practice, I've left it at uint64_t

+  uint64_t requests() const { return requests_; }
+
+  std::unique_ptr<CertVerifier> verifier_;
+
+  CertTrustAnchorProvider* trust_anchor_provider_;
+
+  CertVerificationCache cache_;
+
+  uint64_t requests_;
+  uint64_t cache_hits_;
+
+  DISALLOW_COPY_AND_ASSIGN(CachingCertVerifier);
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_CACHING_CERT_VERIFIER_H_