tls: Remove additional tracking on SSL_ERROR_SYSCALL

Backport of #29263

Fixes #28415

This reverts part of #24923

Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>

envoy/ssl/handshaker.h

@@ -31,7 +31,7 @@ class HandshakeCallbacks {
   /**
    * A callback which will be executed at most once upon handshake failure.
    */
-  virtual void onFailure(bool syscall_error_occurred = false) PURE;
+  virtual void onFailure() PURE;
 
   /**
    * Returns a pointer to the transportSocketCallbacks struct, or nullptr if

source/extensions/transport_sockets/tls/ssl_handshaker.cc

@@ -99,12 +99,6 @@ Network::PostIoAction SslHandshakerImpl::doHandshake() {
     case SSL_ERROR_WANT_CERTIFICATE_VERIFY:
       state_ = Ssl::SocketState::HandshakeInProgress;
       return PostIoAction::KeepOpen;
-    case SSL_ERROR_SYSCALL:
-      // By default, when SSL_ERROR_SYSCALL occurred, the underlying transport does not participate
-      // in the error queue. Therefore, setting `syscall_error_occurred` to true to report the error
-      // in `drainErrorQueue`.
-      handshake_callbacks_->onFailure(/*syscall_error_occurred=*/true);
-      return PostIoAction::Close;
     default:
       handshake_callbacks_->onFailure();
       return PostIoAction::Close;

source/extensions/transport_sockets/tls/ssl_socket.cc

@@ -197,11 +197,11 @@ void SslSocket::onSuccess(SSL* ssl) {
   callbacks_->raiseEvent(Network::ConnectionEvent::Connected);
 }
 
-void SslSocket::onFailure(bool syscall_error_occurred) { drainErrorQueue(syscall_error_occurred); }
+void SslSocket::onFailure() { drainErrorQueue(); }
 
 PostIoAction SslSocket::doHandshake() { return info_->doHandshake(); }
 
-void SslSocket::drainErrorQueue(bool syscall_error_occurred) {
+void SslSocket::drainErrorQueue() {
   bool saw_error = false;
   bool saw_counted_error = false;
   while (uint64_t err = ERR_get_error()) {
@@ -229,18 +229,6 @@ void SslSocket::drainErrorQueue(bool syscall_error_occurred) {
                                         absl::NullSafeStringView(ERR_reason_error_string(err))));
   }
 
-  if (syscall_error_occurred) {
-    if (failure_reason_.empty()) {
-      failure_reason_ = "TLS error:";
-    }
-    failure_reason_.append(
-        "SSL_ERROR_SYSCALL error has occured, which indicates the operation failed externally to "
-        "the library. This is typically |errno| but may be something custom if using a custom "
-        "|BIO|. It may also be signaled if the transport returned EOF, in which case the "
-        "operation's return value will be zero.");
-    saw_error = true;
-  }
-
   if (!failure_reason_.empty()) {
     ENVOY_CONN_LOG(debug, "remote address:{},{}", callbacks_->connection(),
                    callbacks_->connection().connectionInfoProvider().remoteAddress()->asString(),

source/extensions/transport_sockets/tls/ssl_socket.h

@@ -69,7 +69,7 @@ class SslSocket : public Network::TransportSocket,
   // Ssl::HandshakeCallbacks
   Network::Connection& connection() const override;
   void onSuccess(SSL* ssl) override;
-  void onFailure(bool syscall_error_occurred = false) override;
+  void onFailure() override;
   Network::TransportSocketCallbacks* transportSocketCallbacks() override { return callbacks_; }
   void onAsynchronousCertValidationComplete() override;
 
@@ -86,7 +86,7 @@ class SslSocket : public Network::TransportSocket,
   ReadResult sslReadIntoSlice(Buffer::RawSlice& slice);
 
   Network::PostIoAction doHandshake();
-  void drainErrorQueue(bool syscall_error_occurred = false);
+  void drainErrorQueue();
   void shutdownSsl();
   void shutdownBasic();
   void resumeHandshake();

test/extensions/transport_sockets/tls/handshaker_test.cc

@@ -43,7 +43,7 @@ class MockHandshakeCallbacks : public Ssl::HandshakeCallbacks {
   ~MockHandshakeCallbacks() override = default;
   MOCK_METHOD(Network::Connection&, connection, (), (const, override));
   MOCK_METHOD(void, onSuccess, (SSL*), (override));
-  MOCK_METHOD(void, onFailure, (bool syscall_error_occurred), (override));
+  MOCK_METHOD(void, onFailure, (), (override));
   MOCK_METHOD(Network::TransportSocketCallbacks*, transportSocketCallbacks, (), (override));
   MOCK_METHOD(void, onAsynchronousCertValidationComplete, (), (override));
 };

test/extensions/transport_sockets/tls/ssl_socket_test.cc

@@ -573,8 +573,10 @@ class TestUtilOptionsV2 : public TestUtilOptionsBase {
   TestUtilOptionsV2(
       const envoy::config::listener::v3::Listener& listener,
       const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& client_ctx_proto,
-      bool expect_success, Network::Address::IpVersion version)
-      : TestUtilOptionsBase(expect_success, version), listener_(listener),
+      bool expect_success, Network::Address::IpVersion version,
+      bool skip_server_failure_reason_check = false)
+      : TestUtilOptionsBase(expect_success, version),
+        skip_server_failure_reason_check_(skip_server_failure_reason_check), listener_(listener),
         client_ctx_proto_(client_ctx_proto), transport_socket_options_(nullptr) {
     if (expect_success) {
       setExpectedServerStats("ssl.handshake").setExpectedClientStats("ssl.handshake");
@@ -584,6 +586,7 @@ class TestUtilOptionsV2 : public TestUtilOptionsBase {
     }
   }
 
+  bool skipServerFailureReasonCheck() const { return skip_server_failure_reason_check_; }
   const envoy::config::listener::v3::Listener& listener() const { return listener_; }
   const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& clientCtxProto() const {
     return client_ctx_proto_;
@@ -675,6 +678,7 @@ class TestUtilOptionsV2 : public TestUtilOptionsBase {
   }
 
 private:
+  bool skip_server_failure_reason_check_;
   const envoy::config::listener::v3::Listener& listener_;
   const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& client_ctx_proto_;
   std::string expected_client_stats_;
@@ -879,7 +883,9 @@ void testUtilV2(const TestUtilOptionsV2& options) {
   } else {
     EXPECT_THAT(std::string(client_connection->transportFailureReason()),
                 ContainsRegex(options.expectedTransportFailureReasonContains()));
-    EXPECT_NE("", server_connection->transportFailureReason());
+    if (!options.skipServerFailureReasonCheck()) {
+      EXPECT_NE("", server_connection->transportFailureReason());
+    }
   }
 }
 
@@ -7187,11 +7193,12 @@ TEST_P(SslSocketTest, RsaKeyUsageVerificationEnforcementOn) {
 
   // Enable the rsa_key_usage enforcement.
   client_tls_context.mutable_enforce_rsa_key_usage()->set_value(true);
-  TestUtilOptionsV2 test_options(listener, client_tls_context, /*expect_success=*/false, version_);
-  // Client connection is failed with key_usage_mismatch, which is expected.
+  TestUtilOptionsV2 test_options(listener, client_tls_context, /*expect_success=*/false, version_,
+                                 /*skip_server_failure_reason_check=*/true);
+  // Client connection is failed with key_usage_mismatch.
   test_options.setExpectedTransportFailureReasonContains("KEY_USAGE_BIT_INCORRECT");
-  // Server connection failed with connection error.
-  test_options.setExpectedServerStats("ssl.connection_error");
+  // Server connection error was not populated in this case.
+  test_options.setExpectedServerStats("");
   testUtilV2(test_options);
 }