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; }