Add support for walking and modifying the CachingCertVerifier

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"
@@ skipping 18 matching lines @@
 //   - The trust settings (a user may change trust settings at any time)
 //
 // This class tries to optimize by allowing certificate verification results
 // to be cached for a limited amount of time (presently, 30 minutes), which
 // tries to balance the implementation complexity of needing to monitor the
 // above for meaningful changes and the practical utility of being able to
 // cache results when they're not expected to change.
 class NET_EXPORT CachingCertVerifier : public CertVerifier,
                                        public CertDatabase::Observer {
  public:
+  // Visitor class to allow read-only inspection of the verification cache.

[eroman, 2016/06/16 00:25:04]

optional: Either here or in VisitEntries() mention that the CachingCertVerifier
should not be mutated, as it may invalidate iteration.

+  class NET_EXPORT CacheVisitor {
+   public:
+    virtual ~CacheVisitor() {}

[eroman, 2016/06/16 00:25:04]

optional: Move to an out-of-line dtor in .cc file, and use "= default"

+
+    // Called once for each entry in the cache, providing details about the
+    // cached entry.
+    // Returns true to continue iteration, or false to abort.
+    virtual bool VisitEntry(const RequestParams& params,

[eroman, 2016/06/16 00:25:04]

optional: Name this VisitCertificateCacheEntry(). The more specific name may be
convenient if you want to smash it into an existing class.

Or alternately instead of a visitor class use a callback -- this might be
convenient for callers wanting to writing an inline visitor through lambda, or
base::Bind()ing stuff.

At any rate, I am happy with the visitor class as written.

[Ryan Sleevi, 2016/06/16 00:44:34]

I'm pretty anti-smashing of pure interfaces :)
They wouldn't be able to mix a lambda with a callback at present, and
callbacks-called-multiple-times are an anti-pattern (e.g. if you're curring an
owned pointer, everything gets wrong).
SG, opted for no change.

+                            int error,
+                            const CertVerifyResult& verify_result,
+                            base::Time verification_time,
+                            base::Time expiration_time) = 0;
+  };
+
   // Creates a CachingCertVerifier that will use |verifier| to perform the
   // actual verifications if they're not already cached or if the cached
   // item has expired.
   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(
       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;
 
+  // Opportunistically attempts to add |error| and |verify_result| as the
+  // result for |params|, which was obtained at |verification_time| and
+  // expires at |expiration_time|.
+  // This is opportunistic because it is not guaranteed that the entry
+  // will be added (such as if the cache is full or an entry already
+  // exists).
+  bool AddEntry(const RequestParams& params,
+                int error,
+                const CertVerifyResult& verify_result,
+                base::Time verification_time);
+
+  // Iterates through all of the non-expired entries in the cache, calling
+  // VisitEntry on |visitor| for each, until either all entries are
+  // iterated through or the |visitor| aborts.
+  void VisitEntries(CacheVisitor* visitor);

[eroman, 2016/06/16 00:25:04]

Can this method be "const" ?

[Ryan Sleevi, 2016/06/16 00:44:34]

I saw no reason to impose that contract on implementations, since from the POV
of this interface, the const-ness is irrelevant, but imposing it would be
limiting (e.g. if you wanted to build up a serialized version in a member field)

[eroman, 2016/06/16 00:54:51]

I think you misunderstood. What I am asking for is:

  void VisitEntries(CacheVisitor* visitor) const;

+
  private:
+  friend class CacheVisitor;

[eroman, 2016/06/16 00:25:04]

Is this needed?

[Ryan Sleevi, 2016/06/16 00:44:34]

Nope

   FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, CacheHit);
+  FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, Visitor);
+  FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, AddsEntries);
   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.
@@ skipping 60 matching lines @@
 
   uint64_t requests_;
   uint64_t cache_hits_;
 
   DISALLOW_COPY_AND_ASSIGN(CachingCertVerifier);
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_CACHING_CERT_VERIFIER_H_