From 88983684b818897666ee50721babce76c83a13c9 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 28 Jan 2020 11:43:22 -0800 Subject: [PATCH 01/12] Cherry-pick of https://github.com/envoyproxy/envoy/pull/9839 Signed-off-by: Kuat Yessenov --- docs/root/intro/arch_overview/security/rbac_filter.rst | 1 + source/extensions/filters/common/expr/context.cc | 3 +++ source/extensions/filters/common/expr/context.h | 1 + test/extensions/filters/common/expr/context_test.cc | 8 ++++++++ 4 files changed, 13 insertions(+) diff --git a/docs/root/intro/arch_overview/security/rbac_filter.rst b/docs/root/intro/arch_overview/security/rbac_filter.rst index 96685df1ac..e6b2c9d9e2 100644 --- a/docs/root/intro/arch_overview/security/rbac_filter.rst +++ b/docs/root/intro/arch_overview/security/rbac_filter.rst @@ -81,6 +81,7 @@ The following attributes are exposed to the language runtime: response.headers, string map, All response headers response.trailers, string map, All response trailers response.size, int, Size of the response body + response.total_size, int, Total size of the response including the approximate uncompressed size of the headers and the trailers response.flags, int, Additional details about the response beyond the standard response code source.address, string, Downstream connection remote address source.port, int, Downstream connection remote port diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index a6eaf8a459..8477662815 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -131,6 +131,9 @@ absl::optional ResponseWrapper::operator[](CelValue key) const { return CelValue::CreateMap(&trailers_); } else if (value == Flags) { return CelValue::CreateInt64(info_.responseFlags()); + } else if (value == TotalSize) { + return CelValue::CreateInt64(info_.bytesSent() + headers_.value_->byteSize().value() + + trailers_.value_->byteSize().value()); } return {}; } diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 0f79f197e6..77bf9cab80 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -76,6 +76,7 @@ class HeadersWrapper : public google::api::expr::runtime::CelMap { private: friend class RequestWrapper; + friend class ResponseWrapper; const Http::HeaderMap* value_; }; diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index ce4217cc58..db5850efd6 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -214,6 +214,14 @@ TEST(Context, ResponseAttributes) { EXPECT_EQ(123, value.value().Int64OrDie()); } + { + auto value = response[CelValue::CreateString(TotalSize)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(148, value.value().Int64OrDie()); + } + + { auto value = response[CelValue::CreateString(Code)]; EXPECT_TRUE(value.has_value()); From 793809cecd4894b31aa2da112a0d96df8e0f45a9 Mon Sep 17 00:00:00 2001 From: Kuat Date: Wed, 29 Jan 2020 15:18:35 -0800 Subject: [PATCH 02/12] Cherry-pick of https://github.com/envoyproxy/envoy/pull/9862 Signed-off-by: Kuat Yessenov --- .../extensions/filters/common/expr/context.cc | 10 ++++--- .../filters/common/expr/context_test.cc | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 8477662815..befd2de5af 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -77,6 +77,9 @@ absl::optional RequestWrapper::operator[](CelValue key) const { } else { return CelValue::CreateInt64(info_.bytesReceived()); } + } else if (value == TotalSize) { + return CelValue::CreateInt64(info_.bytesReceived() + + (headers_.value_ ? headers_.value_->byteSize().value() : 0)); } else if (value == Duration) { auto duration = info_.requestComplete(); if (duration.has_value()) { @@ -106,8 +109,6 @@ absl::optional RequestWrapper::operator[](CelValue key) const { return convertHeaderEntry(headers_.value_->RequestId()); } else if (value == UserAgent) { return convertHeaderEntry(headers_.value_->UserAgent()); - } else if (value == TotalSize) { - return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize().value()); } } return {}; @@ -132,8 +133,9 @@ absl::optional ResponseWrapper::operator[](CelValue key) const { } else if (value == Flags) { return CelValue::CreateInt64(info_.responseFlags()); } else if (value == TotalSize) { - return CelValue::CreateInt64(info_.bytesSent() + headers_.value_->byteSize().value() + - trailers_.value_->byteSize().value()); + return CelValue::CreateInt64(info_.bytesSent() + + (headers_.value_ ? headers_.value_->byteSize().value() : 0) + + (trailers_.value_ ? trailers_.value_->byteSize().value() : 0)); } return {}; } diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index db5850efd6..75971170c0 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -32,12 +32,14 @@ TEST(Context, EmptyHeadersAttributes) { TEST(Context, RequestAttributes) { NiceMock info; + NiceMock empty_info; Http::TestHeaderMapImpl header_map{ {":method", "POST"}, {":scheme", "http"}, {":path", "/meow?yes=1"}, {":authority", "kittens.com"}, {"referer", "dogs.com"}, {"user-agent", "envoy-mobile"}, {"content-length", "10"}, {"x-request-id", "blah"}, }; RequestWrapper request(&header_map, info); + RequestWrapper empty_request(nullptr, empty_info); EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10)); // "2018-04-03T23:06:09.123Z". @@ -66,6 +68,12 @@ TEST(Context, RequestAttributes) { ASSERT_TRUE(value.value().IsString()); EXPECT_EQ("http", value.value().StringOrDie().value()); } + + { + auto value = empty_request[CelValue::CreateString(Scheme)]; + EXPECT_FALSE(value.has_value()); + } + { auto value = request[CelValue::CreateString(Host)]; EXPECT_TRUE(value.has_value()); @@ -130,6 +138,14 @@ TEST(Context, RequestAttributes) { EXPECT_EQ(138, value.value().Int64OrDie()); } + { + auto value = empty_request[CelValue::CreateString(TotalSize)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + // this includes the headers size + EXPECT_EQ(0, value.value().Int64OrDie()); + } + { auto value = request[CelValue::CreateString(Time)]; EXPECT_TRUE(value.has_value()); @@ -187,11 +203,13 @@ TEST(Context, RequestFallbackAttributes) { TEST(Context, ResponseAttributes) { NiceMock info; + NiceMock empty_info; const std::string header_name = "test-header"; const std::string trailer_name = "test-trailer"; Http::TestHeaderMapImpl header_map{{header_name, "a"}}; Http::TestHeaderMapImpl trailer_map{{trailer_name, "b"}}; ResponseWrapper response(&header_map, &trailer_map, info); + ResponseWrapper empty_response(nullptr, nullptr, empty_info); EXPECT_CALL(info, responseCode()).WillRepeatedly(Return(404)); EXPECT_CALL(info, bytesSent()).WillRepeatedly(Return(123)); @@ -222,6 +240,13 @@ TEST(Context, ResponseAttributes) { } + { + auto value = empty_response[CelValue::CreateString(TotalSize)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(0, value.value().Int64OrDie()); + } + { auto value = response[CelValue::CreateString(Code)]; EXPECT_TRUE(value.has_value()); @@ -259,6 +284,7 @@ TEST(Context, ResponseAttributes) { ASSERT_TRUE(header.value().IsString()); EXPECT_EQ("b", header.value().StringOrDie().value()); } + { auto value = response[CelValue::CreateString(Flags)]; EXPECT_TRUE(value.has_value()); From d6bdad89d3d33b8c763c31af7e9e69f3836e89d4 Mon Sep 17 00:00:00 2001 From: Oliver Liu Date: Fri, 7 Feb 2020 11:17:13 -0800 Subject: [PATCH 03/12] sds: fix combined validation context validation bypassing (#115) Previously, the update callback was called only when the secret was received for the first time or when its value changed. This meant that if the same secret (e.g. trusted CA) was used in multiple resources, then resources using it but configured after the secret was already received, remained unconfigured until the secret's value changed. The missing callback should have resulted in transport factories stuck in the "not ready" state, however, because of an incorrect code, the available secret was processed like inlined validation context, and only rules from the "secret" part of the validation context were applied, leading to a complete bypass of rules from the "default" part. Signed-off-by: Piotr Sikora Co-authored-by: Oliver Liu --- docs/root/intro/version_history.rst | 16 ++ source/common/secret/sds_api.h | 9 ++ .../tls/context_config_impl.cc | 56 +++---- .../sds_dynamic_integration_test.cc | 137 ++++++++++++++++-- 4 files changed, 177 insertions(+), 41 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index d9d352d7ca..da2e1af79e 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -1,6 +1,22 @@ Version history --------------- +1.12.3 (Pending) +========================== +* sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. + +1.12.2 (December 10, 2019) +========================== +* http: fixed CVE-2019-18801 by allocating sufficient memory for request headers. +* http: fixed CVE-2019-18802 by implementing stricter validation of HTTP/1 headers. +* http: trim LWS at the end of header keys, for correct HTTP/1.1 header parsing. +* http: added strict authority checking. This can be reversed temporarily by setting the runtime feature `envoy.reloadable_features.strict_authority_validation` to false. +* route config: fixed CVE-2019-18838 by checking for presence of host/path headers. + +1.12.1 (November 8, 2019) +========================= +* listener: fixed CVE-2019-18836 by clearing accept filters before connection creation. + 1.12.0 (October 31, 2019) ========================= * access log: added a new flag for :ref:`downstream protocol error `. diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 695ecd02e8..7ab69c1e31 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -124,6 +124,9 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider return nullptr; } Common::CallbackHandle* addUpdateCallback(std::function callback) override { + if (secret()) { + callback(); + } return update_callback_manager_.add(callback); } @@ -174,6 +177,9 @@ class CertificateValidationContextSdsApi : public SdsApi, return certificate_validation_context_secrets_.get(); } Common::CallbackHandle* addUpdateCallback(std::function callback) override { + if (secret()) { + callback(); + } return update_callback_manager_.add(callback); } @@ -237,6 +243,9 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon } Common::CallbackHandle* addUpdateCallback(std::function callback) override { + if (secret()) { + callback(); + } return update_callback_manager_.add(callback); } diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc index e7bcc9f661..a3f2704dee 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -158,21 +158,33 @@ ContextConfigImpl::ContextConfigImpl( default_min_protocol_version)), max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), default_max_protocol_version)) { - if (default_cvc_ && certificate_validation_context_provider_ != nullptr) { - // We need to validate combined certificate validation context. - // The default certificate validation context and dynamic certificate validation - // context could only contain partial fields, which is okay to fail the validation. - // But the combined certificate validation context should pass validation. If - // validation of combined certificate validation context fails, - // getCombinedValidationContextConfig() throws exception, validation_context_config_ will not - // get updated. - cvc_validation_callback_handle_ = - certificate_validation_context_provider_->addValidationCallback( - [this](const envoy::api::v2::auth::CertificateValidationContext& dynamic_cvc) { - getCombinedValidationContextConfig(dynamic_cvc); - }); + if (certificate_validation_context_provider_ != nullptr) { + if (default_cvc_) { + // We need to validate combined certificate validation context. + // The default certificate validation context and dynamic certificate validation + // context could only contain partial fields, which is okay to fail the validation. + // But the combined certificate validation context should pass validation. If + // validation of combined certificate validation context fails, + // getCombinedValidationContextConfig() throws exception, validation_context_config_ will not + // get updated. + cvc_validation_callback_handle_ = + certificate_validation_context_provider_->addValidationCallback( + [this](const envoy::api::v2::auth::CertificateValidationContext& dynamic_cvc) { + getCombinedValidationContextConfig(dynamic_cvc); + }); + } + // Load inlined, static or dynamic secret that's already available. + if (certificate_validation_context_provider_->secret() != nullptr) { + if (default_cvc_) { + validation_context_config_ = + getCombinedValidationContextConfig(*certificate_validation_context_provider_->secret()); + } else { + validation_context_config_ = std::make_unique( + *certificate_validation_context_provider_->secret(), api_); + } + } } - // Load inline or static secret into tls_certificate_config_. + // Load inlined, static or dynamic secrets that are already available. if (!tls_certificate_providers_.empty()) { for (auto& provider : tls_certificate_providers_) { if (provider->secret() != nullptr) { @@ -180,12 +192,6 @@ ContextConfigImpl::ContextConfigImpl( } } } - // Load inline or static secret into validation_context_config_. - if (certificate_validation_context_provider_ != nullptr && - certificate_validation_context_provider_->secret() != nullptr) { - validation_context_config_ = std::make_unique( - *certificate_validation_context_provider_->secret(), api_); - } } Ssl::CertificateValidationContextConfigPtr ContextConfigImpl::getCombinedValidationContextConfig( @@ -369,12 +375,10 @@ ServerContextConfigImpl::ServerContextConfigImpl( [this](const envoy::api::v2::auth::TlsSessionTicketKeys& keys) { getSessionTicketKeys(keys); }); - } - - // Load inline or static secrets. - if (session_ticket_keys_provider_ != nullptr && - session_ticket_keys_provider_->secret() != nullptr) { - session_ticket_keys_ = getSessionTicketKeys(*session_ticket_keys_provider_->secret()); + // Load inlined, static or dynamic secret that's already available. + if (session_ticket_keys_provider_->secret() != nullptr) { + session_ticket_keys_ = getSessionTicketKeys(*session_ticket_keys_provider_->secret()); + } } if ((config.common_tls_context().tls_certificates().size() + diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 60f697af7c..38d8391444 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -10,6 +10,7 @@ #include "extensions/transport_sockets/tls/context_config_impl.h" #include "extensions/transport_sockets/tls/context_manager_impl.h" +#include "extensions/transport_sockets/tls/ssl_socket.h" #include "test/common/grpc/grpc_client_integration.h" #include "test/config/integration/certs/clientcert_hash.h" @@ -257,21 +258,7 @@ class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstrea TestEnvironment::runfilesPath("test/config/integration/certs/servercert.pem")); tls_certificate->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/serverkey.pem")); - - if (use_combined_validation_context_) { - // Modify the listener context validation type to use combined certificate validation - // context. - auto* combined_config = common_tls_context->mutable_combined_validation_context(); - auto* default_validation_context = combined_config->mutable_default_validation_context(); - default_validation_context->add_verify_certificate_hash(TEST_CLIENT_CERT_HASH); - auto* secret_config = combined_config->mutable_validation_context_sds_secret_config(); - setUpSdsConfig(secret_config, validation_secret_); - } else { - // Modify the listener context validation type to use dynamic certificate validation - // context. - auto* secret_config = common_tls_context->mutable_validation_context_sds_secret_config(); - setUpSdsConfig(secret_config, validation_secret_); - } + setUpSdsValidationContext(common_tls_context); transport_socket->set_name("envoy.transport_sockets.tls"); transport_socket->mutable_typed_config()->PackFrom(tls_context); @@ -280,16 +267,89 @@ class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstrea sds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); sds_cluster->set_name("sds_cluster"); sds_cluster->mutable_http2_protocol_options(); + + envoy::api::v2::auth::UpstreamTlsContext upstream_tls_context; + if (share_validation_secret_) { + // Configure static cluster with SDS config referencing "validation_secret", + // which is going to be processed before LDS resources. + ASSERT(use_lds_); + setUpSdsValidationContext(upstream_tls_context.mutable_common_tls_context()); + } + // Enable SSL/TLS with a client certificate in the first cluster. + auto* upstream_tls_certificate = + upstream_tls_context.mutable_common_tls_context()->add_tls_certificates(); + upstream_tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientcert.pem")); + upstream_tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); + auto* upstream_transport_socket = + bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_transport_socket(); + upstream_transport_socket->set_name("envoy.transport_sockets.tls"); + upstream_transport_socket->mutable_typed_config()->PackFrom(upstream_tls_context); }); HttpIntegrationTest::initialize(); + registerTestServerPorts({"http"}); client_ssl_ctx_ = createClientSslTransportSocketFactory({}, context_manager_, *api_); } + void setUpSdsValidationContext(envoy::api::v2::auth::CommonTlsContext* common_tls_context) { + if (use_combined_validation_context_) { + // Modify the listener context validation type to use combined certificate validation + // context. + auto* combined_config = common_tls_context->mutable_combined_validation_context(); + auto* default_validation_context = combined_config->mutable_default_validation_context(); + default_validation_context->add_verify_certificate_hash(TEST_CLIENT_CERT_HASH); + auto* secret_config = combined_config->mutable_validation_context_sds_secret_config(); + setUpSdsConfig(secret_config, validation_secret_); + } else { + // Modify the listener context validation type to use dynamic certificate validation + // context. + auto* secret_config = common_tls_context->mutable_validation_context_sds_secret_config(); + setUpSdsConfig(secret_config, validation_secret_); + } + } + + void createUpstreams() override { + // Fake upstream with SSL/TLS for the first cluster. + fake_upstreams_.emplace_back(new FakeUpstream( + createUpstreamSslContext(), 0, FakeHttpConnection::Type::HTTP1, version_, timeSystem())); + create_xds_upstream_ = true; + } + + Network::TransportSocketFactoryPtr createUpstreamSslContext() { + envoy::api::v2::auth::DownstreamTlsContext tls_context; + auto* common_tls_context = tls_context.mutable_common_tls_context(); + auto* tls_certificate = common_tls_context->add_tls_certificates(); + tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientcert.pem")); + tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); + + auto cfg = std::make_unique( + tls_context, factory_context_); + static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); + return std::make_unique( + std::move(cfg), context_manager_, *upstream_stats_store, std::vector{}); + } + + void TearDown() override { + cleanUpXdsConnection(); + + client_ssl_ctx_.reset(); + cleanupUpstreamAndDownstream(); + codec_client_.reset(); + + test_server_.reset(); + fake_upstreams_.clear(); + } + void enableCombinedValidationContext(bool enable) { use_combined_validation_context_ = enable; } + void shareValidationSecret(bool share) { share_validation_secret_ = share; } private: bool use_combined_validation_context_{false}; + bool share_validation_secret_{false}; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicDownstreamCertValidationContextTest, @@ -327,6 +387,53 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedCertValidationCont testRouterHeaderOnlyRequestAndResponse(&creator); } +// A test that verifies that both: static cluster and LDS listener are updated when using +// the same verification secret (standalone validation context) from the SDS server. +TEST_P(SdsDynamicDownstreamCertValidationContextTest, BasicWithSharedSecret) { + shareValidationSecret(true); + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getCvcSecret()); + }; + initialize(); + + // Wait for "ssl_context_updated_by_sds" counters to indicate that both resources + // depending on the verification_secret were updated. + test_server_->waitForCounterGe( + "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 1); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(&creator); +} + +// A test that verifies that both: static cluster and LDS listener are updated when using +// the same verification secret (combined validation context) from the SDS server. +TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedValidationContextWithSharedSecret) { + enableCombinedValidationContext(true); + shareValidationSecret(true); + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getCvcSecretWithOnlyTrustedCa()); + }; + initialize(); + + // Wait for "ssl_context_updated_by_sds" counters to indicate that both resources + // depending on the verification_secret were updated. + test_server_->waitForCounterGe( + "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 1); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(&creator); +} + // Upstream SDS integration test: a static cluster has ssl cert from SDS. class SdsDynamicUpstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { public: From 1ca8596ecf14ab17843fe7b09b3e9169e394fa92 Mon Sep 17 00:00:00 2001 From: Jianfei Hu Date: Fri, 14 Feb 2020 13:43:27 -0800 Subject: [PATCH 04/12] buffer: draining any zero byte fragments (#9837) (#109) (#123) Given that we allow creating zero byte fragments, it'd be good to proactively drain them. For example if someone is doing timing instrumentation and wants to know when Network::Connection data is written to the kernel, it could be useful to have a zero byte sentinel. Risk Level: Low (I don't think anyone is adding zero byte fragments yet) Testing: new unit test Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk Signed-off-by: Jianfei Hu Co-authored-by: Lizan Zhou --- source/common/buffer/buffer_impl.cc | 5 +++++ test/common/buffer/owned_impl_test.cc | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 3f4d6a86f1..005ddcd053 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -213,6 +213,11 @@ void OwnedImpl::drain(uint64_t size) { } } } + // Make sure to drain any zero byte fragments that might have been added as + // sentinels for flushed data. + while (!slices_.empty() && slices_.front()->dataSize() == 0) { + slices_.pop_front(); + } } uint64_t OwnedImpl::getRawSlices(RawSlice* out, uint64_t out_size) const { diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 19e6daa06f..d1b6935546 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -70,6 +70,24 @@ TEST_P(OwnedImplTest, AddBufferFragmentWithCleanup) { EXPECT_TRUE(release_callback_called_); } +TEST_P(OwnedImplTest, AddEmptyFragment) { + char input[] = "hello world"; + BufferFragmentImpl frag1(input, 11, [](const void*, size_t, const BufferFragmentImpl*) {}); + BufferFragmentImpl frag2("", 0, [this](const void*, size_t, const BufferFragmentImpl*) { + release_callback_called_ = true; + }); + Buffer::OwnedImpl buffer; + buffer.addBufferFragment(frag1); + EXPECT_EQ(11, buffer.length()); + + buffer.addBufferFragment(frag2); + EXPECT_EQ(11, buffer.length()); + + buffer.drain(11); + EXPECT_EQ(0, buffer.length()); + EXPECT_TRUE(release_callback_called_); +} + TEST_P(OwnedImplTest, AddBufferFragmentDynamicAllocation) { char input_stack[] = "hello world"; char* input = new char[11]; From e90e85bc1a0104194008e7b17cae315abde3422b Mon Sep 17 00:00:00 2001 From: Jianfei Hu Date: Fri, 14 Feb 2020 13:43:46 -0800 Subject: [PATCH 05/12] network: draining the buffer on close (#9870) (#110) (#124) Signed-off-by: Alyssa Wilk Signed-off-by: Jianfei Hu Co-authored-by: Lizan Zhou --- source/common/network/connection_impl.cc | 7 +++++++ test/common/network/connection_impl_test.cc | 8 ++++++-- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 44b073083c..15e76a9128 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -196,6 +196,13 @@ void ConnectionImpl::closeSocket(ConnectionEvent close_type) { // Drain input and output buffers. updateReadBufferStats(0, 0); updateWriteBufferStats(0, 0); + + // As the socket closes, drain any remaining data. + // The data won't be written out at this point, and where there are reference + // counted buffer fragments, it helps avoid lifetime issues with the + // connection outlasting the subscriber. + write_buffer_->drain(write_buffer_->length()); + connection_stats_.reset(); file_event_.reset(); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 164bcea2bb..272884e3f5 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -136,6 +136,11 @@ class ConnectionImplTest : public testing::TestWithParam { } void disconnect(bool wait_for_remote_close) { + if (client_write_buffer_) { + EXPECT_CALL(*client_write_buffer_, drain(_)) + .Times(AnyNumber()) + .WillOnce(Invoke([&](uint64_t size) -> void { client_write_buffer_->baseDrain(size); })); + } EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::LocalClose)); client_connection_->close(ConnectionCloseType::NoFlush); if (wait_for_remote_close) { @@ -810,8 +815,6 @@ TEST_P(ConnectionImplTest, WriteWithWatermarks) { // call to write() will succeed, bringing the connection back under the low watermark. EXPECT_CALL(*client_write_buffer_, write(_)) .WillOnce(Invoke(client_write_buffer_, &MockWatermarkBuffer::trackWrites)); - EXPECT_CALL(*client_write_buffer_, drain(_)) - .WillOnce(Invoke(client_write_buffer_, &MockWatermarkBuffer::trackDrains)); EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(1); disconnect(true); @@ -889,6 +892,7 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(AnyNumber()); disconnect(true); } diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index a7b07a96ba..db7ff48d96 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -3986,7 +3986,7 @@ class SslReadBufferLimitTest : public SslSocketTest { EXPECT_CALL(*client_write_buffer, move(_)) .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), Invoke(client_write_buffer, &MockWatermarkBuffer::baseMove))); - EXPECT_CALL(*client_write_buffer, drain(_)).WillOnce(Invoke([&](uint64_t n) -> void { + EXPECT_CALL(*client_write_buffer, drain(_)).Times(2).WillOnce(Invoke([&](uint64_t n) -> void { client_write_buffer->baseDrain(n); dispatcher_->exit(); })); From 82b59dab051e3531a17dc51e0d4021ced573d9b3 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Fri, 14 Feb 2020 16:23:17 -0800 Subject: [PATCH 06/12] tls_inspector: enable TLSv1.3. (#119) Previously, TLS inspector didn't support TLSv1.3 and clients configured to use only TLSv1.3 were not recognized as TLS clients. Because TLS extensions (SNI, ALPN) were not inspected, those connections might have been matched to a wrong filter chain, possibly bypassing some security restrictions in the process. Fixes istio/istio#18695. Signed-off-by: Piotr Sikora --- docs/root/intro/version_history.rst | 1 + .../listener/tls_inspector/tls_inspector.cc | 6 +++ .../listener/tls_inspector/tls_inspector.h | 2 + .../tls_inspector/tls_inspector_benchmark.cc | 5 +- .../tls_inspector/tls_inspector_test.cc | 48 ++++++++++++------- .../listener/tls_inspector/tls_utility.cc | 7 +-- .../listener/tls_inspector/tls_utility.h | 5 +- 7 files changed, 51 insertions(+), 23 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index da2e1af79e..867fd91b64 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -3,6 +3,7 @@ Version history 1.12.3 (Pending) ========================== +* listeners: fixed issue where :ref:`TLS inspector listener filter ` could have been bypassed by a client using only TLS 1.3. * sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. 1.12.2 (December 10, 2019) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index d38ca293f8..62fd8aa801 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -23,6 +23,10 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { +// Min/max TLS version recognized by the underlying TLS/SSL library. +const unsigned Config::TLS_MIN_SUPPORTED_VERSION = TLS1_VERSION; +const unsigned Config::TLS_MAX_SUPPORTED_VERSION = TLS1_3_VERSION; + Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) : stats_{ALL_TLS_INSPECTOR_STATS(POOL_COUNTER_PREFIX(scope, "tls_inspector."))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), @@ -33,6 +37,8 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) max_client_hello_size_, size_t(TLS_MAX_CLIENT_HELLO))); } + SSL_CTX_set_min_proto_version(ssl_ctx_.get(), TLS_MIN_SUPPORTED_VERSION); + SSL_CTX_set_max_proto_version(ssl_ctx_.get(), TLS_MAX_SUPPORTED_VERSION); SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); SSL_CTX_set_select_certificate_cb( diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index ee353ce9ef..be232ce792 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -56,6 +56,8 @@ class Config { uint32_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; + static const unsigned TLS_MIN_SUPPORTED_VERSION; + static const unsigned TLS_MAX_SUPPORTED_VERSION; private: TlsInspectorStats stats_; diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc index 5f8c1431ce..b580e6a80e 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc @@ -66,8 +66,9 @@ class FastMockOsSysCalls : public Api::MockOsSysCalls { }; static void BM_TlsInspector(benchmark::State& state) { - NiceMock os_sys_calls( - Tls::Test::generateClientHello("example.com", "\x02h2\x08http/1.1")); + NiceMock os_sys_calls(Tls::Test::generateClientHello( + Config::TLS_MIN_SUPPORTED_VERSION, Config::TLS_MAX_SUPPORTED_VERSION, "example.com", + "\x02h2\x08http/1.1")); TestThreadsafeSingletonInjector os_calls{&os_sys_calls}; NiceMock store; ConfigSharedPtr cfg(std::make_shared(store)); diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index c0416e5282..3247ede588 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -27,7 +27,7 @@ namespace ListenerFilters { namespace TlsInspector { namespace { -class TlsInspectorTest : public testing::Test { +class TlsInspectorTest : public testing::TestWithParam> { public: TlsInspectorTest() : cfg_(std::make_shared(store_)), @@ -68,14 +68,22 @@ class TlsInspectorTest : public testing::Test { Network::IoHandlePtr io_handle_; }; +INSTANTIATE_TEST_SUITE_P(TlsProtocolVersions, TlsInspectorTest, + testing::Values(std::make_tuple(Config::TLS_MIN_SUPPORTED_VERSION, + Config::TLS_MAX_SUPPORTED_VERSION), + std::make_tuple(TLS1_VERSION, TLS1_VERSION), + std::make_tuple(TLS1_1_VERSION, TLS1_1_VERSION), + std::make_tuple(TLS1_2_VERSION, TLS1_2_VERSION), + std::make_tuple(TLS1_3_VERSION, TLS1_3_VERSION))); + // Test that an exception is thrown for an invalid value for max_client_hello_size -TEST_F(TlsInspectorTest, MaxClientHelloSize) { +TEST_P(TlsInspectorTest, MaxClientHelloSize) { EXPECT_THROW_WITH_MESSAGE(Config(store_, Config::TLS_MAX_CLIENT_HELLO + 1), EnvoyException, "max_client_hello_size of 65537 is greater than maximum of 65536."); } // Test that the filter detects Closed events and terminates. -TEST_F(TlsInspectorTest, ConnectionClosed) { +TEST_P(TlsInspectorTest, ConnectionClosed) { init(); EXPECT_CALL(cb_, continueFilterChain(false)); file_event_callback_(Event::FileReadyType::Closed); @@ -83,7 +91,7 @@ TEST_F(TlsInspectorTest, ConnectionClosed) { } // Test that the filter detects detects read errors. -TEST_F(TlsInspectorTest, ReadError) { +TEST_P(TlsInspectorTest, ReadError) { init(); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)).WillOnce(InvokeWithoutArgs([]() { return Api::SysCallSizeResult{ssize_t(-1), ENOTSUP}; @@ -94,10 +102,11 @@ TEST_F(TlsInspectorTest, ReadError) { } // Test that a ClientHello with an SNI value causes the correct name notification. -TEST_F(TlsInspectorTest, SniRegistered) { +TEST_P(TlsInspectorTest, SniRegistered) { init(); const std::string servername("example.com"); - std::vector client_hello = Tls::Test::generateClientHello(servername, ""); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), servername, ""); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce( Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { @@ -116,11 +125,12 @@ TEST_F(TlsInspectorTest, SniRegistered) { } // Test that a ClientHello with an ALPN value causes the correct name notification. -TEST_F(TlsInspectorTest, AlpnRegistered) { +TEST_P(TlsInspectorTest, AlpnRegistered) { init(); const std::vector alpn_protos = {absl::string_view("h2"), absl::string_view("http/1.1")}; - std::vector client_hello = Tls::Test::generateClientHello("", "\x02h2\x08http/1.1"); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), "", "\x02h2\x08http/1.1"); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce( Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { @@ -139,11 +149,12 @@ TEST_F(TlsInspectorTest, AlpnRegistered) { } // Test with the ClientHello spread over multiple socket reads. -TEST_F(TlsInspectorTest, MultipleReads) { +TEST_P(TlsInspectorTest, MultipleReads) { init(); const std::vector alpn_protos = {absl::string_view("h2")}; const std::string servername("example.com"); - std::vector client_hello = Tls::Test::generateClientHello(servername, "\x02h2"); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), servername, "\x02h2"); { InSequence s; EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) @@ -177,9 +188,10 @@ TEST_F(TlsInspectorTest, MultipleReads) { } // Test that the filter correctly handles a ClientHello with no extensions present. -TEST_F(TlsInspectorTest, NoExtensions) { +TEST_P(TlsInspectorTest, NoExtensions) { init(); - std::vector client_hello = Tls::Test::generateClientHello("", ""); + std::vector client_hello = + Tls::Test::generateClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), "", ""); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce( Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { @@ -199,10 +211,11 @@ TEST_F(TlsInspectorTest, NoExtensions) { // Test that the filter fails if the ClientHello is larger than the // maximum allowed size. -TEST_F(TlsInspectorTest, ClientHelloTooBig) { +TEST_P(TlsInspectorTest, ClientHelloTooBig) { const size_t max_size = 50; cfg_ = std::make_shared(store_, max_size); - std::vector client_hello = Tls::Test::generateClientHello("example.com", ""); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), "example.com", ""); ASSERT(client_hello.size() > max_size); init(); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) @@ -218,7 +231,7 @@ TEST_F(TlsInspectorTest, ClientHelloTooBig) { } // Test that the filter fails on non-SSL data -TEST_F(TlsInspectorTest, NotSsl) { +TEST_P(TlsInspectorTest, NotSsl) { init(); std::vector data; @@ -236,7 +249,7 @@ TEST_F(TlsInspectorTest, NotSsl) { EXPECT_EQ(1, cfg_->stats().tls_not_found_.value()); } -TEST_F(TlsInspectorTest, InlineReadSucceed) { +TEST_P(TlsInspectorTest, InlineReadSucceed) { filter_ = std::make_unique(cfg_); EXPECT_CALL(cb_, socket()).WillRepeatedly(ReturnRef(socket_)); @@ -244,7 +257,8 @@ TEST_F(TlsInspectorTest, InlineReadSucceed) { EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_)); const std::vector alpn_protos = {absl::string_view("h2")}; const std::string servername("example.com"); - std::vector client_hello = Tls::Test::generateClientHello(servername, "\x02h2"); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), servername, "\x02h2"); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce(Invoke( diff --git a/test/extensions/filters/listener/tls_inspector/tls_utility.cc b/test/extensions/filters/listener/tls_inspector/tls_utility.cc index f41691dba7..70ed11b1f4 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_utility.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_utility.cc @@ -8,11 +8,12 @@ namespace Envoy { namespace Tls { namespace Test { -std::vector generateClientHello(const std::string& sni_name, const std::string& alpn) { +std::vector generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version, + const std::string& sni_name, const std::string& alpn) { bssl::UniquePtr ctx(SSL_CTX_new(TLS_with_buffers_method())); - const long flags = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_COMPRESSION; - SSL_CTX_set_options(ctx.get(), flags); + SSL_CTX_set_min_proto_version(ctx.get(), tls_min_version); + SSL_CTX_set_max_proto_version(ctx.get(), tls_max_version); bssl::UniquePtr ssl(SSL_new(ctx.get())); diff --git a/test/extensions/filters/listener/tls_inspector/tls_utility.h b/test/extensions/filters/listener/tls_inspector/tls_utility.h index 51cbe7e8c4..13e911e9e3 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_utility.h +++ b/test/extensions/filters/listener/tls_inspector/tls_utility.h @@ -9,12 +9,15 @@ namespace Test { /** * Generate a TLS ClientHello in wire-format. + * @param tls_min_version Minimum supported TLS version to advertise. + * @param tls_max_version Maximum supported TLS version to advertise. * @param sni_name The name to include as a Server Name Indication. * No SNI extension is added if sni_name is empty. * @param alpn Protocol(s) list in the wire-format (i.e. 8-bit length-prefixed string) to advertise * in Application-Layer Protocol Negotiation. No ALPN is advertised if alpn is empty. */ -std::vector generateClientHello(const std::string& sni_name, const std::string& alpn); +std::vector generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version, + const std::string& sni_name, const std::string& alpn); } // namespace Test } // namespace Tls From 0cec3f553b61675d86a80cfefadf626ab4a5fb01 Mon Sep 17 00:00:00 2001 From: Jianfei Hu Date: Tue, 18 Feb 2020 19:37:17 -0800 Subject: [PATCH 07/12] http: adding response flood protection (#113) (#125) This is similar to the http2 frame protection, but rather than try to guard [header block || last body bytes || last chunk in chunk encoding || trailer block] depending on end stream, which just gets messy, I opted to just add an empty reference counted fragment after the body was serialized, which appears to work just as well with a small theoretical overhead. If folks think the complexity is warranted I can of course do that instead. Risk Level: Medium Testing: new unit tests, integration test Docs Changes: stats documented Release Notes: added Signed-off-by: Alyssa Wilk Signed-off-by: Lizan Zhou Signed-off-by: Jianfei Hu --- .../http/http_conn_man/stats.rst | 1 + docs/root/intro/version_history.rst | 1 + source/common/http/http1/codec_impl.cc | 64 ++++++++++++-- source/common/http/http1/codec_impl.h | 33 +++++-- source/common/runtime/runtime_features.cc | 1 + source/common/runtime/runtime_impl.cc | 2 +- test/common/http/http1/codec_impl_test.cc | 86 +++++++++++++++++++ test/integration/integration_admin_test.cc | 3 +- test/integration/integration_test.cc | 49 +++++++++++ 9 files changed, 227 insertions(+), 13 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/stats.rst b/docs/root/configuration/http/http_conn_man/stats.rst index 3a0d6060b4..6d46fb937b 100644 --- a/docs/root/configuration/http/http_conn_man/stats.rst +++ b/docs/root/configuration/http/http_conn_man/stats.rst @@ -111,6 +111,7 @@ All http1 statistics are rooted at *http1.* :widths: 1, 1, 2 metadata_not_supported_error, Counter, Total number of metadata dropped during HTTP/1 encoding + response_flood, Counter, Total number of connections closed due to response flooding Http2 codec statistics ~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 867fd91b64..071ec4f9f9 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ========================== * listeners: fixed issue where :ref:`TLS inspector listener filter ` could have been bypassed by a client using only TLS 1.3. * sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. +* http: added HTTP/1.1 flood protection. Can be temporarily disabled using the runtime feature `envoy.reloadable_features.http1_flood_protection`. 1.12.2 (December 10, 2019) ========================== diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 81b9868981..ef6ece4380 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -198,17 +198,49 @@ void StreamEncoderImpl::endEncode() { connection_.buffer().add(LAST_CHUNK); } - connection_.flushOutput(); + connection_.flushOutput(true); connection_.onEncodeComplete(); } -void ConnectionImpl::flushOutput() { +void ServerConnectionImpl::maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer& output_buffer) { + if (!flood_protection_) { + return; + } + // It's messy and complicated to try to tag the final write of an HTTP response for response + // tracking for flood protection. Instead, write an empty buffer fragment after the response, + // to allow for tracking. + // When the response is written out, the fragment will be deleted and the counter will be updated + // by ServerConnectionImpl::releaseOutboundResponse() + auto fragment = + Buffer::OwnedBufferFragmentImpl::create(absl::string_view("", 0), response_buffer_releasor_); + output_buffer.addBufferFragment(*fragment.release()); + ASSERT(outbound_responses_ < max_outbound_responses_); + outbound_responses_++; +} + +void ServerConnectionImpl::doFloodProtectionChecks() const { + if (!flood_protection_) { + return; + } + // Before sending another response, make sure it won't exceed flood protection thresholds. + if (outbound_responses_ >= max_outbound_responses_) { + ENVOY_CONN_LOG(trace, "error sending response: Too many pending responses queued", connection_); + stats_.response_flood_.inc(); + throw FrameFloodException("Too many responses queued."); + } +} + +void ConnectionImpl::flushOutput(bool end_encode) { if (reserved_current_) { reserved_iovec_.len_ = reserved_current_ - static_cast(reserved_iovec_.mem_); output_buffer_.commit(&reserved_iovec_, 1); reserved_current_ = nullptr; } - + if (end_encode) { + // If this is an HTTP response in ServerConnectionImpl, track outbound responses for flood + // protection + maybeAddSentinelBufferFragment(output_buffer_); + } connection().write(output_buffer_, false); ASSERT(0UL == output_buffer_.length()); } @@ -260,6 +292,9 @@ static const char RESPONSE_PREFIX[] = "HTTP/1.1 "; static const char HTTP_10_RESPONSE_PREFIX[] = "HTTP/1.0 "; void ResponseStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) { + // Do flood checks before attempting to write any responses. + flood_checks_(); + started_response_ = true; uint64_t numeric_status = Utility::getResponseStatus(headers); @@ -581,7 +616,18 @@ ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Stat const uint32_t max_request_headers_count) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, max_request_headers_count, formatter(settings)), - callbacks_(callbacks), codec_settings_(settings) {} + callbacks_(callbacks), codec_settings_(settings), + response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { + releaseOutboundResponse(fragment); + }), + // Pipelining is generally not well supported on the internet and has a series of dangerous + // overflow bugs. As such we are disabling it for now, and removing this temporary override if + // no one objects. If you use this integer to restore prior behavior, contact the + // maintainer team as it will otherwise be removed entirely soon. + max_outbound_responses_( + Runtime::getInteger("envoy.do_not_use_going_away_max_http2_outbound_responses", 2)), + flood_protection_( + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_flood_protection")) {} void ServerConnectionImpl::onEncodeComplete() { ASSERT(active_request_); @@ -681,7 +727,8 @@ int ServerConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { void ServerConnectionImpl::onMessageBegin() { if (!resetStreamCalled()) { ASSERT(!active_request_); - active_request_ = std::make_unique(*this, header_key_formatter_.get()); + active_request_ = + std::make_unique(*this, header_key_formatter_.get(), flood_checks_); active_request_->request_decoder_ = &callbacks_.newStream(active_request_->response_encoder_); } } @@ -752,6 +799,13 @@ void ServerConnectionImpl::onBelowLowWatermark() { } } +void ServerConnectionImpl::releaseOutboundResponse( + const Buffer::OwnedBufferFragmentImpl* fragment) { + ASSERT(outbound_responses_ >= 1); + --outbound_responses_; + delete fragment; +} + ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Stats::Scope& stats, ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 8b00bdc635..714f54ca74 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -27,7 +27,9 @@ namespace Http1 { /** * All stats for the HTTP/1 codec. @see stats_macros.h */ -#define ALL_HTTP1_CODEC_STATS(COUNTER) COUNTER(metadata_not_supported_error) +#define ALL_HTTP1_CODEC_STATS(COUNTER) \ + COUNTER(metadata_not_supported_error) \ + COUNTER(response_flood) /** * Wrapper struct for the HTTP/1 codec stats. @see stats_macros.h @@ -108,8 +110,11 @@ class StreamEncoderImpl : public StreamEncoder, */ class ResponseStreamEncoderImpl : public StreamEncoderImpl { public: - ResponseStreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter) - : StreamEncoderImpl(connection, header_key_formatter) {} + using FloodChecks = std::function; + + ResponseStreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter, + FloodChecks& flood_checks) + : StreamEncoderImpl(connection, header_key_formatter), flood_checks_(flood_checks) {} bool startedResponse() { return started_response_; } @@ -117,6 +122,7 @@ class ResponseStreamEncoderImpl : public StreamEncoderImpl { void encodeHeaders(const HeaderMap& headers, bool end_stream) override; private: + FloodChecks& flood_checks_; bool started_response_{}; }; @@ -166,7 +172,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable; ServerConnectionImpl(Network::Connection& connection, Stats::Scope& stats, ServerConnectionCallbacks& callbacks, Http1Settings settings, uint32_t max_request_headers_kb, const uint32_t max_request_headers_count); @@ -327,8 +335,9 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { * An active HTTP/1.1 request. */ struct ActiveRequest { - ActiveRequest(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter) - : response_encoder_(connection, header_key_formatter) {} + ActiveRequest(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter, + FloodChecks& flood_checks) + : response_encoder_(connection, header_key_formatter, flood_checks) {} HeaderString request_url_; StreamDecoder* request_decoder_{}; @@ -359,9 +368,21 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { void onAboveHighWatermark() override; void onBelowLowWatermark() override; + void releaseOutboundResponse(const Buffer::OwnedBufferFragmentImpl* fragment); + void maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer& output_buffer) override; + void doFloodProtectionChecks() const; + ServerConnectionCallbacks& callbacks_; + std::function flood_checks_{[&]() { this->doFloodProtectionChecks(); }}; std::unique_ptr active_request_; Http1Settings codec_settings_; + const Buffer::OwnedBufferFragmentImpl::Releasor response_buffer_releasor_; + uint32_t outbound_responses_{}; + // This defaults to 2, which functionally disables pipelining. If any users + // of Envoy wish to enable pipelining (which is dangerous and ill supported) + // we could make this configurable. + uint32_t max_outbound_responses_{}; + bool flood_protection_{}; }; /** diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index f08acfbb9d..a3822558a1 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -24,6 +24,7 @@ namespace Runtime { // problem of the bugs being found after the old code path has been removed. constexpr const char* runtime_features[] = { // Enabled + "envoy.reloadable_features.http1_flood_protection", "envoy.reloadable_features.test_feature_true", "envoy.reloadable_features.buffer_filter_populate_content_length", "envoy.reloadable_features.trusted_forwarded_proto", diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 9da583bdb8..c863bc62da 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -37,7 +37,7 @@ bool runtimeFeatureEnabled(absl::string_view feature) { } uint64_t getInteger(absl::string_view feature, uint64_t default_value) { - ASSERT(absl::StartsWith(feature, "envoy.reloadable_features")); + ASSERT(absl::StartsWith(feature, "envoy.")); if (Runtime::LoaderSingleton::getExisting()) { return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger( std::string(feature), default_value); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index cc77c93709..7f5a404cdb 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -427,6 +427,92 @@ TEST_F(Http1ServerConnectionImplTest, BadRequestStartedStream) { EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output); } +TEST_F(Http1ServerConnectionImplTest, FloodProtection) { + initialize(); + + NiceMock decoder; + Buffer::OwnedImpl local_buffer; + // Read a request and send a response, without draining the response from the + // connection buffer. The first two should not cause problems. + for (int i = 0; i < 2; ++i) { + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + + // In most tests the write output is serialized to a buffer here it is + // ignored to build up queued "end connection" sentinels. + EXPECT_CALL(connection_, write(_, _)) + .Times(1) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { + // Move the response out of data while preserving the buffer fragment sentinels. + local_buffer.move(data); + })); + + TestHeaderMapImpl headers{{":status", "200"}}; + response_encoder->encodeHeaders(headers, true); + } + + // Trying to shove a third response in the queue should trigger flood protection. + { + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + + TestHeaderMapImpl headers{{":status", "200"}}; + EXPECT_THROW_WITH_MESSAGE(response_encoder->encodeHeaders(headers, true), FrameFloodException, + "Too many responses queued."); + EXPECT_EQ(1, store_.counter("http1.response_flood").value()); + } +} + +TEST_F(Http1ServerConnectionImplTest, FloodProtectionOff) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.http1_flood_protection", "false"}}); + initialize(); + + NiceMock decoder; + Buffer::OwnedImpl local_buffer; + // With flood protection off, many responses can be queued up. + for (int i = 0; i < 4; ++i) { + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + + // In most tests the write output is serialized to a buffer here it is + // ignored to build up queued "end connection" sentinels. + EXPECT_CALL(connection_, write(_, _)) + .Times(1) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { + // Move the response out of data while preserving the buffer fragment sentinels. + local_buffer.move(data); + })); + + TestHeaderMapImpl headers{{":status", "200"}}; + response_encoder->encodeHeaders(headers, true); + } +} + TEST_F(Http1ServerConnectionImplTest, HostHeaderTranslation) { initialize(); diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 3d97a081db..bb590a860d 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -261,8 +261,9 @@ TEST_P(IntegrationAdminTest, Admin) { case Http::CodecClient::Type::HTTP1: EXPECT_EQ(" Count Lookup\n" " 1 http1.metadata_not_supported_error\n" + " 1 http1.response_flood\n" "\n" - "total: 1\n", + "total: 2\n", response->body()); break; case Http::CodecClient::Type::HTTP2: diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 5dbb8ce7b6..91f38de551 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -959,4 +959,53 @@ TEST_P(UpstreamEndpointIntegrationTest, TestUpstreamEndpointAddress) { Network::Test::getLoopbackAddressString(GetParam()).c_str()); } +// Send continuous pipelined requests while not reading responses, to check +// HTTP/1.1 response flood protection. +TEST_P(IntegrationTest, TestFlood) { + initialize(); + + // Set up a raw connection to easily send requests without reading responses. + Network::ClientConnectionPtr raw_connection = makeClientConnection(lookupPort("http")); + raw_connection->connect(); + + // Read disable so responses will queue up. + uint32_t bytes_to_send = 0; + raw_connection->readDisable(true); + // Track locally queued bytes, to make sure the outbound client queue doesn't back up. + raw_connection->addBytesSentCallback([&](uint64_t bytes) { bytes_to_send -= bytes; }); + + // Keep sending requests until flood protection kicks in and kills the connection. + while (raw_connection->state() == Network::Connection::State::Open) { + // These requests are missing the host header, so will provoke an internally generated error + // response from Envoy. + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\nGET / HTTP/1.1\r\n\r\nGET / HTTP/1.1\r\n\r\n"); + bytes_to_send += buffer.length(); + raw_connection->write(buffer, false); + // Loop until all bytes are sent. + while (bytes_to_send > 0 && raw_connection->state() == Network::Connection::State::Open) { + raw_connection->dispatcher().run(Event::Dispatcher::RunType::NonBlock); + } + } + + // Verify the connection was closed due to flood protection. + EXPECT_EQ(1, test_server_->counter("http1.response_flood")->value()); +} + +// Make sure flood protection doesn't kick in with many requests sent serially. +TEST_P(IntegrationTest, TestManyBadRequests) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + Http::TestHeaderMapImpl bad_request{ + {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}}; + + for (int i = 0; i < 1; ++i) { + IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(bad_request); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_THAT(response->headers(), HttpStatusIs("400")); + } + EXPECT_EQ(0, test_server_->counter("http1.response_flood")->value()); +} + } // namespace Envoy From f7d327a5d84a5e4f6861c6853dc951b6b4cc94c3 Mon Sep 17 00:00:00 2001 From: Yangmin Zhu Date: Wed, 19 Feb 2020 17:26:45 -0800 Subject: [PATCH 08/12] buffer: release empty slices after commit (#116) (#128) Description: Remove empty slices off the end of buffers after calls to OwnedImpl::commit. The slices reserved when OwnedImpl::reserve is called will sit unused in cases where the 0 bytes are commited, for example, when socket read returns 0 bytes EAGAIN. Trapped slices act like a memory leak until there is a successful read or the socket is closed. Risk Level: low Testing: unit Docs Changes: n/a Release Notes: n/a Signed-off-by: Antonio Vicente Signed-off-by: Asra Ali Signed-off-by: Yangmin Zhu --- include/envoy/buffer/buffer.h | 4 +- source/common/buffer/buffer_impl.cc | 13 +++ source/common/buffer/buffer_impl.h | 21 +++++ test/common/buffer/owned_impl_test.cc | 127 +++++++++++++++++++++++--- 4 files changed, 152 insertions(+), 13 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 1e29bf5cd9..801a5c0de9 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -101,8 +101,8 @@ class Instance { /** * Commit a set of slices originally obtained from reserve(). The number of slices should match * the number obtained from reserve(). The size of each slice can also be altered. Commit must - * occur following a reserve() without any mutating operations in between other than to the iovecs - * len_ fields. + * occur once following a reserve() without any mutating operations in between other than to the + * iovecs len_ fields. * @param iovecs supplies the array of slices to commit. * @param num_iovecs supplies the size of the slices array. */ diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 005ddcd053..fee0fa8900 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -150,6 +150,11 @@ void OwnedImpl::commit(RawSlice* iovecs, uint64_t num_iovecs) { } } + // In case an extra slice was reserved, remove empty slices from the end of the buffer. + while (!slices_.empty() && slices_.back()->dataSize() == 0) { + slices_.pop_back(); + } + ASSERT(num_slices_committed > 0); } } @@ -656,6 +661,14 @@ bool OwnedImpl::isSameBufferImpl(const Instance& rhs) const { return usesOldImpl() == other->usesOldImpl(); } +std::vector OwnedImpl::describeSlicesForTest() const { + std::vector slices; + for (const auto& slice : slices_) { + slices.push_back(slice->describeSliceForTest()); + } + return slices; +} + bool OwnedImpl::use_old_impl_ = false; } // namespace Buffer diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 114d14dabc..36715ed42d 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -174,6 +174,20 @@ class Slice { return copy_size; } + /** + * Describe the in-memory representation of the slice. For use + * in tests that want to make assertions about the specific arrangement of + * bytes in a slice. + */ + struct SliceRepresentation { + uint64_t data; + uint64_t reservable; + uint64_t capacity; + }; + SliceRepresentation describeSliceForTest() const { + return SliceRepresentation{dataSize(), reservableSize(), capacity_}; + } + protected: Slice(uint64_t data, uint64_t reservable, uint64_t capacity) : data_(data), reservable_(reservable), capacity_(capacity) {} @@ -541,6 +555,13 @@ class OwnedImpl : public LibEventInstance { */ static void useOldImpl(bool use_old_impl); + /** + * Describe the in-memory representation of the slices in the buffer. For use + * in tests that want to make assertions about the specific arrangement of + * bytes in the buffer. + */ + std::vector describeSlicesForTest() const; + private: /** * @param rhs another buffer diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index d1b6935546..3400e6334d 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -13,6 +13,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::ContainerEq; using testing::Return; namespace Envoy { @@ -34,6 +35,18 @@ class OwnedImplTest : public BufferImplementationParamTest { static void commitReservation(Buffer::RawSlice* iovecs, uint64_t num_iovecs, OwnedImpl& buffer) { buffer.commit(iovecs, num_iovecs); } + + static void expectSlices(std::vector> buffer_list, OwnedImpl& buffer) { + if (buffer.usesOldImpl()) { + return; + } + const auto& buffer_slices = buffer.describeSlicesForTest(); + for (uint64_t i = 0; i < buffer_slices.size(); i++) { + EXPECT_EQ(buffer_slices[i].data, buffer_list[i][0]); + EXPECT_EQ(buffer_slices[i].reservable, buffer_list[i][1]); + EXPECT_EQ(buffer_slices[i].capacity, buffer_list[i][2]); + } + } }; INSTANTIATE_TEST_SUITE_P(OwnedImplTest, OwnedImplTest, @@ -308,23 +321,27 @@ TEST_P(OwnedImplTest, Read) { EXPECT_TRUE(result.ok()); EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); + EXPECT_THAT(buffer.describeSlicesForTest(), testing::IsEmpty()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).WillOnce(Return(Api::SysCallSizeResult{-1, 0})); result = buffer.read(io_handle, 100); EXPECT_EQ(Api::IoError::IoErrorCode::UnknownError, result.err_->getErrorCode()); EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); + EXPECT_THAT(buffer.describeSlicesForTest(), testing::IsEmpty()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).WillOnce(Return(Api::SysCallSizeResult{-1, EAGAIN})); result = buffer.read(io_handle, 100); EXPECT_EQ(Api::IoError::IoErrorCode::Again, result.err_->getErrorCode()); EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); + EXPECT_THAT(buffer.describeSlicesForTest(), testing::IsEmpty()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).Times(0); result = buffer.read(io_handle, 0); EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); + EXPECT_THAT(buffer.describeSlicesForTest(), testing::IsEmpty()); } TEST_P(OwnedImplTest, ReserveCommit) { @@ -372,38 +389,41 @@ TEST_P(OwnedImplTest, ReserveCommit) { // the last slice, and allow the buffer to use only one slice. This should result in the // creation of a new slice within the buffer. num_reserved = buffer.reserve(4096 - sizeof(OwnedSlice), iovecs, 1); - const void* slice2 = iovecs[0].mem_; EXPECT_EQ(1, num_reserved); - EXPECT_NE(slice1, slice2); + EXPECT_NE(slice1, iovecs[0].mem_); clearReservation(iovecs, num_reserved, buffer); // Request the same size reservation, but allow the buffer to use multiple slices. This - // should result in the buffer splitting the reservation between its last two slices. + // should result in the buffer creating a second slice and splitting the reservation between the + // last two slices. num_reserved = buffer.reserve(4096 - sizeof(OwnedSlice), iovecs, NumIovecs); EXPECT_EQ(2, num_reserved); EXPECT_EQ(slice1, iovecs[0].mem_); - EXPECT_EQ(slice2, iovecs[1].mem_); clearReservation(iovecs, num_reserved, buffer); // Request a reservation that too big to fit in the existing slices. This should result // in the creation of a third slice. + expectSlices({{1, 4055, 4056}}, buffer); + buffer.reserve(4096 - sizeof(OwnedSlice), iovecs, NumIovecs); + expectSlices({{1, 4055, 4056}, {0, 4056, 4056}}, buffer); + const void* slice2 = iovecs[1].mem_; num_reserved = buffer.reserve(8192, iovecs, NumIovecs); + expectSlices({{1, 4055, 4056}, {0, 4056, 4056}, {0, 4056, 4056}}, buffer); EXPECT_EQ(3, num_reserved); EXPECT_EQ(slice1, iovecs[0].mem_); EXPECT_EQ(slice2, iovecs[1].mem_); - const void* slice3 = iovecs[2].mem_; clearReservation(iovecs, num_reserved, buffer); // Append a fragment to the buffer, and then request a small reservation. The buffer // should make a new slice to satisfy the reservation; it cannot safely use any of // the previously seen slices, because they are no longer at the end of the buffer. + expectSlices({{1, 4055, 4056}}, buffer); buffer.addBufferFragment(fragment); EXPECT_EQ(13, buffer.length()); num_reserved = buffer.reserve(1, iovecs, NumIovecs); + expectSlices({{1, 4055, 4056}, {12, 0, 12}, {0, 4056, 4056}}, buffer); EXPECT_EQ(1, num_reserved); EXPECT_NE(slice1, iovecs[0].mem_); - EXPECT_NE(slice2, iovecs[0].mem_); - EXPECT_NE(slice3, iovecs[0].mem_); commitReservation(iovecs, num_reserved, buffer); EXPECT_EQ(14, buffer.length()); } @@ -433,18 +453,20 @@ TEST_P(OwnedImplTest, ReserveCommitReuse) { num_reserved = buffer.reserve(16384, iovecs, NumIovecs); EXPECT_EQ(2, num_reserved); const void* first_slice = iovecs[0].mem_; - const void* second_slice = iovecs[1].mem_; iovecs[0].len_ = 1; + expectSlices({{8000, 4248, 12248}, {0, 12248, 12248}}, buffer); buffer.commit(iovecs, 1); EXPECT_EQ(8001, buffer.length()); + EXPECT_EQ(first_slice, iovecs[0].mem_); + // The second slice is now released because there's nothing in the second slice. + expectSlices({{8001, 4247, 12248}}, buffer); - // Reserve 16KB again, and check whether we get back the uncommitted - // second slice from the previous reservation. + // Reserve 16KB again. num_reserved = buffer.reserve(16384, iovecs, NumIovecs); + expectSlices({{8001, 4247, 12248}, {0, 12248, 12248}}, buffer); EXPECT_EQ(2, num_reserved); EXPECT_EQ(static_cast(first_slice) + 1, static_cast(iovecs[0].mem_)); - EXPECT_EQ(second_slice, iovecs[1].mem_); } TEST_P(OwnedImplTest, ReserveReuse) { @@ -470,6 +492,66 @@ TEST_P(OwnedImplTest, ReserveReuse) { EXPECT_EQ(2, num_reserved); EXPECT_EQ(first_slice, iovecs[0].mem_); EXPECT_EQ(second_slice, iovecs[1].mem_); + expectSlices({{0, 12248, 12248}, {0, 8152, 8152}}, buffer); + + // The remaining tests validate internal manipulations of the new slice + // implementation, so they're not valid for the old evbuffer implementation. + if (buffer.usesOldImpl()) { + return; + } + + // Request a larger reservation, verify that the second entry is replaced with a block with a + // larger size. + num_reserved = buffer.reserve(30000, iovecs, NumIovecs); + const void* third_slice = iovecs[1].mem_; + EXPECT_EQ(2, num_reserved); + EXPECT_EQ(first_slice, iovecs[0].mem_); + EXPECT_EQ(12248, iovecs[0].len_); + EXPECT_NE(second_slice, iovecs[1].mem_); + EXPECT_EQ(30000 - iovecs[0].len_, iovecs[1].len_); + expectSlices({{0, 12248, 12248}, {0, 8152, 8152}, {0, 20440, 20440}}, buffer); + + // Repeating a the reservation request for a smaller block returns the previous entry. + num_reserved = buffer.reserve(16384, iovecs, NumIovecs); + EXPECT_EQ(2, num_reserved); + EXPECT_EQ(first_slice, iovecs[0].mem_); + EXPECT_EQ(second_slice, iovecs[1].mem_); + expectSlices({{0, 12248, 12248}, {0, 8152, 8152}, {0, 20440, 20440}}, buffer); + + // Repeat the larger reservation notice that it doesn't match the prior reservation for 30000 + // bytes. + num_reserved = buffer.reserve(30000, iovecs, NumIovecs); + EXPECT_EQ(2, num_reserved); + EXPECT_EQ(first_slice, iovecs[0].mem_); + EXPECT_EQ(12248, iovecs[0].len_); + EXPECT_NE(second_slice, iovecs[1].mem_); + EXPECT_NE(third_slice, iovecs[1].mem_); + EXPECT_EQ(30000 - iovecs[0].len_, iovecs[1].len_); + expectSlices({{0, 12248, 12248}, {0, 8152, 8152}, {0, 20440, 20440}, {0, 20440, 20440}}, buffer); + + // Commit the most recent reservation and verify the representation. + buffer.commit(iovecs, num_reserved); + expectSlices({{12248, 0, 12248}, {0, 8152, 8152}, {0, 20440, 20440}, {17752, 2688, 20440}}, + buffer); + + // Do another reservation. + num_reserved = buffer.reserve(16384, iovecs, NumIovecs); + EXPECT_EQ(2, num_reserved); + expectSlices({{12248, 0, 12248}, + {0, 8152, 8152}, + {0, 20440, 20440}, + {17752, 2688, 20440}, + {0, 16344, 16344}}, + buffer); + + // And commit. + buffer.commit(iovecs, num_reserved); + expectSlices({{12248, 0, 12248}, + {0, 8152, 8152}, + {0, 20440, 20440}, + {20440, 0, 20440}, + {13696, 2648, 16344}}, + buffer); } TEST_P(OwnedImplTest, Search) { @@ -610,6 +692,29 @@ TEST_P(OwnedImplTest, ReserveZeroCommit) { ASSERT_EQ(::close(pipe_fds[1]), 0); ASSERT_EQ(previous_length, buf.search(data.data(), rc, previous_length)); EXPECT_EQ("bbbbb", buf.toString().substr(0, 5)); + expectSlices({{5, 0, 4056}, {1953, 2103, 4056}}, buf); +} + +TEST_P(OwnedImplTest, ReadReserveAndCommit) { + BufferFragmentImpl frag("", 0, nullptr); + Buffer::OwnedImpl buf; + buf.add("bbbbb"); + + int pipe_fds[2] = {0, 0}; + ASSERT_EQ(::pipe(pipe_fds), 0); + Network::IoSocketHandleImpl io_handle(pipe_fds[0]); + ASSERT_EQ(::fcntl(pipe_fds[0], F_SETFL, O_NONBLOCK), 0); + ASSERT_EQ(::fcntl(pipe_fds[1], F_SETFL, O_NONBLOCK), 0); + + const uint32_t read_length = 32768; + std::string data = "e"; + const ssize_t rc = ::write(pipe_fds[1], data.data(), data.size()); + ASSERT_GT(rc, 0); + Api::IoCallUint64Result result = buf.read(io_handle, read_length); + ASSERT_EQ(result.rc_, static_cast(rc)); + ASSERT_EQ(::close(pipe_fds[1]), 0); + EXPECT_EQ("bbbbbe", buf.toString()); + expectSlices({{6, 4050, 4056}}, buf); } TEST(OverflowDetectingUInt64, Arithmetic) { From b91d7f49af679f28b5e38c06baca1c9187bc04fd Mon Sep 17 00:00:00 2001 From: Yangmin Zhu Date: Wed, 19 Feb 2020 20:05:25 -0800 Subject: [PATCH 09/12] buffer: Force copy when appending small slices to OwnedImpl buffer to avoid fragmentation (#117) (#127) Change OwnedImpl::move to force a copy instead of taking ownership of slices in cases where the offered slices are below kCopyThreshold Risk Level: medium, changes to buffer behavior Testing: Unit Tests Docs Changes: N/A Release Notes: N/A Signed-off-by: Antonio Vicente Signed-off-by: Yangmin Zhu --- docs/root/intro/version_history.rst | 1 + source/common/buffer/buffer_impl.cc | 41 +++++++-- source/common/buffer/buffer_impl.h | 21 +++++ test/common/buffer/owned_impl_test.cc | 88 ++++++++++++++----- .../buffer/zero_copy_input_stream_test.cc | 10 ++- test/common/http/http1/codec_impl_test.cc | 9 +- 6 files changed, 139 insertions(+), 31 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 071ec4f9f9..a52195130c 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -3,6 +3,7 @@ Version history 1.12.3 (Pending) ========================== +* buffer: force copy when appending small slices to OwnedImpl buffer to avoid fragmentation. * listeners: fixed issue where :ref:`TLS inspector listener filter ` could have been bypassed by a client using only TLS 1.3. * sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. * http: added HTTP/1.1 flood protection. Can be temporarily disabled using the runtime feature `envoy.reloadable_features.http1_flood_protection`. diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index fee0fa8900..bc7c6fbff4 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -10,8 +10,15 @@ namespace Envoy { namespace Buffer { - -void OwnedImpl::add(const void* data, uint64_t size) { +namespace { +// This size has been determined to be optimal from running the +// //test/integration:http_benchmark benchmark tests. +// TODO(yanavlasov): This may not be optimal for all hardware configurations or traffic patterns and +// may need to be configurable in the future. +constexpr uint64_t CopyThreshold = 512; +} // namespace + +void OwnedImpl::addImpl(const void* data, uint64_t size) { if (old_impl_) { evbuffer_add(buffer_.get(), data, size); } else { @@ -30,6 +37,8 @@ void OwnedImpl::add(const void* data, uint64_t size) { } } +void OwnedImpl::add(const void* data, uint64_t size) { addImpl(data, size); } + void OwnedImpl::addBufferFragment(BufferFragment& fragment) { if (old_impl_) { evbuffer_add_reference( @@ -309,6 +318,26 @@ void* OwnedImpl::linearize(uint32_t size) { } } +void OwnedImpl::coalesceOrAddSlice(SlicePtr&& other_slice) { + const uint64_t slice_size = other_slice->dataSize(); + // The `other_slice` content can be coalesced into the existing slice IFF: + // 1. The `other_slice` can be coalesced. Objects of type UnownedSlice can not be coalesced. See + // comment in the UnownedSlice class definition; + // 2. There are existing slices; + // 3. The `other_slice` content length is under the CopyThreshold; + // 4. There is enough unused space in the existing slice to accommodate the `other_slice` content. + if (other_slice->canCoalesce() && !slices_.empty() && slice_size < CopyThreshold && + slices_.back()->reservableSize() >= slice_size) { + // Copy content of the `other_slice`. The `move` methods which call this method effectively + // drain the source buffer. + addImpl(other_slice->data(), slice_size); + } else { + // Take ownership of the slice. + slices_.emplace_back(std::move(other_slice)); + length_ += slice_size; + } +} + void OwnedImpl::move(Instance& rhs) { ASSERT(&rhs != this); ASSERT(isSameBufferImpl(rhs)); @@ -328,10 +357,9 @@ void OwnedImpl::move(Instance& rhs) { OwnedImpl& other = static_cast(rhs); while (!other.slices_.empty()) { const uint64_t slice_size = other.slices_.front()->dataSize(); - slices_.emplace_back(std::move(other.slices_.front())); - other.slices_.pop_front(); - length_ += slice_size; + coalesceOrAddSlice(std::move(other.slices_.front())); other.length_ -= slice_size; + other.slices_.pop_front(); } other.postProcess(); } @@ -361,9 +389,8 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { other.slices_.front()->drain(copy_size); other.length_ -= copy_size; } else { - slices_.emplace_back(std::move(other.slices_.front())); + coalesceOrAddSlice(std::move(other.slices_.front())); other.slices_.pop_front(); - length_ += slice_size; other.length_ -= slice_size; } length -= copy_size; diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 36715ed42d..77d5f58f7f 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -188,6 +188,11 @@ class Slice { return SliceRepresentation{dataSize(), reservableSize(), capacity_}; } + /** + * @return true if content in this Slice can be coalesced into another Slice. + */ + virtual bool canCoalesce() const { return true; } + protected: Slice(uint64_t data, uint64_t reservable, uint64_t capacity) : data_(data), reservable_(reservable), capacity_(capacity) {} @@ -415,6 +420,13 @@ class UnownedSlice : public Slice { ~UnownedSlice() override { fragment_.done(); } + /** + * BufferFragment objects encapsulated by UnownedSlice are used to track when response content + * is written into transport connection. As a result these slices can not be coalesced when moved + * between buffers. + */ + bool canCoalesce() const override { return false; } + private: BufferFragment& fragment_; }; @@ -570,6 +582,15 @@ class OwnedImpl : public LibEventInstance { */ bool isSameBufferImpl(const Instance& rhs) const; + void addImpl(const void* data, uint64_t size); + + /** + * Moves contents of the `other_slice` by either taking its ownership or coalescing it + * into an existing slice. + * NOTE: the caller is responsible for draining the buffer that contains the `other_slice`. + */ + void coalesceOrAddSlice(SlicePtr&& other_slice); + /** Whether to use the old evbuffer implementation when constructing new OwnedImpl objects. */ static bool use_old_impl_; diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 3400e6334d..6e744a9e41 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -65,20 +65,20 @@ TEST_P(OwnedImplTest, AddBufferFragmentNoCleanup) { } TEST_P(OwnedImplTest, AddBufferFragmentWithCleanup) { - char input[] = "hello world"; - BufferFragmentImpl frag(input, 11, [this](const void*, size_t, const BufferFragmentImpl*) { - release_callback_called_ = true; - }); + std::string input(2048, 'a'); + BufferFragmentImpl frag( + input.c_str(), input.size(), + [this](const void*, size_t, const BufferFragmentImpl*) { release_callback_called_ = true; }); Buffer::OwnedImpl buffer; verifyImplementation(buffer); buffer.addBufferFragment(frag); - EXPECT_EQ(11, buffer.length()); + EXPECT_EQ(2048, buffer.length()); - buffer.drain(5); - EXPECT_EQ(6, buffer.length()); + buffer.drain(2000); + EXPECT_EQ(48, buffer.length()); EXPECT_FALSE(release_callback_called_); - buffer.drain(6); + buffer.drain(48); EXPECT_EQ(0, buffer.length()); EXPECT_TRUE(release_callback_called_); } @@ -102,12 +102,12 @@ TEST_P(OwnedImplTest, AddEmptyFragment) { } TEST_P(OwnedImplTest, AddBufferFragmentDynamicAllocation) { - char input_stack[] = "hello world"; - char* input = new char[11]; - std::copy(input_stack, input_stack + 11, input); + std::string input_str(2048, 'a'); + char* input = new char[2048]; + std::copy(input_str.c_str(), input_str.c_str() + 11, input); BufferFragmentImpl* frag = new BufferFragmentImpl( - input, 11, [this](const void* data, size_t, const BufferFragmentImpl* frag) { + input, 2048, [this](const void* data, size_t, const BufferFragmentImpl* frag) { release_callback_called_ = true; delete[] static_cast(data); delete frag; @@ -116,9 +116,9 @@ TEST_P(OwnedImplTest, AddBufferFragmentDynamicAllocation) { Buffer::OwnedImpl buffer; verifyImplementation(buffer); buffer.addBufferFragment(*frag); - EXPECT_EQ(11, buffer.length()); + EXPECT_EQ(2048, buffer.length()); - buffer.drain(5); + buffer.drain(2042); EXPECT_EQ(6, buffer.length()); EXPECT_FALSE(release_callback_called_); @@ -128,10 +128,10 @@ TEST_P(OwnedImplTest, AddBufferFragmentDynamicAllocation) { } TEST_P(OwnedImplTest, AddOwnedBufferFragmentWithCleanup) { - char input[] = "hello world"; - const size_t expected_length = sizeof(input) - 1; + std::string input(2048, 'a'); + const size_t expected_length = input.size(); auto frag = OwnedBufferFragmentImpl::create( - {input, expected_length}, + {input.c_str(), expected_length}, [this](const OwnedBufferFragmentImpl*) { release_callback_called_ = true; }); Buffer::OwnedImpl buffer; verifyImplementation(buffer); @@ -150,10 +150,10 @@ TEST_P(OwnedImplTest, AddOwnedBufferFragmentWithCleanup) { // Verify that OwnedBufferFragment work correctly when input buffer is allocated on the heap. TEST_P(OwnedImplTest, AddOwnedBufferFragmentDynamicAllocation) { - char input_stack[] = "hello world"; - const size_t expected_length = sizeof(input_stack) - 1; + std::string input_str(2048, 'a'); + const size_t expected_length = input_str.size(); char* input = new char[expected_length]; - std::copy(input_stack, input_stack + expected_length, input); + std::copy(input_str.c_str(), input_str.c_str() + expected_length, input); auto* frag = OwnedBufferFragmentImpl::create({input, expected_length}, [this, input](const OwnedBufferFragmentImpl* frag) { @@ -730,6 +730,54 @@ TEST(OverflowDetectingUInt64, Arithmetic) { EXPECT_DEATH(length += 1, "overflow"); } +void TestBufferMove(uint64_t buffer1_length, uint64_t buffer2_length, + uint64_t expected_slice_count) { + Buffer::OwnedImpl buffer1; + buffer1.add(std::string(buffer1_length, 'a')); + EXPECT_EQ(1, buffer1.getRawSlices(nullptr, 0)); + + Buffer::OwnedImpl buffer2; + buffer2.add(std::string(buffer2_length, 'b')); + EXPECT_EQ(1, buffer2.getRawSlices(nullptr, 0)); + + buffer1.move(buffer2); + EXPECT_EQ(expected_slice_count, buffer1.getRawSlices(nullptr, 0)); + EXPECT_EQ(buffer1_length + buffer2_length, buffer1.length()); + // Make sure `buffer2` was drained. + EXPECT_EQ(0, buffer2.length()); +} + +// Slice size large enough to prevent slice content from being coalesced into an existing slice +constexpr uint64_t kLargeSliceSize = 2048; + +TEST(OwnedImplTest, MoveBuffersWithLargeSlices) { + // Large slices should not be coalesced together + TestBufferMove(kLargeSliceSize, kLargeSliceSize, 2); +} + +TEST(OwnedImplTest, MoveBuffersWithSmallSlices) { + // Small slices should be coalesced together + TestBufferMove(1, 1, 1); +} + +TEST(OwnedImplTest, MoveSmallSliceIntoLargeSlice) { + // Small slices should be coalesced with a large one + TestBufferMove(kLargeSliceSize, 1, 1); +} + +TEST(OwnedImplTest, MoveLargeSliceIntoSmallSlice) { + // Large slice should NOT be coalesced into the small one + TestBufferMove(1, kLargeSliceSize, 2); +} + +TEST(OwnedImplTest, MoveSmallSliceIntoNotEnoughFreeSpace) { + // Small slice will not be coalesced if a previous slice does not have enough free space + // Slice buffer sizes are allocated in 4Kb increments + // Make first slice have 127 of free space (it is actually less as there is small overhead of the + // OwnedSlice object) And second slice 128 bytes + TestBufferMove(4096 - 127, 128, 2); +} + } // namespace } // namespace Buffer } // namespace Envoy diff --git a/test/common/buffer/zero_copy_input_stream_test.cc b/test/common/buffer/zero_copy_input_stream_test.cc index bd747ed20a..055f1050b6 100644 --- a/test/common/buffer/zero_copy_input_stream_test.cc +++ b/test/common/buffer/zero_copy_input_stream_test.cc @@ -3,6 +3,7 @@ #include "test/common/buffer/utility.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" namespace Envoy { @@ -39,7 +40,9 @@ TEST_P(ZeroCopyInputStreamTest, Next) { } TEST_P(ZeroCopyInputStreamTest, TwoSlices) { - Buffer::OwnedImpl buffer("efgh"); + // Make content larger than 512 bytes so it would not be coalesced when + // moved into the stream_ buffer. + Buffer::OwnedImpl buffer(std::string(1024, 'A')); verifyImplementation(buffer); stream_.move(buffer); @@ -48,8 +51,9 @@ TEST_P(ZeroCopyInputStreamTest, TwoSlices) { EXPECT_EQ(4, size_); EXPECT_EQ(0, memcmp(slice_data_.data(), data_, size_)); EXPECT_TRUE(stream_.Next(&data_, &size_)); - EXPECT_EQ(4, size_); - EXPECT_EQ(0, memcmp("efgh", data_, size_)); + EXPECT_EQ(1024, size_); + EXPECT_THAT(absl::string_view(static_cast(data_), size_), + testing::Each(testing::AllOf('A'))); } TEST_P(ZeroCopyInputStreamTest, BackUp) { diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 7f5a404cdb..78e7b0acf6 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -24,6 +24,7 @@ using testing::_; using testing::InSequence; using testing::Invoke; +using testing::InvokeWithoutArgs; using testing::NiceMock; using testing::Return; using testing::ReturnRef; @@ -840,7 +841,13 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedResponse) { EXPECT_EQ(0U, buffer.length()); std::string output; - ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + ON_CALL(connection_, write(_, _)).WillByDefault(Invoke([&output](Buffer::Instance& data, bool) { + // Verify that individual writes into the codec's output buffer were coalesced into a single + // slice + ASSERT_EQ(1, data.getRawSlices(nullptr, 0)); + output.append(data.toString()); + data.drain(data.length()); + })); TestHeaderMapImpl headers{{":status", "200"}}; response_encoder->encodeHeaders(headers, false); From cf0f50b960c00783a867aea8bad0fec31489f51c Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 12 Mar 2020 14:10:23 -0700 Subject: [PATCH 10/12] Updated bssl_wrapper to latest version --- bazel/repository_locations.bzl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index e551efe20d..cb4eaf4ee8 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -23,9 +23,9 @@ REPOSITORY_LOCATIONS = dict( ), #EXTERNAL OPENSSL bssl_wrapper = dict( - sha256 = "81a59d013096015a93269325ee4148d826ffd7a9f019f622850a2b86974b9748", - strip_prefix = "bssl_wrapper-2eaed8832e12a0fada8f08a5e23522e035b80784", - urls = ["https://github.com/maistra/bssl_wrapper/archive/2eaed8832e12a0fada8f08a5e23522e035b80784.tar.gz"], + sha256 = "d84ea7d190210145695e5b172e8e6fb23f3464360da5efab5a1ae1a973c21f57", + strip_prefix = "bssl_wrapper-c9649facde3ab1d8bc871c7375a8946c50950e97", + urls = ["https://github.com/maistra/bssl_wrapper/archive/c9649facde3ab1d8bc871c7375a8946c50950e97.tar.gz"], ), #EXTERNAL OPENSSL openssl_cbs = dict( From 8ccac219c4091e6e60fc0c09ed6d00285071104b Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 12 Mar 2020 10:49:01 -0700 Subject: [PATCH 11/12] Fixing alpn detection in tls_inspector --- bazel/external/openssl_includes-1.patch | 13 +++ bazel/external/openssl_includes.BUILD | 18 +++++ bazel/repositories.bzl | 16 ++++ bazel/repository_locations.bzl | 5 ++ .../filters/listener/tls_inspector/BUILD | 18 +---- .../listener/tls_inspector/openssl_impl.cc | 13 --- .../listener/tls_inspector/openssl_impl.h | 5 -- .../listener/tls_inspector/tls_inspector.cc | 79 +++++++------------ .../listener/tls_inspector/tls_inspector.h | 3 +- .../filters/listener/tls_inspector/BUILD | 1 - .../tls_inspector/tls_inspector_test.cc | 4 +- .../listener/tls_inspector/tls_utility.cc | 8 +- 12 files changed, 88 insertions(+), 95 deletions(-) create mode 100644 bazel/external/openssl_includes-1.patch create mode 100644 bazel/external/openssl_includes.BUILD diff --git a/bazel/external/openssl_includes-1.patch b/bazel/external/openssl_includes-1.patch new file mode 100644 index 0000000000..a86a2820f3 --- /dev/null +++ b/bazel/external/openssl_includes-1.patch @@ -0,0 +1,13 @@ +diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h +index 860360b8b2..49c719285f 100644 +--- a/ssl/packet_locl.h ++++ b/ssl/packet_locl.h +@@ -426,7 +426,7 @@ __owur static ossl_inline int PACKET_memdup(const PACKET *pkt, + if (length == 0) + return 1; + +- *data = OPENSSL_memdup(pkt->curr, length); ++ *data = (unsigned char *)OPENSSL_memdup(pkt->curr, length); + if (*data == NULL) + return 0; + diff --git a/bazel/external/openssl_includes.BUILD b/bazel/external/openssl_includes.BUILD new file mode 100644 index 0000000000..60224a951b --- /dev/null +++ b/bazel/external/openssl_includes.BUILD @@ -0,0 +1,18 @@ +cc_library( + name = "openssl_includes_lib", + copts = ["-Wno-error=error"], + hdrs = [ + "e_os.h", + "ssl/ssl_locl.h", + "ssl/packet_locl.h", + "ssl/record/record.h", + "ssl/statem/statem.h", + "include/internal/dane.h", + "include/internal/nelem.h", + "include/internal/numbers.h", + "include/internal/refcount.h", + "include/internal/tsan_assist.h", + ], + includes = ["ssl", "ssl/record", "ssl/statem", "include",], + visibility = ["//visibility:public"], +) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index c3a2ba1eab..8c32745b41 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -109,6 +109,7 @@ def envoy_dependencies(skip_targets = []): # EXTERNAL OPENSSL _openssl() + _openssl_includes() _bssl_wrapper() _openssl_cbs() @@ -182,6 +183,21 @@ def _openssl(): actual = "@openssl//:openssl-lib", ) +def _openssl_includes(): + _repository_impl( + name = "com_github_openssl_openssl", + build_file = "@envoy//bazel/external:openssl_includes.BUILD", + patches = [ + "@envoy//bazel/external:openssl_includes-1.patch", + ], + patch_args = ["-p1"], + ) + native.bind( + name = "openssl_includes_lib", + actual = "@com_github_openssl_openssl//:openssl_includes_lib", +) + + #EXTERNAL OPENSSL def _bssl_wrapper(): _repository_impl("bssl_wrapper") diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index cb4eaf4ee8..da71422b38 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -21,6 +21,11 @@ REPOSITORY_LOCATIONS = dict( strip_prefix = "envoy-build-tools-a6b28555badcb18d6be924c8fc1bea49971656b8", urls = ["https://github.com/envoyproxy/envoy-build-tools/archive/a6b28555badcb18d6be924c8fc1bea49971656b8.tar.gz"], ), + com_github_openssl_openssl = dict( + sha256 = "cf26f056a955cff721d3a3c08d8126d1e4f69803e08c9600dac3b6b7158586d6", + strip_prefix = "openssl-894da2fb7ed5d314ee5c2fc9fd2d9b8b74111596", + urls = ["https://github.com/openssl/openssl/archive/894da2fb7ed5d314ee5c2fc9fd2d9b8b74111596.tar.gz"], + ), #EXTERNAL OPENSSL bssl_wrapper = dict( sha256 = "d84ea7d190210145695e5b172e8e6fb23f3464360da5efab5a1ae1a973c21f57", diff --git a/source/extensions/filters/listener/tls_inspector/BUILD b/source/extensions/filters/listener/tls_inspector/BUILD index 95eab69e99..f02069946a 100644 --- a/source/extensions/filters/listener/tls_inspector/BUILD +++ b/source/extensions/filters/listener/tls_inspector/BUILD @@ -18,9 +18,9 @@ envoy_cc_library( external_deps = [ "ssl", "bssl_wrapper_lib", + "openssl_includes_lib", ], deps = [ - ":openssl_impl_lib", "//include/envoy/event:dispatcher_interface", "//include/envoy/event:timer_interface", "//include/envoy/network:filter_interface", @@ -41,18 +41,4 @@ envoy_cc_library( "//source/extensions/filters/listener:well_known_names", "//source/extensions/filters/listener/tls_inspector:tls_inspector_lib", ], -) - -envoy_cc_library( - name = "openssl_impl_lib", - srcs = [ - "openssl_impl.cc", - ], - hdrs = [ - "openssl_impl.h", - ], - external_deps = [ - "ssl", - "bssl_wrapper_lib", - ], -) +) \ No newline at end of file diff --git a/source/extensions/filters/listener/tls_inspector/openssl_impl.cc b/source/extensions/filters/listener/tls_inspector/openssl_impl.cc index 6bb9ceb942..3a76a0d7eb 100644 --- a/source/extensions/filters/listener/tls_inspector/openssl_impl.cc +++ b/source/extensions/filters/listener/tls_inspector/openssl_impl.cc @@ -17,35 +17,22 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { -const SSL_METHOD* TLS_with_buffers_method(void) { return TLS_method(); } - -void set_certificate_cb(SSL_CTX* ctx) { - auto cert_cb = [](SSL* ssl, void* arg) -> int { return 0; }; - SSL_CTX_set_cert_cb(ctx, cert_cb, ctx); -} - std::vector getAlpnProtocols(const unsigned char* data, unsigned int len) { std::vector protocols; absl::string_view str(reinterpret_cast(data)); for (int i = 0; i < len;) { - uint32_t protocol_length = 0; protocol_length <<= 8; protocol_length |= data[i]; - ++i; - absl::string_view protocol(str.substr(i, protocol_length)); protocols.push_back(protocol); - i += protocol_length; } return protocols; } -int getServernameCallbackReturn(int* out_alert) { return SSL_TLSEXT_ERR_OK; } - } // namespace TlsInspector } // namespace ListenerFilters } // namespace Extensions diff --git a/source/extensions/filters/listener/tls_inspector/openssl_impl.h b/source/extensions/filters/listener/tls_inspector/openssl_impl.h index ef7c2c0b14..5c397590f9 100644 --- a/source/extensions/filters/listener/tls_inspector/openssl_impl.h +++ b/source/extensions/filters/listener/tls_inspector/openssl_impl.h @@ -16,12 +16,7 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { -const SSL_METHOD* TLS_with_buffers_method(); - -void set_certificate_cb(SSL_CTX* ctx); - std::vector getAlpnProtocols(const unsigned char* data, unsigned int len); -int getServernameCallbackReturn(int* out_alert); } // namespace TlsInspector } // namespace ListenerFilters diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index ff0b8e2c91..a49ee920d2 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -14,10 +14,10 @@ #include "common/api/os_sys_calls_impl.h" #include "common/common/assert.h" -#include "extensions/filters/listener/tls_inspector/openssl_impl.h" #include "extensions/transport_sockets/well_known_names.h" #include "openssl/ssl.h" +#include "ssl/ssl_locl.h" namespace Envoy { namespace Extensions { @@ -31,7 +31,7 @@ const unsigned Config::TLS_MAX_SUPPORTED_VERSION = TLS1_3_VERSION; Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) : stats_{ALL_TLS_INSPECTOR_STATS(POOL_COUNTER_PREFIX(scope, "tls_inspector."))}, ssl_ctx_( - SSL_CTX_new(Envoy::Extensions::ListenerFilters::TlsInspector::TLS_with_buffers_method())), + SSL_CTX_new(TLS_method())), max_client_hello_size_(max_client_hello_size) { if (max_client_hello_size_ > TLS_MAX_CLIENT_HELLO) { @@ -44,48 +44,24 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); - Envoy::Extensions::ListenerFilters::TlsInspector::set_certificate_cb(ssl_ctx_.get()); - - /* - * MAISTRA - * Application protocol hack part 1. When the endpoint is determined to be TLS then the "istio" application protocol is required in order to enable the - * proper TLS filter. BoringSSL has the ability to peek ahead to obtain information during the SSL handshake that is not supposed to be available until - * a later callback. During the certificate callback BoringSSL peeks head and obtains the application protocols that is only available during the later - * ALPN callback in OpenSSL. The BoringSSL code then terminates the filter before the ALPN callback is ever called. BoringSSL can get away with this since - * it has this peek ahead functionality not available in OpenSSL. The following hack simulates this behavior: when there is a TLS endpoint the endpoint - * hostname will contain "outbound_" so that is used as a trigger to add the "istio" application protocol. If the endpoint is not TLS then the servername - * will not contain "outbound_". One issue, of course, is that if the actual endpoint servername contains "outbound_" then this logic will incorrectly - * identify the endpoint as TLS. - */ auto tlsext_servername_cb = +[](SSL* ssl, int* out_alert, void* arg) -> int { Filter* filter = static_cast(SSL_get_app_data(ssl)); absl::string_view servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); filter->onServername(servername); - if (servername.rfind("outbound_") != std::string::npos) { - filter->setIstioApplicationProtocol(); - } - - return Envoy::Extensions::ListenerFilters::TlsInspector::getServernameCallbackReturn(out_alert); - }; - SSL_CTX_set_tlsext_servername_callback(ssl_ctx_.get(), tlsext_servername_cb); - - auto alpn_cb = [](SSL* ssl, const unsigned char** out, unsigned char* outlen, - const unsigned char* in, unsigned int inlen, void* arg) -> int { - Filter* filter = static_cast(SSL_get_app_data(ssl)); - filter->onALPN(in, inlen); - + *out_alert = SSL_AD_USER_CANCELLED; return SSL_TLSEXT_ERR_OK; }; - SSL_CTX_set_alpn_select_cb(ssl_ctx_.get(), alpn_cb, nullptr); + SSL_CTX_set_tlsext_servername_callback(ssl_ctx_.get(), tlsext_servername_cb); auto cert_cb = [](SSL* ssl, void* arg) -> int { Filter* filter = static_cast(SSL_get_app_data(ssl)); - filter->onCert(); + // TODO (dmitri-d) move access to SSL internals into bssl_wrapper + filter->onALPN(ssl->s3->alpn_proposed, ssl->s3->alpn_proposed_len); - return SSL_TLSEXT_ERR_OK; + // Return an error to stop the handshake; we have what we wanted already. + return 0; }; SSL_CTX_set_cert_cb(ssl_ctx_.get(), cert_cb, nullptr); - } bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ctx_.get())}; } @@ -148,23 +124,12 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { } void Filter::onALPN(const unsigned char* data, unsigned int len) { - std::vector protocols = - Envoy::Extensions::ListenerFilters::TlsInspector::getAlpnProtocols(data, len); - cb_->socket().setRequestedApplicationProtocols(protocols); - alpn_found_ = true; -} - -/* - * MAISTRA - * Application protocol hack part 2. If determined that the "istio" application protocol needs to be added (i.e. endpoint is TLS) - * then add it to the application protocol list as part of the certificate callback. - */ -void Filter::onCert() { - std::vector protocols; - if (istio_protocol_required_) { - protocols.emplace_back("istio"); + std::vector protocols = getAlpnProtocols(data, len); + if (protocols.empty()) { + return; } cb_->socket().setRequestedApplicationProtocols(protocols); + alpn_found_ = true; } void Filter::onServername(absl::string_view name) { @@ -178,10 +143,6 @@ void Filter::onServername(absl::string_view name) { clienthello_success_ = true; } -void Filter::setIstioApplicationProtocol() { - istio_protocol_required_ = true; -} - ParseState Filter::onRead() { // This receive code is somewhat complicated, because it must be done as a MSG_PEEK because @@ -268,6 +229,22 @@ ParseState Filter::parseClientHello(const void* data, size_t len) { } } +std::vector Filter::getAlpnProtocols(const unsigned char* data, unsigned int len) { + std::vector protocols; + absl::string_view str(reinterpret_cast(data)); + for (int i = 0; i < len;) { + uint32_t protocol_length = 0; + protocol_length <<= 8; + protocol_length |= data[i]; + ++i; + absl::string_view protocol(str.substr(i, protocol_length)); + protocols.push_back(protocol); + i += protocol_length; + } + + return protocols; +} + } // namespace TlsInspector } // namespace ListenerFilters } // namespace Extensions diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index 2e73a00723..63ea54d11c 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -83,12 +83,12 @@ class Filter : public Network::ListenerFilter, Logger::Loggable getAlpnProtocols(const unsigned char* data, unsigned int len); ParseState parseClientHello(const void* data, size_t len); ParseState onRead(); void onTimeout(); void done(bool success); void onServername(absl::string_view name); - void setIstioApplicationProtocol(); ConfigSharedPtr config_; Network::ListenerFilterCallbacks* cb_; @@ -98,7 +98,6 @@ class Filter : public Network::ListenerFilter, Logger::Loggable ssl_; uint64_t read_{0}; bool alpn_found_{false}; - bool istio_protocol_required_{false}; bool clienthello_success_{false}; static thread_local uint8_t buf_[Config::TLS_MAX_CLIENT_HELLO]; diff --git a/test/extensions/filters/listener/tls_inspector/BUILD b/test/extensions/filters/listener/tls_inspector/BUILD index ed3663b246..d936cd25c8 100644 --- a/test/extensions/filters/listener/tls_inspector/BUILD +++ b/test/extensions/filters/listener/tls_inspector/BUILD @@ -51,6 +51,5 @@ envoy_cc_library( ], deps = [ "//source/common/common:assert_lib", - "//source/extensions/filters/listener/tls_inspector:openssl_impl_lib", ], ) diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index 24c2a621a9..d62ef07a0f 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -116,7 +116,7 @@ TEST_P(TlsInspectorTest, SniRegistered) { })); EXPECT_CALL(socket_, setRequestedServerName(Eq(servername))); // Not valid for ALPN "istio" application protocol hack for openssl - EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(1); + EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(0); EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls"))); EXPECT_CALL(cb_, continueFilterChain(true)); file_event_callback_(Event::FileReadyType::Read); @@ -202,7 +202,7 @@ TEST_P(TlsInspectorTest, NoExtensions) { })); EXPECT_CALL(socket_, setRequestedServerName(_)).Times(0); // 0 times not valid for ALPN "istio" application protocol hack for openssl - EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(1); + EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(0); EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls"))); EXPECT_CALL(cb_, continueFilterChain(true)); file_event_callback_(Event::FileReadyType::Read); diff --git a/test/extensions/filters/listener/tls_inspector/tls_utility.cc b/test/extensions/filters/listener/tls_inspector/tls_utility.cc index 4e460d6579..a4d5b3f96d 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_utility.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_utility.cc @@ -2,8 +2,6 @@ #include "common/common/assert.h" -#include "extensions/filters/listener/tls_inspector/openssl_impl.h" - #include "bssl_wrapper/bssl_wrapper.h" #include "openssl/ssl.h" @@ -12,9 +10,9 @@ namespace Tls { namespace Test { std::vector generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version, - const std::string& sni_name, const std::string& alpn) { - bssl::UniquePtr ctx( - SSL_CTX_new(Envoy::Extensions::ListenerFilters::TlsInspector::TLS_with_buffers_method())); + const std::string& sni_name, const std::string& alpn) { + // TODO (dmitri-d) add an implementation of TLS_with_buffers_method to bssl_wrapper + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); SSL_CTX_set_min_proto_version(ctx.get(), tls_min_version); SSL_CTX_set_max_proto_version(ctx.get(), tls_max_version); From 7f1eb1e5049e879ad4d0c6fa22478954da680cd4 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 12 Mar 2020 16:19:25 -0700 Subject: [PATCH 12/12] Removed no longer relevant comments from tls_inspector_test --- .../filters/listener/tls_inspector/tls_inspector_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index d62ef07a0f..3247ede588 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -115,7 +115,6 @@ TEST_P(TlsInspectorTest, SniRegistered) { return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0}; })); EXPECT_CALL(socket_, setRequestedServerName(Eq(servername))); - // Not valid for ALPN "istio" application protocol hack for openssl EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(0); EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls"))); EXPECT_CALL(cb_, continueFilterChain(true)); @@ -201,7 +200,6 @@ TEST_P(TlsInspectorTest, NoExtensions) { return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0}; })); EXPECT_CALL(socket_, setRequestedServerName(_)).Times(0); - // 0 times not valid for ALPN "istio" application protocol hack for openssl EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(0); EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls"))); EXPECT_CALL(cb_, continueFilterChain(true));