Only set QUIC TLS 0-RTT client state if a 0-RTT handshake was attempted Even if an SSL_SESSION is early data capable, it is still possible (for various reasons) that BoringSSL will decide not to do a 0-RTT handshake. TlsClientHandshaker should wait for a signal from BoringSSL that it is attempting a 0-RTT handshake before it sets saved transport and application state for early data. Client-side only quic behavior change, not flag protected. PiperOrigin-RevId: 320646436 Change-Id: Ib1cfe2640d3cd62e23344ede852b91756a44f687
diff --git a/quic/core/crypto/quic_crypto_client_config.cc b/quic/core/crypto/quic_crypto_client_config.cc index 9314e99..4680785 100644 --- a/quic/core/crypto/quic_crypto_client_config.cc +++ b/quic/core/crypto/quic_crypto_client_config.cc
@@ -67,9 +67,8 @@ std::unique_ptr<SessionCache> session_cache) : proof_verifier_(std::move(proof_verifier)), session_cache_(std::move(session_cache)), - enable_zero_rtt_for_tls_( - GetQuicReloadableFlag(quic_enable_zero_rtt_for_tls)), - ssl_ctx_(TlsClientConnection::CreateSslCtx(enable_zero_rtt_for_tls_)), + ssl_ctx_(TlsClientConnection::CreateSslCtx( + GetQuicReloadableFlag(quic_enable_zero_rtt_for_tls))), disable_chlo_padding_(GetQuicReloadableFlag(quic_dont_pad_chlo)) { DCHECK(proof_verifier_.get()); SetDefaults();
diff --git a/quic/core/crypto/quic_crypto_client_config.h b/quic/core/crypto/quic_crypto_client_config.h index ce0efe5..e355c5e 100644 --- a/quic/core/crypto/quic_crypto_client_config.h +++ b/quic/core/crypto/quic_crypto_client_config.h
@@ -354,8 +354,6 @@ void set_proof_source(std::unique_ptr<ProofSource> proof_source); SSL_CTX* ssl_ctx() const; - bool early_data_enabled_for_tls() const { return enable_zero_rtt_for_tls_; } - // Initialize the CachedState from |canonical_crypto_config| for the // |canonical_server_id| as the initial CachedState for |server_id|. We will // copy config data only if |canonical_crypto_config| has valid proof. @@ -443,8 +441,6 @@ std::unique_ptr<SessionCache> session_cache_; std::unique_ptr<ProofSource> proof_source_; - // Latched value of reloadable flag quic_enable_zero_rtt_for_tls - bool enable_zero_rtt_for_tls_; bssl::UniquePtr<SSL_CTX> ssl_ctx_; // The |user_agent_id_| passed in QUIC's CHLO message.
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc index 7d000d9..76f9b82 100644 --- a/quic/core/tls_client_handshaker.cc +++ b/quic/core/tls_client_handshaker.cc
@@ -68,7 +68,6 @@ pre_shared_key_(crypto_config->pre_shared_key()), crypto_negotiated_params_(new QuicCryptoNegotiatedParameters), has_application_state_(has_application_state), - attempting_zero_rtt_(crypto_config->early_data_enabled_for_tls()), tls_connection_(crypto_config->ssl_ctx(), this) {} TlsClientHandshaker::~TlsClientHandshaker() { @@ -116,18 +115,11 @@ } // Set a session to resume, if there is one. - std::unique_ptr<QuicResumptionState> cached_state; if (session_cache_) { - cached_state = session_cache_->Lookup(server_id_, SSL_get_SSL_CTX(ssl())); + cached_state_ = session_cache_->Lookup(server_id_, SSL_get_SSL_CTX(ssl())); } - if (cached_state) { - SSL_set_session(ssl(), cached_state->tls_session.get()); - if (attempting_zero_rtt_ && - SSL_SESSION_early_data_capable(cached_state->tls_session.get())) { - if (!PrepareZeroRttConfig(cached_state.get())) { - return false; - } - } + if (cached_state_) { + SSL_set_session(ssl(), cached_state_->tls_session.get()); } // Start the handshake. @@ -467,8 +459,11 @@ // 0-RTT-capable, which means that FinishHandshake will get called twice - // the first time after sending the ClientHello, and the second time after // the handshake is complete. If we're in the first time FinishHandshake is - // called, we can't do any end-of-handshake processing, so we return early - // from this function. + // called, we can't do any end-of-handshake processing. + + // If we're attempting a 0-RTT handshake, then we need to let the transport + // and application know what state to apply to early data. + PrepareZeroRttConfig(cached_state_.get()); return; } QUIC_LOG(INFO) << "Client: handshake finished";
diff --git a/quic/core/tls_client_handshaker.h b/quic/core/tls_client_handshaker.h index 573c055..bf05ca8 100644 --- a/quic/core/tls_client_handshaker.h +++ b/quic/core/tls_client_handshaker.h
@@ -176,7 +176,9 @@ bool allow_invalid_sni_for_tests_ = false; const bool has_application_state_; - bool attempting_zero_rtt_; + // Contains the state for performing a resumption, if one is attempted. This + // will always be non-null if a 0-RTT resumption is attempted. + std::unique_ptr<QuicResumptionState> cached_state_; TlsClientConnection tls_connection_;
diff --git a/quic/core/tls_client_handshaker_test.cc b/quic/core/tls_client_handshaker_test.cc index f586028..1832d90 100644 --- a/quic/core/tls_client_handshaker_test.cc +++ b/quic/core/tls_client_handshaker_test.cc
@@ -202,13 +202,18 @@ } void CompleteCryptoHandshake() { + CompleteCryptoHandshakeWithServerALPN( + AlpnForVersion(connection_->version())); + } + + void CompleteCryptoHandshakeWithServerALPN(const std::string& alpn) { EXPECT_CALL(*connection_, SendCryptoData(_, _, _)) .Times(testing::AnyNumber()); stream()->CryptoConnect(); QuicConfig config; crypto_test_utils::HandshakeWithFakeServer( &config, server_crypto_config_.get(), &server_helper_, &alarm_factory_, - connection_, stream(), AlpnForVersion(connection_->version())); + connection_, stream(), alpn); } QuicCryptoClientStream* stream() { @@ -358,6 +363,10 @@ // Create a second connection CreateConnection(); + // OnConfigNegotiated should be called twice - once when processing saved + // 0-RTT transport parameters, and then again when receiving transport + // parameters from the server. + EXPECT_CALL(*session_, OnConfigNegotiated()).Times(2); CompleteCryptoHandshake(); // TODO(b/152551499): Add a test that checks we have keys after calling @@ -383,6 +392,11 @@ SSL_CTX_set_early_data_enabled(server_crypto_config_->ssl_ctx(), false); CreateConnection(); + // OnConfigNegotiated should be called twice - once when processing saved + // 0-RTT transport parameters, and then again when receiving transport + // parameters from the server. + EXPECT_CALL(*session_, OnConfigNegotiated()).Times(2); + // 4 packets will be sent in this connection: initial handshake packet, 0-RTT // packet containing SETTINGS, handshake packet upon 0-RTT rejection, 0-RTT // packet retransmission. @@ -470,6 +484,32 @@ EXPECT_TRUE(server_stream()->encryption_established()); } +TEST_P(TlsClientHandshakerTest, ZeroRTTNotAttemptedOnALPNChange) { + // Finish establishing the first connection: + CompleteCryptoHandshake(); + + EXPECT_EQ(PROTOCOL_TLS1_3, stream()->handshake_protocol()); + EXPECT_TRUE(stream()->encryption_established()); + EXPECT_TRUE(stream()->one_rtt_keys_available()); + EXPECT_FALSE(stream()->IsResumption()); + + // Create a second connection + CreateConnection(); + // Override the ALPN to send on the second connection. + const std::string kTestAlpn = "Test ALPN"; + EXPECT_CALL(*session_, GetAlpnsToOffer()) + .WillRepeatedly(testing::Return(std::vector<std::string>({kTestAlpn}))); + // OnConfigNegotiated should only be called once: when transport parameters + // are received from the server. + EXPECT_CALL(*session_, OnConfigNegotiated()).Times(1); + + CompleteCryptoHandshakeWithServerALPN(kTestAlpn); + EXPECT_EQ(PROTOCOL_TLS1_3, stream()->handshake_protocol()); + EXPECT_TRUE(stream()->encryption_established()); + EXPECT_TRUE(stream()->one_rtt_keys_available()); + EXPECT_FALSE(stream()->EarlyDataAccepted()); +} + TEST_P(TlsClientHandshakerTest, InvalidSNI) { // Test that a client will skip sending SNI if configured to send an invalid // hostname. In this case, the inclusion of '!' is invalid.
diff --git a/quic/test_tools/quic_test_utils.cc b/quic/test_tools/quic_test_utils.cc index 32a8450..7cf2dab 100644 --- a/quic/test_tools/quic_test_utils.cc +++ b/quic/test_tools/quic_test_utils.cc
@@ -776,6 +776,9 @@ server_id, this, crypto_test_utils::ProofVerifyContextForTesting(), crypto_config, this, /*has_application_state = */ false); Initialize(); + ON_CALL(*this, OnConfigNegotiated()) + .WillByDefault( + Invoke(this, &TestQuicSpdyClientSession::RealOnConfigNegotiated)); } TestQuicSpdyClientSession::~TestQuicSpdyClientSession() {} @@ -793,6 +796,10 @@ return crypto_stream_.get(); } +void TestQuicSpdyClientSession::RealOnConfigNegotiated() { + QuicSpdyClientSessionBase::OnConfigNegotiated(); +} + TestPushPromiseDelegate::TestPushPromiseDelegate(bool match) : match_(match), rendezvous_fired_(false), rendezvous_stream_(nullptr) {}
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h index 7be7edf..870f99d 100644 --- a/quic/test_tools/quic_test_utils.h +++ b/quic/test_tools/quic_test_utils.h
@@ -1163,6 +1163,7 @@ MOCK_METHOD(bool, ShouldCreateOutgoingUnidirectionalStream, (), (override)); MOCK_METHOD(std::vector<std::string>, GetAlpnsToOffer, (), (const, override)); MOCK_METHOD(void, OnAlpnSelected, (quiche::QuicheStringPiece), (override)); + MOCK_METHOD(void, OnConfigNegotiated, (), (override)); QuicCryptoClientStream* GetMutableCryptoStream() override; const QuicCryptoClientStream* GetCryptoStream() const override; @@ -1179,6 +1180,10 @@ } private: + // Calls the parent class's OnConfigNegotiated method. Used to set the default + // mock behavior for OnConfigNegotiated. + void RealOnConfigNegotiated(); + std::unique_ptr<QuicCryptoClientStream> crypto_stream_; QuicClientPushPromiseIndex push_promise_index_; std::vector<CryptoHandshakeMessage> sent_crypto_handshake_messages_;