QUIC - Race Cert Verification with host resolution if certs are

net/quic/quic_stream_factory.cc

Index: net/quic/quic_stream_factory.cc
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index 832cf909eb8a9ff21ccacd572ab8e25feb7eac82..d5c7578a6fa312fe826e7edcec57b5ebceb90b7a 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -34,6 +34,7 @@
 #include "net/http/bidirectional_stream_impl.h"
 #include "net/quic/bidirectional_stream_quic_impl.h"
 #include "net/quic/crypto/channel_id_chromium.h"
+#include "net/quic/crypto/proof_verifier.h"
 #include "net/quic/crypto/proof_verifier_chromium.h"
 #include "net/quic/crypto/properties_based_quic_server_info.h"
 #include "net/quic/crypto/quic_random.h"
@@ -167,6 +168,86 @@ QuicConfig InitializeQuicConfig(const QuicTagVector& connection_options,
 
 }  // namespace
 
+class QuicStreamFactory::CertVerifierJob {
+ public:
+  class ProofVerifierCallbackImpl : public ProofVerifierCallback {

[Ryan Hamilton, 2016/07/07 00:24:36]

Hm. I'm surprised this is using the ProofVerifier instead of the using the
CertVerifier directly. Looking at the code, I guess it's non-trivial to get the
certs into the right form and then there are a bunch of check we need to do. OK,
makes sense.

[ramant (doing other things), 2016/07/07 16:18:20]

Wondered about the same (whether to duplicate some of the ProofVerifierChromium
code or not).
Acknowledged.

+   public:
+    explicit ProofVerifierCallbackImpl(CertVerifierJob* job) : job_(job) {}
+
+    ~ProofVerifierCallbackImpl() override {}
+
+    void Run(bool ok,
+             const std::string& error_details,
+             std::unique_ptr<ProofVerifyDetails>* details) override {
+      if (job_ == nullptr)
+        return;
+      job_->verify_callback_ = nullptr;
+      job_->OnComplete();
+    }
+
+    void Cancel() { job_ = nullptr; }
+
+   private:
+    CertVerifierJob* job_;
+  };
+
+  CertVerifierJob(const QuicServerId& server_id,
+                  int cert_verify_flags,
+                  const BoundNetLog& net_log)
+      : server_id_(server_id),
+        verify_callback_(nullptr),
+        verify_context_(base::WrapUnique(
+            new ProofVerifyContextChromium(cert_verify_flags, net_log))),
+        net_log_(net_log),
+        weak_factory_(this) {}
+
+  ~CertVerifierJob() {
+    if (verify_callback_)
+      verify_callback_->Cancel();
+  }
+
+  QuicAsyncStatus Run(QuicCryptoClientConfig* crypto_config,
+                      const CompletionCallback& callback) {
+    QuicCryptoClientConfig::CachedState* cached =
+        crypto_config->LookupOrCreate(server_id_);
+    ProofVerifierCallbackImpl* verify_callback =
+        new ProofVerifierCallbackImpl(this);
+    // Pass empty chlo_hash, signature and cert_sct so that only certificate is
+    // verified.
+    QuicAsyncStatus status = crypto_config->proof_verifier()->VerifyProof(
+        server_id_.host(), server_id_.port(), cached->server_config(),
+        QUIC_VERSION_25, /*chlo_hash=*/"", cached->certs(),

[Ryan Hamilton, 2016/07/07 00:24:36]

hard-coding a quic version here seems scary.

[ramant (doing other things), 2016/07/07 16:18:20]

Deleted it.

Done.

+        /*cert_sct=*/"", /*signature=*/"", verify_context_.get(),
+        &verify_error_details_, &verify_details_, verify_callback);

[Ryan Hamilton, 2016/07/07 00:24:36]

Wow, there are a lot of empty arguments. I'd be inclined to add a new method to
the ProofVerifier just for this purpose.

[ramant (doing other things), 2016/07/07 16:18:20]

Done.

+    if (status == QUIC_PENDING) {
+      verify_callback_ = verify_callback;
+      callback_ = callback;
+    } else {
+      delete verify_callback;
+    }
+    return status;
+  }
+
+  void OnComplete() {
+    if (!callback_.is_null())
+      callback_.Run(OK);
+  }
+
+  const QuicServerId& server_id() const { return server_id_; }
+
+ private:
+  QuicServerId server_id_;
+  ProofVerifierCallbackImpl* verify_callback_;
+  std::unique_ptr<ProofVerifyContext> verify_context_;
+  std::unique_ptr<ProofVerifyDetails> verify_details_;
+  std::string verify_error_details_;
+  const BoundNetLog net_log_;
+  CompletionCallback callback_;
+  base::WeakPtrFactory<CertVerifierJob> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertVerifierJob);
+};
+
 // Responsible for creating a new QUIC session to the specified server, and
 // for notifying any associated requests when complete.
 class QuicStreamFactory::Job {
@@ -632,6 +713,7 @@ QuicStreamFactory::QuicStreamFactory(
     int idle_connection_timeout_seconds,
     bool migrate_sessions_on_network_change,
     bool migrate_sessions_early,
+    bool race_cert_verification,
     const QuicTagVector& connection_options,
     bool enable_token_binding)
     : require_confirmation_(true),
@@ -685,6 +767,7 @@ QuicStreamFactory::QuicStreamFactory(
           NetworkChangeNotifier::AreNetworkHandlesSupported()),
       migrate_sessions_early_(migrate_sessions_early &&
                               migrate_sessions_on_network_change_),
+      race_cert_verification_(race_cert_verification),
       port_seed_(random_generator_->RandUint64()),
       check_persisted_supports_quic_(true),
       has_initialized_data_(false),
@@ -752,6 +835,10 @@ QuicStreamFactory::~QuicStreamFactory() {
     STLDeleteElements(&(active_jobs_[server_id]));
     active_jobs_.erase(server_id);
   }
+  while (!active_cert_verifier_jobs_.empty()) {
+    delete active_cert_verifier_jobs_.begin()->second;
+    active_cert_verifier_jobs_.erase(active_cert_verifier_jobs_.begin());

[Ryan Hamilton, 2016/07/07 00:24:36]

I wonder if the value in the map could be an std::unique_ptr?

[ramant (doing other things), 2016/07/07 16:18:20]

Done.

+  }
   if (ssl_config_service_.get())
     ssl_config_service_->RemoveObserver(this);
   if (migrate_sessions_on_network_change_) {
@@ -888,6 +975,8 @@ int QuicStreamFactory::Create(const QuicServerId& server_id,
     }
   }
 
+  StartCertVerifyJob(server_id, cert_verify_flags, net_log);
+
   QuicSessionKey key(destination, server_id);
   std::unique_ptr<Job> job(
       new Job(this, host_resolver_, key, WasQuicRecentlyBroken(server_id),
@@ -1020,6 +1109,12 @@ void QuicStreamFactory::OnJobComplete(Job* job, int rv) {
   job_requests_map_.erase(server_id);
 }
 
+void QuicStreamFactory::OnCertVerifyJobComplete(CertVerifierJob* job, int rv) {
+  const QuicServerId server_id(job->server_id());
+  delete job;
+  active_cert_verifier_jobs_.erase(server_id);
+}
+
 std::unique_ptr<QuicHttpStream> QuicStreamFactory::CreateFromSession(
     QuicChromiumClientSession* session) {
   return std::unique_ptr<QuicHttpStream>(
@@ -1548,6 +1643,11 @@ bool QuicStreamFactory::HasActiveJob(const QuicServerId& server_id) const {
   return ContainsKey(active_jobs_, server_id);
 }
 
+bool QuicStreamFactory::HasActiveCertVerifierJob(
+    const QuicServerId& server_id) const {
+  return ContainsKey(active_cert_verifier_jobs_, server_id);
+}
+
 int QuicStreamFactory::ConfigureSocket(DatagramClientSocket* socket,
                                        IPEndPoint addr,
                                        NetworkHandle network) {
@@ -1755,6 +1855,27 @@ bool QuicStreamFactory::CryptoConfigCacheIsEmpty(
   return cached->IsEmpty();
 }
 
+void QuicStreamFactory::StartCertVerifyJob(const QuicServerId& server_id,
+                                           int cert_verify_flags,
+                                           const BoundNetLog& net_log) {
+  if (!race_cert_verification_)
+    return;
+  QuicCryptoClientConfig::CachedState* cached =
+      crypto_config_.LookupOrCreate(server_id);
+  if (!cached || cached->certs().empty() ||
+      HasActiveCertVerifierJob(server_id)) {
+    return;
+  }
+  std::unique_ptr<CertVerifierJob> cert_verifier_job(
+      new CertVerifierJob(server_id, cert_verify_flags, net_log));
+  QuicAsyncStatus status = cert_verifier_job->Run(
+      &crypto_config_,
+      base::Bind(&QuicStreamFactory::OnCertVerifyJobComplete,
+                 base::Unretained(this), cert_verifier_job.get()));
+  if (status == QUIC_PENDING)
+    active_cert_verifier_jobs_[server_id] = cert_verifier_job.release();
+}
+
 void QuicStreamFactory::InitializeCachedStateInCryptoConfig(
     const QuicServerId& server_id,
     const std::unique_ptr<QuicServerInfo>& server_info,