[http] Handle faulty filters from removing required request headers

docs/root/configuration/http/http_conn_man/response_code_details.rst

@@ -30,6 +30,7 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen
    low_version, The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured.
    maintenance_mode, The request was rejected by the router filter because the cluster was in maintenance mode.
    max_duration_timeout, The per-stream max duration timeout was exceeded.
+   missing_headers_after_filter_chain, The request was rejected by the router filter because configured filters removed required headers.
    missing_host_header, The request was rejected due to a missing Host: or :authority field.
    missing_path_rejected, The request was rejected due to a missing Path or :path header field.
    no_healthy_upstream, The request was rejected by the router filter because there was no healthy upstream found.

docs/root/version_history/current.rst

@@ -19,6 +19,7 @@ Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
+* http: reject requests with missing required headers after filter chain processing.
 * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
 
 Removed Config or Runtime

include/envoy/stream_info/stream_info.h

@@ -172,6 +172,8 @@ struct ResponseCodeDetailValues {
   const std::string AdminFilterResponse = "admin_filter_response";
   // The original stream was replaced with an internal redirect.
   const std::string InternalRedirect = "internal_redirect";
+  // The request was rejected because configured filters erroneously removed required headers.
+  const std::string MissingHeadersAfterFilterChain = "missing_headers_after_filter_chain";
   // Changes or additions to details should be reflected in
   // docs/root/configuration/http/http_conn_man/response_code_details_details.rst
 };

source/common/http/http1/codec_impl.cc

@@ -372,13 +372,12 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end
   const HeaderEntry* host = headers.Host();
   bool is_connect = HeaderUtility::isConnect(headers);
 
-  // TODO(#10878): Include missing host header for CONNECT.
-  // The RELEASE_ASSERT below does not change the existing behavior of `encodeHeaders`.
-  // The `encodeHeaders` used to throw on errors. Callers of `encodeHeaders()` do not catch
-  // exceptions and this would cause abnormal process termination in error cases. This change
-  // replaces abnormal process termination from unhandled exception with the RELEASE_ASSERT. Further
-  // work will replace this RELEASE_ASSERT with proper error handling.
-  RELEASE_ASSERT(method && (path || is_connect), ":method and :path must be specified");
+  // Headers must be present. If downstream traffic is missing headers, downstream codecs will
+  // reject the request. If a faulty filter removes these headers, the router will reject before
+  // forwarding the request headers here.
+  ASSERT(method, ":method must be specified.");
+  ASSERT(path || is_connect, "path must be specified");
+  ASSERT(host || !is_connect, "host must be specified for a CONNECT request");
 
   if (method->value() == Headers::get().MethodValues.Head) {
     head_request_ = true;

source/common/router/BUILD

@@ -305,6 +305,7 @@ envoy_cc_library(
         "//source/common/http:header_map_lib",
         "//source/common/http:headers_lib",
         "//source/common/http:message_lib",
+        "//source/common/http:status_lib",
         "//source/common/http:utility_lib",
         "//source/common/network:application_protocol_lib",
         "//source/common/network:transport_socket_options_lib",

source/common/router/router.cc

@@ -263,6 +263,22 @@ FilterUtility::StrictHeaderChecker::checkHeader(Http::RequestHeaderMap& headers,
   NOT_REACHED_GCOVR_EXCL_LINE;
 }
 
+Http::Status FilterUtility::checkHeaderMap(const Http::RequestHeaderMap& headers) {
+  if (!headers.Method()) {
+    return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get());
+  }
+  bool is_connect = Http::HeaderUtility::isConnect(headers);
+  if (!headers.Path() && !is_connect) {
+    // :path header must be present for non-CONNECT requests.
+    return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get());
+  }
+  if (!headers.Host() && is_connect) {
+    // Host header must be present for CONNECT request.
+    return absl::InvalidArgumentError(Envoy::Http::Headers::get().Host.get());
+  }
+  return Http::okStatus();
+}
+
 Stats::StatName Filter::upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host) {
   return upstream_host ? upstream_host->localityZoneStatName() : config_.empty_stat_name_;
 }
@@ -329,10 +345,20 @@ void Filter::chargeUpstreamCode(Http::Code code,
 }
 
 Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) {
-  // Do a common header check. We make sure that all outgoing requests have all HTTP/2 headers.
-  // These get stripped by HTTP/1 codec where applicable.
-  ASSERT(headers.Method());
-  ASSERT(headers.Host());
+  // Do a common header check ensuring required headers are present before forwarding. These mimic
+  // checks done in the HTTP Connection Manager before filter processing and ensure that filters did
+  // not erroneously remove required headers.
+  const auto header_status = FilterUtility::checkHeaderMap(headers);
+  if (!header_status.ok()) {
+    callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError);
+    const std::string body = fmt::format("missing required header: {}", header_status.message());
+    const std::string details =
+        absl::StrCat(StreamInfo::ResponseCodeDetails::get().MissingHeadersAfterFilterChain, "{",
+                     header_status.message(), "}");
+    callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt,
+                               details);
+    return Http::FilterHeadersStatus::StopIteration;
+  }
 
   downstream_headers_ = &headers;
 

source/common/router/router.h

@@ -27,6 +27,7 @@
 #include "common/common/linked_object.h"
 #include "common/common/logger.h"
 #include "common/config/well_known_names.h"
+#include "common/http/status.h"
 #include "common/http/utility.h"
 #include "common/router/config_impl.h"
 #include "common/router/upstream_request.h"
@@ -121,6 +122,15 @@ class FilterUtility {
     }
   };
 
+  /* Does a common header check ensuring required headers are present before request headers are
+   * forwarded. This will only fail if configured filters erroneously removes required headers.
+   * Required request headers include :method header, :path for non-CONNECT requests, and
+   * host/authority for CONNECT requests.
+   * @return Status containing the result. If failed, message includes details on which header was
+   * missing.
+   */
+  static Http::Status checkHeaderMap(const Http::RequestHeaderMap& headers);
+
   /**
    * Returns response_time / timeout, as a  percentage as [0, 100]. Returns 0
    * if there is no timeout.

test/integration/BUILD

@@ -442,6 +442,7 @@ envoy_cc_test(
         "//source/extensions/filters/http/health_check:config",
         "//test/integration/filters:continue_headers_only_inject_body",
         "//test/integration/filters:encoder_decoder_buffer_filter_lib",
+        "//test/integration/filters:invalid_header_filter_lib",
         "//test/integration/filters:local_reply_during_encoding_filter_lib",
         "//test/integration/filters:random_pause_filter_lib",
         "//test/test_common:utility_lib",
@@ -883,6 +884,7 @@ envoy_cc_test(
         "//source/extensions/filters/http/health_check:config",
         "//test/integration/filters:clear_route_cache_filter_lib",
         "//test/integration/filters:encoder_decoder_buffer_filter_lib",
+        "//test/integration/filters:invalid_header_filter_lib",
         "//test/integration/filters:process_context_lib",
         "//test/integration/filters:stop_iteration_and_continue",
         "//test/mocks/http:http_mocks",

test/integration/filters/BUILD

@@ -373,3 +373,19 @@ envoy_cc_test_library(
         "@envoy_api//envoy/extensions/network/socket_interface/v3:pkg_cc_proto",
     ],
 )
+
+envoy_cc_test_library(
+    name = "invalid_header_filter_lib",
+    srcs = [
+        "invalid_header_filter.cc",
+    ],
+    deps = [
+        ":common_lib",
+        "//include/envoy/http:filter_interface",
+        "//include/envoy/registry",
+        "//include/envoy/server:filter_config_interface",
+        "//source/common/http:header_utility_lib",
+        "//source/extensions/filters/http/common:pass_through_filter_lib",
+        "//test/extensions/filters/http/common:empty_http_filter_config_lib",
+    ],
+)

test/integration/filters/invalid_header_filter.cc

@@ -0,0 +1,42 @@
+#include <string>
+
+#include "envoy/http/filter.h"
+#include "envoy/registry/registry.h"
+#include "envoy/server/filter_config.h"
+
+#include "common/http/header_utility.h"
+
+#include "extensions/filters/http/common/pass_through_filter.h"
+
+#include "test/extensions/filters/http/common/empty_http_filter_config.h"
+#include "test/integration/filters/common.h"
+
+namespace Envoy {
+
+// Faulty filter that may remove critical headers.
+class InvalidHeaderFilter : public Http::PassThroughFilter {
+public:
+  constexpr static char name[] = "invalid-header-filter";
+
+  Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override {
+    // Remove method when there is a "remove-method" header.
+    if (!headers.get(Http::LowerCaseString("remove-method")).empty()) {
+      headers.removeMethod();
+    }
+    if (!headers.get(Http::LowerCaseString("remove-path")).empty()) {
+      headers.removePath();
+    }
+    if (Http::HeaderUtility::isConnect(headers)) {
+      ENVOY_LOG_MISC(info, "REMOVING Host FROM CONNECT");
+      headers.removeHost();
+    }
+    return Http::FilterHeadersStatus::Continue;
+  }
+};
+
+constexpr char InvalidHeaderFilter::name[];
+static Registry::RegisterFactory<SimpleFilterConfig<InvalidHeaderFilter>,
+                                 Server::Configuration::NamedHttpFilterConfigFactory>
+    decoder_register_;
+
+} // namespace Envoy

test/integration/http2_integration_test.cc

@@ -1561,6 +1561,33 @@ TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) {
   EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers"));
 }
 
+TEST_P(Http2FrameIntegrationTest, MissingHeaders) {
+  config_helper_.addFilter(R"EOF(
+  name: invalid-header-filter
+  typed_config:
+    "@type": type.googleapis.com/google.protobuf.Empty
+  )EOF");
+
+  config_helper_.addConfigModifier(
+      [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+             hcm) -> void {
+        hcm.mutable_http2_protocol_options()
+            ->mutable_override_stream_error_on_invalid_http_message()
+            ->set_value(true);
+      });
+
+  beginSession();
+
+  uint32_t stream_index = 0;
+  auto request = Http::Http2::Http2Frame::makeMalformedRequestWithMissingHeaders(
+      Http2Frame::makeClientStreamId(stream_index), false, "host", "/");
+  sendFrame(request);
+  auto response = readFrame();
+  // Make sure we've got RST_STREAM from the server
+  EXPECT_EQ(Http2Frame::Type::RstStream, response.type());
+  tcp_client_->close();
+}
+
 INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);

test/integration/protocol_integration_test.cc

@@ -333,6 +333,62 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) {
             response->headers().get(Http::LowerCaseString("host"))[0]->value().getStringView());
 }
 
+// Tests missing headers needed for H/1 codec first line.
+TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) {
+  useAccessLog("%RESPONSE_CODE_DETAILS%");
+  config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": "
+                           "type.googleapis.com/google.protobuf.Empty } }");
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); });
+  config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
+    // Clone the whole listener.
+    auto static_resources = bootstrap.mutable_static_resources();
+    auto* old_listener = static_resources->mutable_listeners(0);
+    auto* cloned_listener = static_resources->add_listeners();
+    cloned_listener->CopyFrom(*old_listener);
+    old_listener->set_name("http_forward");
+  });
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  // Missing method
+  auto response = codec_client_->makeHeaderOnlyRequest(
+      Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                     {":path", "/test/url"},
+                                     {":scheme", "http"},
+                                     {":authority", "host"},
+                                     {"remove-method", "yes"}});
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("503", response->headers().getStatusValue());
+  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain"));
+
+  // Missing path for non-CONNECT
+  response =
+      codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                                                          {":path", "/test/url"},
+                                                                          {":scheme", "http"},
+                                                                          {":authority", "host"},
+                                                                          {"remove-path", "yes"}});
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("503", response->headers().getStatusValue());
+  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain"));
+
+  // Missing host for CONNECT
+  response = codec_client_->makeHeaderOnlyRequest(
+      Http::TestRequestHeaderMapImpl{{":method", "CONNECT"},
+                                     {":path", "/test/url"},
+                                     {":scheme", "http"},
+                                     {":authority", "www.host.com:80"}});
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("503", response->headers().getStatusValue());
+  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain"));
+}
+
 // Regression test for https://github.com/envoyproxy/envoy/issues/10270
 TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) {
   // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that