From 4a26fa55f4ad5a89173563623e4e0f3d93dba28d Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Mon, 14 Dec 2020 21:40:37 +0000 Subject: [PATCH 1/2] Disable TLS inspector injection Signed-off-by: Taylor Barrella --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 1 + source/server/listener_impl.cc | 9 ++++++- source/server/listener_impl.h | 1 + test/server/listener_manager_impl_test.cc | 31 +++++++++++++++++++++++ test/server/listener_manager_impl_test.h | 8 ++++++ 6 files changed, 50 insertions(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 94e90b463b728..69c2828d01d59 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -21,6 +21,7 @@ Minor Behavior Changes * http: upstream protocol will now only be logged if an upstream stream was established. * jwt_authn filter: added support of Jwt time constraint verification with a clock skew (default to 60 seconds) and added a filter config field :ref:`clock_skew_seconds ` to configure it. * kill_request: enable a way to configure kill header name in KillRequest proto. +* listener: injection of the :ref:`TLS inspector ` has been disabled by default. This feature is controlled by the runtime guard `envoy.reloadable_features.disable_tls_inspector_injection`. * memory: enable new tcmalloc with restartable sequences for aarch64 builds. * mongo proxy metrics: swapped network connection remote and local closed counters previously set reversed (`cx_destroy_local_with_active_rq` and `cx_destroy_remote_with_active_rq`). * performance: improve performance when handling large HTTP/1 bodies. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 74a3ce75f8457..1ac9652de1b4b 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -63,6 +63,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.allow_response_for_timeout", "envoy.reloadable_features.consume_all_retry_headers", "envoy.reloadable_features.check_ocsp_policy", + "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.disallow_unbounded_access_logs", "envoy.reloadable_features.early_errors_via_hcm", "envoy.reloadable_features.enable_dns_cache_circuit_breakers", diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 3226998611332..2b51749c65f05 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -263,6 +263,8 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config, listener_filters_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, listener_filters_timeout, 15000)), continue_on_listener_filters_timeout_(config.continue_on_listener_filters_timeout()), + disable_tls_inspector_injection_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.disable_tls_inspector_injection")), listener_factory_context_(std::make_shared( parent.server_, validation_visitor_, config, this, *this, parent.factory_.createDrainManager(config.drain_type()))), @@ -342,6 +344,8 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, listener_filters_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, listener_filters_timeout, 15000)), continue_on_listener_filters_timeout_(config.continue_on_listener_filters_timeout()), + disable_tls_inspector_injection_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.disable_tls_inspector_injection")), connection_balancer_(origin.connection_balancer_), listener_factory_context_(std::make_shared( origin.listener_factory_context_->listener_factory_context_base_, this, *this)), @@ -556,6 +560,9 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { } void ListenerImpl::buildTlsInspectorListenerFilter() { + if (disable_tls_inspector_injection_) { + return; + } // TODO(zuercher) remove the deprecated TLS inspector name when the deprecated names are removed. const bool need_tls_inspector = needTlsInspector(config_); // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed. @@ -738,7 +745,7 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L } // See buildTlsInspectorListenerFilter(). - if (needTlsInspector(config_) ^ needTlsInspector(config)) { + if (!disable_tls_inspector_injection_ && (needTlsInspector(config_) ^ needTlsInspector(config))) { return false; } return ListenerMessageUtil::filterChainOnlyChange(config_, config); diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index bd7f33db221de..336d20e6534c4 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -396,6 +396,7 @@ class ListenerImpl final : public Network::ListenerConfig, Network::Socket::OptionsSharedPtr listen_socket_options_; const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; + const bool disable_tls_inspector_injection_; Network::ActiveUdpListenerFactoryPtr udp_listener_factory_; Network::UdpPacketWriterFactoryPtr udp_writer_factory_; Network::UdpListenerWorkerRouterPtr udp_listener_worker_router_; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 98a6977d19b45..1d5da4a8eeabe 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -3296,7 +3296,33 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, MultipleFilterChainsWithOverlappi "overlapping matching rules are defined"); } +TEST_F(ListenerManagerImplWithRealFiltersTest, TlsFilterChainWithTlsInspectorInjectionDisabled) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + address: + socket_address: { address: 127.0.0.1, port_value: 1234 } + filter_chains: + - filter_chain_match: + transport_protocol: "tls" + - filter_chain_match: + # empty + )EOF", + Network::Address::IpVersion::v4); + + EXPECT_CALL(server_.api_.random_, uuid()); + EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, {true})); + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true); + EXPECT_EQ(1U, manager_->listeners().size()); + + // Make sure there are no listener filters (i.e. no automatically injected TLS Inspector). + Network::ListenerConfig& listener = manager_->listeners().back().get(); + Network::FilterChainFactory& filterChainFactory = listener.filterChainFactory(); + Network::MockListenerFilterManager manager; + EXPECT_CALL(manager, addAcceptFilter_(_, _)).Times(0); + EXPECT_TRUE(filterChainFactory.createListenerFilterChain(manager)); +} + TEST_F(ListenerManagerImplWithRealFiltersTest, TlsFilterChainWithoutTlsInspector) { + auto tls_inspector_injection_enabled_guard = enableTlsInspectorInjectionForThisTest(); const std::string yaml = TestEnvironment::substitute(R"EOF( address: socket_address: { address: 127.0.0.1, port_value: 1234 } @@ -3327,6 +3353,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, TlsFilterChainWithoutTlsInspector // Test the tls inspector is not injected twice when the deprecated name is used. TEST_F(ListenerManagerImplWithRealFiltersTest, DEPRECATED_FEATURE_TEST(TlsFilterChainWithDeprecatedTlsInspectorName)) { + auto tls_inspector_injection_enabled_guard = enableTlsInspectorInjectionForThisTest(); const std::string yaml = TestEnvironment::substitute(R"EOF( address: socket_address: { address: 127.0.0.1, port_value: 1234 } @@ -3358,6 +3385,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, } TEST_F(ListenerManagerImplWithRealFiltersTest, SniFilterChainWithoutTlsInspector) { + auto tls_inspector_injection_enabled_guard = enableTlsInspectorInjectionForThisTest(); const std::string yaml = TestEnvironment::substitute(R"EOF( address: socket_address: { address: 127.0.0.1, port_value: 1234 } @@ -3386,6 +3414,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, SniFilterChainWithoutTlsInspector } TEST_F(ListenerManagerImplWithRealFiltersTest, AlpnFilterChainWithoutTlsInspector) { + auto tls_inspector_injection_enabled_guard = enableTlsInspectorInjectionForThisTest(); const std::string yaml = TestEnvironment::substitute(R"EOF( address: socket_address: { address: 127.0.0.1, port_value: 1234 } @@ -3414,6 +3443,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, AlpnFilterChainWithoutTlsInspecto } TEST_F(ListenerManagerImplWithRealFiltersTest, CustomTransportProtocolWithSniWithoutTlsInspector) { + auto tls_inspector_injection_enabled_guard = enableTlsInspectorInjectionForThisTest(); const std::string yaml = TestEnvironment::substitute(R"EOF( address: socket_address: { address: 127.0.0.1, port_value: 1234 } @@ -4689,6 +4719,7 @@ TEST_F(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfAn TEST_F(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfImplicitTlsInspectorChanges) { + auto tls_inspector_injection_enabled_guard = enableTlsInspectorInjectionForThisTest(); EXPECT_CALL(*worker_, start(_)); manager_->startWorkers(guard_dog_); diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index 9b22f3109bf85..01104f1729e46 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -279,6 +279,14 @@ class ListenerManagerImplTest : public testing::Test { return scoped_runtime; } + ABSL_MUST_USE_RESULT + auto enableTlsInspectorInjectionForThisTest() { + auto scoped_runtime = std::make_unique(); + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.disable_tls_inspector_injection", "false"}}); + return scoped_runtime; + } + NiceMock os_sys_calls_; TestThreadsafeSingletonInjector os_calls_{&os_sys_calls_}; Api::OsSysCallsImpl os_sys_calls_actual_; From bd0cb0ef3d2c99178c0c3305934eb7a00b53e8f6 Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Tue, 15 Dec 2020 18:18:32 +0000 Subject: [PATCH 2/2] simplify Signed-off-by: Taylor Barrella --- source/server/listener_impl.cc | 12 ++++-------- source/server/listener_impl.h | 1 - 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 2b51749c65f05..7516efb9f7634 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -46,6 +46,9 @@ bool anyFilterChain( } bool needTlsInspector(const envoy::config::listener::v3::Listener& config) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.disable_tls_inspector_injection")) { + return false; + } return anyFilterChain(config, [](const auto& filter_chain) { const auto& matcher = filter_chain.filter_chain_match(); @@ -263,8 +266,6 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config, listener_filters_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, listener_filters_timeout, 15000)), continue_on_listener_filters_timeout_(config.continue_on_listener_filters_timeout()), - disable_tls_inspector_injection_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.disable_tls_inspector_injection")), listener_factory_context_(std::make_shared( parent.server_, validation_visitor_, config, this, *this, parent.factory_.createDrainManager(config.drain_type()))), @@ -344,8 +345,6 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, listener_filters_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, listener_filters_timeout, 15000)), continue_on_listener_filters_timeout_(config.continue_on_listener_filters_timeout()), - disable_tls_inspector_injection_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.disable_tls_inspector_injection")), connection_balancer_(origin.connection_balancer_), listener_factory_context_(std::make_shared( origin.listener_factory_context_->listener_factory_context_base_, this, *this)), @@ -560,9 +559,6 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { } void ListenerImpl::buildTlsInspectorListenerFilter() { - if (disable_tls_inspector_injection_) { - return; - } // TODO(zuercher) remove the deprecated TLS inspector name when the deprecated names are removed. const bool need_tls_inspector = needTlsInspector(config_); // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed. @@ -745,7 +741,7 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L } // See buildTlsInspectorListenerFilter(). - if (!disable_tls_inspector_injection_ && (needTlsInspector(config_) ^ needTlsInspector(config))) { + if (needTlsInspector(config_) ^ needTlsInspector(config)) { return false; } return ListenerMessageUtil::filterChainOnlyChange(config_, config); diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 336d20e6534c4..bd7f33db221de 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -396,7 +396,6 @@ class ListenerImpl final : public Network::ListenerConfig, Network::Socket::OptionsSharedPtr listen_socket_options_; const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; - const bool disable_tls_inspector_injection_; Network::ActiveUdpListenerFactoryPtr udp_listener_factory_; Network::UdpPacketWriterFactoryPtr udp_writer_factory_; Network::UdpListenerWorkerRouterPtr udp_listener_worker_router_;