http: stream error on invalid HTTP messaging for HTTP/1 (#11884)

Signed-off-by: Erica Manno <[email protected]>

api/envoy/config/core/v3/protocol.proto

@@ -100,7 +100,7 @@ message HttpProtocolOptions {
   HeadersWithUnderscoresAction headers_with_underscores_action = 5;
 }
 
-// [#next-free-field: 7]
+// [#next-free-field: 8]
 message Http1ProtocolOptions {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.api.v2.core.Http1ProtocolOptions";
@@ -167,6 +167,14 @@ message Http1ProtocolOptions {
   //   Enabling this option might lead to request smuggling vulnerability, especially if traffic
   //   is proxied via multiple layers of proxies.
   bool allow_chunked_length = 6;
+
+  // Allows invalid HTTP messaging. When this option is false, then Envoy will terminate
+  // HTTP/1.1 connections upon receiving an invalid HTTP message. However,
+  // when this option is true, then Envoy will leave the HTTP/1.1 connection
+  // open where possible.
+  // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging
+  // <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>`.
+  google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 7;
 }
 
 // [#next-free-field: 15]

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -552,7 +552,8 @@ message HttpConnectionManager {
   // company-internal mesh) and false when receiving untrusted traffic (edge deployments).
   //
   // If different behaviors for invalid_http_message for HTTP/1 and HTTP/2 are
-  // desired, one *must* use the new HTTP/2 option
+  // desired, one should use the new HTTP/1 option :ref:`override_stream_error_on_invalid_http_message
+  // <envoy_v3_api_field_config.core.v3.Http1ProtocolOptions.override_stream_error_on_invalid_http_message>` or the new HTTP/2 option
   // :ref:`override_stream_error_on_invalid_http_message
   // <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.override_stream_error_on_invalid_http_message>`
   // *not* the deprecated but similarly named :ref:`stream_error_on_invalid_http_messaging

docs/root/version_history/current.rst

@@ -18,6 +18,7 @@ Minor Behavior Changes
 * http: added :ref:`contains <envoy_api_msg_type.matcher.StringMatcher>` a new string matcher type which matches if the value of the string has the substring mentioned in contains matcher.
 * http: added :ref:`contains <envoy_api_msg_route.HeaderMatcher>` a new header matcher type which matches if the value of the header has the substring mentioned in contains matcher.
 * http: added :ref:`headers_to_add <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.ResponseMapper.headers_to_add>` to :ref:`local reply mapper <config_http_conn_man_local_reply>` to allow its users to add/append/override response HTTP headers to local replies.
+* http: added HCM level configuration of :ref:`error handling on invalid messaging <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>` which substantially changes Envoy's behavior when encountering invalid HTTP/1.1 defaulting to closing the connection instead of allowing reuse. This can temporarily be reverted by setting `envoy.reloadable_features.hcm_stream_error_on_invalid_message` to false, or permanently reverted by setting the HTTP/1 configuration :ref:`override_stream_error_on_invalid_http_message <envoy_v3_api_field_config.core.v3.Http1ProtocolOptions.override_stream_error_on_invalid_http_message>` to true to restore prior HTTP/1.1 behavior (i.e. connection isn't terminated) and to retain prior HTTP/2 behavior (i.e. connection is terminated).
 * http: added HCM level configuration of :ref:`error handling on invalid messaging <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>` which substantially changes Envoy's behavior when encountering invalid HTTP/1.1 defaulting to closing the connection instead of allowing reuse. This can temporarily be reverted by setting `envoy.reloadable_features.hcm_stream_error_on_invalid_message` to false, or permanently reverted by setting the :ref:`HCM option <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>` to true to restore prior HTTP/1.1 beavior and setting the *new* HTTP/2 configuration :ref:`override_stream_error_on_invalid_http_message <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.override_stream_error_on_invalid_http_message>` to false to retain prior HTTP/2 behavior.
 * http: changed Envoy to send GOAWAY to HTTP2 downstreams when the :ref:`disable_keepalive <config_overload_manager_overload_actions>` overload action is active. This behavior may be temporarily reverted by setting `envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2` to false.
 * http: changed Envoy to send error headers and body when possible. This behavior may be temporarily reverted by setting `envoy.reloadable_features.allow_response_for_timeout` to false.

include/envoy/http/codec.h

@@ -142,6 +142,12 @@ class ResponseEncoder : public virtual StreamEncoder {
    * @param trailers supplies the trailers to encode.
    */
   virtual void encodeTrailers(const ResponseTrailerMap& trailers) PURE;
+
+  /**
+   * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection
+   * error.
+   */
+  virtual bool streamErrorOnInvalidHttpMessage() const PURE;
 };
 
 /**
@@ -390,6 +396,11 @@ struct Http1Settings {
 
   // How header keys should be formatted when serializing HTTP/1.1 headers.
   HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default};
+
+  // Behaviour on invalid HTTP messaging:
+  // - if true, the HTTP/1.1 connection is left open (where possible)
+  // - if false, the HTTP/1.1 connection is terminated
+  bool stream_error_on_invalid_http_message_{false};
 };
 
 /**

source/common/http/conn_manager_impl.cc

@@ -1167,8 +1167,8 @@ void ConnectionManagerImpl::ActiveStream::onLocalReply(Code code) {
   // The BadRequest error code indicates there has been a messaging error.
   if (Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.hcm_stream_error_on_invalid_message") &&
-      !connection_manager_.config_.streamErrorOnInvalidHttpMessaging() &&
-      code == Http::Code::BadRequest && connection_manager_.codec_->protocol() < Protocol::Http2) {
+      code == Http::Code::BadRequest && connection_manager_.codec_->protocol() < Protocol::Http2 &&
+      !response_encoder_->streamErrorOnInvalidHttpMessage()) {
     state_.saw_connection_close_ = true;
   }
 }

source/common/http/http1/codec_impl.h

@@ -123,8 +123,10 @@ class StreamEncoderImpl : public virtual StreamEncoder,
  */
 class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder {
 public:
-  ResponseEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter)
-      : StreamEncoderImpl(connection, header_key_formatter) {}
+  ResponseEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter,
+                      bool stream_error_on_invalid_http_message)
+      : StreamEncoderImpl(connection, header_key_formatter),
+        stream_error_on_invalid_http_message_(stream_error_on_invalid_http_message) {}
 
   bool startedResponse() { return started_response_; }
 
@@ -133,8 +135,13 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder {
   void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
   void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); }
 
+  bool streamErrorOnInvalidHttpMessage() const override {
+    return stream_error_on_invalid_http_message_;
+  }
+
 private:
   bool started_response_{};
+  const bool stream_error_on_invalid_http_message_;
 };
 
 /**
@@ -462,8 +469,9 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
    * An active HTTP/1.1 request.
    */
   struct ActiveRequest {
-    ActiveRequest(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter)
-        : response_encoder_(connection, header_key_formatter) {}
+    ActiveRequest(ServerConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter)
+        : response_encoder_(connection, header_key_formatter,
+                            connection.codec_settings_.stream_error_on_invalid_http_message_) {}
 
     HeaderString request_url_;
     RequestDecoder* request_decoder_{};

source/common/http/http1/codec_impl_legacy.h

@@ -126,8 +126,10 @@ class StreamEncoderImpl : public virtual StreamEncoder,
 class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder {
 public:
   ResponseEncoderImpl(ConnectionImpl& connection,
-                      Http::Http1::HeaderKeyFormatter* header_key_formatter)
-      : StreamEncoderImpl(connection, header_key_formatter) {}
+                      Http::Http1::HeaderKeyFormatter* header_key_formatter,
+                      bool stream_error_on_invalid_http_message)
+      : StreamEncoderImpl(connection, header_key_formatter),
+        stream_error_on_invalid_http_message_(stream_error_on_invalid_http_message) {}
 
   bool startedResponse() { return started_response_; }
 
@@ -136,8 +138,13 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder {
   void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
   void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); }
 
+  bool streamErrorOnInvalidHttpMessage() const override {
+    return stream_error_on_invalid_http_message_;
+  }
+
 private:
   bool started_response_{};
+  const bool stream_error_on_invalid_http_message_;
 };
 
 /**
@@ -436,8 +443,10 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
    * An active HTTP/1.1 request.
    */
   struct ActiveRequest {
-    ActiveRequest(ConnectionImpl& connection, Http::Http1::HeaderKeyFormatter* header_key_formatter)
-        : response_encoder_(connection, header_key_formatter) {}
+    ActiveRequest(ServerConnectionImpl& connection,
+                  Http::Http1::HeaderKeyFormatter* header_key_formatter)
+        : response_encoder_(connection, header_key_formatter,
+                            connection.codec_settings_.stream_error_on_invalid_http_message_) {}
 
     HeaderString request_url_;
     RequestDecoder* request_decoder_{};

source/common/http/http2/codec_impl.h

@@ -391,6 +391,10 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 
     RequestDecoder* request_decoder_{};
     absl::variant<RequestHeaderMapPtr, RequestTrailerMapPtr> headers_or_trailers_;
+
+    bool streamErrorOnInvalidHttpMessage() const override {
+      return parent_.stream_error_on_invalid_http_messaging_;
+    }
   };
 
   using ServerStreamImplPtr = std::unique_ptr<ServerStreamImpl>;

source/common/http/http2/codec_impl_legacy.h

@@ -359,7 +359,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
    */
   struct ServerStreamImpl : public StreamImpl, public ResponseEncoder {
     ServerStreamImpl(ConnectionImpl& parent, uint32_t buffer_limit)
-        : StreamImpl(parent, buffer_limit), headers_or_trailers_(RequestHeaderMapImpl::create()) {}
+        : StreamImpl(parent, buffer_limit), headers_or_trailers_(RequestHeaderMapImpl::create()) {
+      stream_error_on_invalid_http_message_ = parent.stream_error_on_invalid_http_messaging_;
+    }
 
     // StreamImpl
     void submitHeaders(const std::vector<nghttp2_nv>& final_headers,
@@ -391,6 +393,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 
     RequestDecoder* request_decoder_{};
     absl::variant<RequestHeaderMapPtr, RequestTrailerMapPtr> headers_or_trailers_;
+    bool stream_error_on_invalid_http_message_;
+
+    bool streamErrorOnInvalidHttpMessage() const override {
+      return stream_error_on_invalid_http_message_;
+    }
   };
 
   using ServerStreamImplPtr = std::unique_ptr<ServerStreamImpl>;

source/common/http/utility.cc

@@ -415,6 +415,24 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions&
   return ret;
 }
 
+Http1Settings
+Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config,
+                            const Protobuf::BoolValue& hcm_stream_error) {
+  Http1Settings ret = parseHttp1Settings(config);
+
+  if (config.has_override_stream_error_on_invalid_http_message()) {
+    // override_stream_error_on_invalid_http_message, if set, takes precedence over any HCM
+    // stream_error_on_invalid_http_message
+    ret.stream_error_on_invalid_http_message_ =
+        config.override_stream_error_on_invalid_http_message().value();
+  } else {
+    // fallback to HCM value
+    ret.stream_error_on_invalid_http_message_ = hcm_stream_error.value();
+  }
+
+  return ret;
+}
+
 void Utility::sendLocalReply(const bool& is_reset, StreamDecoderFilterCallbacks& callbacks,
                              const LocalReplyData& local_reply_data) {
   sendLocalReply(

source/common/http/utility.h

@@ -269,6 +269,9 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers);
  */
 Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config);
 
+Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config,
+                                 const Protobuf::BoolValue& hcm_stream_error);
+
 struct EncodeFunctions {
   // Function to modify locally generated response headers.
   std::function<void(ResponseHeaderMap& headers)> modify_headers_;

source/extensions/filters/network/http_connection_manager/config.cc

@@ -207,7 +207,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
       http2_options_(Http2::Utility::initializeAndValidateOptions(
           config.http2_protocol_options(), config.has_stream_error_on_invalid_http_message(),
           config.stream_error_on_invalid_http_message())),
-      http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())),
+      http1_settings_(Http::Utility::parseHttp1Settings(
+          config.http_protocol_options(), config.stream_error_on_invalid_http_message())),
       max_request_headers_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
           config, max_request_headers_kb, Http::DEFAULT_MAX_REQUEST_HEADERS_KB)),
       max_request_headers_count_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h

@@ -36,6 +36,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase,
   Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override {
     return absl::nullopt;
   }
+  bool streamErrorOnInvalidHttpMessage() const override { return false; }
 
   // Http::Stream
   void resetStream(Http::StreamResetReason reason) override;