[http] Handle faulty filters from removing required request headers

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

@@ -26,6 +26,7 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen
    duration_timeout, The max connection duration was exceeded.
    direct_response, A direct response was generated by the router filter.
    filter_chain_not_found, The request was rejected due to no matching filter chain.
+   filter_removed_required_headers, The request was rejected in the filter manager because a configured filter removed required headers.
    internal_redirect, The original stream was replaced with an internal redirect.
    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.

docs/root/version_history/current.rst

@@ -26,6 +26,7 @@ Bug Fixes
 
 * dns: fix a bug where custom resolvers provided in configuration were not preserved after network issues.
 * http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses.
+* http: reject requests with missing required headers after filter chain processing.
 * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
 * proxy_proto: fixed a bug where the wrong downstream address got sent to upstream connections.
 * tls: fix detection of the upstream connection close event.

include/envoy/http/codec.h

@@ -105,10 +105,12 @@ class RequestEncoder : public virtual StreamEncoder {
 public:
   /**
    * Encode headers, optionally indicating end of stream.
-   * @param headers supplies the header map to encode.
+   * @param headers supplies the header map to encode. Must have required HTTP headers.
    * @param end_stream supplies whether this is a header only request.
+   * @return Status indicating whether encoding succeeded. Encoding will fail if request
+   * headers are missing required HTTP headers (method, path for non-CONNECT, host for CONNECT).
    */
-  virtual void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE;
+  virtual Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE;
 
   /**
    * Encode trailers. This implicitly ends the stream.

include/envoy/router/router.h

@@ -1274,8 +1274,10 @@ class GenericUpstream {
    * Encode headers, optionally indicating end of stream.
    * @param headers supplies the header map to encode.
    * @param end_stream supplies whether this is a header only request.
+   * @return status indicating success. Encoding will fail if headers do not have required HTTP
+   * headers.
    */
-  virtual void encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) PURE;
+  virtual Http::Status encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) PURE;
   /**
    * Encode trailers. This implicitly ends the stream.
    * @param trailers supplies the trailers to encode.

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 FilterRemovedRequiredHeaders = "filter_removed_required_headers";
   // Changes or additions to details should be reflected in
   // docs/root/configuration/http/http_conn_man/response_code_details_details.rst
 };

source/common/http/BUILD

@@ -405,6 +405,7 @@ envoy_cc_library(
     ],
     deps = [
         ":header_map_lib",
+        ":status_lib",
         ":utility_lib",
         "//include/envoy/common:regex_interface",
         "//include/envoy/http:header_map_interface",

source/common/http/codec_wrappers.h

@@ -68,11 +68,12 @@ class ResponseDecoderWrapper : public ResponseDecoder {
 class RequestEncoderWrapper : public RequestEncoder {
 public:
   // RequestEncoder
-  void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override {
-    inner_.encodeHeaders(headers, end_stream);
+  Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override {
+    RETURN_IF_ERROR(inner_.encodeHeaders(headers, end_stream));
     if (end_stream) {
       onEncodeComplete();
     }
+    return okStatus();
   }
 
   void encodeData(Buffer::Instance& data, bool end_stream) override {

source/common/http/header_utility.cc

@@ -276,5 +276,27 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol,
   return false;
 }
 
+Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) {
+  if (!headers.Method()) {
+    return absl::InvalidArgumentError(
+        absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Method.get()));
+  }
+  bool is_connect = Http::HeaderUtility::isConnect(headers);
+  if (is_connect) {
+    if (!headers.Host()) {
+      // Host header must be present for CONNECT request.
+      return absl::InvalidArgumentError(
+          absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get()));
+    }
+  } else {
+    if (!headers.Path()) {
+      // :path header must be present for non-CONNECT requests.
+      return absl::InvalidArgumentError(
+          absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Path.get()));
+    }
+  }
+  return Http::okStatus();
+}
+
 } // namespace Http
 } // namespace Envoy

source/common/http/header_utility.h

@@ -8,6 +8,7 @@
 #include "envoy/http/protocol.h"
 #include "envoy/type/v3/range.pb.h"
 
+#include "common/http/status.h"
 #include "common/protobuf/protobuf.h"
 
 namespace Envoy {
@@ -174,6 +175,14 @@ class HeaderUtility {
    * @brief Remove the port part from host/authority header if it is equal to provided port
    */
   static void stripPortFromHost(RequestHeaderMap& headers, uint32_t listener_port);
+
+  /* Does a common header check ensuring required headers are present.
+   * Required request headers include :method header, :path for non-CONNECT requests, and
+   * host/authority for HTTP/1.1 or CONNECT requests.
+   * @return Status containing the result. If failed, message includes details on which header was
+   * missing.
+   */
+  static Http::Status checkRequiredHeaders(const Http::RequestHeaderMap& headers);
 };
 } // namespace Http
 } // namespace Envoy

source/common/http/http1/codec_impl.cc

@@ -366,20 +366,16 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e
 
 static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n";
 
-void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) {
+Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) {
+  // Required headers must be present. This can only happen by some erroneous processing after the
+  // downstream codecs decode.
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers));
+
   const HeaderEntry* method = headers.Method();
   const HeaderEntry* path = headers.Path();
   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");
-
   if (method->value() == Headers::get().MethodValues.Head) {
     head_request_ = true;
   } else if (method->value() == Headers::get().MethodValues.Connect) {
@@ -400,6 +396,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end
   connection_.copyToBuffer(REQUEST_POSTFIX, sizeof(REQUEST_POSTFIX) - 1);
 
   encodeHeadersBase(headers, absl::nullopt, end_stream);
+  return okStatus();
 }
 
 int ConnectionImpl::setAndCheckCallbackStatus(Status&& status) {

source/common/http/http1/codec_impl.h

@@ -156,7 +156,7 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder {
   bool connectRequest() const { return connect_request_; }
 
   // Http::RequestEncoder
-  void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
+  Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
   void encodeTrailers(const RequestTrailerMap& trailers) override { encodeTrailersBase(trailers); }
 
 private:

source/common/http/http1/codec_impl_legacy.cc

@@ -365,7 +365,7 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e
 
 static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n";
 
-void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) {
+Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) {
   const HeaderEntry* method = headers.Method();
   const HeaderEntry* path = headers.Path();
   const HeaderEntry* host = headers.Host();
@@ -397,6 +397,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end
   connection_.copyToBuffer(REQUEST_POSTFIX, sizeof(REQUEST_POSTFIX) - 1);
 
   encodeHeadersBase(headers, absl::nullopt, end_stream);
+  return okStatus();
 }
 
 http_parser_settings ConnectionImpl::settings_{

source/common/http/http1/codec_impl_legacy.h

@@ -160,7 +160,7 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder {
   bool connectRequest() const { return connect_request_; }
 
   // Http::RequestEncoder
-  void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
+  Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
   void encodeTrailers(const RequestTrailerMap& trailers) override { encodeTrailersBase(trailers); }
 
 private:

source/common/http/http2/codec_impl.cc

@@ -183,8 +183,11 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector<nghttp2_nv>
   }
 }
 
-void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
-                                                     bool end_stream) {
+Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
+                                                       bool end_stream) {
+  // Required headers must be present. This can only happen by some erroneous processing after the
+  // downstream codecs decode.
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers));
   // This must exist outside of the scope of isUpgrade as the underlying memory is
   // needed until encodeHeadersBase has been called.
   std::vector<nghttp2_nv> final_headers;
@@ -211,6 +214,7 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea
     buildHeaders(final_headers, headers);
   }
   encodeHeadersBase(final_headers, end_stream);
+  return okStatus();
 }
 
 void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers,

source/common/http/http2/codec_impl.h

@@ -351,7 +351,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
     }
 
     // RequestEncoder
-    void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
+    Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
     void encodeTrailers(const RequestTrailerMap& trailers) override {
       encodeTrailersBase(trailers);
     }

source/common/http/http2/codec_impl_legacy.cc

@@ -183,8 +183,8 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector<nghttp2_nv>
   parent_.checkProtocolConstraintViolation();
 }
 
-void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
-                                                     bool end_stream) {
+Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
+                                                       bool end_stream) {
   // This must exist outside of the scope of isUpgrade as the underlying memory is
   // needed until encodeHeadersBase has been called.
   std::vector<nghttp2_nv> final_headers;
@@ -211,6 +211,7 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea
     buildHeaders(final_headers, headers);
   }
   encodeHeadersBase(final_headers, end_stream);
+  return okStatus();
 }
 
 void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers,

source/common/http/http2/codec_impl_legacy.h

@@ -351,7 +351,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
     }
 
     // RequestEncoder
-    void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
+    Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override;
     void encodeTrailers(const RequestTrailerMap& trailers) override {
       encodeTrailersBase(trailers);
     }

source/common/router/router.cc

@@ -329,11 +329,6 @@ 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());
-
   downstream_headers_ = &headers;
 
   // Extract debug configuration from filter state. This is used further along to determine whether

source/common/router/upstream_request.cc

@@ -416,10 +416,22 @@ void UpstreamRequest::onPoolReady(
     }
   }
 
-  upstream_->encodeHeaders(*parent_.downstreamHeaders(), shouldSendEndStream());
-
+  const Http::Status status =
+      upstream_->encodeHeaders(*parent_.downstreamHeaders(), shouldSendEndStream());
   calling_encode_headers_ = false;
 
+  if (!status.ok()) {
+    // It is possible that encodeHeaders() fails. This can happen if filters or other extensions
+    // erroneously remove required headers.
+    stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError);
+    const std::string details =
+        absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{",
+                     status.message(), "}");
+    parent_.callbacks()->sendLocalReply(Http::Code::ServiceUnavailable, status.message(), nullptr,
+                                        absl::nullopt, details);
+    return;
+  }
+
   if (!paused_for_connect_) {
     encodeBodyAndTrailers();
   }

source/common/tcp_proxy/upstream.cc

@@ -123,7 +123,9 @@ void HttpUpstream::setRequestEncoder(Http::RequestEncoder& request_encoder, bool
        {Http::Headers::get().Scheme, scheme},
        {Http::Headers::get().Path, "/"},
        {Http::Headers::get().Host, hostname_}});
-  request_encoder_->encodeHeaders(*headers, false);
+  const auto status = request_encoder_->encodeHeaders(*headers, false);
+  // Encoding can only fail on missing required request headers.
+  ASSERT(status.ok());
 }
 
 void HttpUpstream::resetEncoder(Network::ConnectionEvent event, bool inform_downstream) {

source/common/upstream/health_checker_impl.cc

@@ -274,7 +274,9 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
   stream_info.setDownstreamRemoteAddress(local_address_);
   stream_info.onUpstreamHostSelected(host_);
   parent_.request_headers_parser_->evaluateHeaders(*request_headers, stream_info);
-  request_encoder->encodeHeaders(*request_headers, true);
+  auto status = request_encoder->encodeHeaders(*request_headers, true);
+  // Encoding will only fail if required request headers are missing.
+  ASSERT(status.ok());
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResetStream(Http::StreamResetReason,
@@ -691,7 +693,9 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() {
   Router::FilterUtility::setUpstreamScheme(
       headers_message->headers(), host_->transportSocketFactory().implementsSecureTransport());
 
-  request_encoder_->encodeHeaders(headers_message->headers(), false);
+  auto status = request_encoder_->encodeHeaders(headers_message->headers(), false);
+  // Encoding will only fail if required headers are missing.
+  ASSERT(status.ok());
 
   grpc::health::v1::HealthCheckRequest request;
   if (parent_.service_name_.has_value()) {

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc

@@ -46,7 +46,8 @@ EnvoyQuicClientStream::EnvoyQuicClientStream(quic::PendingStream* pending,
           16 * 1024, [this]() { runLowWatermarkCallbacks(); },
           [this]() { runHighWatermarkCallbacks(); }) {}
 
-void EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) {
+Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers,
+                                                  bool end_stream) {
   ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers);
   quic::QuicStream* writing_stream =
       quic::VersionUsesHttp3(transport_version())
@@ -60,6 +61,7 @@ void EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers,
   // IETF QUIC sends HEADER frame on current stream. After writing headers, the
   // buffer may increase.
   maybeCheckWatermark(bytes_to_send_old, bytes_to_send_new, *filterManagerConnection());
+  return Http::okStatus();
 }
 
 void EnvoyQuicClientStream::encodeData(Buffer::Instance& data, bool end_stream) {

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h

@@ -37,7 +37,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream,
   }
 
   // Http::RequestEncoder
-  void encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) override;
+  Http::Status encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) override;
   void encodeTrailers(const Http::RequestTrailerMap& trailers) override;
 
   // Http::Stream

source/extensions/upstreams/http/http/upstream_request.h

@@ -66,8 +66,9 @@ class HttpUpstream : public Router::GenericUpstream, public Envoy::Http::StreamC
   void encodeMetadata(const Envoy::Http::MetadataMapVector& metadata_map_vector) override {
     request_encoder_->encodeMetadata(metadata_map_vector);
   }
-  void encodeHeaders(const Envoy::Http::RequestHeaderMap& headers, bool end_stream) override {
-    request_encoder_->encodeHeaders(headers, end_stream);
+  Envoy::Http::Status encodeHeaders(const Envoy::Http::RequestHeaderMap& headers,
+                                    bool end_stream) override {
+    return request_encoder_->encodeHeaders(headers, end_stream);
   }
   void encodeTrailers(const Envoy::Http::RequestTrailerMap& trailers) override {
     request_encoder_->encodeTrailers(trailers);

source/extensions/upstreams/http/tcp/upstream_request.cc

@@ -43,7 +43,8 @@ void TcpUpstream::encodeData(Buffer::Instance& data, bool end_stream) {
   upstream_conn_data_->connection().write(data, end_stream);
 }
 
-void TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) {
+Envoy::Http::Status TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&,
+                                               bool end_stream) {
   // Headers should only happen once, so use this opportunity to add the proxy
   // proto header, if configured.
   ASSERT(upstream_request_->routeEntry().connectConfig().has_value());
@@ -64,6 +65,7 @@ void TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_s
       Envoy::Http::createHeaderMap<Envoy::Http::ResponseHeaderMapImpl>(
           {{Envoy::Http::Headers::get().Status, "200"}})};
   upstream_request_->decodeHeaders(std::move(headers), false);
+  return Envoy::Http::okStatus();
 }
 
 void TcpUpstream::encodeTrailers(const Envoy::Http::RequestTrailerMap&) {

source/extensions/upstreams/http/tcp/upstream_request.h

@@ -70,7 +70,7 @@ class TcpUpstream : public Router::GenericUpstream,
   // GenericUpstream
   void encodeData(Buffer::Instance& data, bool end_stream) override;
   void encodeMetadata(const Envoy::Http::MetadataMapVector&) override {}
-  void encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) override;
+  Envoy::Http::Status encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) override;
   void encodeTrailers(const Envoy::Http::RequestTrailerMap&) override;
   void readDisable(bool disable) override;
   void resetStream() override;