Commit 2cb6817d authored by Ryan Sleevi's avatar Ryan Sleevi Committed by Commit Bot

Extract coalescing cert verification into a CoalescingCertVerifier

This moves the coalescing logic that is currently part of
MultiThreadedCertVerifier into a dedicated class, which
will coalesce multiple Verify() requests into a single
request to the underlying CertVerifier, if there is one
pending.

This allows for the simplification of the
TrialComparisonCertVerifier, which relied on an additional
signal from the MultiThreadedCertVerifier to know when to
trigger cert verification, allowing the TCCV to be composed
on the MTCV.

While this class can be composed on top of a
CachingCertVerifier, it will generally be more efficient if
it sits as the underlying CertVerifier for the cache, due to
additional bookkeeping involved with the coalescing.

Bug: None
Change-Id: I0c745cf52b83428549869a70aa397c1ef14de033
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838657Reviewed-by: default avatarMatt Mueller <mattm@chromium.org>
Reviewed-by: default avatarJulian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704176}
parent 70eec51d
......@@ -554,6 +554,8 @@ component("net") {
"cert/cert_verify_proc_nss.h",
"cert/cert_verify_proc_win.cc",
"cert/cert_verify_proc_win.h",
"cert/coalescing_cert_verifier.cc",
"cert/coalescing_cert_verifier.h",
"cert/ct_log_response_parser.cc",
"cert/ct_log_response_parser.h",
"cert/ct_log_verifier.cc",
......@@ -5113,6 +5115,7 @@ test("net_unittests") {
"cert/cert_verify_proc_ios_unittest.cc",
"cert/cert_verify_proc_mac_unittest.cc",
"cert/cert_verify_proc_unittest.cc",
"cert/coalescing_cert_verifier_unittest.cc",
"cert/crl_set_unittest.cc",
"cert/ct_log_response_parser_unittest.cc",
"cert/ct_log_verifier_unittest.cc",
......
......@@ -18,6 +18,7 @@
#include "base/logging.h"
#else
#include "net/cert/caching_cert_verifier.h"
#include "net/cert/coalescing_cert_verifier.h"
#include "net/cert/multi_threaded_cert_verifier.h"
#endif
......@@ -101,7 +102,8 @@ std::unique_ptr<CertVerifier> CertVerifier::CreateDefault(
#endif
return std::make_unique<CachingCertVerifier>(
std::make_unique<MultiThreadedCertVerifier>(std::move(verify_proc)));
std::make_unique<CoalescingCertVerifier>(
std::make_unique<MultiThreadedCertVerifier>(std::move(verify_proc))));
#endif
}
......
This diff is collapsed.
// Copyright 2019 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_COALESCING_CERT_VERIFIER_H_
#define NET_CERT_COALESCING_CERT_VERIFIER_H_
#include <stdint.h>
#include <map>
#include <memory>
#include <vector>
#include "base/macros.h"
#include "net/base/net_export.h"
#include "net/cert/cert_verifier.h"
namespace net {
// CoalescingCertVerifier is a CertVerifier that keeps track of in-flight
// CertVerifier Verify() requests. If a new call to Verify() is started that
// matches the same parameters as an in-progress verification, the new
// Verify() call will be joined to the existing, in-progress verification,
// completing when it does. If no in-flight requests match, a new request to
// the underlying verifier will be started.
//
// If the underlying configuration changes, existing requests are allowed to
// complete, but any new requests will not be seen as matching, even if they
// share the same parameters. This ensures configuration changes propagate
// "immediately" for all new requests.
class NET_EXPORT CoalescingCertVerifier : public CertVerifier {
public:
// Create a new verifier that will forward calls to |verifier|, coalescing
// any in-flight, not-yet-completed calls to Verify().
explicit CoalescingCertVerifier(std::unique_ptr<CertVerifier> verifier);
~CoalescingCertVerifier() override;
// CertVerifier implementation:
int Verify(const RequestParams& params,
CertVerifyResult* verify_result,
CompletionOnceCallback callback,
std::unique_ptr<CertVerifier::Request>* out_req,
const NetLogWithSource& net_log) override;
void SetConfig(const CertVerifier::Config& config) override;
uint64_t requests_for_testing() const { return requests_; }
uint64_t inflight_joins_for_testing() const { return inflight_joins_; }
private:
class Job;
class Request;
// If there is a pending request that matches |params|, and which can be
// joined (it shares the same config), returns that Job.
// Otherwise, returns nullptr, meaning a new Job should be started.
Job* FindJob(const RequestParams& params);
void RemoveJob(Job* job);
// Contains the set of Jobs for which an active verification is taking
// place and which can be used for new requests (e.g. the config is the
// same).
std::map<CertVerifier::RequestParams, std::unique_ptr<Job>> joinable_jobs_;
// Contains all pending Jobs that are in-flight, but cannot be joined, due
// to the configuration having changed since they were started.
std::vector<std::unique_ptr<Job>> inflight_jobs_;
std::unique_ptr<CertVerifier> verifier_;
uint32_t config_id_;
uint64_t requests_;
uint64_t inflight_joins_;
DISALLOW_COPY_AND_ASSIGN(CoalescingCertVerifier);
};
} // namespace net
#endif // NET_CERT_COALESCING_CERT_VERIFIER_H_
This diff is collapsed.
......@@ -99,6 +99,10 @@ void MockCertVerifier::AddResultForCertAndHost(
rules_.push_back(Rule(std::move(cert), host_pattern, verify_result, rv));
}
void MockCertVerifier::ClearRules() {
rules_.clear();
}
int MockCertVerifier::VerifyImpl(const RequestParams& params,
CertVerifyResult* verify_result) {
for (const Rule& rule : rules_) {
......
......@@ -58,6 +58,9 @@ class MockCertVerifier : public CertVerifier {
const CertVerifyResult& verify_result,
int rv);
// Clear all existing rules.
void ClearRules();
private:
struct Rule;
using RuleList = std::list<Rule>;
......
This diff is collapsed.
......@@ -10,55 +10,27 @@
#include <map>
#include <memory>
#include <string>
#include <vector>
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/threading/thread_checker.h"
#include "net/base/completion_once_callback.h"
#include "net/base/net_export.h"
#include "net/cert/cert_verifier.h"
namespace net {
class CertVerifierJob;
class CertVerifierRequest;
class CertVerifyProc;
// MultiThreadedCertVerifier is a CertVerifier implementation that runs
// synchronous CertVerifier implementations on worker threads.
class NET_EXPORT_PRIVATE MultiThreadedCertVerifier : public CertVerifier {
public:
using VerifyCompleteCallback =
base::RepeatingCallback<void(const RequestParams&,
const NetLogWithSource&,
int,
const CertVerifyResult&,
base::TimeDelta,
bool)>;
explicit MultiThreadedCertVerifier(scoped_refptr<CertVerifyProc> verify_proc);
// When the verifier is destroyed, all certificate verifications requests are
// canceled, and their completion callbacks will not be called.
~MultiThreadedCertVerifier() override;
// Creates a MultiThreadedCertVerifier that will call
// |verify_complete_callback| once for each verification that has completed
// (if it is non-null). Histograms will have |histogram_suffix| appended, if
// it is non-empty.
// This factory method is temporary, and should not be used without
// consulting with OWNERS.
// TODO(mattm): remove this once the dual verification trial is complete.
// (See https://crbug.com/649026.)
static std::unique_ptr<MultiThreadedCertVerifier>
CreateForDualVerificationTrial(
scoped_refptr<CertVerifyProc> verify_proc,
VerifyCompleteCallback verify_complete_callback,
bool should_record_histograms);
// CertVerifier implementation
int Verify(const RequestParams& params,
CertVerifyResult* verify_result,
......@@ -68,64 +40,9 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier : public CertVerifier {
void SetConfig(const CertVerifier::Config& config) override;
private:
struct JobToRequestParamsComparator;
friend class CertVerifierRequest;
friend class CertVerifierJob;
friend class MultiThreadedCertVerifierTest;
FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, InflightJoin);
FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, MultipleInflightJoin);
FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, CancelRequest);
struct JobComparator {
bool operator()(const CertVerifierJob* job1,
const CertVerifierJob* job2) const;
};
// TODO(mattm): remove this once the dual verification trial is complete.
// (See https://crbug.com/649026.)
MultiThreadedCertVerifier(scoped_refptr<CertVerifyProc> verify_proc,
VerifyCompleteCallback verify_complete_callback,
bool should_record_histograms);
// Returns an inflight job for |key|, if it can be joined. If there is no
// such job then returns null.
CertVerifierJob* FindJob(const RequestParams& key);
// Removes |job| from the inflight set, and passes ownership back to the
// caller. |job| must already be |inflight_|.
std::unique_ptr<CertVerifierJob> RemoveJob(CertVerifierJob* job);
// For unit testing.
uint64_t requests() const { return requests_; }
uint64_t inflight_joins() const { return inflight_joins_; }
// |joinable_| holds the jobs for which an active verification is taking
// place and can be joined by new requests (e.g. the config is the same),
// mapping the job's raw pointer to an owned pointer.
// TODO(rsleevi): Once C++17 is supported, switch this to be a std::set<>,
// which supports extracting owned objects from the set.
std::map<CertVerifierJob*, std::unique_ptr<CertVerifierJob>, JobComparator>
joinable_;
// |inflight_| contains all jobs that are still undergoing active
// verification, but which can no longer be joined - such as due to the
// underlying configuration changing.
std::map<CertVerifierJob*, std::unique_ptr<CertVerifierJob>, JobComparator>
inflight_;
uint32_t config_id_;
Config config_;
uint64_t requests_;
uint64_t inflight_joins_;
scoped_refptr<CertVerifyProc> verify_proc_;
// Members for dual verification trial. TODO(mattm): Remove these.
// (See https://crbug.com/649026.)
VerifyCompleteCallback verify_complete_callback_;
bool should_record_histograms_ = true;
THREAD_CHECKER(thread_checker_);
DISALLOW_COPY_AND_ASSIGN(MultiThreadedCertVerifier);
......
......@@ -90,43 +90,6 @@ class MultiThreadedCertVerifierTest : public TestWithTaskEnvironment {
MultiThreadedCertVerifier verifier_;
};
// Tests an inflight join.
TEST_F(MultiThreadedCertVerifierTest, InflightJoin) {
base::FilePath certs_dir = GetTestCertsDirectory();
scoped_refptr<X509Certificate> test_cert(
ImportCertFromFile(certs_dir, "ok_cert.pem"));
ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
int error;
CertVerifyResult verify_result;
TestCompletionCallback callback;
std::unique_ptr<CertVerifier::Request> request;
CertVerifyResult verify_result2;
TestCompletionCallback callback2;
std::unique_ptr<CertVerifier::Request> request2;
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, "www.example.com", 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result, callback.callback(), &request, NetLogWithSource());
ASSERT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request);
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, "www.example.com", 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result2, callback2.callback(), &request2, NetLogWithSource());
EXPECT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request2);
error = callback.WaitForResult();
EXPECT_TRUE(IsCertificateError(error));
error = callback2.WaitForResult();
ASSERT_TRUE(IsCertificateError(error));
ASSERT_EQ(2u, verifier_.requests());
ASSERT_EQ(1u, verifier_.inflight_joins());
}
// Tests that the callback of a canceled request is never made.
TEST_F(MultiThreadedCertVerifierTest, CancelRequest) {
base::FilePath certs_dir = GetTestCertsDirectory();
......@@ -195,89 +158,6 @@ TEST_F(MultiThreadedCertVerifierTest, CancelRequestThenQuit) {
// Destroy |verifier| by going out of scope.
}
// Tests de-duplication of requests.
// Starts up 5 requests, of which 3 are unique.
TEST_F(MultiThreadedCertVerifierTest, MultipleInflightJoin) {
base::FilePath certs_dir = GetTestCertsDirectory();
scoped_refptr<X509Certificate> test_cert(
ImportCertFromFile(certs_dir, "ok_cert.pem"));
ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
int error;
CertVerifyResult verify_result1;
TestCompletionCallback callback1;
std::unique_ptr<CertVerifier::Request> request1;
CertVerifyResult verify_result2;
TestCompletionCallback callback2;
std::unique_ptr<CertVerifier::Request> request2;
CertVerifyResult verify_result3;
TestCompletionCallback callback3;
std::unique_ptr<CertVerifier::Request> request3;
CertVerifyResult verify_result4;
TestCompletionCallback callback4;
std::unique_ptr<CertVerifier::Request> request4;
CertVerifyResult verify_result5;
TestCompletionCallback callback5;
std::unique_ptr<CertVerifier::Request> request5;
const char domain1[] = "www.example1.com";
const char domain2[] = "www.exampleB.com";
const char domain3[] = "www.example3.com";
// Start 3 unique requests.
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, domain2, 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result1, callback1.callback(), &request1, NetLogWithSource());
ASSERT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request1);
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, domain2, 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result2, callback2.callback(), &request2, NetLogWithSource());
EXPECT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request2);
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, domain3, 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result3, callback3.callback(), &request3, NetLogWithSource());
EXPECT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request3);
// Start duplicate requests (which should join to existing jobs).
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, domain1, 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result4, callback4.callback(), &request4, NetLogWithSource());
EXPECT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request4);
error = verifier_.Verify(
CertVerifier::RequestParams(test_cert, domain2, 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result5, callback5.callback(), &request5, NetLogWithSource());
EXPECT_THAT(error, IsError(ERR_IO_PENDING));
EXPECT_TRUE(request5);
error = callback1.WaitForResult();
EXPECT_TRUE(IsCertificateError(error));
error = callback2.WaitForResult();
ASSERT_TRUE(IsCertificateError(error));
error = callback4.WaitForResult();
ASSERT_TRUE(IsCertificateError(error));
// Let the other requests automatically cancel.
ASSERT_EQ(5u, verifier_.requests());
ASSERT_EQ(2u, verifier_.inflight_joins());
}
// Tests propagation of configuration options into CertVerifyProc flags
TEST_F(MultiThreadedCertVerifierTest, ConvertsConfigToFlags) {
base::FilePath certs_dir = GetTestCertsDirectory();
......
This diff is collapsed.
......@@ -22,6 +22,10 @@
namespace net {
class CertVerifyProc;
// TrialComparisonCertVerifier is a CertVerifier that can be used to compare
// the results between two different CertVerifyProcs. The results are reported
// back to the caller via a ReportCallback, allowing the caller to further
// examine the differences.
class NET_EXPORT TrialComparisonCertVerifier : public CertVerifier {
public:
// These values are persisted to logs. Entries should not be renumbered and
......@@ -51,8 +55,33 @@ class NET_EXPORT TrialComparisonCertVerifier : public CertVerifier {
const net::CertVerifyResult& primary_result,
const net::CertVerifyResult& trial_result)>;
TrialComparisonCertVerifier(bool initial_allowed,
scoped_refptr<CertVerifyProc> primary_verify_proc,
// Create a new TrialComparisonCertVerifier. Initially, no trial
// verifications will actually be performed; that is, calls to Verify() will
// be dispatched to the underlying |primary_verify_proc|. This can be changed
// by calling set_trial_allowed().
//
// When trial verifications are enabled, calls to Verify() will first call
// into |primary_verify_proc| to verify. The result of this verification will
// be immediately returned to the caller of Verify, allowing them to proceed.
// However, the verifier will continue in the background, attempting to
// verify the same RequestParams using |trial_verify_proc|. If there are
// differences in the results, they will be reported via |report_callback|,
// allowing the creator to receive information about differences.
//
// If the caller abandons the CertVerifier::Request prior to the primary
// verification completed, no trial verification will be done. However, once
// the primary verifier has returned, the trial verifications will continue,
// provided that the underlying configuration has not been changed by
// calling SetConfig().
//
// Note that there may be multiple calls to both |primary_verify_proc| and
// |trial_verify_proc|, using different parameters to account for platform
// differences.
//
// TODO(rsleevi): Make the types distinct, to guarantee that
// |primary_verify_proc| is a System CertVerifyProc, and |trial_verify_proc|
// is the Builtin CertVerifyProc.
TrialComparisonCertVerifier(scoped_refptr<CertVerifyProc> primary_verify_proc,
scoped_refptr<CertVerifyProc> trial_verify_proc,
ReportCallback report_callback);
......@@ -69,36 +98,21 @@ class NET_EXPORT TrialComparisonCertVerifier : public CertVerifier {
const NetLogWithSource& net_log) override;
void SetConfig(const Config& config) override;
// Returns a CertVerifier using the primary CertVerifyProc, which will not
// cause OnPrimaryVerifierComplete to be called. This can be used to
// attempt to re-verify a cert with different chain or flags without
// messing up the stats or potentially causing an infinite loop.
private:
class Job;
friend class Job;
CertVerifier* primary_verifier() const { return primary_verifier_.get(); }
CertVerifier* primary_reverifier() const { return primary_reverifier_.get(); }
CertVerifier* trial_verifier() const { return trial_verifier_.get(); }
CertVerifier* revocation_trial_verifier() const {
return revocation_trial_verifier_.get();
}
private:
class TrialVerificationJob;
void OnPrimaryVerifierComplete(const RequestParams& params,
const NetLogWithSource& net_log,
int primary_error,
const CertVerifyResult& primary_result,
base::TimeDelta primary_latency,
bool is_first_job);
void OnTrialVerifierComplete(const RequestParams& params,
const NetLogWithSource& net_log,
int trial_error,
const CertVerifyResult& trial_result,
base::TimeDelta latency,
bool is_first_job);
void RemoveJob(TrialVerificationJob* job_ptr);
void RemoveJob(Job* job_ptr);
// Whether the trial is allowed.
bool allowed_;
bool allowed_ = false;
// Callback that reports are sent to.
ReportCallback report_callback_;
......@@ -111,8 +125,7 @@ class NET_EXPORT TrialComparisonCertVerifier : public CertVerifier {
// revocation information.
std::unique_ptr<CertVerifier> revocation_trial_verifier_;
std::set<std::unique_ptr<TrialVerificationJob>, base::UniquePtrComparator>
jobs_;
std::set<std::unique_ptr<Job>, base::UniquePtrComparator> jobs_;
THREAD_CHECKER(thread_checker_);
......
......@@ -11,6 +11,7 @@
#include "net/base/net_errors.h"
#include "net/cert/caching_cert_verifier.h"
#include "net/cert/cert_verify_proc.h"
#include "net/cert/coalescing_cert_verifier.h"
#include "net/cert/multi_threaded_cert_verifier.h"
namespace network {
......@@ -65,7 +66,8 @@ void CertVerifierWithTrustAnchors::InitializeOnIOThread(
<< "Additional trust anchors not supported on the current platform!";
}
delegate_ = std::make_unique<net::CachingCertVerifier>(
std::make_unique<net::MultiThreadedCertVerifier>(verify_proc.get()));
std::make_unique<net::CoalescingCertVerifier>(
std::make_unique<net::MultiThreadedCertVerifier>(verify_proc.get())));
delegate_->SetConfig(ExtendTrustAnchors(orig_config_, trust_anchors_));
}
......
......@@ -42,6 +42,7 @@
#include "net/base/network_isolation_key.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "net/cert/cert_verifier.h"
#include "net/cert/coalescing_cert_verifier.h"
#include "net/cert/ct_verify_result.h"
#include "net/cert_net/cert_net_fetcher_impl.h"
#include "net/cookies/cookie_monster.h"
......@@ -1541,14 +1542,18 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext() {
#elif BUILDFLAG(TRIAL_COMPARISON_CERT_VERIFIER_SUPPORTED)
if (params_->trial_comparison_cert_verifier_params) {
cert_verifier = std::make_unique<net::CachingCertVerifier>(
std::make_unique<TrialComparisonCertVerifierMojo>(
params_->trial_comparison_cert_verifier_params->initial_allowed,
std::move(params_->trial_comparison_cert_verifier_params
->config_client_request),
std::move(params_->trial_comparison_cert_verifier_params
->report_client),
net::CertVerifyProc::CreateSystemVerifyProc(cert_net_fetcher_),
net::CertVerifyProc::CreateBuiltinVerifyProc(cert_net_fetcher_)));
std::make_unique<net::CoalescingCertVerifier>(
std::make_unique<TrialComparisonCertVerifierMojo>(
params_->trial_comparison_cert_verifier_params
->initial_allowed,
std::move(params_->trial_comparison_cert_verifier_params
->config_client_request),
std::move(params_->trial_comparison_cert_verifier_params
->report_client),
net::CertVerifyProc::CreateSystemVerifyProc(
cert_net_fetcher_),
net::CertVerifyProc::CreateBuiltinVerifyProc(
cert_net_fetcher_))));
}
#endif
if (!cert_verifier)
......
......@@ -29,12 +29,13 @@ TrialComparisonCertVerifierMojo::TrialComparisonCertVerifierMojo(
report_client_(std::move(report_client)) {
trial_comparison_cert_verifier_ =
std::make_unique<net::TrialComparisonCertVerifier>(
initial_allowed, primary_verify_proc, trial_verify_proc,
primary_verify_proc, trial_verify_proc,
base::BindRepeating(
&TrialComparisonCertVerifierMojo::OnSendTrialReport,
// Unretained safe because the report_callback will not be called
// after trial_comparison_cert_verifier_ is destroyed.
base::Unretained(this)));
trial_comparison_cert_verifier_->set_trial_allowed(initial_allowed);
}
TrialComparisonCertVerifierMojo::~TrialComparisonCertVerifierMojo() = default;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment