Minor tweaks to TlsServerHandshaker: - Change TlsServerHandshaker::SelectCertStatus() to TlsServerHandshaker::select_cert_status(). - Set expected_ssl_error to SSL_ERROR_WANT_READ in OnSelectCertificateDone(). - Add a test for async select cert and async signature. Protected by FLAGS_quic_tls_use_per_handshaker_proof_source. PiperOrigin-RevId: 351864828 Change-Id: I650c786a0a74bba0df1063be525a028485f5d0dc
diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc index 8fc52f1..2b14657 100644 --- a/quic/core/tls_server_handshaker.cc +++ b/quic/core/tls_server_handshaker.cc
@@ -62,7 +62,13 @@ handshaker_->OnSelectCertificateDone( /*ok=*/true, /*is_sync=*/true, chain.get()); - return handshaker_->SelectCertStatus(); + if (!handshaker_->select_cert_status().has_value()) { + QUIC_BUG + << "select_cert_status() has no value after a synchronous select cert"; + // Return success to continue the handshake. + return QUIC_SUCCESS; + } + return handshaker_->select_cert_status().value(); } QuicAsyncStatus TlsServerHandshaker::DefaultProofSourceHandle::ComputeSignature( @@ -567,6 +573,7 @@ size_t max_out, uint16_t sig_alg, absl::string_view in) { + DCHECK_EQ(expected_ssl_error(), SSL_ERROR_WANT_READ); if (use_proof_source_handle_) { QUIC_RELOADABLE_FLAG_COUNT_N(quic_tls_use_per_handshaker_proof_source, 2, 3); @@ -774,7 +781,7 @@ set_transport_params_result.quic_transport_params, set_transport_params_result.early_data_context); - DCHECK_EQ(status, SelectCertStatus()); + DCHECK_EQ(status, select_cert_status().value()); if (status == QUIC_PENDING) { set_expected_ssl_error(SSL_ERROR_PENDING_CERTIFICATE); @@ -835,21 +842,14 @@ QUIC_LOG(ERROR) << "No certs provided for host '" << hostname_ << "'"; } } - + const int last_expected_ssl_error = expected_ssl_error(); + set_expected_ssl_error(SSL_ERROR_WANT_READ); if (!is_sync) { + DCHECK_EQ(last_expected_ssl_error, SSL_ERROR_PENDING_CERTIFICATE); AdvanceHandshakeFromCallback(); } } -QuicAsyncStatus TlsServerHandshaker::SelectCertStatus() const { - if (!select_cert_status_.has_value()) { - QUIC_BUG << "SelectCertStatus should be called after select cert started"; - return QUIC_PENDING; - } - - return select_cert_status_.value(); -} - bool TlsServerHandshaker::ValidateHostname(const std::string& hostname) const { if (!QuicHostnameUtils::IsValidSNI(hostname)) { // TODO(b/151676147): Include this error string in the CONNECTION_CLOSE
diff --git a/quic/core/tls_server_handshaker.h b/quic/core/tls_server_handshaker.h index 9b1cdae..2a09b50 100644 --- a/quic/core/tls_server_handshaker.h +++ b/quic/core/tls_server_handshaker.h
@@ -144,8 +144,10 @@ absl::string_view in) override; TlsConnection::Delegate* ConnectionDelegate() override { return this; } - // The status of cert selection. Only called after cert selection started. - QuicAsyncStatus SelectCertStatus() const; + // The status of cert selection. nullopt means it hasn't started. + const absl::optional<QuicAsyncStatus>& select_cert_status() const { + return select_cert_status_; + } // Whether |cert_verify_sig_| contains a valid signature. // NOTE: BoringSSL queries the result of a async signature operation using // PrivateKeyComplete(), a successful PrivateKeyComplete() will clear the
diff --git a/quic/core/tls_server_handshaker_test.cc b/quic/core/tls_server_handshaker_test.cc index e650a4a..c0b1337 100644 --- a/quic/core/tls_server_handshaker_test.cc +++ b/quic/core/tls_server_handshaker_test.cc
@@ -101,6 +101,8 @@ return fake_proof_source_handle_; } + using TlsServerHandshaker::expected_ssl_error; + private: std::unique_ptr<ProofSourceHandle> RealMaybeCreateProofSourceHandle() { return TlsServerHandshaker::MaybeCreateProofSourceHandle(); @@ -424,6 +426,47 @@ EXPECT_EQ(moved_messages_counts_.second, 0u); } +TEST_P(TlsServerHandshakerTest, HandshakeWithAsyncSelectCertAndSignature) { + if (!(GetQuicReloadableFlag(quic_tls_use_early_select_cert) && + GetQuicReloadableFlag(quic_tls_use_per_handshaker_proof_source))) { + return; + } + + InitializeServerWithFakeProofSourceHandle(); + server_handshaker_->SetupProofSourceHandle( + /*select_cert_action=*/FakeProofSourceHandle::Action::DELEGATE_ASYNC, + /*compute_signature_action=*/FakeProofSourceHandle::Action:: + DELEGATE_ASYNC); + + EXPECT_CALL(*client_connection_, CloseConnection(_, _, _)).Times(0); + EXPECT_CALL(*server_connection_, CloseConnection(_, _, _)).Times(0); + + // Start handshake. + AdvanceHandshakeWithFakeClient(); + + // A select cert operation is now pending. + ASSERT_TRUE( + server_handshaker_->fake_proof_source_handle()->HasPendingOperation()); + EXPECT_EQ(server_handshaker_->expected_ssl_error(), + SSL_ERROR_PENDING_CERTIFICATE); + + // Complete the pending select cert. It should advance the handshake to + // compute a signature, which will also be saved as a pending operation. + server_handshaker_->fake_proof_source_handle()->CompletePendingOperation(); + + // A compute signature operation is now pending. + ASSERT_TRUE( + server_handshaker_->fake_proof_source_handle()->HasPendingOperation()); + EXPECT_EQ(server_handshaker_->expected_ssl_error(), + SSL_ERROR_WANT_PRIVATE_KEY_OPERATION); + + server_handshaker_->fake_proof_source_handle()->CompletePendingOperation(); + + CompleteCryptoHandshake(); + + ExpectHandshakeSuccessful(); +} + TEST_P(TlsServerHandshakerTest, HandshakeWithAsyncSignature) { EXPECT_CALL(*client_connection_, CloseConnection(_, _, _)).Times(0); EXPECT_CALL(*server_connection_, CloseConnection(_, _, _)).Times(0);
diff --git a/quic/test_tools/fake_proof_source_handle.cc b/quic/test_tools/fake_proof_source_handle.cc index daa47b5..437a913 100644 --- a/quic/test_tools/fake_proof_source_handle.cc +++ b/quic/test_tools/fake_proof_source_handle.cc
@@ -139,8 +139,10 @@ if (select_cert_op_.has_value()) { select_cert_op_->Run(); + select_cert_op_.reset(); } else if (compute_signature_op_.has_value()) { compute_signature_op_->Run(); + compute_signature_op_.reset(); } }