diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index f397882e3199..3e20f3b449ae 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/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 + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 7; } // [#next-free-field: 15] diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 746d53c557ee..19e5de6d8b1a 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/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.config.core.v3.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 + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 7; } // [#next-free-field: 15] diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 04a132ad2672..ebb110fcc202 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/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 + // ` or the new HTTP/2 option // :ref:`override_stream_error_on_invalid_http_message // ` // *not* the deprecated but similarly named :ref:`stream_error_on_invalid_http_messaging diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 042a39863f81..631913354644 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -551,7 +551,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 + // ` or the new HTTP/2 option // :ref:`override_stream_error_on_invalid_http_message // ` // *not* the deprecated but similarly named :ref:`stream_error_on_invalid_http_messaging diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 1d4897b5c87e..3a1f0fe1bafd 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -18,6 +18,7 @@ Minor Behavior Changes * http: added :ref:`contains ` a new string matcher type which matches if the value of the string has the substring mentioned in contains matcher. * http: added :ref:`contains ` 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 ` to :ref:`local reply mapper ` 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 ` 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 ` 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 ` 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 ` to true to restore prior HTTP/1.1 beavior and setting the *new* HTTP/2 configuration :ref:`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 ` 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. diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index f397882e3199..3e20f3b449ae 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/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 + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 7; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 5afb71210fbb..44314f61fbae 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/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.config.core.v3.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 + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 7; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 0439633d6e6e..4b038f1eb6d6 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -554,7 +554,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 + // ` or the new HTTP/2 option // :ref:`override_stream_error_on_invalid_http_message // ` // *not* the deprecated but similarly named :ref:`stream_error_on_invalid_http_messaging diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 042a39863f81..631913354644 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -551,7 +551,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 + // ` or the new HTTP/2 option // :ref:`override_stream_error_on_invalid_http_message // ` // *not* the deprecated but similarly named :ref:`stream_error_on_invalid_http_messaging diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 23d94197585a..877313e5f849 100644 --- a/include/envoy/http/codec.h +++ b/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}; }; /** diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 4852ab044d78..1d90f8a869e7 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/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; } } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 16575bf9b98f..0b66ea129171 100644 --- a/source/common/http/http1/codec_impl.h +++ b/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_{}; diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 642c504a2258..01c8a51aea25 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/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_{}; diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index ce8e6f809a0d..dbc13068346a 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -391,6 +391,10 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; + + bool streamErrorOnInvalidHttpMessage() const override { + return parent_.stream_error_on_invalid_http_messaging_; + } }; using ServerStreamImplPtr = std::unique_ptr; diff --git a/source/common/http/http2/codec_impl_legacy.h b/source/common/http/http2/codec_impl_legacy.h index 47065d643806..2b6db85cf731 100644 --- a/source/common/http/http2/codec_impl_legacy.h +++ b/source/common/http/http2/codec_impl_legacy.h @@ -359,7 +359,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, @@ -391,6 +393,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable 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; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index f3509958c453..d612a55898c3 100644 --- a/source/common/http/utility.cc +++ b/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( diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 6754c9b0179c..691960b4f437 100644 --- a/source/common/http/utility.h +++ b/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 modify_headers_; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 29078e4f59c5..82a65448222f 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/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( diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h index a9393a1761ff..060cb237d669 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h +++ b/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; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index e105d94082df..408ede16d548 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -847,6 +847,7 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { data.drain(4); return Http::okStatus(); })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(true)); // This test also verifies that decoder/encoder filters have onDestroy() called only once. auto* filter = new MockStreamFilter(); @@ -856,7 +857,6 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { @@ -2167,6 +2167,62 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { conn_manager_->onData(fake_input, false); } +class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest { +public: + void sendInvalidRequestAndVerifyConnectionState(bool stream_error_on_invalid_http_message) { + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { + RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + // These request headers are missing the necessary ":host" + RequestHeaderMapPtr headers{ + new TestRequestHeaderMapImpl{{":method", "GET"}, {":path", "/"}}}; + decoder->decodeHeaders(std::move(headers), true); + data.drain(0); + return Http::okStatus(); + })); + + auto* filter = new MockStreamFilter(); + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillOnce(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(StreamFilterSharedPtr{filter}); + })); + EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); + EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); + + // codec stream error + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(stream_error_on_invalid_http_message)); + EXPECT_CALL(*filter, encodeHeaders(_, true)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ("400", headers.getStatusValue()); + EXPECT_EQ("missing_host_header", + filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); + if (!stream_error_on_invalid_http_message) { + EXPECT_NE(nullptr, headers.Connection()); + EXPECT_EQ("close", headers.getConnectionValue()); + } else { + EXPECT_EQ(nullptr, headers.Connection()); + } + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); + } +}; + +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecStreamErrorIsFalse) { + sendInvalidRequestAndVerifyConnectionState(false); +} + +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionOpenIfCodecStreamErrorIsTrue) { + sendInvalidRequestAndVerifyConnectionState(true); +} + TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) { setup(true, ""); @@ -5040,6 +5096,7 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { return FilterHeadersStatus::Continue; })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(true)); EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, true)) .WillOnce(Invoke([&](ResponseHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_EQ("11", headers.getContentLengthValue()); @@ -5081,6 +5138,7 @@ TEST_F(HttpConnectionManagerImplTest, ResetWithStoppedFilter) { return FilterHeadersStatus::Continue; })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(true)); EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) .WillOnce(Invoke([&](ResponseHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_EQ("11", headers.getContentLengthValue()); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 9cd59c74c5c0..0a78208255ef 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -671,6 +671,72 @@ TEST_P(Http1ServerConnectionImplTest, InnerLWSIsPreserved) { } } +TEST_P(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfTrue) { + codec_settings_.stream_error_on_invalid_http_message_ = true; + if (GetParam()) { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + } else { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + } + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n"); + NiceMock decoder; + Http::ResponseEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(response_encoder->streamErrorOnInvalidHttpMessage()); +} + +TEST_P(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfFalse) { + codec_settings_.stream_error_on_invalid_http_message_ = false; + if (GetParam()) { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + } else { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + } + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n"); + NiceMock decoder; + Http::ResponseEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + auto status = codec_->dispatch(buffer); + EXPECT_FALSE(response_encoder->streamErrorOnInvalidHttpMessage()); +} + +TEST_P(Http1ServerConnectionImplTest, CodecHasDefaultStreamErrorIfNotSet) { + initialize(); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n"); + NiceMock decoder; + Http::ResponseEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + auto status = codec_->dispatch(buffer); + EXPECT_FALSE(response_encoder->streamErrorOnInvalidHttpMessage()); +} + TEST_P(Http1ServerConnectionImplTest, Http10) { initialize(); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 0c5c6d68f8c6..9b4fdeccc1b9 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -493,6 +493,29 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFinAllowed) { expectDetailsRequest("http2.violation.of.messaging.rule"); } +TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfFalse) { + initialize(); + + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); + + EXPECT_FALSE(response_encoder_->streamErrorOnInvalidHttpMessage()); +} + +TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfTrue) { + stream_error_on_invalid_http_messaging_ = true; + initialize(); + + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); + + EXPECT_TRUE(response_encoder_->streamErrorOnInvalidHttpMessage()); +} + TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { initialize(); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 687c5255ee36..ec31fee046a0 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -421,6 +421,39 @@ TEST(HttpUtility, ValidateStreamErrorsWithHcm) { } } +TEST(HttpUtility, ValidateStreamErrorConfigurationForHttp1) { + envoy::config::core::v3::Http1ProtocolOptions http1_options; + Protobuf::BoolValue hcm_value; + + // nothing explicitly configured, default to false (i.e. default stream error behavior for HCM) + EXPECT_FALSE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); + + // http1_options.stream_error overrides HCM.stream_error + http1_options.mutable_override_stream_error_on_invalid_http_message()->set_value(true); + hcm_value.set_value(false); + EXPECT_TRUE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); + + // http1_options.stream_error overrides HCM.stream_error (flip boolean value) + http1_options.mutable_override_stream_error_on_invalid_http_message()->set_value(false); + hcm_value.set_value(true); + EXPECT_FALSE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); + + http1_options.clear_override_stream_error_on_invalid_http_message(); + + // fallback to HCM.stream_error + hcm_value.set_value(true); + EXPECT_TRUE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); + + // fallback to HCM.stream_error (flip boolean value) + hcm_value.set_value(false); + EXPECT_FALSE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); +} + TEST(HttpUtility, getLastAddressFromXFF) { { const std::string first_address = "192.0.2.10"; diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index db174db5cd2e..9f74fc8c4e77 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1573,4 +1573,68 @@ TEST_P(IntegrationTest, QuitQuitQuit) { test_server_->useAdminInterfaceToQuit(true); } +// override_stream_error_on_invalid_http_message=true and HCM +// stream_error_on_invalid_http_message=false: test that HTTP/1.1 connection is left open on invalid +// HTTP message (missing :host header) +TEST_P(IntegrationTest, ConnectionIsLeftOpenIfHCMStreamErrorIsFalseAndOverrideIsTrue) { + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_stream_error_on_invalid_http_message()->set_value(false); + hcm.mutable_http_protocol_options() + ->mutable_override_stream_error_on_invalid_http_message() + ->set_value(true); + }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, {":path", "/test/long/url"}, {"content-length", "0"}}); + auto response = std::move(encoder_decoder.second); + + ASSERT_FALSE(codec_client_->waitForDisconnect()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().getStatusValue()); +} + +// override_stream_error_on_invalid_http_message is not set and HCM +// stream_error_on_invalid_http_message=true: test that HTTP/1.1 connection is left open on invalid +// HTTP message (missing :host header) +TEST_P(IntegrationTest, ConnectionIsLeftOpenIfHCMStreamErrorIsTrueAndOverrideNotSet) { + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.mutable_stream_error_on_invalid_http_message()->set_value(true); }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, {":path", "/test/long/url"}, {"content-length", "0"}}); + auto response = std::move(encoder_decoder.second); + + ASSERT_FALSE(codec_client_->waitForDisconnect()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().getStatusValue()); +} + +// override_stream_error_on_invalid_http_message is not set and HCM +// stream_error_on_invalid_http_message=false: test that HTTP/1.1 connection is terminated on +// invalid HTTP message (missing :host header) +TEST_P(IntegrationTest, ConnectionIsTerminatedIfHCMStreamErrorIsFalseAndOverrideNotSet) { + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_stream_error_on_invalid_http_message()->set_value(false); + }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, {":path", "/test/long/url"}, {"content-length", "0"}}); + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(codec_client_->waitForDisconnect()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().getStatusValue()); +} + } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 684cff7251a1..77b282b20be1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1132,7 +1132,6 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLength) { } } -// TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc. TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& @@ -1140,6 +1139,9 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) { hcm.mutable_http2_protocol_options() ->mutable_override_stream_error_on_invalid_http_message() ->set_value(true); + hcm.mutable_http_protocol_options() + ->mutable_override_stream_error_on_invalid_http_message() + ->set_value(true); }); initialize(); diff --git a/test/mocks/http/stream_encoder.h b/test/mocks/http/stream_encoder.h index fa302cdefbe2..d2c682889ea3 100644 --- a/test/mocks/http/stream_encoder.h +++ b/test/mocks/http/stream_encoder.h @@ -49,6 +49,7 @@ class MockResponseEncoder : public ResponseEncoder { MOCK_METHOD(void, encodeData, (Buffer::Instance & data, bool end_stream)); MOCK_METHOD(void, encodeMetadata, (const MetadataMapVector& metadata_map_vector)); MOCK_METHOD(Http1StreamEncoderOptionsOptRef, http1StreamEncoderOptions, ()); + MOCK_METHOD(bool, streamErrorOnInvalidHttpMessage, (), (const)); MOCK_METHOD(Stream&, getStream, (), ()); testing::NiceMock stream_;