HCM: guard Envoy from removal of critical response headers.

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

@@ -26,7 +26,8 @@ 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.
+   filter_removed_required_request_headers, The request was rejected in the filter manager because a configured filter removed required request headers.
+   filter_removed_required_response_headers, The response was rejected in the filter manager because a configured filter removed required response headers or these values were invalid (e.g. overflown).
    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.

include/envoy/http/codec.h

@@ -139,16 +139,18 @@ class ResponseEncoder : public virtual StreamEncoder {
   /**
    * Encode 100-Continue headers.
    * @param headers supplies the 100-Continue header map to encode.
+   * @return indicates the result of encoding and has Http::StatusCode::Ok for success.
    */
-  virtual void encode100ContinueHeaders(const ResponseHeaderMap& headers) PURE;
+  virtual Http::Status encode100ContinueHeaders(const ResponseHeaderMap& headers) PURE;
 
   /**
    * Encode headers, optionally indicating end of stream. Response headers must
-   * have a valid :status set.
+   * have a valid :status set
    * @param headers supplies the header map to encode.
    * @param end_stream supplies whether this is a header only response.
+   * @return indicates the result of encoding and has Http::StatusCode::Ok for success.
    */
-  virtual void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) PURE;
+  virtual Http::Status encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) PURE;
 
   /**
    * Encode trailers. This implicitly ends the stream.

include/envoy/stream_info/stream_info.h

@@ -180,9 +180,11 @@ struct ResponseCodeDetailValues {
   // 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";
+  const std::string FilterRemovedRequiredRequestHeaders = "filter_removed_required_request_headers";
+  const std::string FilterRemovedRequiredResponseHeaders =
+      "filter_removed_required_response_headers";
   // Changes or additions to details should be reflected in
-  // docs/root/configuration/http/http_conn_man/response_code_details_details.rst
+  // docs/root/configuration/http/http_conn_man/response_code_details.rst
 };
 
 using ResponseCodeDetails = ConstSingleton<ResponseCodeDetailValues>;

source/common/http/conn_manager_impl.cc

@@ -901,8 +901,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
       request_headers_->Expect()->value() == Headers::get().ExpectValues._100Continue.c_str()) {
     // Note in the case Envoy is handling 100-Continue complexity, it skips the filter chain
     // and sends the 100-Continue directly to the encoder.
-    chargeStats(continueHeader());
-    response_encoder_->encode100ContinueHeaders(continueHeader());
+    const auto& continue_headers = continueHeader();
+    const auto status = response_encoder_->encode100ContinueHeaders(continue_headers);
+    // We must make sure static `continue_headers` is valid in its content and encoding not fail.
+    ASSERT(status.ok());
+    chargeStats(continue_headers);
     // Remove the Expect header so it won't be handled again upstream.
     request_headers_->removeExpect();
   }
@@ -1334,13 +1337,15 @@ void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders(
   ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers_.get(),
                                                   connection_manager_.config_, EMPTY_STRING);
 
-  // Count both the 1xx and follow-up response code in stats.
-  chargeStats(response_headers);
-
   ENVOY_STREAM_LOG(debug, "encoding 100 continue headers via codec:\n{}", *this, response_headers);
 
   // Now actually encode via the codec.
-  response_encoder_->encode100ContinueHeaders(response_headers);
+  const auto status = response_encoder_->encode100ContinueHeaders(response_headers);
+  // encode100ContinueHeaders is always called with valid headers soon after parsing so must be OK.
+  ASSERT(status.ok());
+
+  // Count both the 1xx and follow-up response code in stats.
+  chargeStats(response_headers);
 }
 
 void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& headers,
@@ -1456,14 +1461,26 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& heade
     }
   }
 
-  chargeStats(headers);
-
   ENVOY_STREAM_LOG(debug, "encoding headers via codec (end_stream={}):\n{}", *this, end_stream,
                    headers);
 
   // Now actually encode via the codec.
+  const auto status = response_encoder_->encodeHeaders(headers, end_stream);
+  if (!status.ok()) {
+    // This branch can happen when a misbehaving filter chain removed critical headers or set
+    // malformed header values.
+    // Also the bad http response status from upstream over HTTP2/3 (e.g. overflown status value)
+    // reaches here.
+    const auto request_headers = requestHeaders();
+    sendLocalReply(
+        request_headers.has_value() && Grpc::Common::isGrpcRequestHeaders(request_headers.ref()),
+        Http::Code::BadGateway, status.message(), nullptr, absl::nullopt,
+        absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredResponseHeaders,
+                     "{", status.message(), "}"));
+    return;
+  }
   filter_manager_.streamInfo().onFirstDownstreamTxByteSent();
-  response_encoder_->encodeHeaders(headers, end_stream);
+  chargeStats(headers);
 }
 
 void ConnectionManagerImpl::ActiveStream::encodeData(Buffer::Instance& data, bool end_stream) {

source/common/http/conn_manager_impl.h

@@ -250,12 +250,14 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     }
 
     void endStream() override {
+      // This assertion must satisfy and be guarded by callers with streamEnded().
       ASSERT(!state_.codec_saw_local_complete_);
       state_.codec_saw_local_complete_ = true;
       filter_manager_.streamInfo().onLastDownstreamTxByteSent();
       request_response_timespan_->complete();
       connection_manager_.doEndStream(*this);
     }
+    bool streamEnded() override { return state_.codec_saw_local_complete_; }
     void onDecoderFilterBelowWriteBufferLowWatermark() override;
     void onDecoderFilterAboveWriteBufferHighWatermark() override;
     void upgradeFilterChainCreated() override {

source/common/http/filter_manager.cc

@@ -1011,8 +1011,10 @@ void FilterManager::maybeContinueEncoding(
 void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHeaderMap& headers,
                                   bool end_stream) {
   // See encodeHeaders() comments in include/envoy/http/filter.h for why the 1xx precondition holds.
-  ASSERT(!CodeUtility::is1xx(Utility::getResponseStatus(headers)) ||
-         Utility::getResponseStatus(headers) == enumToInt(Http::Code::SwitchingProtocols));
+  ASSERT(HeaderUtility::checkRequiredResponseHeaders(headers).ok() &&
+         (!CodeUtility::is1xx(Utility::getResponseStatus(headers)) ||
+          Utility::getResponseStatus(headers) == enumToInt(Http::Code::SwitchingProtocols)));
+
   filter_manager_callbacks_.resetIdleTimer();
   disarmRequestTimeout();
 
@@ -1065,8 +1067,8 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea
   }
 
   const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end());
-  state_.non_100_response_headers_encoded_ = true;
   filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream);
+  state_.non_100_response_headers_encoded_ = true;
   maybeEndEncode(modified_end_stream);
 
   if (!modified_end_stream) {
@@ -1247,7 +1249,10 @@ void FilterManager::encodeTrailers(ActiveStreamEncoderFilter* filter,
 }
 
 void FilterManager::maybeEndEncode(bool end_stream) {
-  if (end_stream) {
+  // filter_manager_callbacks_.streamEnded() returns True here when the codec failed to encode
+  // headers due to the lack of required response headers, and filter_manager_callbacks already sent
+  // the local reply and ended the stream by itself. So in that case this function must be no-op.
+  if (end_stream && !filter_manager_callbacks_.streamEnded()) {
     filter_manager_callbacks_.endStream();
   }
 }

source/common/http/filter_manager.h

@@ -460,6 +460,11 @@ class FilterManagerCallbacks {
    */
   virtual void endStream() PURE;
 
+  /**
+   * Called to check if endStream() is called by filter manager.
+   */
+  virtual bool streamEnded() PURE;
+
   /**
    * Called when the stream write buffer is no longer above the low watermark.
    */

source/common/http/header_utility.cc

@@ -300,7 +300,7 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol,
   return false;
 }
 
-Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) {
+Http::Status HeaderUtility::checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers) {
   if (!headers.Method()) {
     return absl::InvalidArgumentError(
         absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Method.get()));
@@ -322,6 +322,16 @@ Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& h
   return Http::okStatus();
 }
 
+Http::Status HeaderUtility::checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers) {
+  const HeaderEntry* header = headers.Status();
+  uint64_t response_code;
+  if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) {
+    return absl::InvalidArgumentError(
+        absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Status.get()));
+  }
+  return Http::okStatus();
+}
+
 bool HeaderUtility::isRemovableHeader(absl::string_view header) {
   return (header.empty() || header[0] != ':') &&
          !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get());

source/common/http/header_utility.h

@@ -189,13 +189,20 @@ class HeaderUtility {
    */
   static void stripPortFromHost(RequestHeaderMap& headers, absl::optional<uint32_t> listener_port);
 
-  /* Does a common header check ensuring required headers are present.
+  /* Does a common header check ensuring required request 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);
+  static Http::Status checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers);
+
+  /* Does a common header check ensuring required response headers are present.
+   * Current required request headers only includes :status.
+   * @return Status containing the result. If failed, message includes details on which header was
+   * missing.
+   */
+  static Http::Status checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers);
 
   /**
    * Returns true if a header may be safely removed without causing additional

source/common/http/http1/codec_impl.cc

@@ -119,9 +119,9 @@ void StreamEncoderImpl::encodeFormattedHeader(absl::string_view key, absl::strin
   }
 }
 
-void ResponseEncoderImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) {
+Http::Status ResponseEncoderImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) {
   ASSERT(headers.Status()->value() == "100");
-  encodeHeaders(headers, false);
+  return encodeHeaders(headers, false);
 }
 
 void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& headers,
@@ -366,11 +366,12 @@ const Network::Address::InstanceConstSharedPtr& StreamEncoderImpl::connectionLoc
 static const char RESPONSE_PREFIX[] = "HTTP/1.1 ";
 static const char HTTP_10_RESPONSE_PREFIX[] = "HTTP/1.0 ";
 
-void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) {
+Http::Status ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) {
   started_response_ = true;
 
-  // The contract is that client codecs must ensure that :status is present.
-  ASSERT(headers.Status() != nullptr);
+  // The contract is that client codecs must ensure that required headers are present.
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredResponseHeaders(headers));
+
   uint64_t numeric_status = Utility::getResponseStatus(headers);
 
   if (connection_.protocol() == Protocol::Http10 && connection_.supportsHttp10()) {
@@ -394,14 +395,15 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e
   }
 
   encodeHeadersBase(headers, absl::make_optional<uint64_t>(numeric_status), end_stream, false);
+  return Http::okStatus();
 }
 
 static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n";
 
 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));
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers));
 
   const HeaderEntry* method = headers.Method();
   const HeaderEntry* path = headers.Path();

source/common/http/http1/codec_impl.h

@@ -133,8 +133,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder {
   bool startedResponse() { return started_response_; }
 
   // Http::ResponseEncoder
-  void encode100ContinueHeaders(const ResponseHeaderMap& headers) override;
-  void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
+  Http::Status encode100ContinueHeaders(const ResponseHeaderMap& headers) override;
+  Http::Status encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
   void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); }
 
   bool streamErrorOnInvalidHttpMessage() const override {

source/common/http/http2/codec_impl.cc

@@ -24,6 +24,7 @@
 #include "common/http/header_utility.h"
 #include "common/http/headers.h"
 #include "common/http/http2/codec_stats.h"
+#include "common/http/status.h"
 #include "common/http/utility.h"
 #include "common/runtime/runtime_features.h"
 
@@ -172,9 +173,10 @@ void ConnectionImpl::StreamImpl::buildHeaders(std::vector<nghttp2_nv>& final_hea
   });
 }
 
-void ConnectionImpl::ServerStreamImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) {
+Http::Status
+ConnectionImpl::ServerStreamImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) {
   ASSERT(headers.Status()->value() == "100");
-  encodeHeaders(headers, false);
+  return encodeHeaders(headers, false);
 }
 
 void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector<nghttp2_nv>& final_headers,
@@ -201,7 +203,7 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h
                                                        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));
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(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;
@@ -231,10 +233,10 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h
   return okStatus();
 }
 
-void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers,
-                                                     bool end_stream) {
-  // The contract is that client codecs must ensure that :status is present.
-  ASSERT(headers.Status() != nullptr);
+Http::Status ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers,
+                                                             bool end_stream) {
+  // The contract is that client codecs must ensure that required headers are present.
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredResponseHeaders(headers));
 
   // This must exist outside of the scope of isUpgrade as the underlying memory is
   // needed until encodeHeadersBase has been called.
@@ -248,6 +250,8 @@ void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& he
     buildHeaders(final_headers, headers);
   }
   encodeHeadersBase(final_headers, end_stream);
+
+  return Http::okStatus();
 }
 
 void ConnectionImpl::StreamImpl::encodeTrailersBase(const HeaderMap& trailers) {

source/common/http/http2/codec_impl.h

@@ -411,8 +411,8 @@ class ConnectionImpl : public virtual Connection,
     void createPendingFlushTimer() override;
 
     // ResponseEncoder
-    void encode100ContinueHeaders(const ResponseHeaderMap& headers) override;
-    void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
+    Http::Status encode100ContinueHeaders(const ResponseHeaderMap& headers) override;
+    Http::Status encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
     void encodeTrailers(const ResponseTrailerMap& trailers) override {
       encodeTrailersBase(trailers);
     }

source/common/quic/envoy_quic_client_stream.cc

@@ -57,7 +57,7 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
                                                   bool end_stream) {
   // Required headers must be present. This can only happen by some erroneous processing after the
   // downstream codecs decode.
-  RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredHeaders(headers));
+  RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredRequestHeaders(headers));
 
   ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers);
   local_end_stream_ = end_stream;

source/common/quic/envoy_quic_server_stream.cc

@@ -5,6 +5,8 @@
 
 #include <memory>
 
+#include "common/http/status.h"
+
 #if defined(__GNUC__)
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-parameter"
@@ -64,12 +66,15 @@ EnvoyQuicServerStream::EnvoyQuicServerStream(
           stats, http3_options),
       headers_with_underscores_action_(headers_with_underscores_action) {}
 
-void EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) {
+Http::Status
+EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) {
   ASSERT(headers.Status()->value() == "100");
-  encodeHeaders(headers, false);
+  return encodeHeaders(headers, false);
 }
 
-void EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers, bool end_stream) {
+Http::Status EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers,
+                                                  bool end_stream) {
+  RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredResponseHeaders(headers));
   ENVOY_STREAM_LOG(debug, "encodeHeaders (end_stream={}) {}.", *this, end_stream, headers);
   // QUICHE guarantees to take all the headers. This could cause infinite data to
   // be buffered on headers stream in Google QUIC implementation because
@@ -83,6 +88,7 @@ void EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers
   local_end_stream_ = end_stream;
   SendBufferMonitor::ScopedWatermarkBufferUpdater updater(this, this);
   WriteHeaders(envoyHeadersToSpdyHeaderBlock(headers), end_stream, nullptr);
+  return Http::okStatus();
 }
 
 void EnvoyQuicServerStream::encodeData(Buffer::Instance& data, bool end_stream) {