In TlsClientHandshaker, only insert data into cache if all required states are present. ApplicationState is now optional to TlsClientHandshaker because not all applications have it. gfe-relnote: unused code. not protected. PiperOrigin-RevId: 307719344 Change-Id: Ib4e359c6a13c9b7b805dca7a1cfd3b1bf89af6ef
diff --git a/quic/core/http/quic_spdy_client_session.cc b/quic/core/http/quic_spdy_client_session.cc index 07b9c7a..65751b9 100644 --- a/quic/core/http/quic_spdy_client_session.cc +++ b/quic/core/http/quic_spdy_client_session.cc
@@ -191,7 +191,7 @@ return std::make_unique<QuicCryptoClientStream>( server_id_, this, crypto_config_->proof_verifier()->CreateDefaultContext(), crypto_config_, - this); + this, /*has_application_state = */ true); } bool QuicSpdyClientSession::IsAuthorized(const std::string& /*authority*/) {
diff --git a/quic/core/http/quic_spdy_client_session_test.cc b/quic/core/http/quic_spdy_client_session_test.cc index dc72a73..eea081c 100644 --- a/quic/core/http/quic_spdy_client_session_test.cc +++ b/quic/core/http/quic_spdy_client_session_test.cc
@@ -173,6 +173,9 @@ } std::unique_ptr<QuicCryptoServerConfig> crypto_config = crypto_test_utils::CryptoServerConfigForTesting(); + if (connection_->version().handshake_protocol == PROTOCOL_TLS1_3) { + SSL_CTX_clear_options(crypto_config->ssl_ctx(), SSL_OP_NO_TICKET); + } crypto_test_utils::HandshakeWithFakeServer( &config, crypto_config.get(), &helper_, &alarm_factory_, connection_, stream, AlpnForVersion(connection_->version()));
diff --git a/quic/core/quic_crypto_client_handshaker_test.cc b/quic/core/quic_crypto_client_handshaker_test.cc index 92de76f..7e941c2 100644 --- a/quic/core/quic_crypto_client_handshaker_test.cc +++ b/quic/core/quic_crypto_client_handshaker_test.cc
@@ -136,11 +136,13 @@ {version_})), session_(connection_, false), crypto_client_config_(std::make_unique<InsecureProofVerifier>()), - client_stream_(new QuicCryptoClientStream(server_id_, - &session_, - nullptr, - &crypto_client_config_, - &proof_handler_)), + client_stream_( + new QuicCryptoClientStream(server_id_, + &session_, + nullptr, + &crypto_client_config_, + &proof_handler_, + /*has_application_state = */ false)), handshaker_(server_id_, client_stream_, &session_,
diff --git a/quic/core/quic_crypto_client_stream.cc b/quic/core/quic_crypto_client_stream.cc index 538d828..6f2ffcf 100644 --- a/quic/core/quic_crypto_client_stream.cc +++ b/quic/core/quic_crypto_client_stream.cc
@@ -32,7 +32,8 @@ QuicSession* session, std::unique_ptr<ProofVerifyContext> verify_context, QuicCryptoClientConfig* crypto_config, - ProofHandler* proof_handler) + ProofHandler* proof_handler, + bool has_application_state) : QuicCryptoClientStreamBase(session) { DCHECK_EQ(Perspective::IS_CLIENT, session->connection()->perspective()); switch (session->connection()->version().handshake_protocol) { @@ -44,7 +45,7 @@ case PROTOCOL_TLS1_3: handshaker_ = std::make_unique<TlsClientHandshaker>( server_id, this, session, std::move(verify_context), crypto_config, - proof_handler); + proof_handler, has_application_state); break; case PROTOCOL_UNSUPPORTED: QUIC_BUG << "Attempting to create QuicCryptoClientStream for unknown "
diff --git a/quic/core/quic_crypto_client_stream.h b/quic/core/quic_crypto_client_stream.h index 5ec1b01..d095deb 100644 --- a/quic/core/quic_crypto_client_stream.h +++ b/quic/core/quic_crypto_client_stream.h
@@ -187,7 +187,8 @@ QuicSession* session, std::unique_ptr<ProofVerifyContext> verify_context, QuicCryptoClientConfig* crypto_config, - ProofHandler* proof_handler); + ProofHandler* proof_handler, + bool has_application_state); QuicCryptoClientStream(const QuicCryptoClientStream&) = delete; QuicCryptoClientStream& operator=(const QuicCryptoClientStream&) = delete;
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc index 4b6c2af..f16fdd5 100644 --- a/quic/core/tls_client_handshaker.cc +++ b/quic/core/tls_client_handshaker.cc
@@ -52,7 +52,8 @@ QuicSession* session, std::unique_ptr<ProofVerifyContext> verify_context, QuicCryptoClientConfig* crypto_config, - QuicCryptoClientStream::ProofHandler* proof_handler) + QuicCryptoClientStream::ProofHandler* proof_handler, + bool has_application_state) : TlsHandshaker(stream, session), session_(session), server_id_(server_id), @@ -63,6 +64,7 @@ user_agent_id_(crypto_config->user_agent_id()), pre_shared_key_(crypto_config->pre_shared_key()), crypto_negotiated_params_(new QuicCryptoNegotiatedParameters), + has_application_state_(has_application_state), tls_connection_(crypto_config->ssl_ctx(), this) {} TlsClientHandshaker::~TlsClientHandshaker() { @@ -501,11 +503,25 @@ } void TlsClientHandshaker::InsertSession(bssl::UniquePtr<SSL_SESSION> session) { + if (!received_transport_params_) { + QUIC_BUG << "Transport parameters isn't received"; + return; + } if (session_cache_ == nullptr) { QUIC_DVLOG(1) << "No session cache, not inserting a session"; return; } - session_cache_->Insert(server_id_, std::move(session), nullptr, nullptr); + if (has_application_state_ && !received_application_state_) { + // Application state is not received yet. cache the sessions. + if (cached_tls_sessions_[0] != nullptr) { + cached_tls_sessions_[1] = std::move(cached_tls_sessions_[0]); + } + cached_tls_sessions_[0] = std::move(session); + return; + } + session_cache_->Insert(server_id_, std::move(session), + received_transport_params_.get(), + received_application_state_.get()); } void TlsClientHandshaker::WriteMessage(EncryptionLevel level, @@ -522,9 +538,17 @@ void TlsClientHandshaker::OnApplicationState( std::unique_ptr<ApplicationState> application_state) { received_application_state_ = std::move(application_state); - if (session_cache_ != nullptr) { - // TODO(renjietang): cache the TLS session ticket and insert them together. - session_cache_->Insert(server_id_, nullptr, nullptr, + // At least one tls session is cached before application state is received. So + // insert now. + if (session_cache_ != nullptr && cached_tls_sessions_[0] != nullptr) { + if (cached_tls_sessions_[1] != nullptr) { + // Insert the older session first. + session_cache_->Insert(server_id_, std::move(cached_tls_sessions_[1]), + received_transport_params_.get(), + received_application_state_.get()); + } + session_cache_->Insert(server_id_, std::move(cached_tls_sessions_[0]), + received_transport_params_.get(), received_application_state_.get()); } }
diff --git a/quic/core/tls_client_handshaker.h b/quic/core/tls_client_handshaker.h index 0128c8b..7acec4f 100644 --- a/quic/core/tls_client_handshaker.h +++ b/quic/core/tls_client_handshaker.h
@@ -34,7 +34,8 @@ QuicSession* session, std::unique_ptr<ProofVerifyContext> verify_context, QuicCryptoClientConfig* crypto_config, - QuicCryptoClientStream::ProofHandler* proof_handler); + QuicCryptoClientStream::ProofHandler* proof_handler, + bool has_application_state); TlsClientHandshaker(const TlsClientHandshaker&) = delete; TlsClientHandshaker& operator=(const TlsClientHandshaker&) = delete; @@ -165,8 +166,14 @@ bool allow_empty_alpn_for_tests_ = false; + const bool has_application_state_; + TlsClientConnection tls_connection_; + // If |has_application_state_|, stores the tls session tickets before + // application state is received. The latest one is put in the front. + bssl::UniquePtr<SSL_SESSION> cached_tls_sessions_[2] = {}; + std::unique_ptr<TransportParameters> received_transport_params_ = nullptr; std::unique_ptr<ApplicationState> received_application_state_ = nullptr; };
diff --git a/quic/core/tls_handshaker_test.cc b/quic/core/tls_handshaker_test.cc index 6103743..f774e28 100644 --- a/quic/core/tls_handshaker_test.cc +++ b/quic/core/tls_handshaker_test.cc
@@ -250,7 +250,8 @@ session, crypto_test_utils::ProofVerifyContextForTesting(), &crypto_config_, - &proof_handler_)) {} + &proof_handler_, + /*has_application_state = */ false)) {} ~TestQuicCryptoClientStream() override = default;
diff --git a/quic/qbone/qbone_client_session.cc b/quic/qbone/qbone_client_session.cc index 13f8b5f..52ba763 100644 --- a/quic/qbone/qbone_client_session.cc +++ b/quic/qbone/qbone_client_session.cc
@@ -30,7 +30,8 @@ std::unique_ptr<QuicCryptoStream> QboneClientSession::CreateCryptoStream() { return std::make_unique<QuicCryptoClientStream>( - server_id_, this, nullptr, quic_crypto_client_config_, this); + server_id_, this, nullptr, quic_crypto_client_config_, this, + /*has_application_state = */ true); } void QboneClientSession::Initialize() {
diff --git a/quic/quartc/quartc_session.cc b/quic/quartc/quartc_session.cc index eab7512..0a7fca9 100644 --- a/quic/quartc/quartc_session.cc +++ b/quic/quartc/quartc_session.cc
@@ -418,7 +418,7 @@ crypto_stream_ = std::make_unique<QuicCryptoClientStream>( server_id, this, client_crypto_config_->proof_verifier()->CreateDefaultContext(), - client_crypto_config_.get(), this); + client_crypto_config_.get(), this, /*has_application_state = */ true); Initialize(); crypto_stream_->CryptoConnect(); }
diff --git a/quic/quic_transport/quic_transport_client_session.cc b/quic/quic_transport/quic_transport_client_session.cc index a35d084..3a8eba1 100644 --- a/quic/quic_transport/quic_transport_client_session.cc +++ b/quic/quic_transport/quic_transport_client_session.cc
@@ -66,7 +66,7 @@ crypto_stream_ = std::make_unique<QuicCryptoClientStream>( QuicServerId(url.host(), url.EffectiveIntPort()), this, crypto_config->proof_verifier()->CreateDefaultContext(), crypto_config, - proof_handler); + proof_handler, /*has_application_state = */ true); } void QuicTransportClientSession::OnAlpnSelected(
diff --git a/quic/test_tools/quic_test_utils.cc b/quic/test_tools/quic_test_utils.cc index bb45ae3..9fab452 100644 --- a/quic/test_tools/quic_test_utils.cc +++ b/quic/test_tools/quic_test_utils.cc
@@ -746,9 +746,11 @@ &push_promise_index_, config, supported_versions) { + // TODO(b/153726130): Consider adding OnApplicationState calls in tests and + // set |has_application_state| to true. crypto_stream_ = std::make_unique<QuicCryptoClientStream>( server_id, this, crypto_test_utils::ProofVerifyContextForTesting(), - crypto_config, this); + crypto_config, this, /*has_application_state = */ false); Initialize(); }