[http] Handle faulty filters from removing required request headers

source/common/http/http1/codec_impl.cc

@@ -378,13 +378,11 @@ 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. This would only happen from downstream traffic (in which case
+  // downstream codecs would reject) or a faulty filter (in which case the router would reject).
+  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;
@@ -398,7 +396,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end
 
   connection_.copyToBuffer(method->value().getStringView().data(), method->value().size());
   connection_.addCharToBuffer(' ');
-  if (is_connect) {
+  if (is_connect) { // Maybe add && host?
     connection_.copyToBuffer(host->value().getStringView().data(), host->value().size());
   } else {
     connection_.copyToBuffer(path->value().getStringView().data(), path->value().size());

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,23 @@ FilterUtility::StrictHeaderChecker::checkHeader(Http::RequestHeaderMap& headers,
   NOT_REACHED_GCOVR_EXCL_LINE;
 }
 
+Http::Status FilterUtility::checkHeaderMap(const Http::RequestHeaderMap& headers) {
+  if (!headers.Method()) {
+    // Must have a method header.
+    return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get());
+  }
+  bool is_connect = Http::HeaderUtility::isConnect(headers);
+  if (!headers.Path() && !is_connect) {
+    // Must have a path header unless it is a CONNECT request.
+    return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get());
+  }
+  if (!headers.Host() && is_connect) {
+    // Must have a host header with a 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 +346,19 @@ 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 for minimum constraints. We make sure that all outgoing requests have
+  // all HTTP/2 headers.
+  const auto header_status = FilterUtility::checkHeaderMap(headers);
+  if (!header_status.ok()) {
+    callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError);
+    ENVOY_LOG_MISC(info, "HEADER STATUS MESSAGE {}", header_status.message());
+    const std::string body = fmt::format("missing required header: {}", header_status.message());
+    const std::string details = absl::StrCat(StreamInfo::ResponseCodeDetails::get().DirectResponse,
+                                             "{", 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,12 @@ 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.
+   * @return Status containing the result along with a message if false.
+   */
+  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/common/http/http2/http2_frame.cc

@@ -287,6 +287,26 @@ Http2Frame Http2Frame::makeMalformedRequestWithZerolenHeader(uint32_t stream_ind
   return frame;
 }
 
+Http2Frame Http2Frame::makeMalformedRequestWithMissingHeaders(uint32_t stream_index, bool method,
+                                                              absl::string_view host,
+                                                              absl::string_view path) {
+  Http2Frame frame;
+  frame.buildHeader(Type::Headers, 0, orFlags(HeadersFlags::EndStream, HeadersFlags::EndHeaders),
+                    makeNetworkOrderStreamId(stream_index));
+  if (method) {
+    frame.appendStaticHeader(StaticHeaderIndex::MethodGet);
+  }
+  frame.appendStaticHeader(StaticHeaderIndex::SchemeHttps);
+  if (!path.empty()) {
+    frame.appendHeaderWithoutIndexing(StaticHeaderIndex::Path, path);
+  }
+  if (!host.empty()) {
+    frame.appendHeaderWithoutIndexing(StaticHeaderIndex::Host, host);
+  }
+  frame.adjustPayloadSize();
+  return frame;
+}
+
 Http2Frame Http2Frame::makeRequest(uint32_t stream_index, absl::string_view host,
                                    absl::string_view path) {
   Http2Frame frame;

test/common/http/http2/http2_frame.h

@@ -132,6 +132,9 @@ class Http2Frame {
   static Http2Frame makeMalformedRequestWithZerolenHeader(uint32_t stream_index,
                                                           absl::string_view host,
                                                           absl::string_view path);
+  static Http2Frame makeMalformedRequestWithMissingHeaders(uint32_t stream_index, bool method,
+                                                           absl::string_view host,
+                                                           absl::string_view path);
   static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host,
                                 absl::string_view path);
   static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host,

test/integration/BUILD

@@ -327,6 +327,7 @@ envoy_cc_test(
         "//source/extensions/filters/http/buffer:config",
         "//source/extensions/filters/http/health_check:config",
         "//test/common/http/http2:http2_frame",
+        "//test/integration/filters:invalid_header_filter_lib",
         "//test/integration/filters:metadata_stop_all_filter_config_lib",
         "//test/integration/filters:request_metadata_filter_config_lib",
         "//test/integration/filters:response_metadata_filter_config_lib",
@@ -420,6 +421,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",
@@ -860,6 +862,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

@@ -388,3 +388,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"))) {
+      headers.removeMethod();
+    }
+    if (headers.get(Http::LowerCaseString("remove-path"))) {
+      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

@@ -1612,6 +1612,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,57 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) {
             response->headers().get(Http::LowerCaseString("host"))->value().getStringView());
 }
 
+// Tests missing headers needed for H/1 codec first line.
+// Missing first line headers can either occur from bad requests, or from a faulty filter.
+TEST_P(ProtocolIntegrationTest, Http1DownstreamRequestWithMissingHeaders) {
+  std::function<void(absl::string_view, absl::string_view, absl::string_view)>
+      sendRequestAndExpectResponse = [this](absl::string_view request,
+                                            absl::string_view expected_response,
+                                            absl::string_view expected_body) {
+        std::string response;
+        sendRawHttpAndWaitForResponse(lookupPort("http"), request.data(), &response, true);
+        EXPECT_THAT(response, HasSubstr(expected_response));
+        if (!expected_body.empty()) {
+          EXPECT_THAT(response, HasSubstr(expected_body));
+        }
+      };
+
+  if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
+    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();
+    // Faulty downstream traffic.
+    sendRequestAndExpectResponse("/ HTTP/1.1\r\nHost: host\r\n\r\n", "HTTP/1.1 400 Bad Request\r\n",
+                                 "");
+    sendRequestAndExpectResponse("GET   HTTP/1.1\r\nHost: host\r\n\r\n",
+                                 "HTTP/1.1 400 Bad Request\r\n", "");
+    sendRequestAndExpectResponse("CONNECT   HTTP/1.1\r\nHost: host\r\n\r\n",
+                                 "HTTP/1.1 400 Bad Request\r\n", "");
+    // Faulty filter code.
+    sendRequestAndExpectResponse("GET / HTTP/1.1\r\nHost: host\r\nremove-method: yes\r\n\r\n",
+                                 "HTTP/1.1 503 Service Unavailable\r\n",
+                                 "missing required header: :method");
+    sendRequestAndExpectResponse("GET / HTTP/1.1\r\nHost: host\r\nremove-path: yes\r\n\r\n",
+                                 "HTTP/1.1 503 Service Unavailable\r\n",
+                                 "missing required header: :path");
+    sendRequestAndExpectResponse("CONNECT www.host.com:80 HTTP/1.1\r\n\r\n",
+                                 "HTTP/1.1 503 Service Unavailable\r\n",
+                                 "missing required header: :authority");
+  }
+}
+
 // 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