http: fixing upgrade response bug

docs/root/intro/version_history.rst

@@ -21,6 +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 <envoy_api_field_config.common.dynamic_forward_proxy.v2alpha.DnsCacheConfig.dns_failure_refresh_rate>` for the :ref:`dns cache <envoy_api_msg_config.common.dynamic_forward_proxy.v2alpha.DnsCacheConfig>` 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 <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.
 * eds: added :ref:`hostname <envoy_v3_api_field_config.endpoint.v3.Endpoint.hostname>` field for endpoints and :ref:`hostname <envoy_v3_api_field_config.endpoint.v3.Endpoint.HealthCheckConfig.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.

source/common/http/conn_manager_utility.cc

@@ -378,7 +378,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()) {

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());
@@ -579,7 +582,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() &&
@@ -1007,6 +1010,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(Buffer::Instance& data) {
   ASSERT(!deferred_end_stream_headers_);
   if (pending_response_.has_value()) {

source/common/http/http1/codec_impl.h

@@ -148,6 +148,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_{};
 };
@@ -312,6 +314,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   int onHeadersCompleteBase();
   virtual int onHeadersComplete() PURE;
 
+  /**
+   * Called to see if upgrade transition is allowed.
+   */
+  virtual bool upgradeAllowed() const PURE;
+
   /**
    * Called with body data is available for processing when either:
    * - There is an accumulated partial body after the parser is done processing bytes read from the
@@ -418,6 +425,8 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
   void onMessageBegin() override;
   void onUrl(const char* data, size_t length) override;
   int onHeadersComplete() override;
+  // If upgrade behavior is not allowed, the HCM will have sanitized the headers out.
+  bool upgradeAllowed() const override { return true; }
   void onBody(Buffer::Instance& data) override;
   void onMessageComplete() override;
   void onResetStream(StreamResetReason reason) override;
@@ -500,6 +509,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
   void onMessageBegin() override {}
   void onUrl(const char*, size_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
   int onHeadersComplete() override;
+  bool upgradeAllowed() const override;
   void onBody(Buffer::Instance& data) override;
   void onMessageComplete() override;
   void onResetStream(StreamResetReason reason) override;

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

test/common/http/conn_manager_utility_test.cc

@@ -792,6 +792,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"}};

test/common/http/http1/codec_impl_test.cc

@@ -1815,14 +1815,27 @@ 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();
 
   InSequence s;
 
   NiceMock<MockResponseDecoder> 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
@@ -1853,7 +1866,11 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponseWithEarlyData) {
 
   NiceMock<MockResponseDecoder> 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

test/integration/integration_test.cc

@@ -1254,4 +1254,30 @@ 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());
+  EXPECT_EQ(nullptr, response->headers().Connection());
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("Hello World", response->body());
+  cleanupUpstreamAndDownstream();
+}
+
 } // namespace Envoy