backport to 1.16: tls: fix detection of the upstream connection close…

… event. (#13858) (#14452) Fixes #13856. This change also contains the following backports: - build: Fix some unused variable warnings (#13987) - test: Check in all TLS test certs (#13702) Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
envoyproxy · Dec 29, 2020 · 15f02f0 · 15f02f0
1 parent db0ae3d
commit 15f02f0
Show file tree

Hide file tree

Showing 48 changed files with 921 additions and 340 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -14,6 +14,7 @@ Bug Fixes
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
 * lua: fixed crash when Lua script contains streamInfo():downstreamSslConnection().
+* tls: fix detection of the upstream connection close event.
 
 Removed Config or Runtime
 -------------------------

diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h
@@ -346,6 +346,7 @@ template <class Base> class FactoryRegistry : public Logger::Loggable<Logger::Id
     auto mapping = std::make_unique<absl::flat_hash_map<std::string, Base*>>();
 
     for (const auto& [factory_name, factory] : factories()) {
+      UNREFERENCED_PARAMETER(factory_name);
       if (factory == nullptr) {
         continue;
       }

diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc
@@ -90,6 +90,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse(
 
 void NewGrpcMuxImpl::onStreamEstablished() {
   for (auto& [type_url, subscription] : subscriptions_) {
+    UNREFERENCED_PARAMETER(type_url);
     subscription->sub_state_.markStreamFresh();
   }
   trySendDiscoveryRequests();

diff --git a/source/common/init/manager_impl.cc b/source/common/init/manager_impl.cc
@@ -71,6 +71,7 @@ void ManagerImpl::dumpUnreadyTargets(envoy::admin::v3::UnreadyTargetsDumps& unre
   auto& message = *unready_targets_dumps.mutable_unready_targets_dumps()->Add();
   message.set_name(name_);
   for (const auto& [target_name, count] : target_names_count_) {
+    UNREFERENCED_PARAMETER(count);
     message.add_target_names(target_name);
   }
 }

diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc
@@ -427,6 +427,7 @@ ScopedRdsConfigSubscription::detectUpdateConflictAndCleanupRemoved(
   absl::flat_hash_map<uint64_t, std::string> scope_name_by_hash = scope_name_by_hash_;
   absl::erase_if(scope_name_by_hash, [&updated_or_removed_scopes](const auto& key_name) {
     auto const& [key, name] = key_name;
+    UNREFERENCED_PARAMETER(key);
     return updated_or_removed_scopes.contains(name);
   });
   absl::flat_hash_map<std::string, envoy::config::route::v3::ScopedRouteConfiguration>

diff --git a/source/extensions/transport_sockets/tls/ssl_handshaker.h b/source/extensions/transport_sockets/tls/ssl_handshaker.h
@@ -67,7 +67,7 @@ class SslHandshakerImpl : public Ssl::ConnectionInfo, public Ssl::Handshaker {
   // Ssl::Handshaker
   Network::PostIoAction doHandshake() override;
 
-  Ssl::SocketState state() { return state_; }
+  Ssl::SocketState state() const { return state_; }
   void setState(Ssl::SocketState state) { state_ = state; }
   SSL* ssl() const { return ssl_.get(); }
   Ssl::HandshakeCallbacks* handshakeCallbacks() { return handshake_callbacks_; }

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc
@@ -140,10 +140,18 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) {
         case SSL_ERROR_WANT_READ:
           break;
         case SSL_ERROR_ZERO_RETURN:
+          // Graceful shutdown using close_notify TLS alert.
           end_stream = true;
           break;
+        case SSL_ERROR_SYSCALL:
+          if (result.error_.value() == 0) {
+            // Non-graceful shutdown by closing the underlying socket.
+            end_stream = true;
+            break;
+          }
+          FALLTHRU;
         case SSL_ERROR_WANT_WRITE:
-        // Renegotiation has started. We don't handle renegotiation so just fall through.
+          // Renegotiation has started. We don't handle renegotiation so just fall through.
         default:
           drainErrorQueue();
           action = PostIoAction::Close;

diff --git a/source/server/admin/config_dump_handler.cc b/source/server/admin/config_dump_handler.cc
@@ -155,6 +155,7 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump,
   }
 
   for (const auto& [name, callback] : callbacks_map) {
+    UNREFERENCED_PARAMETER(name);
     ProtobufTypes::MessagePtr message = callback();
     ASSERT(message);
 
@@ -200,6 +201,7 @@ void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
   }
 
   for (const auto& [name, callback] : callbacks_map) {
+    UNREFERENCED_PARAMETER(name);
     ProtobufTypes::MessagePtr message = callback();
     ASSERT(message);
 
@@ -220,6 +222,7 @@ ProtobufTypes::MessagePtr ConfigDumpHandler::dumpEndpointConfigs() const {
   auto endpoint_config_dump = std::make_unique<envoy::admin::v3::EndpointsConfigDump>();
 
   for (const auto& [name, cluster_ref] : server_.clusterManager().clusters()) {
+    UNREFERENCED_PARAMETER(name);
     const Upstream::Cluster& cluster = cluster_ref.get();
     Upstream::ClusterInfoConstSharedPtr cluster_info = cluster.info();
     envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;

diff --git a/source/server/filter_chain_manager_impl.cc b/source/server/filter_chain_manager_impl.cc
@@ -555,6 +555,7 @@ const Network::FilterChain* FilterChainManagerImpl::findFilterChainForSourceIpAn
 
 void FilterChainManagerImpl::convertIPsToTries() {
   for (auto& [destination_port, destination_ips_pair] : destination_ports_map_) {
+    UNREFERENCED_PARAMETER(destination_port);
     // These variables are used as we build up the destination CIDRs used for the trie.
     auto& [destination_ips_map, destination_ips_trie] = destination_ips_pair;
     std::vector<std::pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>>
@@ -568,8 +569,11 @@ void FilterChainManagerImpl::convertIPsToTries() {
       // We need to get access to all of the source IP strings so that we can convert them into
       // a trie like we did for the destination IPs above.
       for (auto& [server_name, transport_protocols_map] : *server_names_map_ptr) {
+        UNREFERENCED_PARAMETER(server_name);
         for (auto& [transport_protocol, application_protocols_map] : transport_protocols_map) {
+          UNREFERENCED_PARAMETER(transport_protocol);
           for (auto& [application_protocol, source_arrays] : application_protocols_map) {
+            UNREFERENCED_PARAMETER(application_protocol);
             for (auto& [source_ips_map, source_ips_trie] : source_arrays) {
               std::vector<
                   std::pair<SourcePortsMapSharedPtr, std::vector<Network::Address::CidrRange>>>

diff --git a/test/extensions/transport_sockets/tls/BUILD b/test/extensions/transport_sockets/tls/BUILD
@@ -16,12 +16,11 @@ envoy_cc_test(
         "ssl_socket_test.cc",
     ],
     data = [
-        "gen_unittest_certs.sh",
         # TODO(mattklein123): We should consolidate all of our test certs in a single place as
         # right now we have a bunch of duplication which is confusing.
         "//test/config/integration/certs",
+        "//test/extensions/transport_sockets/tls/ocsp/test_data:certs",
         "//test/extensions/transport_sockets/tls/test_data:certs",
-        "//test/extensions/transport_sockets/tls/ocsp:gen_ocsp_data",
     ],
     external_deps = ["ssl"],
     shard_count = 4,
@@ -74,12 +73,9 @@ envoy_cc_test(
         "ssl_certs_test.h",
     ],
     data = [
-        "gen_unittest_certs.sh",
-        "//test/extensions/transport_sockets/tls/ocsp:gen_ocsp_data",
+        "//test/extensions/transport_sockets/tls/ocsp/test_data:certs",
         "//test/extensions/transport_sockets/tls/test_data:certs",
     ],
-    # Fails intermittantly on local build
-    tags = ["flaky_on_windows"],
     deps = [
         ":ssl_test_utils",
         "//source/common/common:base64_lib",
@@ -121,8 +117,6 @@ envoy_cc_test(
         "utility_test.cc",
     ],
     data = [
-        "gen_unittest_certs.sh",
-        "//test/extensions/transport_sockets/tls/ocsp:gen_ocsp_data",
         "//test/extensions/transport_sockets/tls/test_data:certs",
     ],
     external_deps = ["ssl"],
@@ -171,14 +165,9 @@ envoy_cc_test(
     name = "handshaker_test",
     srcs = ["handshaker_test.cc"],
     data = [
-        "gen_unittest_certs.sh",
-        "//test/config/integration/certs",
         "//test/extensions/transport_sockets/tls/test_data:certs",
     ],
     external_deps = ["ssl"],
-    # TODO(sunjayBhatia): Diagnose openssl DLL load issue on Windows
-    # See: https://github.com/envoyproxy/envoy/pull/13276
-    tags = ["flaky_on_windows"],
     deps = [
         ":ssl_socket_test",
         ":ssl_test_utils",