Update CertVerifier::Verify to use RequestParams instead

net/cert/multi_threaded_cert_verifier.cc

 // 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.
 
 #include "net/cert/multi_threaded_cert_verifier.h"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
@@ skipping 240 matching lines @@
   // destructors run.
   PR_DetachThread();
 #endif
 }
 
 // CertVerifierJob lives only on the verifier's origin message loop.
 class CertVerifierJob {
  public:
   CertVerifierJob(const CertVerifier::RequestParams& key,
                   NetLog* net_log,
-                  X509Certificate* cert,
                   MultiThreadedCertVerifier* cert_verifier)
       : key_(key),
         start_time_(base::TimeTicks::Now()),
         wall_start_time_(base::Time::Now()),
         net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_CERT_VERIFIER_JOB)),
         cert_verifier_(cert_verifier),
         is_first_job_(false),
         weak_ptr_factory_(this) {
-    net_log_.BeginEvent(
-        NetLog::TYPE_CERT_VERIFIER_JOB,
-        base::Bind(&NetLogX509CertificateCallback, base::Unretained(cert)));
+    net_log_.BeginEvent(NetLog::TYPE_CERT_VERIFIER_JOB,
+                        base::Bind(&NetLogX509CertificateCallback,
+                                   base::Unretained(key.certificate().get())));
   }
 
   // Indicates whether this was the first job started by the CertVerifier. This
   // is only used for logging certain UMA stats.
   void set_is_first_job(bool is_first_job) { is_first_job_ = is_first_job; }
 
   const CertVerifier::RequestParams& key() const { return key_; }
 
   // Posts a task to the worker pool to do the verification. Once the
   // verification has completed on the worker thread, it will call
   // OnJobCompleted() on the origin thread.
   bool Start(const scoped_refptr<CertVerifyProc>& verify_proc,
-             const scoped_refptr<X509Certificate>& cert,
-             const std::string& hostname,
-             const std::string& ocsp_response,
-             int flags,
-             const scoped_refptr<CRLSet>& crl_set,
-             const CertificateList& additional_trust_anchors) {
+             const scoped_refptr<CRLSet>& crl_set) {
     // Owned by the bound reply callback.
     std::unique_ptr<MultiThreadedCertVerifier::CachedResult> owned_result(
         new MultiThreadedCertVerifier::CachedResult());
 
     // Parameter evaluation order is undefined in C++. Ensure the pointer value
     // is gotten before calling base::Passed().
     auto result = owned_result.get();
 
     return base::WorkerPool::PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&DoVerifyOnWorkerThread, verify_proc, cert, hostname,
-                   ocsp_response, flags, crl_set, additional_trust_anchors,
-                   &result->error, &result->result),
+        base::Bind(&DoVerifyOnWorkerThread, verify_proc, key_.certificate(),
+                   key_.hostname(), key_.ocsp_response(), key_.flags(), crl_set,
+                   key_.additional_trust_anchors(), &result->error,
+                   &result->result),
         base::Bind(&CertVerifierJob::OnJobCompleted,
                    weak_ptr_factory_.GetWeakPtr(), base::Passed(&owned_result)),
         true /* task is slow */);
   }
 
   ~CertVerifierJob() {
     // If the job is in progress, cancel it.
     if (cert_verifier_) {
       cert_verifier_ = nullptr;
 
@@ skipping 102 matching lines @@
   STLDeleteElements(&inflight_);
   CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
 void MultiThreadedCertVerifier::SetCertTrustAnchorProvider(
     CertTrustAnchorProvider* trust_anchor_provider) {
   DCHECK(CalledOnValidThread());
   trust_anchor_provider_ = trust_anchor_provider;
 }
 
-int MultiThreadedCertVerifier::Verify(X509Certificate* cert,
-                                      const std::string& hostname,
-                                      const std::string& ocsp_response,
-                                      int flags,
+int MultiThreadedCertVerifier::Verify(const CertVerifier::RequestParams& params,
                                       CRLSet* crl_set,
                                       CertVerifyResult* verify_result,
                                       const CompletionCallback& callback,
                                       std::unique_ptr<Request>* out_req,
                                       const BoundNetLog& net_log) {
   out_req->reset();
 
   DCHECK(CalledOnValidThread());
 
-  if (callback.is_null() || !verify_result || hostname.empty())
+  if (callback.is_null() || !verify_result || params.hostname().empty())
     return ERR_INVALID_ARGUMENT;
 
   requests_++;
 
-  const CertificateList empty_cert_list;
-  const CertificateList& additional_trust_anchors =
-      trust_anchor_provider_ ?
-          trust_anchor_provider_->GetAdditionalTrustAnchors() : empty_cert_list;
+  CertificateList new_trust_anchors(params.additional_trust_anchors());
+  if (trust_anchor_provider_) {
+    new_trust_anchors.insert(
+        new_trust_anchors.end(),
+        trust_anchor_provider_->GetAdditionalTrustAnchors().begin(),

[eroman, 2016/05/20 00:41:18]

My expectation is for GetAdditionalTrustAnchors() to return a CertificateList.
But it does in fact return a |const CertificateList&| so this could be right.

But certainly it looks very fishy when reading it.

Do you have a suggestion for improving this? Or just kill off TrustAnchor
Provider.

[Ryan Sleevi, 2016/05/20 02:39:43]

Why? It's the same as GetIntermediateCertificates() on X509Certificate.
Killing off TAP is the next cleanup, once this lands. Well, ok, second to next -
killing off fingerprint/ca_fingerprint is next, then killing off TAP as it's
unnecessary once PolicyCertVerifier can just do the injection (that is, moving
this entirely out of MTCV, or CachingCertVerifier where I next move it up to -
that is, I'm gently shuffling the TAP into the higher layer with each CL)

+        trust_anchor_provider_->GetAdditionalTrustAnchors().end());
+  }
 
-  const CertVerifier::RequestParams key(cert, hostname, flags, ocsp_response,
-                                        additional_trust_anchors);
+  const CertVerifier::RequestParams key(params.certificate(), params.hostname(),
+                                        params.flags(), params.ocsp_response(),
+                                        new_trust_anchors);
   const CertVerifierCache::value_type* cached_entry =
       cache_.Get(key, CacheValidityPeriod(base::Time::Now()));
   if (cached_entry) {
     ++cache_hits_;
     *verify_result = cached_entry->result;
     return cached_entry->error;
   }
 
   // No cache hit. See if an identical request is currently in flight.
   CertVerifierJob* job = FindJob(key);
   if (job) {
     // An identical request is in flight already. We'll just attach our
     // callback.
     inflight_joins_++;
   } else {
     // Need to make a new job.
     std::unique_ptr<CertVerifierJob> new_job(
-        new CertVerifierJob(key, net_log.net_log(), cert, this));
+        new CertVerifierJob(key, net_log.net_log(), this));
 
-    if (!new_job->Start(verify_proc_, cert, hostname, ocsp_response, flags,
-                        crl_set, additional_trust_anchors)) {
+    if (!new_job->Start(verify_proc_, crl_set)) {
       // TODO(wtc): log to the NetLog.
       LOG(ERROR) << "CertVerifierJob couldn't be started.";
       return ERR_INSUFFICIENT_RESOURCES;  // Just a guess.
     }
 
     job = new_job.release();
     inflight_.insert(job);
 
     if (requests_ == 1)
       job->set_is_first_job(true);
@@ skipping 79 matching lines @@
   // The JobSet is kept in sorted order so items can be found using binary
   // search.
   auto it = std::lower_bound(inflight_.begin(), inflight_.end(), key,
                              JobToRequestParamsComparator());
   if (it != inflight_.end() && !(key < (*it)->key()))
     return *it;
   return nullptr;
 }
 
 }  // namespace net