net: make QUIC ProofVerifier more generic.

net/quic/crypto/proof_verifier.h

Index: net/quic/crypto/proof_verifier.h
diff --git a/net/quic/crypto/proof_verifier.h b/net/quic/crypto/proof_verifier.h
index eb96898950278a6ef86e6f12bc39360933923c63..8b99395aa1e04d82430aacb702d29269bb43d49a 100644
--- a/net/quic/crypto/proof_verifier.h
+++ b/net/quic/crypto/proof_verifier.h
@@ -15,32 +15,73 @@ namespace net {
 
 class CertVerifyResult;
 
+// ProofVerifyDetails is an abstract class that acts as a container for any
+// implementation specific details that a ProofVerifier wishes to return. These
+// details are saved in the CachedInfo for the origin in question.
+class ProofVerifyDetails {
+ public:
+  virtual ~ProofVerifyDetails();
+};
+
+// ProofVerifierCallback provides a generic mechanism for a ProofVerifier to
+// call back after an asynchronous verification.
+class ProofVerifierCallback {
+ public:
+  virtual ~ProofVerifierCallback();
+
+  // Run is called on the original thread to mark the completion of an
+  // asynchonous verification. If |ok| is true then the certificate is valid
+  // and |*error_details| is unused. Otherwise, |*error_details| contains a
+  // description of the error. |details| contains implementation-specific
+  // details of the verification.
+  //
+  // |error_details| is owned by the ProofVerifier and is valid only for the
+  // duration of |Run|.
+  //
+  // |details| is owned by the callback and the callback must delete it if not

[Ryan Hamilton, 2013/07/23 18:40:50]

Is |details| owned by the callback to avoid a buffer copy?  It'd be nice if
|details| and |Error_details| had the same ownership, but I think that's
probably not worth doing...  

Alternatively, could you pass in a scoped_ptr<>* for |details| and a const
std::string& for |error_details|?  I think that might obviate any ownership
issues.

[agl, 2013/07/23 21:04:57]

|details| is owned by the callback because the intent is that ProofVerifier's be
able to run multiple verifications at the same time. Chromium can't do that
currently, but the interface imagines that it can. So the ProofVerifier is about
to delete its state after this callback and so must pass ownership.

The callback doesn't do a copy because it's an abstract type. We would need a
virtual copy method in order for the callback to duplicate the value and that
seems like too much work.
const std::string& is a good idea - done.

I've done the scoped_ptr<>*. I'm not totally sure about it. I would have liked
to have done a scoped_ptr<> but, although Chromium's scoped_ptr has .Pass(),
other versions do not.

The scoped_ptr<>* it a little awkward, but it does solve the possibility of
leaks: if the callback doesn't call release() on it then it'll get automatically
destroyed. So that's nice. 

+  // NULL.
+  virtual void Run(bool ok,
+                   std::string* error_details,
+                   ProofVerifyDetails* details) = 0;
+};
+
 // A ProofVerifier checks the signature on a server config, and the certificate
 // chain that backs the public key.
 class NET_EXPORT_PRIVATE ProofVerifier {
  public:
-  virtual ~ProofVerifier() {}
+  // Status enumerates the possible results of verifying a proof.
+  enum Status {
+    OK = 0,
+    ERROR = 1,
+    // PENDING results from a verification which will occur asynchonously. When
+    // the verification is complete, |callback|'s |Run| method will be called.
+    PENDING = 2,
+  };
+
+  virtual ~ProofVerifier();
 
   // VerifyProof checks that |signature| is a valid signature of
   // |server_config| by the public key in the leaf certificate of |certs|, and
   // that |certs| is a valid chain for |hostname|. On success, it returns OK.
-  // On failure, it returns ERR_FAILED and sets |*error_details| to a
-  // description of the problem. This function may also return ERR_IO_PENDING,
-  // in which case the |callback| will be run on the calling thread with the
-  // final OK/ERR_FAILED result when the proof is verified.
+  // On failure, it returns ERROR and sets |*error_details| to a description of
+  // the problem. In either case it may set |*details|, which the caller takes
+  // ownership of.
+  //
+  // This function may also return PENDING, in which case the ProofVerifier
+  // will call back, on the original thread, via |callback| when complete.
+  //
+  // This function takes ownership of |callback|. It will be deleted even if
+  // the call returns immediately.
   //
   // The signature uses SHA-256 as the hash function and PSS padding in the
   // case of RSA.
-  //
-  // Note: this is just for testing. The CN of the certificate is ignored and
-  // wildcards in the SANs are not supported.
-  virtual int VerifyProof(const std::string& hostname,
-                          const std::string& server_config,
-                          const std::vector<std::string>& certs,
-                          const std::string& signature,
-                          std::string* error_details,
-                          CertVerifyResult* cert_verify_result,
-                          const CompletionCallback& callback) = 0;
+  virtual Status VerifyProof(const std::string& hostname,
+                             const std::string& server_config,
+                             const std::vector<std::string>& certs,
+                             const std::string& signature,
+                             std::string* error_details,
+                             scoped_ptr<ProofVerifyDetails>* details,
+                             ProofVerifierCallback* callback) = 0;
 };
 
 }  // namespace net