From ffa8e149fd7916aa25df82e3d6b6806486c40ac1 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 1 Apr 2020 14:34:44 -0400 Subject: [PATCH 1/6] http: fixing upgrade response bug Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 2 ++ source/common/http/conn_manager_utility.cc | 4 +++ source/common/runtime/runtime_features.cc | 1 + test/common/http/conn_manager_utility_test.cc | 34 +++++++++++++++++++ 4 files changed, 41 insertions(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 90641ec226c6d..6591da268ef53 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -26,6 +26,8 @@ Version history * dns: added support for :ref:`dns_failure_refresh_rate ` for the :ref:`dns cache ` to set the DNS refresh rate during failures. * http filters: http filter extensions use the "envoy.filters.http" name space. A mapping of extension names is available in the :ref:`deprecated ` documentation. +* http: fixing a bug where the upgrade header was not cleared on responses to non-upgrade requests. + Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false. * ext_authz: disabled the use of lowercase string matcher for headers matching in HTTP-based `ext_authz`. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher` to false. * fault: added support for controlling abort faults with :ref:`HTTP header fault configuration ` to the HTTP fault filter. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 33574f66ba9bc..a7ed6ab2febfd 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -376,7 +376,11 @@ void ConnectionManagerUtility::mutateResponseHeaders( } } else { response_headers.removeConnection(); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fix_upgrade_response")) { + response_headers.removeUpgrade(); + } } + response_headers.removeTransferEncoding(); if (request_headers != nullptr && request_headers->EnvoyForceTrace()) { diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index a78868b3ff3cb..6870883358459 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -35,6 +35,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.new_http2_connection_pool_behavior", "envoy.deprecated_features.allow_deprecated_extension_names", "envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher", + "envoy.reloadable_features.fix_upgrade_response", }; // This is a section for officially sanctioned runtime features which are too diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 1e00a5ce10671..d2a5c0442c25f 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -790,6 +790,40 @@ TEST_F(ConnectionManagerUtilityTest, ClearUpgradeHeadersForNonUpgradeRequests) { EXPECT_EQ("custom_value", response_headers.get_("custom_header")); } + // Test with the request headers not valid upgrade headers + { + TestRequestHeaderMapImpl request_headers{{"upgrade", "foo"}}; + TestResponseHeaderMapImpl response_headers{{"connection", "upgrade"}, + {"transfer-encoding", "eep"}, + {"upgrade", "foo"}, + {"custom_header", "custom_value"}}; + EXPECT_FALSE(Utility::isUpgrade(request_headers)); + EXPECT_TRUE(Utility::isUpgrade(response_headers)); + ConnectionManagerUtility::mutateResponseHeaders(response_headers, &request_headers, + config_.requestIDExtension(), ""); + + EXPECT_EQ(1UL, response_headers.size()) << response_headers; + EXPECT_EQ("custom_value", response_headers.get_("custom_header")); + } + + // Test with the response headers not valid upgrade headers + { + TestRequestHeaderMapImpl request_headers{{"connection", "UpGrAdE"}, {"upgrade", "foo"}}; + TestResponseHeaderMapImpl response_headers{{"transfer-encoding", "foo"}, {"upgrade", "bar"}}; + EXPECT_TRUE(Utility::isUpgrade(request_headers)); + EXPECT_FALSE(Utility::isUpgrade(response_headers)); + ConnectionManagerUtility::mutateResponseHeaders(response_headers, &request_headers, + config_.requestIDExtension(), ""); + + EXPECT_EQ(0UL, response_headers.size()) << response_headers; + } +} + +TEST_F(ConnectionManagerUtilityTest, ClearUpgradeHeadersForNonUpgradeRequestsLegacy) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.fix_upgrade_response", "false"}}); + // Test with the request headers not valid upgrade headers { TestRequestHeaderMapImpl request_headers{{"upgrade", "foo"}}; From 96557c02e20a575ad5a900054779b3971006da4b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 1 Apr 2020 16:58:58 -0400 Subject: [PATCH 2/6] fixing both sides Signed-off-by: Alyssa Wilk --- source/common/http/http1/codec_impl.cc | 12 ++++++++++- source/common/http/http1/codec_impl.h | 10 +++++++++ test/common/http/http1/codec_impl_test.cc | 12 +++++++++-- test/integration/integration_test.cc | 25 +++++++++++++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 7312f322cae3c..a3a50560f6c43 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -357,6 +357,9 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end if (method->value() == Headers::get().MethodValues.Head) { head_request_ = true; } + if (Utility::isUpgrade(headers)) { + upgrade_request_ = true; + } connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); @@ -565,7 +568,7 @@ int ConnectionImpl::onHeadersCompleteBase() { protocol_ = Protocol::Http10; } RequestOrResponseHeaderMap& request_or_response_headers = requestOrResponseHeaders(); - if (Utility::isUpgrade(request_or_response_headers)) { + if (Utility::isUpgrade(request_or_response_headers) && upgradeAllowed()) { // Ignore h2c upgrade requests until we support them. // See https://github.com/envoyproxy/envoy/issues/7161 for details. if (request_or_response_headers.Upgrade() && @@ -949,6 +952,13 @@ int ClientConnectionImpl::onHeadersComplete() { return cannotHaveBody() ? 1 : 0; } +bool ClientConnectionImpl::upgradeAllowed() const { + if (pending_response_.has_value()) { + return pending_response_->encoder_.upgrade_request_; + } + return false; +} + void ClientConnectionImpl::onBody(const char* data, size_t length) { ASSERT(!deferred_end_stream_headers_); if (pending_response_.has_value()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 60f15e10df10c..5224fdfdabf68 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -145,6 +145,8 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; void encodeTrailers(const RequestTrailerMap& trailers) override { encodeTrailersBase(trailers); } + bool upgrade_request_{}; + private: bool head_request_{}; }; @@ -283,6 +285,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + TestRequestHeaderMapImpl headers{{":method", "GET"}, + {":path", "/"}, + {":authority", "host"}, + {"connection", "upgrade"}, + {"upgrade", "websocket"}}; request_encoder.encodeHeaders(headers, true); // Send upgrade headers @@ -1649,7 +1653,11 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponseWithEarlyData) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + TestRequestHeaderMapImpl headers{{":method", "GET"}, + {":path", "/"}, + {":authority", "host"}, + {"connection", "upgrade"}, + {"upgrade", "websocket"}}; request_encoder.encodeHeaders(headers, true); // Send upgrade headers diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 92908225fec6b..40f86d1b72c53 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1227,4 +1227,29 @@ TEST_P(IntegrationTest, TestManyBadRequests) { EXPECT_EQ(0, test_server_->counter("http1.response_flood")->value()); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10566 +TEST_P(IntegrationTest, TestUpgradeHeaderInResponse) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); + ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 200 OK\r\n" + "connection: upgrade\r\n" + "upgrade: h2\r\n" + "Transfer-encoding: chunked\r\n\r\n" + "b\r\nHello World\r\n0\r\n\r\n", + false)); + + response->waitForHeaders(); + EXPECT_EQ(nullptr, response->headers().Upgrade()); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("Hello World", response->body()); + cleanupUpstreamAndDownstream(); +} + } // namespace Envoy From 50ab26ee1d601fe4b98b6db37d20fd090514353b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 2 Apr 2020 09:23:41 -0400 Subject: [PATCH 3/6] reviewer comments! Signed-off-by: Alyssa Wilk --- test/common/http/http1/codec_impl_test.cc | 9 +++++++++ test/integration/integration_test.cc | 1 + 2 files changed, 10 insertions(+) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 3990b75949e6e..580c666763602 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1611,6 +1611,15 @@ TEST_F(Http1ClientConnectionImplTest, GiantPath) { codec_->dispatch(response); } +TEST_F(Http1ClientConnectionImplTest, PrematureUpgradeResponse) { + initialize(); + + // make sure upgradeAllowed doesn't cause crashes if run with no pending response. + Buffer::OwnedImpl response( + "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); + EXPECT_THROW(codec_->dispatch(response), PrematureResponseException); +} + TEST_F(Http1ClientConnectionImplTest, UpgradeResponse) { initialize(); diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 40f86d1b72c53..8333b5c830577 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1246,6 +1246,7 @@ TEST_P(IntegrationTest, TestUpgradeHeaderInResponse) { response->waitForHeaders(); EXPECT_EQ(nullptr, response->headers().Upgrade()); + EXPECT_EQ(nullptr, response->headers().Connection()); response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("Hello World", response->body()); From 90f7ab1064896a5aeaeb45b3609e4b012f9ac178 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 8 Apr 2020 09:52:48 -0400 Subject: [PATCH 4/6] alpha Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 0db9b77755c75..d61bda56b9f87 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -21,10 +21,10 @@ Version history * datasource: added retry policy for remote async data source. * dns: the STRICT_DNS cluster now only resolves to 0 hosts if DNS resolution successfully returns 0 hosts. * dns: added support for :ref:`dns_failure_refresh_rate ` for the :ref:`dns cache ` to set the DNS refresh rate during failures. -* http filters: http filter extensions use the "envoy.filters.http" name space. A mapping - of extension names is available in the :ref:`deprecated ` documentation. * http: fixing a bug where the upgrade header was not cleared on responses to non-upgrade requests. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false. +* http filters: http filter extensions use the "envoy.filters.http" name space. A mapping + of extension names is available in the :ref:`deprecated ` documentation. * eds: added :ref:`hostname ` field for endpoints and :ref:`hostname ` field for endpoint's health check config. This enables auto host rewrite and customizing the host header during health checks for eds endpoints. * ext_authz: disabled the use of lowercase string matcher for headers matching in HTTP-based `ext_authz`. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher` to false. From 78ecf9d72630618020a0cad7b772e5bdf2ce3643 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 8 Apr 2020 15:08:14 -0400 Subject: [PATCH 5/6] check_format still not as good as Matt =P Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 88258f54f0e4d..015ec13d6aa2a 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,7 +4,7 @@ Version history 1.15.0 (Pending) ================ -* http: fixing a bug where the upgrade header was not cleared on responses to non-upgrade requests. +* http: fixed a bug where the upgrade header was not cleared on responses to non-upgrade requests. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false. 1.14.0 (April 8, 2020) From d979a4ac33fd810bb36223efdf28cb1bf566a4d2 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Fri, 10 Apr 2020 12:30:54 -0400 Subject: [PATCH 6/6] newline? Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index edc328fd219e3..4e24332d7b712 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -6,6 +6,7 @@ Version history * http: fixed a bug where the upgrade header was not cleared on responses to non-upgrade requests. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false. + 1.14.1 (April 8, 2020) ====================== * request_id_extension: fixed static initialization for noop request id extension.