Introduce CertVerifier::RequestParams

net/cert/cert_verifier.h

Index: net/cert/cert_verifier.h
diff --git a/net/cert/cert_verifier.h b/net/cert/cert_verifier.h
index 26ee988355ae96bf8e4b71f7e726e12d2b72e34c..3f6c9d05066e65689d431081ae6bf771079b22c7 100644
--- a/net/cert/cert_verifier.h
+++ b/net/cert/cert_verifier.h
@@ -7,17 +7,19 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "net/base/completion_callback.h"
+#include "net/base/hash_value.h"
 #include "net/base/net_export.h"
+#include "net/cert/x509_certificate.h"
 
 namespace net {
 
 class BoundNetLog;
 class CertVerifyResult;
 class CRLSet;
-class X509Certificate;
 
 // CertVerifier represents a service for verifying certificates.
 //
@@ -74,6 +76,32 @@ class NET_EXPORT CertVerifier {
     VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS = 1 << 4,
   };
 
+  // The parameters for doing a Verify(). |certificate|, |hostname|, and
+  // |flags| are required. The rest are optional.

[eroman, 2016/05/18 01:43:18]

Are you going to move documentation for the various inputs when updating
Verify() to use this?

[Ryan Sleevi, 2016/05/18 02:02:02]

Yes, when updating Verify() to use this.

+  class RequestParams {
+   public:
+    RequestParams(X509Certificate* certificate,
+                  const std::string& hostname,
+                  int flags,
+                  const std::string& ocsp_response,
+                  const CertificateList& additional_trust_anchors);
+    RequestParams(const RequestParams& other);
+    ~RequestParams();
+
+    const std::string& hostname() const { return hostname_; }
+    int flags() const { return flags_; }
+    const std::vector<SHA1HashValue> request_data() const {
+      return request_data_;
+    }
+
+    bool operator<(const RequestParams& other) const;
+
+   private:
+    std::string hostname_;
+    int flags_;
+    std::vector<SHA1HashValue> request_data_;

[eroman, 2016/05/18 01:43:18]

Can you speak to the design choice of using a vector of hashes rather than a
single hash? Was this to provide a particular sorting stability? Reduce the time
spent hashing? Other?
Using a single cryptographically secure hash would use less memory.

Also why are we OK with using SHA1 here? If we believe SHA1 to be susceptible to
preimage attacks (now or in the near future), using it to key the certificate
cache doesn't sound like a good idea.

[Ryan Sleevi, 2016/05/18 02:02:02]

While I appreciate all of this feedback, I don't think it's good to do on this
CL. I'm trying to minimize changes to ease the implementation transition, and am
not comfortable with scope creep.

(In my earlier version, I actually did move to switch to SHA-256, but felt that
was too significant a change for this CL; similar, using a single hash -
although that has issues with separating out distinct fields)

[eroman, 2016/05/18 02:06:43]

OK, we can discuss that separately.

+  };
+
   // When the verifier is destroyed, all certificate verification requests are
   // canceled, and their completion callbacks will not be called.
   virtual ~CertVerifier() {}
@@ -113,7 +141,7 @@ class NET_EXPORT CertVerifier {
   // nullptr. However it is not guaranteed that all implementations will reset
   // it in this case.
   //
-  // TODO(rsleevi): Move CRLSet* out of the CertVerifier signature.
+  // TODO(rsleevi): Update this to use RequestParams as part of the signature.
   virtual int Verify(X509Certificate* cert,
                      const std::string& hostname,
                      const std::string& ocsp_response,