In TlsHandshaker::AdvanceHandshake, retry SSL_do_handshake once if it "succeeded" when entering early data. Protected by FLAGS_quic_reloadable_flag_quic_tls_retry_handshake_on_early_data. PiperOrigin-RevId: 371358037 Change-Id: I8bc9d39d4f455e5d01bea392c27e9423bf552a1f
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 536f139..4b3fcc8 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -63,6 +63,7 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_start_peer_migration_earlier, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_testonly_default_false, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_testonly_default_true, true) +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_retry_handshake_on_early_data, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_server_use_handshake_hints, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_unified_iw_options, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_connection_id_on_default_path_v2, false)
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc index eb1859d..f2b8931 100644 --- a/quic/core/tls_client_handshaker.cc +++ b/quic/core/tls_client_handshaker.cc
@@ -444,26 +444,23 @@ } void TlsClientHandshaker::FinishHandshake() { - // Fill crypto_negotiated_params_: - const SSL_CIPHER* cipher = SSL_get_current_cipher(ssl()); - if (cipher) { - crypto_negotiated_params_->cipher_suite = - SSL_CIPHER_get_protocol_id(cipher); - } - crypto_negotiated_params_->key_exchange_group = SSL_get_curve_id(ssl()); - crypto_negotiated_params_->peer_signature_algorithm = - SSL_get_peer_signature_algorithm(ssl()); - if (SSL_in_early_data(ssl())) { - // SSL_do_handshake returns after sending the ClientHello if the session is - // 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. + FillNegotiatedParams(); - // 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; + if (retry_handshake_on_early_data_) { + QUICHE_CHECK(!SSL_in_early_data(ssl())); + } else { + if (SSL_in_early_data(ssl())) { + // SSL_do_handshake returns after sending the ClientHello if the session + // is 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. + + // 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"; @@ -522,6 +519,30 @@ handshaker_delegate()->OnTlsHandshakeComplete(); } +void TlsClientHandshaker::OnEnterEarlyData() { + QUICHE_DCHECK(retry_handshake_on_early_data_); + QUICHE_DCHECK(SSL_in_early_data(ssl())); + + // TODO(wub): It might be unnecessary to FillNegotiatedParams() at this time, + // because we fill it again when handshake completes. + FillNegotiatedParams(); + + // 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()); +} + +void TlsClientHandshaker::FillNegotiatedParams() { + const SSL_CIPHER* cipher = SSL_get_current_cipher(ssl()); + if (cipher) { + crypto_negotiated_params_->cipher_suite = + SSL_CIPHER_get_protocol_id(cipher); + } + crypto_negotiated_params_->key_exchange_group = SSL_get_curve_id(ssl()); + crypto_negotiated_params_->peer_signature_algorithm = + SSL_get_peer_signature_algorithm(ssl()); +} + void TlsClientHandshaker::ProcessPostHandshakeMessage() { int rv = SSL_process_quic_post_handshake(ssl()); if (rv != 1) {
diff --git a/quic/core/tls_client_handshaker.h b/quic/core/tls_client_handshaker.h index 24f6b26..eb39ccf 100644 --- a/quic/core/tls_client_handshaker.h +++ b/quic/core/tls_client_handshaker.h
@@ -90,6 +90,8 @@ } void FinishHandshake() override; + void OnEnterEarlyData() override; + void FillNegotiatedParams(); void ProcessPostHandshakeMessage() override; bool ShouldCloseConnectionOnUnexpectedError(int ssl_error) override; QuicAsyncStatus VerifyCertChain(
diff --git a/quic/core/tls_client_handshaker_test.cc b/quic/core/tls_client_handshaker_test.cc index 807f49e..e498acb 100644 --- a/quic/core/tls_client_handshaker_test.cc +++ b/quic/core/tls_client_handshaker_test.cc
@@ -423,6 +423,62 @@ EXPECT_EQ(stream()->EarlyDataReason(), ssl_early_data_accepted); } +// Regression test for b/186438140. +TEST_P(TlsClientHandshakerTest, ZeroRttResumptionWithAyncProofVerifier) { + // Finish establishing the first connection, so the second connection can + // resume. + 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(); + InitializeFakeServer(); + EXPECT_CALL(*session_, OnConfigNegotiated()); + EXPECT_CALL(*connection_, SendCryptoData(_, _, _)) + .Times(testing::AnyNumber()); + // Enable TestProofVerifier to capture the call to VerifyCertChain and run it + // asynchronously. + TestProofVerifier* proof_verifier = + static_cast<TestProofVerifier*>(crypto_config_->proof_verifier()); + proof_verifier->Activate(); + // Start the second handshake. + stream()->CryptoConnect(); + + ASSERT_EQ(proof_verifier->NumPendingCallbacks(), 1u); + + // Advance the handshake with the server. Since cert verification has not + // finished yet, client cannot derive HANDSHAKE and 1-RTT keys. + crypto_test_utils::AdvanceHandshake(connection_, stream(), 0, + server_connection_, server_stream(), 0); + + EXPECT_FALSE(stream()->one_rtt_keys_available()); + EXPECT_FALSE(server_stream()->one_rtt_keys_available()); + + // Finish cert verification after receiving packets from server. + proof_verifier->InvokePendingCallback(0); + + QuicFramer* framer = QuicConnectionPeer::GetFramer(connection_); + if (!GetQuicReloadableFlag(quic_tls_retry_handshake_on_early_data)) { + // Client does not have HANDSHAKE key due to b/186438140. + EXPECT_EQ(nullptr, + QuicFramerPeer::GetEncrypter(framer, ENCRYPTION_HANDSHAKE)); + return; + } + + // Verify client has derived HANDSHAKE key. + EXPECT_NE(nullptr, + QuicFramerPeer::GetEncrypter(framer, ENCRYPTION_HANDSHAKE)); + + // Ideally, we should also verify that the process_undecryptable_packets_alarm + // is set and processing the undecryptable packets can advance the handshake + // to completion. Unfortunately, the test facilities used in this test does + // not support queuing and processing undecryptable packets. +} + TEST_P(TlsClientHandshakerTest, ZeroRttRejection) { // Finish establishing the first connection: CompleteCryptoHandshake();
diff --git a/quic/core/tls_handshaker.cc b/quic/core/tls_handshaker.cc index cbc1998..9a3e668 100644 --- a/quic/core/tls_handshaker.cc +++ b/quic/core/tls_handshaker.cc
@@ -12,9 +12,12 @@ #include "quic/core/quic_crypto_stream.h" #include "quic/core/tls_client_handshaker.h" #include "quic/platform/api/quic_bug_tracker.h" +#include "quic/platform/api/quic_stack_trace.h" namespace quic { +#define ENDPOINT (SSL_is_server(ssl()) ? "TlsServer: " : "TlsClient: ") + TlsHandshaker::ProofVerifierCallbackImpl::ProofVerifierCallbackImpl( TlsHandshaker* parent) : parent_(parent) {} @@ -100,8 +103,38 @@ << ", add_packet_flusher_on_async_op_done_:" << add_packet_flusher_on_async_op_done_; - QUIC_VLOG(1) << "TlsHandshaker: continuing handshake"; + QUIC_VLOG(1) << ENDPOINT << "Continuing handshake"; int rv = SSL_do_handshake(ssl()); + + // If SSL_do_handshake return success(1) and we are in early data, it is + // possible that we have provided ServerHello to BoringSSL but it hasn't been + // processed. Retry SSL_do_handshake once will advance the handshake more in + // that case. If there are no unprocessed ServerHello, the retry will return a + // non-positive number. + if (retry_handshake_on_early_data_ && rv == 1 && SSL_in_early_data(ssl())) { + OnEnterEarlyData(); + rv = SSL_do_handshake(ssl()); + QUIC_VLOG(1) << ENDPOINT + << "SSL_do_handshake returned when entering early data. After " + << "retry, rv=" << rv + << ", SSL_in_early_data=" << SSL_in_early_data(ssl()); + // The retry should either + // - Return <= 0 if the handshake is still pending, likely still in early + // data. + // - Return 1 if the handshake has _actually_ finished. i.e. + // SSL_in_early_data should be false. + // + // In either case, it should not both return 1 and stay in early data. + if (rv == 1 && SSL_in_early_data(ssl()) && !is_connection_closed_) { + QUIC_BUG(quic_handshaker_stay_in_early_data) + << "The original and the retry of SSL_do_handshake both returned " + "success and in early data"; + CloseConnection(QUIC_HANDSHAKE_FAILED, + "TLS handshake failed: Still in early data after retry"); + return; + } + } + if (rv == 1) { FinishHandshake(); return; @@ -207,7 +240,7 @@ void TlsHandshaker::SetWriteSecret(EncryptionLevel level, const SSL_CIPHER* cipher, const std::vector<uint8_t>& write_secret) { - QUIC_DVLOG(1) << "SetWriteSecret level=" << level; + QUIC_DVLOG(1) << ENDPOINT << "SetWriteSecret level=" << level; std::unique_ptr<QuicEncrypter> encrypter = QuicEncrypter::CreateFromCipherSuite(SSL_CIPHER_get_id(cipher)); const EVP_MD* prf = Prf(cipher); @@ -230,7 +263,7 @@ bool TlsHandshaker::SetReadSecret(EncryptionLevel level, const SSL_CIPHER* cipher, const std::vector<uint8_t>& read_secret) { - QUIC_DVLOG(1) << "SetReadSecret level=" << level; + QUIC_DVLOG(1) << ENDPOINT << "SetReadSecret level=" << level; std::unique_ptr<QuicDecrypter> decrypter = QuicDecrypter::CreateFromCipherSuite(SSL_CIPHER_get_id(cipher)); const EVP_MD* prf = Prf(cipher);
diff --git a/quic/core/tls_handshaker.h b/quic/core/tls_handshaker.h index 0b9c7f6..0b5ddc3 100644 --- a/quic/core/tls_handshaker.h +++ b/quic/core/tls_handshaker.h
@@ -74,8 +74,20 @@ // finished. Note that due to 0-RTT, the handshake may "finish" twice; // |SSL_in_early_data| can be used to determine whether the handshake is truly // done. + // TODO(wub): When --quic_tls_retry_handshake_on_early_data is true, this + // function will only be called once when the handshake actually finishes. + // Update comment when deprecating the flag. virtual void FinishHandshake() = 0; + // Called when |SSL_do_handshake| returns 1 and the connection is in early + // data. In that case, |AdvanceHandshake| will call |OnEnterEarlyData| and + // retry |SSL_do_handshake| once. + virtual void OnEnterEarlyData() { + // By default, do nothing but check the preconditions. + QUICHE_DCHECK(retry_handshake_on_early_data_); + QUICHE_DCHECK(SSL_in_early_data(ssl())); + } + // Called when a handshake message is received after the handshake is // complete. virtual void ProcessPostHandshakeMessage() = 0; @@ -159,6 +171,9 @@ const bool add_packet_flusher_on_async_op_done_ = GetQuicReloadableFlag(quic_add_packet_flusher_on_async_op_done); + const bool retry_handshake_on_early_data_ = + GetQuicReloadableFlag(quic_tls_retry_handshake_on_early_data); + private: // ProofVerifierCallbackImpl handles the result of an asynchronous certificate // verification operation.
diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc index b2424a5..2c1f2a1 100644 --- a/quic/core/tls_server_handshaker.cc +++ b/quic/core/tls_server_handshaker.cc
@@ -519,15 +519,19 @@ } void TlsServerHandshaker::FinishHandshake() { - if (SSL_in_early_data(ssl())) { - // If the server accepts early data, SSL_do_handshake returns success twice: - // once after processing the ClientHello and sending the server's first - // flight, and then again after the handshake is complete. This results in - // FinishHandshake getting called twice. On the first call to - // FinishHandshake, we don't have any confirmation that the client is live, - // so all end of handshake processing is deferred until the handshake is - // actually complete. - return; + if (retry_handshake_on_early_data_) { + QUICHE_DCHECK(!SSL_in_early_data(ssl())); + } else { + if (SSL_in_early_data(ssl())) { + // If the server accepts early data, SSL_do_handshake returns success + // twice: once after processing the ClientHello and sending the server's + // first flight, and then again after the handshake is complete. This + // results in FinishHandshake getting called twice. On the first call to + // FinishHandshake, we don't have any confirmation that the client is + // live, so all end of handshake processing is deferred until the + // handshake is actually complete. + return; + } } if (!valid_alpn_received_) { QUIC_DLOG(ERROR)