Revert of QUIC - Race Cert Verification with host resolution if certs are (patchset #6 id:140001 of https://codereview.chromium.org/2120703003/ )
Reason for revert:
Net unit tests are failing on buildbot
Original issue's description:
> QUIC - Race Cert Verification with host resolution if certs are
> availiable when race_cert_verification network session param is enabled.
>
> This experiment is not enabled in chromium.
> CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
>
> R=eroman@chromium.org, mef@chromium.org, rch@chromium.org,
> TBR=eroman@chromium.org (for net-internals changes)
>
> Committed: https://crrev.com/74acd6f26ea2e71a06a6d09cd2eb2f6d751627cc
> Cr-Commit-Position: refs/heads/master@{#404568}
TBR=eroman@chromium.org,mef@chromium.org,rch@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Review-Url: https://codereview.chromium.org/2137843002
Cr-Commit-Position: refs/heads/master@{#404581}
diff --git a/chrome/browser/resources/net_internals/quic_view.html b/chrome/browser/resources/net_internals/quic_view.html
index e5cf4bd..9ee0590 100644
--- a/chrome/browser/resources/net_internals/quic_view.html
+++ b/chrome/browser/resources/net_internals/quic_view.html
@@ -15,7 +15,6 @@
<li>Idle Connection Timeout In Seconds: <span jscontent="$this.idle_connection_timeout_seconds"></span></li>
<li>Disable PreConnect If 0RTT: <span jscontent="$this.disable_preconnect_if_0rtt"></span></li>
<li>Disable QUIC On Timeout With Open Streams: <span jscontent="$this.disable_quic_on_timeout_with_open_streams"></span></li>
- <li>Race Cert Verification: <span jscontent="!!race_cert_verification"></span></li>
<li jsdisplay="$this.disabled_reason && disabled_reason.length > 0">QUIC dynamically disabled: <span jscontent="disabled_reason"></span></li>
</ul>
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
index a346282c..ec7d461 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
@@ -51,8 +51,7 @@
.put("idle_connection_timeout_seconds", 300)
.put("close_sessions_on_ip_change", false)
.put("migrate_sessions_on_network_change", true)
- .put("migrate_sessions_early", true)
- .put("race_cert_verification", true);
+ .put("migrate_sessions_early", true);
JSONObject experimentalOptions = new JSONObject().put("QUIC", quicParams);
mBuilder.setExperimentalOptions(experimentalOptions.toString());
mBuilder.setMockCertVerifierForTesting(QuicTestServer.createMockCertVerifier());
diff --git a/components/cronet/url_request_context_config.cc b/components/cronet/url_request_context_config.cc
index 61dc9d4..fa080d2d 100644
--- a/components/cronet/url_request_context_config.cc
+++ b/components/cronet/url_request_context_config.cc
@@ -53,7 +53,6 @@
const char kQuicMigrateSessionsEarly[] = "migrate_sessions_early";
const char kQuicDisableBidirectionalStreams[] =
"quic_disable_bidirectional_streams";
-const char kQuicRaceCertVerification[] = "race_cert_verification";
// AsyncDNS experiment dictionary name.
const char kAsyncDnsFieldTrialName[] = "AsyncDNS";
@@ -188,13 +187,6 @@
context_builder->set_quic_disable_bidirectional_streams(
quic_disable_bidirectional_streams);
}
-
- bool quic_race_cert_verification = false;
- if (quic_args->GetBoolean(kQuicRaceCertVerification,
- &quic_race_cert_verification)) {
- context_builder->set_quic_race_cert_verification(
- quic_race_cert_verification);
- }
}
const base::DictionaryValue* async_dns_args = nullptr;
diff --git a/components/cronet/url_request_context_config_unittest.cc b/components/cronet/url_request_context_config_unittest.cc
index d06522291..a0cef2a 100644
--- a/components/cronet/url_request_context_config_unittest.cc
+++ b/components/cronet/url_request_context_config_unittest.cc
@@ -46,7 +46,6 @@
"\"packet_loss_threshold\":0.5,"
"\"idle_connection_timeout_seconds\":300,"
"\"close_sessions_on_ip_change\":true,"
- "\"race_cert_verification\":true,"
"\"connection_options\":\"TIME,TBBR,REJ\"},"
"\"AsyncDNS\":{\"enable\":true}}",
// Data reduction proxy key.
@@ -104,9 +103,6 @@
EXPECT_TRUE(params->quic_close_sessions_on_ip_change);
EXPECT_FALSE(params->quic_migrate_sessions_on_network_change);
- // Check race_cert_verification.
- EXPECT_TRUE(params->quic_race_cert_verification);
-
// Check AsyncDNS resolver is enabled.
EXPECT_TRUE(context->host_resolver()->GetDnsConfigAsValue());
}
diff --git a/net/http/http_network_session.cc b/net/http/http_network_session.cc
index 532f5b1..06651e96 100644
--- a/net/http/http_network_session.cc
+++ b/net/http/http_network_session.cc
@@ -129,7 +129,6 @@
quic_migrate_sessions_early(false),
quic_disable_bidirectional_streams(false),
quic_force_hol_blocking(false),
- quic_race_cert_verification(false),
proxy_delegate(NULL),
enable_token_binding(false) {
quic_supported_versions.push_back(QUIC_VERSION_34);
@@ -189,7 +188,6 @@
params.quic_migrate_sessions_on_network_change,
params.quic_migrate_sessions_early,
params.quic_force_hol_blocking,
- params.quic_race_cert_verification,
params.quic_connection_options,
params.enable_token_binding),
spdy_session_pool_(params.host_resolver,
@@ -342,8 +340,6 @@
dict->SetString("disabled_reason",
quic_stream_factory_.QuicDisabledReasonString());
dict->SetBoolean("force_hol_blocking", params_.quic_force_hol_blocking);
- dict->SetBoolean("race_cert_verification",
- params_.quic_race_cert_verification);
return std::move(dict);
}
diff --git a/net/http/http_network_session.h b/net/http/http_network_session.h
index 2a40a0c..b4a72ea 100644
--- a/net/http/http_network_session.h
+++ b/net/http/http_network_session.h
@@ -180,8 +180,6 @@
bool quic_disable_bidirectional_streams;
// If true, enable force HOL blocking. For measurement purposes.
bool quic_force_hol_blocking;
- // If true, race cert verification with host resolution.
- bool quic_race_cert_verification;
ProxyDelegate* proxy_delegate;
// Enable support for Token Binding.
diff --git a/net/quic/crypto/proof_verifier.h b/net/quic/crypto/proof_verifier.h
index c551e40..d05e1dd 100644
--- a/net/quic/crypto/proof_verifier.h
+++ b/net/quic/crypto/proof_verifier.h
@@ -86,26 +86,6 @@
std::string* error_details,
std::unique_ptr<ProofVerifyDetails>* details,
std::unique_ptr<ProofVerifierCallback> callback) = 0;
-
- // VerifyCertChain checks that |certs| is a valid chain for |hostname|. On
- // success, it returns QUIC_SUCCESS. On failure, it returns QUIC_FAILURE and
- // sets |*error_details| to a description of the problem. In either case it
- // may set |*details|, which the caller takes ownership of.
- //
- // |context| specifies an implementation specific struct (which may be nullptr
- // for some implementations) that provides useful information for the
- // verifier, e.g. logging handles.
- //
- // This function may also return QUIC_PENDING, in which case the ProofVerifier
- // will call back, on the original thread, via |callback| when complete.
- // In this case, the ProofVerifier will take ownership of |callback|.
- virtual QuicAsyncStatus VerifyCertChain(
- const std::string& hostname,
- const std::vector<std::string>& certs,
- const ProofVerifyContext* context,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* details,
- std::unique_ptr<ProofVerifierCallback> callback) = 0;
};
} // namespace net
diff --git a/net/quic/crypto/proof_verifier_chromium.cc b/net/quic/crypto/proof_verifier_chromium.cc
index 61eea5a..621eb1f 100644
--- a/net/quic/crypto/proof_verifier_chromium.cc
+++ b/net/quic/crypto/proof_verifier_chromium.cc
@@ -78,16 +78,6 @@
std::unique_ptr<ProofVerifyDetails>* verify_details,
std::unique_ptr<ProofVerifierCallback> callback);
- // Starts the certificate chain verification of |certs|. If |QUIC_PENDING| is
- // returned, then |callback| will be invoked asynchronously when the
- // verification completes.
- QuicAsyncStatus VerifyCertChain(
- const std::string& hostname,
- const std::vector<std::string>& certs,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback);
-
private:
enum State {
STATE_NONE,
@@ -95,19 +85,6 @@
STATE_VERIFY_CERT_COMPLETE,
};
- // Convert |certs| to |cert_|(X509Certificate). Returns true if successful.
- bool GetX509Certificate(const vector<string>& certs,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details);
-
- // Start the cert verification.
- QuicAsyncStatus VerifyCert(
- const string& hostname,
- const uint16_t port,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback);
-
int DoLoop(int last_io_result);
void OnIOComplete(int result);
int DoVerifyCert(int result);
@@ -148,9 +125,6 @@
// passed to CertVerifier::Verify.
int cert_verify_flags_;
- // If set to true, enforces policy checking in DoVerifyCertComplete().
- bool enforce_policy_checking_;
-
State next_state_;
base::TimeTicks start_time_;
@@ -174,7 +148,6 @@
transport_security_state_(transport_security_state),
cert_transparency_verifier_(cert_transparency_verifier),
cert_verify_flags_(cert_verify_flags),
- enforce_policy_checking_(true),
next_state_(STATE_NONE),
start_time_(base::TimeTicks::Now()),
net_log_(net_log) {
@@ -222,9 +195,27 @@
verify_details_.reset(new ProofVerifyDetailsChromium);
- // Converts |certs| to |cert_|.
- if (!GetX509Certificate(certs, error_details, verify_details))
+ if (certs.empty()) {
+ *error_details = "Failed to create certificate chain. Certs are empty.";
+ DLOG(WARNING) << *error_details;
+ verify_details_->cert_verify_result.cert_status = CERT_STATUS_INVALID;
+ *verify_details = std::move(verify_details_);
return QUIC_FAILURE;
+ }
+
+ // Convert certs to X509Certificate.
+ vector<StringPiece> cert_pieces(certs.size());
+ for (unsigned i = 0; i < certs.size(); i++) {
+ cert_pieces[i] = base::StringPiece(certs[i]);
+ }
+ cert_ = X509Certificate::CreateFromDERCertChain(cert_pieces);
+ if (!cert_.get()) {
+ *error_details = "Failed to create certificate chain";
+ DLOG(WARNING) << *error_details;
+ verify_details_->cert_verify_result.cert_status = CERT_STATUS_INVALID;
+ *verify_details = std::move(verify_details_);
+ return QUIC_FAILURE;
+ }
if (!cert_sct.empty()) {
// Note that this is a completely synchronous operation: The CT Log Verifier
@@ -246,75 +237,6 @@
return QUIC_FAILURE;
}
- DCHECK(enforce_policy_checking_);
- return VerifyCert(hostname, port, error_details, verify_details,
- std::move(callback));
-}
-
-QuicAsyncStatus ProofVerifierChromium::Job::VerifyCertChain(
- const string& hostname,
- const vector<string>& certs,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback) {
- DCHECK(error_details);
- DCHECK(verify_details);
- DCHECK(callback);
-
- error_details->clear();
-
- if (STATE_NONE != next_state_) {
- *error_details = "Certificate is already set and VerifyCertChain has begun";
- DLOG(DFATAL) << *error_details;
- return QUIC_FAILURE;
- }
-
- verify_details_.reset(new ProofVerifyDetailsChromium);
-
- // Converts |certs| to |cert_|.
- if (!GetX509Certificate(certs, error_details, verify_details))
- return QUIC_FAILURE;
-
- enforce_policy_checking_ = false;
- // |port| is not needed because |enforce_policy_checking_| is false.
- return VerifyCert(hostname, /*port=*/0, error_details, verify_details,
- std::move(callback));
-}
-
-bool ProofVerifierChromium::Job::GetX509Certificate(
- const vector<string>& certs,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details) {
- if (certs.empty()) {
- *error_details = "Failed to create certificate chain. Certs are empty.";
- DLOG(WARNING) << *error_details;
- verify_details_->cert_verify_result.cert_status = CERT_STATUS_INVALID;
- *verify_details = std::move(verify_details_);
- return false;
- }
-
- // Convert certs to X509Certificate.
- vector<StringPiece> cert_pieces(certs.size());
- for (unsigned i = 0; i < certs.size(); i++) {
- cert_pieces[i] = base::StringPiece(certs[i]);
- }
- cert_ = X509Certificate::CreateFromDERCertChain(cert_pieces);
- if (!cert_.get()) {
- *error_details = "Failed to create certificate chain";
- DLOG(WARNING) << *error_details;
- verify_details_->cert_verify_result.cert_status = CERT_STATUS_INVALID;
- *verify_details = std::move(verify_details_);
- return false;
- }
- return true;
-}
-
-QuicAsyncStatus ProofVerifierChromium::Job::VerifyCert(
- const string& hostname,
- const uint16_t port,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback) {
hostname_ = hostname;
port_ = port;
@@ -393,8 +315,7 @@
// If the connection was good, check HPKP and CT status simultaneously,
// but prefer to treat the HPKP error as more serious, if there was one.
- if (enforce_policy_checking_ &&
- (result == OK ||
+ if ((result == OK ||
(IsCertificateError(result) && IsCertStatusMinorError(cert_status)))) {
if ((cert_verify_result.cert_status & CERT_STATUS_IS_EV)) {
ct::EVPolicyCompliance ev_policy_compliance =
@@ -584,32 +505,9 @@
QuicAsyncStatus status = job->VerifyProof(
hostname, port, server_config, quic_version, chlo_hash, certs, cert_sct,
signature, error_details, verify_details, std::move(callback));
- if (status == QUIC_PENDING)
+ if (status == QUIC_PENDING) {
active_jobs_.insert(job.release());
- return status;
-}
-
-QuicAsyncStatus ProofVerifierChromium::VerifyCertChain(
- const std::string& hostname,
- const std::vector<std::string>& certs,
- const ProofVerifyContext* verify_context,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback) {
- if (!verify_context) {
- *error_details = "Missing context";
- return QUIC_FAILURE;
}
- const ProofVerifyContextChromium* chromium_context =
- reinterpret_cast<const ProofVerifyContextChromium*>(verify_context);
- std::unique_ptr<Job> job(
- new Job(this, cert_verifier_, ct_policy_enforcer_,
- transport_security_state_, cert_transparency_verifier_,
- chromium_context->cert_verify_flags, chromium_context->net_log));
- QuicAsyncStatus status = job->VerifyCertChain(
- hostname, certs, error_details, verify_details, std::move(callback));
- if (status == QUIC_PENDING)
- active_jobs_.insert(job.release());
return status;
}
diff --git a/net/quic/crypto/proof_verifier_chromium.h b/net/quic/crypto/proof_verifier_chromium.h
index ee9ddab..b15751c 100644
--- a/net/quic/crypto/proof_verifier_chromium.h
+++ b/net/quic/crypto/proof_verifier_chromium.h
@@ -85,13 +85,6 @@
std::string* error_details,
std::unique_ptr<ProofVerifyDetails>* verify_details,
std::unique_ptr<ProofVerifierCallback> callback) override;
- QuicAsyncStatus VerifyCertChain(
- const std::string& hostname,
- const std::vector<std::string>& certs,
- const ProofVerifyContext* verify_context,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback) override;
private:
class Job;
diff --git a/net/quic/crypto/proof_verifier_chromium_test.cc b/net/quic/crypto/proof_verifier_chromium_test.cc
index 47d2637..fff1bf6d 100644
--- a/net/quic/crypto/proof_verifier_chromium_test.cc
+++ b/net/quic/crypto/proof_verifier_chromium_test.cc
@@ -587,34 +587,5 @@
CERT_STATUS_CERTIFICATE_TRANSPARENCY_REQUIRED);
}
-// Tests that the VerifyCertChain verifies certificates.
-TEST_F(ProofVerifierChromiumTest, VerifyCertChain) {
- scoped_refptr<X509Certificate> test_cert = GetTestServerCertificate();
- ASSERT_TRUE(test_cert);
-
- CertVerifyResult dummy_result;
- dummy_result.verified_cert = test_cert;
- dummy_result.cert_status = 0;
-
- MockCertVerifier dummy_verifier;
- dummy_verifier.AddResultForCert(test_cert.get(), dummy_result, OK);
-
- ProofVerifierChromium proof_verifier(&dummy_verifier, &ct_policy_enforcer_,
- &transport_security_state_,
- ct_verifier_.get());
-
- std::unique_ptr<DummyProofVerifierCallback> callback(
- new DummyProofVerifierCallback);
- QuicAsyncStatus status = proof_verifier.VerifyCertChain(
- kTestHostname, certs_, verify_context_.get(), &error_details_, &details_,
- std::move(callback));
- ASSERT_EQ(QUIC_SUCCESS, status);
-
- ASSERT_TRUE(details_.get());
- ProofVerifyDetailsChromium* verify_details =
- static_cast<ProofVerifyDetailsChromium*>(details_.get());
- EXPECT_EQ(0u, verify_details->cert_verify_result.cert_status);
-}
-
} // namespace test
} // namespace net
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index e3aa922..91e442d 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -34,7 +34,6 @@
#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"
@@ -168,88 +167,6 @@
} // namespace
-// Responsible for verifying the certificates saved in
-// QuicCryptoClientConfig, and for notifying any associated requests when
-// complete. Results from cert verification are ignored.
-class QuicStreamFactory::CertVerifierJob {
- public:
- // ProofVerifierCallbackImpl is passed as the callback method to
- // VerifyCertChain. The ProofVerifier calls this class with the result of cert
- // verification when verification is performed asynchronously.
- class ProofVerifierCallbackImpl : public ProofVerifierCallback {
- 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();
- }
-
- // Starts verification of certs cached in the |crypto_config|.
- QuicAsyncStatus Run(QuicCryptoClientConfig* crypto_config,
- const CompletionCallback& callback) {
- QuicCryptoClientConfig::CachedState* cached =
- crypto_config->LookupOrCreate(server_id_);
- ProofVerifierCallbackImpl* verify_callback =
- new ProofVerifierCallbackImpl(this);
- QuicAsyncStatus status = crypto_config->proof_verifier()->VerifyCertChain(
- server_id_.host(), cached->certs(), verify_context_.get(),
- &verify_error_details_, &verify_details_,
- std::unique_ptr<ProofVerifierCallback>(verify_callback));
- if (status == QUIC_PENDING) {
- verify_callback_ = verify_callback;
- callback_ = 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 {
@@ -447,8 +364,9 @@
int QuicStreamFactory::Job::DoResolveHost() {
// Start loading the data now, and wait for it after we resolve the host.
- if (server_info_)
+ if (server_info_) {
server_info_->Start();
+ }
io_state_ = STATE_RESOLVE_HOST_COMPLETE;
dns_resolution_start_time_ = base::TimeTicks::Now();
@@ -470,8 +388,9 @@
// Inform the factory of this resolution, which will set up
// a session alias, if possible.
- if (factory_->OnResolution(key_, address_list_))
+ if (factory_->OnResolution(key_, address_list_)) {
return OK;
+ }
if (server_info_)
io_state_ = STATE_LOAD_SERVER_INFO;
@@ -550,12 +469,14 @@
return rv;
}
- if (!session_->connection()->connected())
+ if (!session_->connection()->connected()) {
return ERR_CONNECTION_CLOSED;
+ }
session_->StartReading();
- if (!session_->connection()->connected())
+ if (!session_->connection()->connected()) {
return ERR_QUIC_PROTOCOL_ERROR;
+ }
bool require_confirmation = factory_->require_confirmation() ||
was_alternative_service_recently_broken_;
@@ -564,9 +485,8 @@
base::Bind(&QuicStreamFactory::Job::OnIOComplete, GetWeakPtr()));
if (!session_->connection()->connected() &&
- session_->error() == QUIC_PROOF_INVALID) {
+ session_->error() == QUIC_PROOF_INVALID)
return ERR_QUIC_HANDSHAKE_FAILED;
- }
return rv;
}
@@ -583,8 +503,9 @@
int QuicStreamFactory::Job::DoConnectComplete(int rv) {
if (session_ && session_->error() == QUIC_CRYPTO_HANDSHAKE_STATELESS_REJECT) {
num_sent_client_hellos_ += session_->GetNumSentClientHellos();
- if (num_sent_client_hellos_ >= QuicCryptoClientStream::kMaxClientHellos)
+ if (num_sent_client_hellos_ >= QuicCryptoClientStream::kMaxClientHellos) {
return ERR_QUIC_HANDSHAKE_FAILED;
+ }
// The handshake was rejected statelessly, so create another connection
// to resume the handshake.
io_state_ = STATE_CONNECT;
@@ -712,7 +633,6 @@
bool migrate_sessions_on_network_change,
bool migrate_sessions_early,
bool force_hol_blocking,
- bool race_cert_verification,
const QuicTagVector& connection_options,
bool enable_token_binding)
: require_confirmation_(true),
@@ -767,7 +687,6 @@
migrate_sessions_early_(migrate_sessions_early &&
migrate_sessions_on_network_change_),
force_hol_blocking_(force_hol_blocking),
- race_cert_verification_(race_cert_verification),
port_seed_(random_generator_->RandUint64()),
check_persisted_supports_quic_(true),
has_initialized_data_(false),
@@ -835,8 +754,6 @@
STLDeleteElements(&(active_jobs_[server_id]));
active_jobs_.erase(server_id);
}
- while (!active_cert_verifier_jobs_.empty())
- active_cert_verifier_jobs_.erase(active_cert_verifier_jobs_.begin());
if (ssl_config_service_.get())
ssl_config_service_->RemoveObserver(this);
if (migrate_sessions_on_network_change_) {
@@ -968,12 +885,11 @@
// don't wait for Cache thread to load the data for that server.
load_from_disk_cache = false;
}
- if (load_from_disk_cache && CryptoConfigCacheIsEmpty(server_id))
+ if (load_from_disk_cache && CryptoConfigCacheIsEmpty(server_id)) {
quic_server_info = quic_server_info_factory_->GetForServer(server_id);
+ }
}
- ignore_result(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),
@@ -1034,8 +950,9 @@
const AddressList& address_list) {
const QuicServerId& server_id(key.server_id());
DCHECK(!HasActiveSession(server_id));
- if (disable_connection_pooling_)
+ if (disable_connection_pooling_) {
return false;
+ }
for (const IPEndPoint& address : address_list) {
if (!ContainsKey(ip_aliases_, address))
continue;
@@ -1105,10 +1022,6 @@
job_requests_map_.erase(server_id);
}
-void QuicStreamFactory::OnCertVerifyJobComplete(CertVerifierJob* job, int rv) {
- active_cert_verifier_jobs_.erase(job->server_id());
-}
-
std::unique_ptr<QuicHttpStream> QuicStreamFactory::CreateFromSession(
QuicChromiumClientSession* session) {
return std::unique_ptr<QuicHttpStream>(
@@ -1224,8 +1137,9 @@
DCHECK_EQ(session, active_sessions_[server_id]);
// Track sessions which have recently gone away so that we can disable
// port suggestions.
- if (session->goaway_received())
+ if (session->goaway_received()) {
gone_away_aliases_.insert(*it);
+ }
active_sessions_.erase(server_id);
ProcessGoingAwaySession(session, server_id, true);
@@ -1234,8 +1148,9 @@
if (!aliases.empty()) {
const IPEndPoint peer_address = session->connection()->peer_address();
ip_aliases_[peer_address].erase(session);
- if (ip_aliases_[peer_address].empty())
+ if (ip_aliases_[peer_address].empty()) {
ip_aliases_.erase(peer_address);
+ }
}
session_aliases_.erase(session);
}
@@ -1340,8 +1255,9 @@
QuicChromiumClientSession* session) {
const AliasSet& aliases = session_aliases_[session];
- if (aliases.empty())
+ if (aliases.empty()) {
return;
+ }
for (const QuicSessionKey& key : aliases) {
const QuicServerId& server_id = key.server_id();
@@ -1353,8 +1269,9 @@
const IPEndPoint peer_address = session->connection()->peer_address();
ip_aliases_[peer_address].erase(session);
- if (ip_aliases_[peer_address].empty())
+ if (ip_aliases_[peer_address].empty()) {
ip_aliases_.erase(peer_address);
+ }
QuicSessionKey key = *aliases.begin();
session_aliases_.erase(session);
Job* job = new Job(this, host_resolver_, session, key);
@@ -1450,8 +1367,9 @@
NetworkChangeNotifier::NetworkList network_list;
NetworkChangeNotifier::GetConnectedNetworks(&network_list);
for (NetworkHandle new_network : network_list) {
- if (new_network != old_network)
+ if (new_network != old_network) {
return new_network;
+ }
}
return NetworkChangeNotifier::kInvalidNetworkHandle;
}
@@ -1639,11 +1557,6 @@
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) {
@@ -1732,13 +1645,15 @@
// Passing in kInvalidNetworkHandle binds socket to default network.
int rv = ConfigureSocket(socket.get(), addr,
NetworkChangeNotifier::kInvalidNetworkHandle);
- if (rv != OK)
+ if (rv != OK) {
return rv;
+ }
- if (enable_port_selection)
+ if (enable_port_selection) {
DCHECK_LE(1u, port_suggester->call_count());
- else
+ } else {
DCHECK_EQ(0u, port_suggester->call_count());
+ }
if (!helper_.get()) {
helper_.reset(
@@ -1852,29 +1767,6 @@
return cached->IsEmpty();
}
-QuicAsyncStatus QuicStreamFactory::StartCertVerifyJob(
- const QuicServerId& server_id,
- int cert_verify_flags,
- const BoundNetLog& net_log) {
- if (!race_cert_verification_)
- return QUIC_FAILURE;
- QuicCryptoClientConfig::CachedState* cached =
- crypto_config_.LookupOrCreate(server_id);
- if (!cached || cached->certs().empty() ||
- HasActiveCertVerifierJob(server_id)) {
- return QUIC_FAILURE;
- }
- 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] = std::move(cert_verifier_job);
- return status;
-}
-
void QuicStreamFactory::InitializeCachedStateInCryptoConfig(
const QuicServerId& server_id,
const std::unique_ptr<QuicServerInfo>& server_info,
diff --git a/net/quic/quic_stream_factory.h b/net/quic/quic_stream_factory.h
index 7fcb960..fbb91ab 100644
--- a/net/quic/quic_stream_factory.h
+++ b/net/quic/quic_stream_factory.h
@@ -193,7 +193,6 @@
bool migrate_sessions_on_network_change,
bool migrate_sessions_early,
bool force_hol_blocking,
- bool race_cert_verification,
const QuicTagVector& connection_options,
bool enable_token_binding);
~QuicStreamFactory() override;
@@ -366,7 +365,6 @@
private:
class Job;
- class CertVerifierJob;
friend class test::QuicStreamFactoryPeer;
FRIEND_TEST_ALL_PREFIXES(HttpStreamFactoryTest, QuicLossyProxyMarkedAsBad);
@@ -384,8 +382,6 @@
typedef std::map<QuicServerId, RequestSet> ServerIDRequestsMap;
typedef std::deque<enum QuicChromiumClientSession::QuicDisabledReason>
DisabledReasonsQueue;
- typedef std::map<QuicServerId, std::unique_ptr<CertVerifierJob>>
- CertVerifierJobMap;
enum FactoryStatus {
OPEN, // New streams may be created.
@@ -405,10 +401,8 @@
bool OnResolution(const QuicSessionKey& key, const AddressList& address_list);
void OnJobComplete(Job* job, int rv);
- void OnCertVerifyJobComplete(CertVerifierJob* job, int rv);
bool HasActiveSession(const QuicServerId& server_id) const;
bool HasActiveJob(const QuicServerId& server_id) const;
- bool HasActiveCertVerifierJob(const QuicServerId& server_id) const;
int CreateSession(const QuicSessionKey& key,
int cert_verify_flags,
std::unique_ptr<QuicServerInfo> quic_server_info,
@@ -430,13 +424,6 @@
bool CryptoConfigCacheIsEmpty(const QuicServerId& server_id);
- // Starts an asynchronous job for cert verification if
- // |race_cert_verification_| is enabled and if there are cached certs for the
- // given |server_id|.
- QuicAsyncStatus StartCertVerifyJob(const QuicServerId& server_id,
- int cert_verify_flags,
- const BoundNetLog& net_log);
-
// Initializes the cached state associated with |server_id| in
// |crypto_config_| with the information in |server_info|. Populates
// |connection_id| with the next server designated connection id,
@@ -504,8 +491,6 @@
ServerIDRequestsMap job_requests_map_;
RequestMap active_requests_;
- CertVerifierJobMap active_cert_verifier_jobs_;
-
QuicVersionVector supported_versions_;
// Determine if we should consistently select a client UDP port. If false,
@@ -591,9 +576,6 @@
// If set, force HOL blocking. For measurement purposes.
const bool force_hol_blocking_;
- // Set if cert verification is to be raced with host resolution.
- bool race_cert_verification_;
-
// Each profile will (probably) have a unique port_seed_ value. This value
// is used to help seed a pseudo-random number generator (PortSuggester) so
// that we consistently (within this profile) suggest the same ephemeral
diff --git a/net/quic/quic_stream_factory_test.cc b/net/quic/quic_stream_factory_test.cc
index a536b22..cb159de8 100644
--- a/net/quic/quic_stream_factory_test.cc
+++ b/net/quic/quic_stream_factory_test.cc
@@ -321,16 +321,16 @@
idle_connection_timeout_seconds_(kIdleConnectionTimeoutSeconds),
migrate_sessions_on_network_change_(false),
migrate_sessions_early_(false),
- force_hol_blocking_(false),
- race_cert_verification_(false) {
+ force_hol_blocking_(false) {
clock_->AdvanceTime(QuicTime::Delta::FromSeconds(1));
}
~QuicStreamFactoryTestBase() {
// If |factory_| was initialized, then it took over ownership of |clock_|.
// If |factory_| was not initialized, then |clock_| needs to be destroyed.
- if (!factory_)
+ if (!factory_) {
delete clock_;
+ }
}
void Initialize() {
@@ -353,8 +353,8 @@
close_sessions_on_ip_change_,
disable_quic_on_timeout_with_open_streams_,
idle_connection_timeout_seconds_, migrate_sessions_on_network_change_,
- migrate_sessions_early_, force_hol_blocking_, race_cert_verification_,
- QuicTagVector(), /*enable_token_binding*/ false));
+ migrate_sessions_early_, force_hol_blocking_, QuicTagVector(),
+ /*enable_token_binding*/ false));
factory_->set_require_confirmation(false);
EXPECT_FALSE(factory_->has_quic_server_info_factory());
factory_->set_quic_server_info_factory(new MockQuicServerInfoFactory());
@@ -379,11 +379,6 @@
return QuicStreamFactoryPeer::HasActiveSession(factory_.get(), server_id);
}
- bool HasActiveCertVerifierJob(const QuicServerId& server_id) {
- return QuicStreamFactoryPeer::HasActiveCertVerifierJob(factory_.get(),
- server_id);
- }
-
QuicChromiumClientSession* GetActiveSession(
const HostPortPair& host_port_pair) {
QuicServerId server_id(host_port_pair, PRIVACY_MODE_DISABLED);
@@ -552,7 +547,6 @@
bool migrate_sessions_on_network_change_;
bool migrate_sessions_early_;
bool force_hol_blocking_;
- bool race_cert_verification_;
};
class QuicStreamFactoryTest : public QuicStreamFactoryTestBase,
@@ -4142,64 +4136,6 @@
EXPECT_EQ(test_cert2, cached2->certs()[0]);
}
-TEST_P(QuicStreamFactoryTest, StartCertVerifyJob) {
- Initialize();
-
- MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING, 0)};
- SequencedSocketData socket_data(reads, arraysize(reads), nullptr, 0);
- socket_factory_.AddSocketDataProvider(&socket_data);
-
- // Save current state of |race_cert_verification|.
- bool race_cert_verification =
- QuicStreamFactoryPeer::GetRaceCertVerification(factory_.get());
-
- // Load server config.
- HostPortPair host_port_pair("test.example.com", kDefaultServerPort);
- QuicServerId quic_server_id(host_port_pair_, privacy_mode_);
- QuicStreamFactoryPeer::CacheDummyServerConfig(factory_.get(), quic_server_id);
-
- QuicStreamFactoryPeer::SetRaceCertVerification(factory_.get(), true);
-
- // Start CertVerifyJob.
- QuicAsyncStatus status = QuicStreamFactoryPeer::StartCertVerifyJob(
- factory_.get(), quic_server_id, /*cert_verify_flags=*/0, net_log_);
- EXPECT_NE(QUIC_FAILURE, status);
-
- if (status == QUIC_PENDING) {
- // Verify CertVerifierJob has started.
- EXPECT_TRUE(HasActiveCertVerifierJob(quic_server_id));
-
- while (HasActiveCertVerifierJob(quic_server_id)) {
- base::RunLoop().RunUntilIdle();
- }
- }
- // Verify CertVerifierJob has finished.
- EXPECT_FALSE(HasActiveCertVerifierJob(quic_server_id));
-
- // Start a QUIC request.
- QuicStreamRequest request(factory_.get());
- EXPECT_EQ(ERR_IO_PENDING,
- request.Request(host_port_pair_, privacy_mode_,
- /*cert_verify_flags=*/0, url_, "GET", net_log_,
- callback_.callback()));
-
- EXPECT_EQ(OK, callback_.WaitForResult());
-
- std::unique_ptr<QuicHttpStream> stream = request.CreateStream();
- EXPECT_TRUE(stream.get());
-
- // Restore |race_cert_verification|.
- QuicStreamFactoryPeer::SetRaceCertVerification(factory_.get(),
- race_cert_verification);
-
- EXPECT_TRUE(socket_data.AllReadDataConsumed());
- EXPECT_TRUE(socket_data.AllWriteDataConsumed());
-
- // Verify there are no outstanding CertVerifierJobs after request has
- // finished.
- EXPECT_FALSE(HasActiveCertVerifierJob(quic_server_id));
-}
-
TEST_P(QuicStreamFactoryTest, QuicDoingZeroRTT) {
Initialize();
diff --git a/net/quic/test_tools/quic_stream_factory_peer.cc b/net/quic/test_tools/quic_stream_factory_peer.cc
index ec7034a4..d891cfe 100644
--- a/net/quic/test_tools/quic_stream_factory_peer.cc
+++ b/net/quic/test_tools/quic_stream_factory_peer.cc
@@ -7,14 +7,11 @@
#include <string>
#include <vector>
-#include "net/cert/x509_certificate.h"
#include "net/quic/crypto/quic_crypto_client_config.h"
#include "net/quic/quic_chromium_client_session.h"
#include "net/quic/quic_clock.h"
#include "net/quic/quic_http_stream.h"
#include "net/quic/quic_stream_factory.h"
-#include "net/test/cert_test_util.h"
-#include "net/test/test_data_directory.h"
using std::string;
using std::vector;
@@ -36,12 +33,6 @@
return factory->HasActiveSession(server_id);
}
-bool QuicStreamFactoryPeer::HasActiveCertVerifierJob(
- QuicStreamFactory* factory,
- const QuicServerId& server_id) {
- return factory->HasActiveCertVerifierJob(server_id);
-}
-
QuicChromiumClientSession* QuicStreamFactoryPeer::GetActiveSession(
QuicStreamFactory* factory,
const QuicServerId& server_id) {
@@ -91,25 +82,6 @@
factory->delay_tcp_race_ = delay_tcp_race;
}
-bool QuicStreamFactoryPeer::GetRaceCertVerification(
- QuicStreamFactory* factory) {
- return factory->race_cert_verification_;
-}
-
-void QuicStreamFactoryPeer::SetRaceCertVerification(
- QuicStreamFactory* factory,
- bool race_cert_verification) {
- factory->race_cert_verification_ = race_cert_verification;
-}
-
-QuicAsyncStatus QuicStreamFactoryPeer::StartCertVerifyJob(
- QuicStreamFactory* factory,
- const QuicServerId& server_id,
- int cert_verify_flags,
- const BoundNetLog& net_log) {
- return factory->StartCertVerifyJob(server_id, cert_verify_flags, net_log);
-}
-
void QuicStreamFactoryPeer::SetYieldAfterPackets(QuicStreamFactory* factory,
int yield_after_packets) {
factory->yield_after_packets_ = yield_after_packets;
@@ -177,15 +149,9 @@
string server_config(reinterpret_cast<const char*>(&scfg), sizeof(scfg));
string source_address_token("test_source_address_token");
string signature("test_signature");
-
+ string test_cert("test_cert");
vector<string> certs;
- // Load a certificate that is valid for *.example.org
- scoped_refptr<X509Certificate> cert(
- ImportCertFromFile(GetTestCertsDirectory(), "wildcard.pem"));
- DCHECK(cert);
- std::string der_bytes;
- DCHECK(X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_bytes));
- certs.push_back(der_bytes);
+ certs.push_back(test_cert);
QuicCryptoClientConfig* crypto_config = &factory->crypto_config_;
QuicCryptoClientConfig::CachedState* cached =
diff --git a/net/quic/test_tools/quic_stream_factory_peer.h b/net/quic/test_tools/quic_stream_factory_peer.h
index 385ebe3d..bf18a18 100644
--- a/net/quic/test_tools/quic_stream_factory_peer.h
+++ b/net/quic/test_tools/quic_stream_factory_peer.h
@@ -12,7 +12,6 @@
#include "base/task_runner.h"
#include "net/base/host_port_pair.h"
#include "net/base/privacy_mode.h"
-#include "net/log/net_log.h"
#include "net/quic/quic_protocol.h"
#include "net/quic/quic_server_id.h"
#include "net/quic/quic_time.h"
@@ -37,9 +36,6 @@
static bool HasActiveSession(QuicStreamFactory* factory,
const QuicServerId& server_id);
- static bool HasActiveCertVerifierJob(QuicStreamFactory* factory,
- const QuicServerId& server_id);
-
static QuicChromiumClientSession* GetActiveSession(
QuicStreamFactory* factory,
const QuicServerId& server_id);
@@ -63,16 +59,6 @@
static void SetDelayTcpRace(QuicStreamFactory* factory, bool delay_tcp_race);
- static bool GetRaceCertVerification(QuicStreamFactory* factory);
-
- static void SetRaceCertVerification(QuicStreamFactory* factory,
- bool race_cert_verification);
-
- static QuicAsyncStatus StartCertVerifyJob(QuicStreamFactory* factory,
- const QuicServerId& server_id,
- int cert_verify_flags,
- const BoundNetLog& net_log);
-
static void SetYieldAfterPackets(QuicStreamFactory* factory,
int yield_after_packets);
diff --git a/net/tools/quic/test_tools/quic_test_client.cc b/net/tools/quic/test_tools/quic_test_client.cc
index 0e80d61d..ad5748d 100644
--- a/net/tools/quic/test_tools/quic_test_client.cc
+++ b/net/tools/quic/test_tools/quic_test_client.cc
@@ -83,16 +83,6 @@
return QUIC_SUCCESS;
}
- QuicAsyncStatus VerifyCertChain(
- const std::string& hostname,
- const std::vector<std::string>& certs,
- const ProofVerifyContext* verify_context,
- std::string* error_details,
- std::unique_ptr<ProofVerifyDetails>* verify_details,
- std::unique_ptr<ProofVerifierCallback> callback) override {
- return QUIC_SUCCESS;
- }
-
const string& common_name() const { return common_name_; }
const string& cert_sct() const { return cert_sct_; }
diff --git a/net/url_request/url_request_context_builder.cc b/net/url_request/url_request_context_builder.cc
index 8eded65..3f93b3e 100644
--- a/net/url_request/url_request_context_builder.cc
+++ b/net/url_request/url_request_context_builder.cc
@@ -193,8 +193,7 @@
quic_close_sessions_on_ip_change(false),
quic_migrate_sessions_on_network_change(false),
quic_migrate_sessions_early(false),
- quic_disable_bidirectional_streams(false),
- quic_race_cert_verification(false) {}
+ quic_disable_bidirectional_streams(false) {}
URLRequestContextBuilder::HttpNetworkSessionParams::~HttpNetworkSessionParams()
{}
@@ -442,8 +441,6 @@
http_network_session_params_.quic_migrate_sessions_early;
network_session_params.quic_disable_bidirectional_streams =
http_network_session_params_.quic_disable_bidirectional_streams;
- network_session_params.quic_race_cert_verification =
- http_network_session_params_.quic_race_cert_verification;
if (proxy_delegate_) {
network_session_params.proxy_delegate = proxy_delegate_.get();
storage->set_proxy_delegate(std::move(proxy_delegate_));
diff --git a/net/url_request/url_request_context_builder.h b/net/url_request/url_request_context_builder.h
index 58f0516a..ed76c00 100644
--- a/net/url_request/url_request_context_builder.h
+++ b/net/url_request/url_request_context_builder.h
@@ -105,7 +105,6 @@
bool quic_migrate_sessions_on_network_change;
bool quic_migrate_sessions_early;
bool quic_disable_bidirectional_streams;
- bool quic_race_cert_verification;
};
URLRequestContextBuilder();
@@ -284,11 +283,6 @@
quic_disable_bidirectional_streams;
}
- void set_quic_race_cert_verification(bool quic_race_cert_verification) {
- http_network_session_params_.quic_race_cert_verification =
- quic_race_cert_verification;
- }
-
void set_throttling_enabled(bool throttling_enabled) {
throttling_enabled_ = throttling_enabled;
}
