Cert - protobufs to serialize and deserialize CertVerifierCache.

net/cert/multi_threaded_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_CERT_MULTI_THREADED_CERT_VERIFIER_H_
 #define NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 52 matching lines @@
              const std::string& ocsp_response,
              int flags,
              CRLSet* crl_set,
              CertVerifyResult* verify_result,
              const CompletionCallback& callback,
              std::unique_ptr<Request>* out_req,
              const BoundNetLog& net_log) override;
 
   bool SupportsOCSPStapling() override;
 
+  // Adds explicitly-specified data to CertVerifierCache. Returns true if
+  // |cache_| is updated.

[Ryan Sleevi, 2016/04/29 23:33:21]

This is not a helpful comment, because it provides no details about what these
parameters are, how they should be set, or even why this method exists. It goes
directly to talking about implementation details, but that shouldn't be done for
a public header and public method.

// Caches |result| as the result for |hostname|, with the error code
// of |error|, which was previously obtained by calling |Verify()|
// with |flags| at |start_time|, for the certificate whose ordered
// chain was |hash_values|, which was completed at
// |verification_time|, and should expire by |expiration_time|.
// If it returns true, subsequent calls to |Verify()| will return this
// result, if it is before |expiration_time| and matches the
// |hostname| and |flags|.


(If you read the above, you'll see how it's unclear the difference
between |start_time| and |verification_time|. I don't consider the
above comment complete, but mostly wanted to explore how to be clearer
in the comment).

It's also unclear, from this, why you return a bool result, but I'm less picky
about that. Just that it's not clear *why* someone should care about that (just
for metrics?). That one I'm not picky about documenting, other than 

[ramant (doing other things), 2016/04/30 22:58:11]

Many many thanks for the above comments.

Done. 

+  bool AddCertResult(std::string& hostname,
+                     int flags,
+                     std::vector<SHA1HashValue>& hash_values,
+                     base::Time start_time,
+                     int error,
+                     const CertVerifyResult& result,
+                     base::Time verification_time,
+                     base::Time expiration_time);
+
  private:
   struct JobToRequestParamsComparator;
+  friend class CertVerifierCacheIterator;
+  friend class CertVerifierCachePersisterTest;
   friend class CertVerifierRequest;
   friend class CertVerifierJob;
   friend class MultiThreadedCertVerifierTest;
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, CacheHit);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, DifferentCACerts);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, InflightJoin);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, MultipleInflightJoin);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, CancelRequest);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest,
                            RequestParamsComparators);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest,
                            CertTrustAnchorProvider);
+  FRIEND_TEST_ALL_PREFIXES(CertVerifierCachePersisterTest, PersistCache);
+  FRIEND_TEST_ALL_PREFIXES(CertVerifierCachePersisterTest,
+                           PersistCacheExpiredEntry);

[Ryan Sleevi, 2016/04/29 23:33:21]

This (and line 87) improperly creates the circular dependency, except via tests,
because now this code depends on knowing how it's used. You should be testing
the interface, not the implementation.

[ramant (doing other things), 2016/04/30 22:58:11]

Done.

 
   // Input parameters of a certificate verification request.
   struct NET_EXPORT_PRIVATE RequestParams {
     RequestParams(const SHA1HashValue& cert_fingerprint_arg,
                   const SHA1HashValue& ca_fingerprint_arg,
                   const std::string& hostname_arg,
                   const std::string& ocsp_response_arg,
                   int flags_arg,
                   const CertificateList& additional_trust_anchors);
     RequestParams(const RequestParams& other);
+    RequestParams(std::string& hostname_arg,
+                  int flags_arg,
+                  std::vector<SHA1HashValue>& hash_values_arg,

[Ryan Sleevi, 2016/04/29 23:33:21]

Don't pass by non-const reference. I realize you did this to swap, but you need
to take it explicitly, and std::move.

At your call site, if you're trying to optimize, you should std::move() into
that.

[ramant (doing other things), 2016/04/30 22:58:11]

Done.

+                  base::Time start_time_arg);
     ~RequestParams();
 
     bool operator<(const RequestParams& other) const;
 
     std::string hostname;
     int flags;
     std::vector<SHA1HashValue> hash_values;
     // The time when verification started.
     // Note: This uses base::Time, rather than base::TimeTicks, to
     // account for system clock changes.
     base::Time start_time;
   };
 
   // CachedResult contains the result of a certificate verification.
   struct NET_EXPORT_PRIVATE CachedResult {
     CachedResult();
+    CachedResult(int error_arg, CertVerifyResult result_arg);
     ~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 {
+  struct NET_EXPORT_PRIVATE CacheValidityPeriod {

[Ryan Sleevi, 2016/04/29 23:33:21]

Why?

[ramant (doing other things), 2016/04/30 22:58:11]

Deleted them. Changed the tests to test the interface and not the
implementation.

     explicit CacheValidityPeriod(const base::Time& now);
     CacheValidityPeriod(const base::Time& now, const base::Time& expiration);
 
     base::Time verification_time;
     base::Time expiration_time;
   };
 
-  struct CacheExpirationFunctor {
+  struct NET_EXPORT_PRIVATE CacheExpirationFunctor {

[Ryan Sleevi, 2016/04/29 23:33:21]

Why?

[ramant (doing other things), 2016/04/30 22:58:11]

Deleted them. Changed the tests to test the interface and not the
implementation.

     // Returns true iff |now| is within the validity period of |expiration|.
     bool operator()(const CacheValidityPeriod& now,
                     const CacheValidityPeriod& expiration) const;
   };
 
   struct JobComparator {
     bool operator()(const CertVerifierJob* job1,
                     const CertVerifierJob* job2) const;
   };
 
   using JobSet = std::set<CertVerifierJob*, JobComparator>;
 
-  typedef ExpiringCache<RequestParams, CachedResult, CacheValidityPeriod,
-                        CacheExpirationFunctor> CertVerifierCache;
+  typedef ExpiringCache<RequestParams,
+                        CachedResult,
+                        CacheValidityPeriod,
+                        CacheExpirationFunctor>
+      CertVerifierCache;
 
   // Saves |result| into the cache, keyed by |key|.
   void SaveResultToCache(const RequestParams& key, const CachedResult& result);
 
   // CertDatabase::Observer methods:
   void OnCACertChanged(const X509Certificate* cert) override;
 
   // Returns an inflight job for |key|. If there is no such job then returns
   // null.
   CertVerifierJob* FindJob(const RequestParams& key);
@@ skipping 19 matching lines @@
   uint64_t cache_hits_;
   uint64_t inflight_joins_;
 
   scoped_refptr<CertVerifyProc> verify_proc_;
 
   CertTrustAnchorProvider* trust_anchor_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(MultiThreadedCertVerifier);
 };
 
+class NET_EXPORT_PRIVATE CertVerifierCacheIterator {

[Ryan Sleevi, 2016/04/29 23:33:21]

I suggested several times that this be a member.

MultiThreadedCertVerifier::Iterator

To be more explicit: CertVerifierCacheIterator is extremely confusing with
CertVerifierCache::Iterator, which this is not, and has no fundamental relation
to.

[ramant (doing other things), 2016/04/30 22:58:11]

Sincere apologies. Undid this change.

Was trying to avoid forward declaration of struct's required by
CertVerifierCache typedef. 

Done.

+ public:
+  explicit CertVerifierCacheIterator(const MultiThreadedCertVerifier& verifier);
+  ~CertVerifierCacheIterator();
+
+  bool HasNext() const { return iterator_.HasNext(); }
+  void Advance() { iterator_.Advance(); }
+
+  const std::string& hostname() const { return iterator_.key().hostname; }
+  int flags() const { return iterator_.key().flags; }
+  const std::vector<SHA1HashValue>& hash_values() const {
+    return iterator_.key().hash_values;
+  }
+  const base::Time& start_time() const { return iterator_.key().start_time; }
+  int error() const { return iterator_.value().error; }
+  const CertVerifyResult& result() const { return iterator_.value().result; }
+  const base::Time& verification_time() const {
+    return iterator_.expiration().verification_time;
+  }
+  const base::Time& expiration_time() const {
+    return iterator_.expiration().expiration_time;
+  }
+
+ private:
+  MultiThreadedCertVerifier::CertVerifierCache::Iterator iterator_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertVerifierCacheIterator);
+};
+
 }  // namespace net
 
 #endif  // NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_