From ee7cb0d92712f59fcaf8d1fd006b1e18d237ca4c Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 3 Jul 2020 15:59:20 +0200 Subject: [PATCH 01/40] Add config for behavior on HTTP invalid message to HTTP1 options. Also, extend codec interface so that hcm can pull this new option from the codec. Signed-off-by: Erica Manno --- api/envoy/api/v2/core/protocol.proto | 6 +++++- api/envoy/config/core/v3/protocol.proto | 6 +++++- api/envoy/config/core/v4alpha/protocol.proto | 6 +++++- .../envoy/api/v2/core/protocol.proto | 6 +++++- .../envoy/config/core/v3/protocol.proto | 6 +++++- .../envoy/config/core/v4alpha/protocol.proto | 6 +++++- source/common/http/http1/codec_impl.h | 17 +++++++++++++---- source/common/http/http2/codec_impl.h | 9 ++++++++- source/common/http/utility.cc | 5 +++++ 9 files changed, 56 insertions(+), 11 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 9c47e388ee1a..39b88ca3f2a4 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -93,7 +93,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { message HeaderKeyFormat { message ProperCaseWords { @@ -142,6 +142,10 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // [#not-implemented-hide:] + // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 339601feab3d..3b0ab578940e 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: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http1ProtocolOptions"; @@ -157,6 +157,10 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // [#not-implemented-hide:] + // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 2ec9244124bd..57571c2121f2 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: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.Http1ProtocolOptions"; @@ -157,6 +157,10 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // [#not-implemented-hide:] + // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 9c47e388ee1a..39b88ca3f2a4 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -93,7 +93,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { message HeaderKeyFormat { message ProperCaseWords { @@ -142,6 +142,10 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // [#not-implemented-hide:] + // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 339601feab3d..3b0ab578940e 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: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http1ProtocolOptions"; @@ -157,6 +157,10 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // [#not-implemented-hide:] + // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 2ec9244124bd..57571c2121f2 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: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.Http1ProtocolOptions"; @@ -157,6 +157,10 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // [#not-implemented-hide:] + // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index f21231d71fa5..260f6c43ae7e 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -123,8 +123,11 @@ 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, + absl::optional& 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 +136,13 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } + absl::optional streamErrorOnInvalidHTTPMessage() override { + return stream_error_on_invalid_http_message_; + } + private: bool started_response_{}; + absl::optional stream_error_on_invalid_http_message_; }; /** @@ -432,8 +440,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/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index cf848599c800..8d3e88171b71 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -347,7 +347,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, @@ -379,6 +381,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; + absl::optional stream_error_on_invalid_http_message_; + + absl::optional streamErrorOnInvalidHTTPMessage() 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 bee04a98cabe..2735f811c92e 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -399,6 +399,11 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& ret.header_key_format_ = Http1Settings::HeaderKeyFormat::Default; } + if (config.has_override_stream_error_on_invalid_http_message()) { + ret.stream_error_on_invalid_http_message_ = + config.override_stream_error_on_invalid_http_message().value(); + } + return ret; } From b04b98e4c101658f5e9590ce9673a0ee93283bfc Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 3 Jul 2020 16:12:55 +0200 Subject: [PATCH 02/40] Correct typo Signed-off-by: Erica Manno --- api/envoy/api/v2/core/protocol.proto | 2 +- api/envoy/config/core/v3/protocol.proto | 2 +- api/envoy/config/core/v4alpha/protocol.proto | 2 +- generated_api_shadow/envoy/api/v2/core/protocol.proto | 2 +- generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 39b88ca3f2a4..4482e6f28ee1 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -144,7 +144,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 3b0ab578940e..686ad72a2838 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -159,7 +159,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 57571c2121f2..4b3529120576 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -159,7 +159,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 39b88ca3f2a4..4482e6f28ee1 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -144,7 +144,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 57571c2121f2..4b3529120576 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -159,7 +159,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } From 46768beb9044b3ad6dd5588c58023dd4ad5cfb94 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 3 Jul 2020 16:24:37 +0200 Subject: [PATCH 03/40] Shadow API Signed-off-by: Erica Manno --- generated_api_shadow/envoy/config/core/v3/protocol.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 3b0ab578940e..686ad72a2838 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -159,7 +159,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messahing configured in the hcm + // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } From d1bf85dbe5b235604f37fd8ae9bc1c134491799e Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Thu, 9 Jul 2020 08:30:01 +0200 Subject: [PATCH 04/40] Extend codec to return behavior on invalid HTTP messaging Signed-off-by: Erica Manno --- include/envoy/http/codec.h | 7 +++++++ source/common/http/http1/codec_impl.h | 2 +- source/common/http/http2/codec_impl.h | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 46a9bf4e4f2b..84efd256af9c 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -142,6 +142,11 @@ 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 absl::optional streamErrorOnInvalidHttpMessage() PURE; }; /** @@ -388,6 +393,8 @@ struct Http1Settings { // How header keys should be formatted when serializing HTTP/1.1 headers. HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default}; + + absl::optional stream_error_on_invalid_http_message_{}; }; /** diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 260f6c43ae7e..7a3eca9862ee 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -136,7 +136,7 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } - absl::optional streamErrorOnInvalidHTTPMessage() override { + absl::optional streamErrorOnInvalidHttpMessage() override { return stream_error_on_invalid_http_message_; } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 8d3e88171b71..e730ecaf6dd0 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -383,7 +383,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; absl::optional stream_error_on_invalid_http_message_; - absl::optional streamErrorOnInvalidHTTPMessage() override { + absl::optional streamErrorOnInvalidHttpMessage() override { return stream_error_on_invalid_http_message_; } }; From 2d7e93d515c3503776ccab842edf8c2b343089a8 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Thu, 9 Jul 2020 08:42:52 +0200 Subject: [PATCH 05/40] Update API documentation Signed-off-by: Erica Manno --- api/envoy/api/v2/core/protocol.proto | 4 +++- api/envoy/config/core/v3/protocol.proto | 4 +++- api/envoy/config/core/v4alpha/protocol.proto | 4 +++- generated_api_shadow/envoy/api/v2/core/protocol.proto | 4 +++- generated_api_shadow/envoy/config/core/v3/protocol.proto | 4 +++- generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 4 +++- 6 files changed, 18 insertions(+), 6 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 4482e6f28ee1..9c6a1ab8c7f0 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -144,7 +144,9 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm + // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 686ad72a2838..decafc971ba5 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -159,7 +159,9 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm + // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 4b3529120576..5a5946f109d3 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -159,7 +159,9 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm + // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 4482e6f28ee1..9c6a1ab8c7f0 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -144,7 +144,9 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm + // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 686ad72a2838..decafc971ba5 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -159,7 +159,9 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm + // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 4b3529120576..5a5946f109d3 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -159,7 +159,9 @@ message Http1ProtocolOptions { bool enable_trailers = 5; // [#not-implemented-hide:] - // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm + // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } From 86031f03144425580fe1ce150e2511b18243863c Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Thu, 9 Jul 2020 09:34:09 +0200 Subject: [PATCH 06/40] Add TODO Signed-off-by: Erica Manno --- source/common/http/http2/codec_impl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index e730ecaf6dd0..35af927f339a 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -348,6 +348,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Thu, 9 Jul 2020 14:42:55 +0200 Subject: [PATCH 07/40] Add utility function that determines behavior on invalid HTTP messaging for HTTP/1.1 Signed-off-by: Erica Manno --- source/common/http/conn_manager_config.h | 6 ++++++ source/common/http/utility.cc | 9 +++++++++ source/common/http/utility.h | 4 ++++ .../network/http_connection_manager/config.cc | 1 + .../network/http_connection_manager/config.h | 5 +++++ source/server/admin/admin.h | 1 + test/common/http/utility_test.cc | 20 +++++++++++++++++++ 7 files changed, 46 insertions(+) diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index faf691b6fc20..b67afc95a64c 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -409,6 +409,12 @@ class ConnectionManagerConfig { */ virtual bool proxy100Continue() const PURE; + /** + * @return bool supplies if the HttpConnectionManager should handle invalid HTTP with a stream + * error or connection error. + */ + virtual bool streamErrorOnInvalidHttpMessaging() const PURE; + /** * @return supplies the http1 settings. */ diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 2735f811c92e..5083c3ecd653 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -407,6 +407,15 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& return ret; } +bool Utility::streamErrorOnInvalidHttpMessageForHttp1(bool hcm_stream_error_on_invalid_http_message, + const absl::optional& override_stream_error_on_invalid_http_message) { + if (override_stream_error_on_invalid_http_message.has_value()) { + return override_stream_error_on_invalid_http_message.value(); + } else { + return hcm_stream_error_on_invalid_http_message; + } +} + 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 4fef6cc23327..4794e6697ba8 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -265,6 +265,10 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); */ Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config); +bool streamErrorOnInvalidHttpMessageForHttp1( + const bool hcm_stream_error_on_invalid_http_message, + const absl::optional& override_stream_error_on_invalid_http_message); + struct EncodeFunctions { // Function to rewrite locally generated response. std::function, const absl::optional& userAgent() override { return user_agent_; } Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } + bool streamErrorOnInvalidHttpMessaging() const override { + return stream_error_on_invalid_http_messaging_; + } + const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } @@ -228,6 +232,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::DateProvider& date_provider_; Http::ConnectionManagerListenerStats listener_stats_; const bool proxy_100_continue_; + bool stream_error_on_invalid_http_messaging_; std::chrono::milliseconds delayed_close_timeout_; const bool normalize_path_; const bool merge_slashes_; diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 9c035d123e80..45d4c1f3db71 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -168,6 +168,7 @@ class AdminImpl : public Admin, const Http::TracingConnectionManagerConfig* tracingConfig() override { return nullptr; } Http::ConnectionManagerListenerStats& listenerStats() override { return listener_->stats_; } bool proxy100Continue() const override { return false; } + bool streamErrorOnInvalidHttpMessaging() const override { return false; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return true; } bool shouldMergeSlashes() const override { return true; } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 47c12303d0b6..d8f834a1da86 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -355,6 +355,26 @@ initial_connection_window_size: 65535 } } +TEST(HttpUtility, ValidateStreamErrorsWithHcmForHttp1) { + // If the override value is present, it will take precedence over the HCM value. + bool hcm_stream_error = false; + absl::optional override_stream_error = true; + EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + + hcm_stream_error = true; + override_stream_error = false; + EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + + // If the override value is not set, the HCM value will be used. + hcm_stream_error = false; + override_stream_error.reset(); + EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + + hcm_stream_error = true; + override_stream_error.reset(); + EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); +} + TEST(HttpUtility, getLastAddressFromXFF) { { const std::string first_address = "192.0.2.10"; From c139ff1d049ed8b17cc07b9723586c3b27e5811d Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Thu, 9 Jul 2020 15:05:23 +0200 Subject: [PATCH 08/40] Format code Signed-off-by: Erica Manno --- include/envoy/http/codec.h | 3 ++- source/common/http/utility.cc | 6 ++++-- source/common/http/utility.h | 6 +++--- .../network/http_connection_manager/config.cc | 4 ++-- test/common/http/utility_test.cc | 12 ++++++++---- 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 84efd256af9c..3fec74335da1 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -144,7 +144,8 @@ class ResponseEncoder : public virtual StreamEncoder { virtual void encodeTrailers(const ResponseTrailerMap& trailers) PURE; /** - * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection error. + * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection + * error. */ virtual absl::optional streamErrorOnInvalidHttpMessage() PURE; }; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5083c3ecd653..dc40a5d19076 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -19,6 +19,7 @@ #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/message_impl.h" +#include "common/http/utility.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" @@ -407,8 +408,9 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& return ret; } -bool Utility::streamErrorOnInvalidHttpMessageForHttp1(bool hcm_stream_error_on_invalid_http_message, - const absl::optional& override_stream_error_on_invalid_http_message) { +bool Utility::streamErrorOnInvalidHttpMessageForHttp1( + bool hcm_stream_error_on_invalid_http_message, + const absl::optional& override_stream_error_on_invalid_http_message) { if (override_stream_error_on_invalid_http_message.has_value()) { return override_stream_error_on_invalid_http_message.value(); } else { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 4794e6697ba8..fc0f269e16c7 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -265,9 +265,9 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); */ Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config); -bool streamErrorOnInvalidHttpMessageForHttp1( - const bool hcm_stream_error_on_invalid_http_message, - const absl::optional& override_stream_error_on_invalid_http_message); +bool streamErrorOnInvalidHttpMessageForHttp1( + const bool hcm_stream_error_on_invalid_http_message, + const absl::optional& override_stream_error_on_invalid_http_message); struct EncodeFunctions { // Function to rewrite locally generated response. diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 38298c7b3409..700723797132 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -1,5 +1,3 @@ -#include "extensions/filters/network/http_connection_manager/config.h" - #include #include #include @@ -33,6 +31,8 @@ #include "common/tracing/http_tracer_config_impl.h" #include "common/tracing/http_tracer_manager_impl.h" +#include "extensions/filters/network/http_connection_manager/config.h" + namespace Envoy { namespace Extensions { namespace NetworkFilters { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index d8f834a1da86..f7241d1a9d7c 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -359,20 +359,24 @@ TEST(HttpUtility, ValidateStreamErrorsWithHcmForHttp1) { // If the override value is present, it will take precedence over the HCM value. bool hcm_stream_error = false; absl::optional override_stream_error = true; - EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, + override_stream_error)); hcm_stream_error = true; override_stream_error = false; - EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, + override_stream_error)); // If the override value is not set, the HCM value will be used. hcm_stream_error = false; override_stream_error.reset(); - EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, + override_stream_error)); hcm_stream_error = true; override_stream_error.reset(); - EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, override_stream_error)); + EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, + override_stream_error)); } TEST(HttpUtility, getLastAddressFromXFF) { From 2d6cac883c067fd9868dfde08d867bfc6398af54 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 13 Jul 2020 18:13:40 +0200 Subject: [PATCH 09/40] Fix format Signed-off-by: Erica Manno --- source/common/http/utility.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index dc40a5d19076..d9cdf462f2ba 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -19,7 +19,6 @@ #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/message_impl.h" -#include "common/http/utility.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" From d30e8549cd425b2ff8cc51889b8a0c34ac15982f Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 17 Jul 2020 17:54:50 +0200 Subject: [PATCH 10/40] Add const Signed-off-by: Erica Manno --- include/envoy/http/codec.h | 2 +- source/common/http/http1/codec_impl.h | 2 +- source/common/http/http2/codec_impl.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 3fec74335da1..0a8828e1187c 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -147,7 +147,7 @@ class ResponseEncoder : public virtual StreamEncoder { * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection * error. */ - virtual absl::optional streamErrorOnInvalidHttpMessage() PURE; + virtual absl::optional streamErrorOnInvalidHttpMessage() const PURE; }; /** diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 7a3eca9862ee..77914b13e733 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -136,7 +136,7 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } - absl::optional streamErrorOnInvalidHttpMessage() override { + absl::optional streamErrorOnInvalidHttpMessage() const override { return stream_error_on_invalid_http_message_; } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 35af927f339a..e405dda0c654 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -384,7 +384,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; absl::optional stream_error_on_invalid_http_message_; - absl::optional streamErrorOnInvalidHttpMessage() override { + absl::optional streamErrorOnInvalidHttpMessage() const override { return stream_error_on_invalid_http_message_; } }; From aa421fddfc966f2b4182bf3a1d7ce3bfde6b718d Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 17 Jul 2020 18:01:48 +0200 Subject: [PATCH 11/40] Plug logic into HCM Signed-off-by: Erica Manno --- source/common/http/conn_manager_impl.cc | 8 ++++++++ test/common/http/conn_manager_impl_test.cc | 7 +++++++ test/mocks/http/stream_encoder.h | 1 + 3 files changed, 16 insertions(+) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fb9c36d5d730..23cfe8941e24 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1699,6 +1699,14 @@ void ConnectionManagerImpl::ActiveStream::encodeHeadersInternal(ResponseHeaderMa ENVOY_STREAM_LOG(debug, "drain closing connection", *this); } + if (Utility::getResponseStatus(headers) == enumToInt(Http::Code::BadRequest) && + connection_manager_.codec_->protocol() < Protocol::Http2 && + !Utility::streamErrorOnInvalidHttpMessageForHttp1( + connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), + response_encoder_->streamErrorOnInvalidHttpMessage())) { + state_.saw_connection_close_ = true; + } + if (connection_manager_.codec_->protocol() == Protocol::Http10) { // As HTTP/1.0 and below can not do chunked encoding, if there is no content // length the response will be framed by connection close. diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a22aa5432296..bbd4baef7be7 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -348,6 +348,9 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan const TracingConnectionManagerConfig* tracingConfig() override { return tracing_config_.get(); } ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } + bool streamErrorOnInvalidHttpMessaging() const override { + return stream_error_on_invalid_http_messaging_; + } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } @@ -410,6 +413,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan Stats::IsolatedStoreImpl fake_listener_stats_; ConnectionManagerListenerStats listener_stats_; bool proxy_100_continue_ = false; + bool stream_error_on_invalid_http_messaging_ = false; bool preserve_external_request_id_ = false; Http::Http1Settings http1_settings_; bool normalize_path_ = false; @@ -814,6 +818,7 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); EXPECT_CALL(*filter, encodeHeaders(_, true)); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(absl::optional(true))); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { EXPECT_EQ("400", headers.getStatusValue()); @@ -4854,6 +4859,7 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { return FilterHeadersStatus::Continue; })); EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(absl::optional(false))); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)); expectOnDestroy(); EXPECT_CALL(*decoder_filters_[0], decodeComplete()); @@ -4894,6 +4900,7 @@ TEST_F(HttpConnectionManagerImplTest, ResetWithStoppedFilter) { EXPECT_EQ("11", headers.getContentLengthValue()); return FilterHeadersStatus::Continue; })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(absl::optional(false))); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance&, bool) -> FilterDataStatus { diff --git a/test/mocks/http/stream_encoder.h b/test/mocks/http/stream_encoder.h index fa302cdefbe2..01a7402e4de6 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(absl::optional, streamErrorOnInvalidHttpMessage, (), (const)); MOCK_METHOD(Stream&, getStream, (), ()); testing::NiceMock stream_; From 97182460ed4538aa66604d1cd5aa0fb28660161a Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 17 Jul 2020 18:04:28 +0200 Subject: [PATCH 12/40] Format Signed-off-by: Erica Manno --- source/common/http/conn_manager_impl.cc | 4 ++-- test/common/http/conn_manager_impl_test.cc | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 23cfe8941e24..157fd92651d8 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1702,8 +1702,8 @@ void ConnectionManagerImpl::ActiveStream::encodeHeadersInternal(ResponseHeaderMa if (Utility::getResponseStatus(headers) == enumToInt(Http::Code::BadRequest) && connection_manager_.codec_->protocol() < Protocol::Http2 && !Utility::streamErrorOnInvalidHttpMessageForHttp1( - connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), - response_encoder_->streamErrorOnInvalidHttpMessage())) { + connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), + response_encoder_->streamErrorOnInvalidHttpMessage())) { state_.saw_connection_close_ = true; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index bbd4baef7be7..f8e55461143c 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -818,7 +818,8 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); EXPECT_CALL(*filter, encodeHeaders(_, true)); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(absl::optional(true))); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(true))); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { EXPECT_EQ("400", headers.getStatusValue()); @@ -4859,7 +4860,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { return FilterHeadersStatus::Continue; })); EXPECT_CALL(*encoder_filters_[0], encodeComplete()); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(absl::optional(false))); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(false))); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)); expectOnDestroy(); EXPECT_CALL(*decoder_filters_[0], decodeComplete()); @@ -4900,7 +4902,8 @@ TEST_F(HttpConnectionManagerImplTest, ResetWithStoppedFilter) { EXPECT_EQ("11", headers.getContentLengthValue()); return FilterHeadersStatus::Continue; })); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(absl::optional(false))); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(false))); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance&, bool) -> FilterDataStatus { From cc33ccf5ba7e875fe8e370c0335a7fc7d7a87d52 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Sun, 19 Jul 2020 14:25:20 +0200 Subject: [PATCH 13/40] Add unit test for HCM logic Signed-off-by: Erica Manno --- test/common/http/conn_manager_impl_test.cc | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index f8e55461143c..91abe0e36789 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2092,6 +2092,84 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { conn_manager_->onData(fake_input, false); } +TEST_F(HttpConnectionManagerImplTest, TestConnectionTerminatedIfNotStreamError) { + 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(_)); + + EXPECT_CALL(*filter, encodeHeaders(_, true)); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(false))); + 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()); + EXPECT_EQ("close", headers.getConnectionValue()); + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); +} + +TEST_F(HttpConnectionManagerImplTest, TestConnectionIsLeftOpenIfStreamError) { + 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(_)); + + EXPECT_CALL(*filter, encodeHeaders(_, true)); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(false))); + 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()); + EXPECT_EQ(nullptr, headers.getConnectionValue()); + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); +} + TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) { setup(true, ""); From 00814d27e16e8bae5f6d29f68f0a19fb2b6e1b9e Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 20 Jul 2020 16:03:46 +0200 Subject: [PATCH 14/40] Add unit tests for HCM Signed-off-by: Erica Manno --- source/common/http/conn_manager_impl.cc | 17 +- source/common/http/http1/codec_impl_legacy.h | 5 + source/common/http/http2/codec_impl.h | 1 - source/common/http/http2/codec_impl_legacy.h | 9 +- source/common/http/utility.cc | 5 +- source/common/http/utility.h | 2 +- .../network/http_connection_manager/config.h | 4 - test/common/http/conn_manager_impl_test.cc | 148 ++++++++++++++++-- test/common/http/utility_test.cc | 16 +- 9 files changed, 166 insertions(+), 41 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index b2c89414099a..eed80e2e4713 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1,5 +1,3 @@ -#include "common/http/conn_manager_impl.h" - #include #include #include @@ -27,6 +25,7 @@ #include "common/common/scope_tracker.h" #include "common/common/utility.h" #include "common/http/codes.h" +#include "common/http/conn_manager_impl.h" #include "common/http/conn_manager_utility.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" @@ -1524,8 +1523,10 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( // 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 && + !Utility::streamErrorOnInvalidHttpMessage( + connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), + response_encoder_->streamErrorOnInvalidHttpMessage())) { state_.saw_connection_close_ = true; } @@ -1703,14 +1704,6 @@ void ConnectionManagerImpl::ActiveStream::encodeHeadersInternal(ResponseHeaderMa ENVOY_STREAM_LOG(debug, "drain closing connection", *this); } - if (Utility::getResponseStatus(headers) == enumToInt(Http::Code::BadRequest) && - connection_manager_.codec_->protocol() < Protocol::Http2 && - !Utility::streamErrorOnInvalidHttpMessageForHttp1( - connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), - response_encoder_->streamErrorOnInvalidHttpMessage())) { - state_.saw_connection_close_ = true; - } - if (connection_manager_.codec_->protocol() == Protocol::Http10) { // As HTTP/1.0 and below can not do chunked encoding, if there is no content // length the response will be framed by connection close. diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index f5e9811ede87..0dda2aa9cf67 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -136,8 +136,13 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } + absl::optional streamErrorOnInvalidHttpMessage() const override { + return stream_error_on_invalid_http_message_; + } + private: bool started_response_{}; + absl::optional stream_error_on_invalid_http_message_; }; /** diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 671c11044769..156d889db347 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -350,7 +350,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, @@ -382,6 +384,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; + absl::optional stream_error_on_invalid_http_message_; + + absl::optional 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 6599a6a51403..884694997e43 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -1,5 +1,3 @@ -#include "common/http/utility.h" - #include #include #include @@ -19,6 +17,7 @@ #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/message_impl.h" +#include "common/http/utility.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" #include "common/runtime/runtime_features.h" @@ -418,7 +417,7 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& return ret; } -bool Utility::streamErrorOnInvalidHttpMessageForHttp1( +bool Utility::streamErrorOnInvalidHttpMessage( bool hcm_stream_error_on_invalid_http_message, const absl::optional& override_stream_error_on_invalid_http_message) { if (override_stream_error_on_invalid_http_message.has_value()) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index bee9bf8b977f..7dac5d53fdd0 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -269,7 +269,7 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); */ Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config); -bool streamErrorOnInvalidHttpMessageForHttp1( +bool streamErrorOnInvalidHttpMessage( const bool hcm_stream_error_on_invalid_http_message, const absl::optional& override_stream_error_on_invalid_http_message); diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index f7baf04867d4..d2fef63dedb1 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -231,11 +231,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::DateProvider& date_provider_; Http::ConnectionManagerListenerStats listener_stats_; const bool proxy_100_continue_; -<<<<<<< HEAD - bool stream_error_on_invalid_http_messaging_; -======= const bool stream_error_on_invalid_http_messaging_; ->>>>>>> master std::chrono::milliseconds delayed_close_timeout_; const bool normalize_path_; const bool merge_slashes_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 10abe243949c..f3510c69a420 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -819,9 +819,9 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) .WillOnce(Return(absl::optional(true))); + EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { EXPECT_EQ("400", headers.getStatusValue()); @@ -2094,9 +2094,8 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { conn_manager_->onData(fake_input, false); } -TEST_F(HttpConnectionManagerImplTest, TestConnectionTerminatedIfNotStreamError) { +TEST_F(HttpConnectionManagerImplTest, ConnectionTerminatedIfCodecOverridesStreamError) { setup(false, ""); - EXPECT_CALL(*codec_, dispatch(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_); @@ -2117,14 +2116,17 @@ TEST_F(HttpConnectionManagerImplTest, TestConnectionTerminatedIfNotStreamError) EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - EXPECT_CALL(*filter, encodeHeaders(_, true)); + // stream_error from codec, if specified, determines whether the HTTP/1 connection is terminated + // or left open EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) .WillOnce(Return(absl::optional(false))); + 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()); + EXPECT_NE(nullptr, headers.Connection()); EXPECT_EQ("close", headers.getConnectionValue()); })); EXPECT_CALL(*filter, onDestroy()); @@ -2133,9 +2135,50 @@ TEST_F(HttpConnectionManagerImplTest, TestConnectionTerminatedIfNotStreamError) conn_manager_->onData(fake_input, false); } -TEST_F(HttpConnectionManagerImplTest, TestConnectionIsLeftOpenIfStreamError) { +TEST_F(HttpConnectionManagerImplTest, ConnectionOpenIfCodecOverridesStreamError) { 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(_)); + + // stream_error from codec, if specified, determines whether the HTTP/1 connection is terminated + // or left open + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(true))); + 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()); + EXPECT_EQ(nullptr, headers.Connection()); + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); +} +TEST_F(HttpConnectionManagerImplTest, ConnectionTerminatedIfCodecDoesntOverrideStreamError) { + setup(false, ""); + // HCM stream error + stream_error_on_invalid_http_messaging_ = false; EXPECT_CALL(*codec_, dispatch(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_); @@ -2156,15 +2199,98 @@ TEST_F(HttpConnectionManagerImplTest, TestConnectionIsLeftOpenIfStreamError) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); + // stream_error from codec is not specified, so stream_error from HCM determines whether the + // HTTP/1 connection is terminated or not + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional())); 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()); + EXPECT_NE(nullptr, headers.Connection()); + EXPECT_EQ("close", headers.getConnectionValue()); + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); +} + +TEST_F(HttpConnectionManagerImplTest, ConnectionLeftOpenIfCodecDoesntOverrideStreamError) { + setup(false, ""); + // HCM stream_error + stream_error_on_invalid_http_messaging_ = true; + 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(_)); + + // stream_error from codec is not specified, so stream_error from HCM determines whether the + // HTTP/1 connection is terminated or not EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(false))); + .WillOnce(Return(absl::optional())); + 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()); - EXPECT_EQ(nullptr, headers.getConnectionValue()); + EXPECT_EQ(nullptr, headers.Connection()); + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); +} + +TEST_F(HttpConnectionManagerImplTest, ConnectionLeftOpenIfOnCodecDoesntOverrideStreamError) { + 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(_)); + + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(true))); + 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()); + EXPECT_EQ(nullptr, headers.Connection()); })); EXPECT_CALL(*filter, onDestroy()); @@ -4934,14 +5060,14 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { return FilterHeadersStatus::Continue; })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(true))); EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, true)) .WillOnce(Invoke([&](ResponseHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_EQ("11", headers.getContentLengthValue()); return FilterHeadersStatus::Continue; })); EXPECT_CALL(*encoder_filters_[0], encodeComplete()); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(false))); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)); expectOnDestroy(); EXPECT_CALL(*decoder_filters_[0], decodeComplete()); @@ -4977,13 +5103,13 @@ TEST_F(HttpConnectionManagerImplTest, ResetWithStoppedFilter) { return FilterHeadersStatus::Continue; })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(true))); EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) .WillOnce(Invoke([&](ResponseHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_EQ("11", headers.getContentLengthValue()); return FilterHeadersStatus::Continue; })); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(false))); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance&, bool) -> FilterDataStatus { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 1b87da8ba001..836e2e769fea 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -361,24 +361,24 @@ TEST(HttpUtility, ValidateStreamErrorsWithHcmForHttp1) { // If the override value is present, it will take precedence over the HCM value. bool hcm_stream_error = false; absl::optional override_stream_error = true; - EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, - override_stream_error)); + EXPECT_TRUE( + Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); hcm_stream_error = true; override_stream_error = false; - EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, - override_stream_error)); + EXPECT_FALSE( + Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); // If the override value is not set, the HCM value will be used. hcm_stream_error = false; override_stream_error.reset(); - EXPECT_FALSE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, - override_stream_error)); + EXPECT_FALSE( + Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); hcm_stream_error = true; override_stream_error.reset(); - EXPECT_TRUE(Http::Utility::streamErrorOnInvalidHttpMessageForHttp1(hcm_stream_error, - override_stream_error)); + EXPECT_TRUE( + Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); } TEST(HttpUtility, ValidateStreamErrors) { From 7f61bdf5b7f889226431c1abe7b27c80c13610df Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 20 Jul 2020 16:37:47 +0200 Subject: [PATCH 15/40] Update API to remove hide-implementation annotation Signed-off-by: Erica Manno --- api/envoy/config/core/v3/protocol.proto | 1 - api/envoy/config/core/v4alpha/protocol.proto | 1 - generated_api_shadow/envoy/config/core/v3/protocol.proto | 1 - generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 1 - 4 files changed, 4 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index fcca14b3f147..13d38d292b28 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -158,7 +158,6 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // [#not-implemented-hide:] // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index ca1dbc56a74e..518af8806384 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -158,7 +158,6 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // [#not-implemented-hide:] // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index fcca14b3f147..13d38d292b28 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -158,7 +158,6 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // [#not-implemented-hide:] // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 1d074742723b..ddd378e0eccb 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -158,7 +158,6 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // [#not-implemented-hide:] // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` From 908bdfc6170b8c2713091e07bba621ac6ca8f2f4 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 20 Jul 2020 16:49:18 +0200 Subject: [PATCH 16/40] Remove changes from v2 API Signed-off-by: Erica Manno --- api/envoy/api/v2/core/protocol.proto | 8 +------- generated_api_shadow/envoy/api/v2/core/protocol.proto | 8 +------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 9c6a1ab8c7f0..9c47e388ee1a 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -93,7 +93,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 7] +// [#next-free-field: 6] message Http1ProtocolOptions { message HeaderKeyFormat { message ProperCaseWords { @@ -142,12 +142,6 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; - - // [#not-implemented-hide:] - // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate - // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging - // ` - google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 9c6a1ab8c7f0..9c47e388ee1a 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -93,7 +93,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 7] +// [#next-free-field: 6] message Http1ProtocolOptions { message HeaderKeyFormat { message ProperCaseWords { @@ -142,12 +142,6 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; - - // [#not-implemented-hide:] - // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate - // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging - // ` - google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } // [#next-free-field: 14] From e5239e8f8dc77ebb1d162428731156f5339d9ad0 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 22 Jul 2020 16:47:02 +0200 Subject: [PATCH 17/40] Code format headaches Signed-off-by: Erica Manno --- source/common/http/conn_manager_impl.cc | 3 ++- source/common/http/utility.cc | 3 ++- .../filters/network/http_connection_manager/config.cc | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index eed80e2e4713..85229b74c867 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1,3 +1,5 @@ +#include "common/http/conn_manager_impl.h" + #include #include #include @@ -25,7 +27,6 @@ #include "common/common/scope_tracker.h" #include "common/common/utility.h" #include "common/http/codes.h" -#include "common/http/conn_manager_impl.h" #include "common/http/conn_manager_utility.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 884694997e43..f8727fbdecba 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -1,3 +1,5 @@ +#include "common/common/utility.h" + #include #include #include @@ -11,7 +13,6 @@ #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" #include "common/common/fmt.h" -#include "common/common/utility.h" #include "common/grpc/status.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 49113e976b15..888709fe4251 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -1,3 +1,5 @@ +#include "extensions/filters/network/http_connection_manager/config.h" + #include #include #include @@ -33,8 +35,6 @@ #include "common/tracing/http_tracer_config_impl.h" #include "common/tracing/http_tracer_manager_impl.h" -#include "extensions/filters/network/http_connection_manager/config.h" - namespace Envoy { namespace Extensions { namespace NetworkFilters { From 4b2f72ad1bd42503048382bacad1f814fc244465 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 22 Jul 2020 17:34:51 +0200 Subject: [PATCH 18/40] Revisit API docs Signed-off-by: Erica Manno --- api/envoy/config/core/v3/protocol.proto | 7 +++++-- api/envoy/config/core/v4alpha/protocol.proto | 7 +++++-- generated_api_shadow/envoy/config/core/v3/protocol.proto | 7 +++++-- .../envoy/config/core/v4alpha/protocol.proto | 7 +++++-- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 13d38d292b28..1cd53e884020 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -158,8 +158,11 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate - // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. However, + // when this option is enabled, then Envoy will leave the HTTP/1.1 connection + // open where possible. + // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 518af8806384..79825f267c76 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -158,8 +158,11 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate - // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. However, + // when this option is enabled, then Envoy will leave the HTTP/1.1 connection + // open where possible. + // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 13d38d292b28..1cd53e884020 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -158,8 +158,11 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate - // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. However, + // when this option is enabled, then Envoy will leave the HTTP/1.1 connection + // open where possible. + // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index ddd378e0eccb..7d5693802c3d 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -158,8 +158,11 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate - // HTTP/1.1 connections upon receiving an invalid HTTP message. This overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // HTTP/1.1 connections upon receiving an invalid HTTP message. However, + // when this option is enabled, then Envoy will leave the HTTP/1.1 connection + // open where possible. + // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } From 922bb6fde51640ecb9df125920388d56a4adbd6d Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 24 Jul 2020 08:40:12 +0200 Subject: [PATCH 19/40] Add unit test for codec HTTP/1.1 Signed-off-by: Erica Manno --- source/common/http/http1/codec_impl_legacy.h | 13 ++-- source/common/http/http2/codec_impl.h | 7 +- test/common/http/http1/codec_impl_test.cc | 70 ++++++++++++++++++++ 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 0dda2aa9cf67..1d7f313cf2ac 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -126,8 +126,11 @@ 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, + absl::optional& 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_; } @@ -441,8 +444,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 156d889db347..bdc5873ddae9 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -349,9 +349,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, @@ -383,10 +381,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; - absl::optional stream_error_on_invalid_http_message_; absl::optional streamErrorOnInvalidHttpMessage() const override { - return stream_error_on_invalid_http_message_; + return parent_.stream_error_on_invalid_http_messaging_; } }; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 77565550dc26..6d1aee4b4b13 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -608,6 +608,76 @@ 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); + auto stream_error = response_encoder->streamErrorOnInvalidHttpMessage(); + EXPECT_TRUE(stream_error.has_value()); + EXPECT_TRUE(stream_error.value()); +} + +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); + auto stream_error = response_encoder->streamErrorOnInvalidHttpMessage(); + EXPECT_TRUE(stream_error.has_value()); + EXPECT_FALSE(stream_error.value()); +} + +TEST_P(Http1ServerConnectionImplTest, CodecHasNoStreamErrorIfNotSet) { + 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(); From 9dec35af2e54b2947004d10d2c83ab9631477892 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 24 Jul 2020 17:36:09 +0200 Subject: [PATCH 20/40] Add unit test for codec HTTP/2 Signed-off-by: Erica Manno --- test/common/http/http2/codec_impl_test.cc | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index fa1c92346e73..6a43a7da068c 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -420,6 +420,33 @@ 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); + + auto stream_error = response_encoder_->streamErrorOnInvalidHttpMessage(); + EXPECT_TRUE(stream_error.has_value()); + EXPECT_FALSE(stream_error.value()); +} + +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); + + auto stream_error = response_encoder_->streamErrorOnInvalidHttpMessage(); + EXPECT_TRUE(stream_error.has_value()); + EXPECT_TRUE(stream_error.value()); +} + TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { initialize(); From 5fb9be3ad9f5f81a86a558d1fc62b0933d716b16 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 27 Jul 2020 15:49:30 +0200 Subject: [PATCH 21/40] Integration tests for HTTP/1.1 Signed-off-by: Erica Manno --- test/common/http/http2/codec_impl_test.cc | 4 +- test/integration/protocol_integration_test.cc | 70 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 6a43a7da068c..9468d9773fde 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -422,7 +422,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFinAllowed) { TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfFalse) { initialize(); - + TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); @@ -436,7 +436,7 @@ TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfFalse) { 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)); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e41cd9eabe90..3d43f2348318 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1167,6 +1167,76 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { } } +TEST_P(DownstreamProtocolIntegrationTest, + Http1ConnectionIsLeftOpenIfHCMStreamErrorIsFalseAndOverrideIsTrue) { + if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { + return; + } + + 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()); +} + +TEST_P(DownstreamProtocolIntegrationTest, + Http1ConnectionIsLeftOpenIfHCMStreamErrorIsTrueAndOverrideNotSet) { + if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { + return; + } + + 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()); +} + +TEST_P(DownstreamProtocolIntegrationTest, + Http1ConnectionIsTerminatedIfHCMStreamErrorIsFalseAndOverrideNotSet) { + if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { + return; + } + + 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()); +} + // TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc. TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { config_helper_.addConfigModifier( From 83cbadf6b140ac8c44d878c39ae677ea71ba2395 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 27 Jul 2020 15:58:58 +0200 Subject: [PATCH 22/40] Integration test Signed-off-by: Erica Manno --- test/integration/protocol_integration_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 3d43f2348318..1a984244e267 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1117,6 +1117,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(); From c9c55bca17e13d2b5ca8e8cf26d0ad2beef2c242 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 27 Jul 2020 18:22:09 +0200 Subject: [PATCH 23/40] Formatting, again Signed-off-by: Erica Manno --- source/common/http/utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index f8727fbdecba..940a5f604137 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -1,4 +1,4 @@ -#include "common/common/utility.h" +#include "common/http/utility.h" #include #include @@ -13,12 +13,12 @@ #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" #include "common/common/fmt.h" +#include "common/common/utility.h" #include "common/grpc/status.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/message_impl.h" -#include "common/http/utility.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" #include "common/runtime/runtime_features.h" From a31323fc0a3f87fbd3b97d08050f8a533f4f4a99 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Tue, 28 Jul 2020 12:20:38 +0200 Subject: [PATCH 24/40] HTTP/3 stub out method for response encoder Signed-off-by: Erica Manno --- .../quic_listeners/quiche/envoy_quic_server_stream.h | 3 +++ 1 file changed, 3 insertions(+) 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..8d46422ffcca 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,9 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { return absl::nullopt; } + absl::optional streamErrorOnInvalidHttpMessage() const override { + return absl::optional{}; + } // Http::Stream void resetStream(Http::StreamResetReason reason) override; From 354c08b6e569896c28aad427d4872a747ff6be77 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 29 Jul 2020 19:08:55 +0200 Subject: [PATCH 25/40] Update HCM API docs and release notes Signed-off-by: Erica Manno --- .../http_connection_manager/v3/http_connection_manager.proto | 3 ++- .../v4alpha/http_connection_manager.proto | 3 ++- docs/root/version_history/current.rst | 2 +- .../http_connection_manager/v3/http_connection_manager.proto | 3 ++- .../v4alpha/http_connection_manager.proto | 3 ++- 5 files changed, 9 insertions(+), 5 deletions(-) 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 87e629f4f441..e2e74734f73f 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 @@ -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 *must* use the new HTTP/1 option :ref:`override_stream_error_on_invalid_http_message + // ` and 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 ac31bf1ecd62..f7082322b807 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 @@ -550,7 +550,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 *must* use the new HTTP/1 option :ref:`override_stream_error_on_invalid_http_message + // ` and 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 5acb53a13a3d..ee52dd8b5636 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -11,7 +11,7 @@ Minor Behavior Changes * compressor: always insert `Vary` headers for compressible resources even if it's decided not to compress a response due to incompatible `Accept-Encoding` value. The `Vary` header needs to be inserted to let a caching proxy in front of Envoy know that the requested resource still can be served with compression applied. * 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 :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: 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 beavior. * http: the per-stream FilterState maintained by the HTTP connection manager will now provide read/write access to the downstream connection FilterState. As such, code that relies on interacting with this might see a change in behavior. * logging: nghttp2 log messages no longer appear at trace level unless `ENVOY_NGHTTP2_TRACE` is set 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 a25759c85fc7..282dbe421dc1 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 @@ -553,7 +553,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 *must* use the new HTTP/1 option :ref:`override_stream_error_on_invalid_http_message + // ` and 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 ac31bf1ecd62..f7082322b807 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 @@ -550,7 +550,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 *must* use the new HTTP/1 option :ref:`override_stream_error_on_invalid_http_message + // ` and 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 From 07c3ee524baa6fe2bdc2c650d31355ac7628efe3 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 31 Jul 2020 12:03:07 +0200 Subject: [PATCH 26/40] Address code review feedback Signed-off-by: Erica Manno --- api/envoy/config/core/v3/protocol.proto | 4 ++-- api/envoy/config/core/v4alpha/protocol.proto | 4 ++-- .../http_connection_manager/v3/http_connection_manager.proto | 4 ++-- .../v4alpha/http_connection_manager.proto | 4 ++-- generated_api_shadow/envoy/config/core/v3/protocol.proto | 4 ++-- generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 4 ++-- .../http_connection_manager/v3/http_connection_manager.proto | 4 ++-- .../v4alpha/http_connection_manager.proto | 4 ++-- include/envoy/http/codec.h | 4 ++++ 9 files changed, 20 insertions(+), 16 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 1cd53e884020..4d3910b70d36 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -158,9 +158,9 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // 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 enabled, then Envoy will leave the HTTP/1.1 connection + // when this option is true, then Envoy will leave the HTTP/1.1 connection // open where possible. // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 79825f267c76..3c043dc138cc 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -158,9 +158,9 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // 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 enabled, then Envoy will leave the HTTP/1.1 connection + // when this option is true, then Envoy will leave the HTTP/1.1 connection // open where possible. // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` 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 e2e74734f73f..f9a54f63b643 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 @@ -551,8 +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/1 option :ref:`override_stream_error_on_invalid_http_message - // ` and 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 f7082322b807..5c67d0925115 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 @@ -550,8 +550,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/1 option :ref:`override_stream_error_on_invalid_http_message - // ` and 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/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 1cd53e884020..4d3910b70d36 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -158,9 +158,9 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // 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 enabled, then Envoy will leave the HTTP/1.1 connection + // when this option is true, then Envoy will leave the HTTP/1.1 connection // open where possible. // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 7d5693802c3d..d89e47bad24b 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -158,9 +158,9 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows invalid HTTP messaging. When this option is disabled, then Envoy will terminate + // 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 enabled, then Envoy will leave the HTTP/1.1 connection + // when this option is true, then Envoy will leave the HTTP/1.1 connection // open where possible. // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging // ` 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 282dbe421dc1..028dac74d261 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 @@ -553,8 +553,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/1 option :ref:`override_stream_error_on_invalid_http_message - // ` and 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 f7082322b807..5c67d0925115 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 @@ -550,8 +550,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/1 option :ref:`override_stream_error_on_invalid_http_message - // ` and 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 0a8828e1187c..1850ab5ad04b 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -395,6 +395,10 @@ struct Http1Settings { // How header keys should be formatted when serializing HTTP/1.1 headers. HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default}; + // Behavior on invalid HTTP messaging: + // - if set and true, the HTTP/1.1 connection is left open (where possible) + // - if set and false, the HTTP/1.1 connection is terminated + // - if not set, fall back to the HCM behaviour on invalid HTTP absl::optional stream_error_on_invalid_http_message_{}; }; From 5e1b95ee2302af9e4e63eb8cff768150683f230b Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 31 Jul 2020 13:00:05 +0200 Subject: [PATCH 27/40] Move code from utility class to anon namespace Signed-off-by: Erica Manno --- source/common/http/conn_manager_impl.cc | 12 ++++++++++- source/common/http/utility.cc | 10 --------- source/common/http/utility.h | 4 ---- test/common/http/conn_manager_impl_test.cc | 8 ++++---- test/common/http/utility_test.cc | 24 ---------------------- 5 files changed, 15 insertions(+), 43 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 85229b74c867..912ec259fb03 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -78,6 +78,16 @@ void recordLatestDataFilter(const typename FilterList::iterator current_filte } } +bool streamErrorOnInvalidHttpMessage( + bool hcm_stream_error_on_invalid_http_message, + const absl::optional& override_stream_error_on_invalid_http_message) { + if (override_stream_error_on_invalid_http_message.has_value()) { + return override_stream_error_on_invalid_http_message.value(); + } else { + return hcm_stream_error_on_invalid_http_message; + } +} + } // namespace ConnectionManagerStats ConnectionManagerImpl::generateStats(const std::string& prefix, @@ -1525,7 +1535,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.hcm_stream_error_on_invalid_message") && code == Http::Code::BadRequest && connection_manager_.codec_->protocol() < Protocol::Http2 && - !Utility::streamErrorOnInvalidHttpMessage( + !streamErrorOnInvalidHttpMessage( connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), response_encoder_->streamErrorOnInvalidHttpMessage())) { state_.saw_connection_close_ = true; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 940a5f604137..399ec33141ae 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -418,16 +418,6 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& return ret; } -bool Utility::streamErrorOnInvalidHttpMessage( - bool hcm_stream_error_on_invalid_http_message, - const absl::optional& override_stream_error_on_invalid_http_message) { - if (override_stream_error_on_invalid_http_message.has_value()) { - return override_stream_error_on_invalid_http_message.value(); - } else { - return hcm_stream_error_on_invalid_http_message; - } -} - 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 7dac5d53fdd0..492193c4e2ff 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -269,10 +269,6 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); */ Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config); -bool streamErrorOnInvalidHttpMessage( - const bool hcm_stream_error_on_invalid_http_message, - const absl::optional& override_stream_error_on_invalid_http_message); - struct EncodeFunctions { // Function to rewrite locally generated response. std::function(false))); EXPECT_CALL(*filter, encodeHeaders(_, true)); @@ -2158,7 +2158,7 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionOpenIfCodecOverridesStreamError) EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); // stream_error from codec, if specified, determines whether the HTTP/1 connection is terminated - // or left open + // or left open (irrespective of behaviour configured in HCM) EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) .WillOnce(Return(absl::optional(true))); EXPECT_CALL(*filter, encodeHeaders(_, true)); @@ -2199,7 +2199,7 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionTerminatedIfCodecDoesntOverrideS EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - // stream_error from codec is not specified, so stream_error from HCM determines whether the + // stream_error from codec is not specified, so HCM config determines whether the // HTTP/1 connection is terminated or not EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) .WillOnce(Return(absl::optional())); @@ -2242,7 +2242,7 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionLeftOpenIfCodecDoesntOverrideStr EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - // stream_error from codec is not specified, so stream_error from HCM determines whether the + // stream_error from codec is not specified, so HCM config determines whether the // HTTP/1 connection is terminated or not EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) .WillOnce(Return(absl::optional())); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 836e2e769fea..687c5255ee36 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -357,30 +357,6 @@ initial_connection_window_size: 65535 } } -TEST(HttpUtility, ValidateStreamErrorsWithHcmForHttp1) { - // If the override value is present, it will take precedence over the HCM value. - bool hcm_stream_error = false; - absl::optional override_stream_error = true; - EXPECT_TRUE( - Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); - - hcm_stream_error = true; - override_stream_error = false; - EXPECT_FALSE( - Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); - - // If the override value is not set, the HCM value will be used. - hcm_stream_error = false; - override_stream_error.reset(); - EXPECT_FALSE( - Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); - - hcm_stream_error = true; - override_stream_error.reset(); - EXPECT_TRUE( - Http::Utility::streamErrorOnInvalidHttpMessage(hcm_stream_error, override_stream_error)); -} - TEST(HttpUtility, ValidateStreamErrors) { // Both false, the result should be false. envoy::config::core::v3::Http2ProtocolOptions http2_options; From eec3bb24e8a83ef5727039dad588e8398ebdab7c Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 31 Jul 2020 13:57:27 +0200 Subject: [PATCH 28/40] Remove TODO Signed-off-by: Erica Manno --- test/integration/protocol_integration_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 1a984244e267..41aa5a97b69d 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1109,7 +1109,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& From f1787af542084e3fc7c287a2be36d0250a48db7c Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 31 Jul 2020 16:35:48 +0200 Subject: [PATCH 29/40] Fix typo Signed-off-by: Erica Manno --- include/envoy/http/codec.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 1850ab5ad04b..39207719b7d7 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -395,10 +395,10 @@ struct Http1Settings { // How header keys should be formatted when serializing HTTP/1.1 headers. HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default}; - // Behavior on invalid HTTP messaging: + // Behaviour on invalid HTTP messaging: // - if set and true, the HTTP/1.1 connection is left open (where possible) // - if set and false, the HTTP/1.1 connection is terminated - // - if not set, fall back to the HCM behaviour on invalid HTTP + // - if not set, fall back to the HCM behaviour on invalid HTTP messaging absl::optional stream_error_on_invalid_http_message_{}; }; From 447b71909ea100ff0d2591f8ad5c8571995eb30c Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Sun, 2 Aug 2020 11:01:53 +0200 Subject: [PATCH 30/40] Fix CI Signed-off-by: Erica Manno --- test/common/http/conn_manager_impl_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 3c3a1eaca8d7..00f715f2ec19 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -817,6 +817,8 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { data.drain(4); return Http::okStatus(); })); + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) + .WillOnce(Return(absl::optional(true))); // This test also verifies that decoder/encoder filters have onDestroy() called only once. auto* filter = new MockStreamFilter(); @@ -826,9 +828,6 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(true))); EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { From 30154d780ca011398803a8503b1db0be458e4952 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 12 Aug 2020 15:06:08 +0200 Subject: [PATCH 31/40] Address code review feedback Signed-off-by: Erica Manno --- api/envoy/config/core/v3/protocol.proto | 6 ++++-- api/envoy/config/core/v4alpha/protocol.proto | 6 ++++-- docs/root/version_history/current.rst | 3 +-- generated_api_shadow/envoy/config/core/v3/protocol.proto | 6 ++++-- .../envoy/config/core/v4alpha/protocol.proto | 6 ++++-- include/envoy/http/codec.h | 2 +- source/common/http/conn_manager_impl.cc | 2 +- source/common/http/http1/codec_impl.h | 7 +++---- source/common/http/http1/codec_impl_legacy.h | 7 +++---- .../quic_listeners/quiche/envoy_quic_server_stream.h | 2 +- test/integration/protocol_integration_test.cc | 9 +++++++++ 11 files changed, 35 insertions(+), 21 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 4d3910b70d36..f9231df90da5 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -162,8 +162,10 @@ message Http1ProtocolOptions { // 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. - // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging - // ` + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `; + // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 3c043dc138cc..e9bc66111115 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -162,8 +162,10 @@ message Http1ProtocolOptions { // 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. - // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging - // ` + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `; + // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 57ee16477abc..cfa6d3e98883 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -14,8 +14,7 @@ Minor Behavior Changes * compressor: always insert `Vary` headers for compressible resources even if it's decided not to compress a response due to incompatible `Accept-Encoding` value. The `Vary` header needs to be inserted to let a caching proxy in front of Envoy know that the requested resource still can be served with compression applied. * decompressor: headers-only requests were incorrectly not advertising accept-encoding when configured to do so. This is now fixed. * 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 beavior. -* 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: 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. * 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. * http: changed empty trailers encoding behavior by sending empty data with ``end_stream`` true (instead of sending empty trailers) for HTTP/2. This behavior can be reverted temporarily by setting runtime feature ``envoy.reloadable_features.http2_skip_encoding_empty_trailers`` to false. * http: clarified and enforced 1xx handling. Multiple 100-continue headers are coalesced when proxying. 1xx headers other than {100, 101} are dropped. diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 4d3910b70d36..f9231df90da5 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -162,8 +162,10 @@ message Http1ProtocolOptions { // 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. - // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging - // ` + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `; + // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index d89e47bad24b..1e28829e1e88 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -162,8 +162,10 @@ message Http1ProtocolOptions { // 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. - // This overrides any HCM :ref:`stream_error_on_invalid_http_messaging - // ` + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `; + // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index fc15fa394751..b49f02ad46c1 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -397,7 +397,7 @@ struct Http1Settings { // - if set and true, the HTTP/1.1 connection is left open (where possible) // - if set and false, the HTTP/1.1 connection is terminated // - if not set, fall back to the HCM behaviour on invalid HTTP messaging - absl::optional stream_error_on_invalid_http_message_{}; + absl::optional stream_error_on_invalid_http_message_; }; /** diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 1482fa7fe77b..aa98ee040b05 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -82,7 +82,7 @@ void recordLatestDataFilter(const typename FilterList::iterator current_filte bool streamErrorOnInvalidHttpMessage( bool hcm_stream_error_on_invalid_http_message, - const absl::optional& override_stream_error_on_invalid_http_message) { + const absl::optional override_stream_error_on_invalid_http_message) { if (override_stream_error_on_invalid_http_message.has_value()) { return override_stream_error_on_invalid_http_message.value(); } else { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 3e2d0bc9ab02..2ee9833137f9 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -125,9 +125,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { public: ResponseEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter, absl::optional& stream_error_on_invalid_http_message) - : StreamEncoderImpl(connection, header_key_formatter) { - stream_error_on_invalid_http_message_ = 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_; } @@ -142,7 +141,7 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { private: bool started_response_{}; - absl::optional stream_error_on_invalid_http_message_; + const absl::optional stream_error_on_invalid_http_message_; }; /** diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index c6234d8e5d63..050924f766e8 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -128,9 +128,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { ResponseEncoderImpl(ConnectionImpl& connection, Http::Http1::HeaderKeyFormatter* header_key_formatter, absl::optional& stream_error_on_invalid_http_message) - : StreamEncoderImpl(connection, header_key_formatter) { - stream_error_on_invalid_http_message_ = 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_; } @@ -145,7 +144,7 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { private: bool started_response_{}; - absl::optional stream_error_on_invalid_http_message_; + const absl::optional stream_error_on_invalid_http_message_; }; /** 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 8d46422ffcca..fe48e5341ff8 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h @@ -37,7 +37,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, return absl::nullopt; } absl::optional streamErrorOnInvalidHttpMessage() const override { - return absl::optional{}; + return absl::nullopt; } // Http::Stream diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 8541a9ad5363..f195b4900c3b 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1191,6 +1191,9 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { } } +// 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(DownstreamProtocolIntegrationTest, Http1ConnectionIsLeftOpenIfHCMStreamErrorIsFalseAndOverrideIsTrue) { if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { @@ -1217,6 +1220,9 @@ TEST_P(DownstreamProtocolIntegrationTest, 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(DownstreamProtocolIntegrationTest, Http1ConnectionIsLeftOpenIfHCMStreamErrorIsTrueAndOverrideNotSet) { if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { @@ -1238,6 +1244,9 @@ TEST_P(DownstreamProtocolIntegrationTest, 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(DownstreamProtocolIntegrationTest, Http1ConnectionIsTerminatedIfHCMStreamErrorIsFalseAndOverrideNotSet) { if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { From f99724ddbca4ad2d217f7f231540214aa1d9efaf Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 12 Aug 2020 15:55:34 +0200 Subject: [PATCH 32/40] Fix formatting Signed-off-by: Erica Manno --- .../quic_listeners/quiche/envoy_quic_server_stream.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 fe48e5341ff8..390c817e204d 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h @@ -36,9 +36,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { return absl::nullopt; } - absl::optional streamErrorOnInvalidHttpMessage() const override { - return absl::nullopt; - } + absl::optional streamErrorOnInvalidHttpMessage() const override { return absl::nullopt; } // Http::Stream void resetStream(Http::StreamResetReason reason) override; From a35bada34758d8ac9312c7547f5aeb193faa00c7 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Mon, 17 Aug 2020 15:17:47 +0200 Subject: [PATCH 33/40] More code review feedback Signed-off-by: Erica Manno --- docs/root/version_history/current.rst | 2 +- include/envoy/http/codec.h | 2 +- test/common/http/conn_manager_impl_test.cc | 250 +++++---------------- 3 files changed, 61 insertions(+), 193 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index cfa6d3e98883..e1bf3af9e65f 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -14,7 +14,7 @@ Minor Behavior Changes * compressor: always insert `Vary` headers for compressible resources even if it's decided not to compress a response due to incompatible `Accept-Encoding` value. The `Vary` header needs to be inserted to let a caching proxy in front of Envoy know that the requested resource still can be served with compression applied. * decompressor: headers-only requests were incorrectly not advertising accept-encoding when configured to do so. This is now fixed. * 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. +* 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: 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. * http: changed empty trailers encoding behavior by sending empty data with ``end_stream`` true (instead of sending empty trailers) for HTTP/2. This behavior can be reverted temporarily by setting runtime feature ``envoy.reloadable_features.http2_skip_encoding_empty_trailers`` to false. * http: clarified and enforced 1xx handling. Multiple 100-continue headers are coalesced when proxying. 1xx headers other than {100, 101} are dropped. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index b49f02ad46c1..571d85f3b567 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -145,7 +145,7 @@ class ResponseEncoder : public virtual StreamEncoder { /** * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection - * error. + * error; if value is missing, then HCM drives behaviour on invalid HTTP messaging. */ virtual absl::optional streamErrorOnInvalidHttpMessage() const PURE; }; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 8d5ede73384f..f609df1173d3 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2132,208 +2132,76 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { conn_manager_->onData(fake_input, false); } -TEST_F(HttpConnectionManagerImplTest, ConnectionTerminatedIfCodecOverridesStreamError) { - 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(_)); - - // stream_error from codec, if specified, determines whether the HTTP/1 connection is terminated - // or left open (irrespective of behaviour configured in HCM) - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(false))); - 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()); - EXPECT_NE(nullptr, headers.Connection()); - EXPECT_EQ("close", headers.getConnectionValue()); - })); - EXPECT_CALL(*filter, onDestroy()); - - Buffer::OwnedImpl fake_input; - conn_manager_->onData(fake_input, false); -} +class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest { +public: + void sendInvalidRequestAndVerifyConnectionState( + bool hcm_stream_error_on_invalid_http_message, + absl::optional stream_error_on_invalid_http_message, bool is_connection_terminated) { + setup(false, ""); + // HCM stream error + stream_error_on_invalid_http_messaging_ = hcm_stream_error_on_invalid_http_message; -TEST_F(HttpConnectionManagerImplTest, ConnectionOpenIfCodecOverridesStreamError) { - setup(false, ""); - EXPECT_CALL(*codec_, dispatch(_)) - .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { - RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_); + 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(); - })); + // 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(_)); + 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 (is_connection_terminated) { + EXPECT_NE(nullptr, headers.Connection()); + EXPECT_EQ("close", headers.getConnectionValue()); + } else { + EXPECT_EQ(nullptr, headers.Connection()); + } + })); + EXPECT_CALL(*filter, onDestroy()); - // stream_error from codec, if specified, determines whether the HTTP/1 connection is terminated - // or left open (irrespective of behaviour configured in HCM) - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(true))); - 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()); - EXPECT_EQ(nullptr, headers.Connection()); - })); - EXPECT_CALL(*filter, onDestroy()); + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); + } +}; - Buffer::OwnedImpl fake_input; - conn_manager_->onData(fake_input, false); +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecOverridesStreamError) { + // codec stream error, if set, drives behavior (irrespective of HCM stream error) + sendInvalidRequestAndVerifyConnectionState(false, false, true); } -TEST_F(HttpConnectionManagerImplTest, ConnectionTerminatedIfCodecDoesntOverrideStreamError) { - setup(false, ""); - // HCM stream error - stream_error_on_invalid_http_messaging_ = 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(_)); - - // stream_error from codec is not specified, so HCM config determines whether the - // HTTP/1 connection is terminated or not - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional())); - 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()); - EXPECT_NE(nullptr, headers.Connection()); - EXPECT_EQ("close", headers.getConnectionValue()); - })); - EXPECT_CALL(*filter, onDestroy()); - - Buffer::OwnedImpl fake_input; - conn_manager_->onData(fake_input, false); +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionOpenIfCodecOverridesStreamError) { + // codec stream error, if set, drives behavior (irrespective of HCM stream error) + sendInvalidRequestAndVerifyConnectionState(false, true, false); } -TEST_F(HttpConnectionManagerImplTest, ConnectionLeftOpenIfCodecDoesntOverrideStreamError) { - setup(false, ""); - // HCM stream_error - stream_error_on_invalid_http_messaging_ = true; - 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(_)); - - // stream_error from codec is not specified, so HCM config determines whether the - // HTTP/1 connection is terminated or not - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional())); - 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()); - EXPECT_EQ(nullptr, headers.Connection()); - })); - EXPECT_CALL(*filter, onDestroy()); - - Buffer::OwnedImpl fake_input; - conn_manager_->onData(fake_input, false); +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecDoesntOverrideStreamError) { + // codec stream error not set, so fall back to HCM stream error + sendInvalidRequestAndVerifyConnectionState(false, absl::nullopt, true); } -TEST_F(HttpConnectionManagerImplTest, ConnectionLeftOpenIfOnCodecDoesntOverrideStreamError) { - 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(_)); - - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(true))); - 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()); - EXPECT_EQ(nullptr, headers.Connection()); - })); - EXPECT_CALL(*filter, onDestroy()); - - Buffer::OwnedImpl fake_input; - conn_manager_->onData(fake_input, false); +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionLeftOpenIfCodecDoesntOverrideStreamError) { + // codec stream error not set, so fall back to HCM stream error + sendInvalidRequestAndVerifyConnectionState(true, absl::nullopt, false); } TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) { From 16414a0ded513aeabeca580b71305f79cb6f3d91 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 26 Aug 2020 19:45:33 +0200 Subject: [PATCH 34/40] Have logic that determines stream error on codec instantiation Signed-off-by: Erica Manno --- include/envoy/http/codec.h | 9 ++--- source/common/http/conn_manager_impl.cc | 18 +-------- source/common/http/http1/codec_impl.h | 6 +-- source/common/http/http1/codec_impl_legacy.h | 6 +-- source/common/http/http2/codec_impl.h | 2 +- source/common/http/http2/codec_impl_legacy.h | 4 +- source/common/http/utility.cc | 14 +++++++ source/common/http/utility.h | 4 ++ .../network/http_connection_manager/config.cc | 4 +- .../quiche/envoy_quic_server_stream.h | 2 +- test/common/http/conn_manager_impl_test.cc | 38 +++++-------------- test/mocks/http/stream_encoder.h | 2 +- 12 files changed, 47 insertions(+), 62 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 571d85f3b567..96272f53f618 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -147,7 +147,7 @@ class ResponseEncoder : public virtual StreamEncoder { * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection * error; if value is missing, then HCM drives behaviour on invalid HTTP messaging. */ - virtual absl::optional streamErrorOnInvalidHttpMessage() const PURE; + virtual bool streamErrorOnInvalidHttpMessage() const PURE; }; /** @@ -394,10 +394,9 @@ struct Http1Settings { HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default}; // Behaviour on invalid HTTP messaging: - // - if set and true, the HTTP/1.1 connection is left open (where possible) - // - if set and false, the HTTP/1.1 connection is terminated - // - if not set, fall back to the HCM behaviour on invalid HTTP messaging - absl::optional stream_error_on_invalid_http_message_; + // - 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 fec262b43aba..6acebfca491e 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -52,20 +52,6 @@ namespace Envoy { namespace Http { -namespace { - -bool streamErrorOnInvalidHttpMessage( - bool hcm_stream_error_on_invalid_http_message, - const absl::optional override_stream_error_on_invalid_http_message) { - if (override_stream_error_on_invalid_http_message.has_value()) { - return override_stream_error_on_invalid_http_message.value(); - } else { - return hcm_stream_error_on_invalid_http_message; - } -} - -} // namespace - ConnectionManagerStats ConnectionManagerImpl::generateStats(const std::string& prefix, Stats::Scope& scope) { return ConnectionManagerStats( @@ -1185,9 +1171,7 @@ void ConnectionManagerImpl::ActiveStream::onLocalReply(Code code) { if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.hcm_stream_error_on_invalid_message") && code == Http::Code::BadRequest && connection_manager_.codec_->protocol() < Protocol::Http2 && - !streamErrorOnInvalidHttpMessage( - connection_manager_.config_.streamErrorOnInvalidHttpMessaging(), - response_encoder_->streamErrorOnInvalidHttpMessage())) { + !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 b9abc34f73a1..2cf12aedba62 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -124,7 +124,7 @@ class StreamEncoderImpl : public virtual StreamEncoder, class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { public: ResponseEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter, - absl::optional& stream_error_on_invalid_http_message) + bool stream_error_on_invalid_http_message) : StreamEncoderImpl(connection, header_key_formatter), stream_error_on_invalid_http_message_(stream_error_on_invalid_http_message) {} @@ -135,13 +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); } - absl::optional streamErrorOnInvalidHttpMessage() const override { + bool streamErrorOnInvalidHttpMessage() const override { return stream_error_on_invalid_http_message_; } private: bool started_response_{}; - const absl::optional stream_error_on_invalid_http_message_; + const bool stream_error_on_invalid_http_message_; }; /** diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 05f898cb5c4d..f6ed88be8a40 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -127,7 +127,7 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { public: ResponseEncoderImpl(ConnectionImpl& connection, Http::Http1::HeaderKeyFormatter* header_key_formatter, - absl::optional& stream_error_on_invalid_http_message) + bool stream_error_on_invalid_http_message) : StreamEncoderImpl(connection, header_key_formatter), stream_error_on_invalid_http_message_(stream_error_on_invalid_http_message) {} @@ -138,13 +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); } - absl::optional streamErrorOnInvalidHttpMessage() const override { + bool streamErrorOnInvalidHttpMessage() const override { return stream_error_on_invalid_http_message_; } private: bool started_response_{}; - const absl::optional stream_error_on_invalid_http_message_; + const bool stream_error_on_invalid_http_message_; }; /** diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 80c9666cccd1..dbc13068346a 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -392,7 +392,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; - absl::optional streamErrorOnInvalidHttpMessage() const override { + bool streamErrorOnInvalidHttpMessage() const override { return parent_.stream_error_on_invalid_http_messaging_; } }; diff --git a/source/common/http/http2/codec_impl_legacy.h b/source/common/http/http2/codec_impl_legacy.h index e2c667e388f6..2b6db85cf731 100644 --- a/source/common/http/http2/codec_impl_legacy.h +++ b/source/common/http/http2/codec_impl_legacy.h @@ -393,9 +393,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable headers_or_trailers_; - absl::optional stream_error_on_invalid_http_message_; + bool stream_error_on_invalid_http_message_; - absl::optional streamErrorOnInvalidHttpMessage() const override { + bool streamErrorOnInvalidHttpMessage() const override { return stream_error_on_invalid_http_message_; } }; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index ae994fc3e9f5..a31dd583e045 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -411,9 +411,23 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& ret.header_key_format_ = Http1Settings::HeaderKeyFormat::Default; } + return ret; +} + +Http1Settings +Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config, + bool hcm_stream_error_set, + 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 if (hcm_stream_error_set) { + // HCM, if set, overrides override_stream_error_on_invalid_http_message + ret.stream_error_on_invalid_http_message_ = hcm_stream_error.value(); } return ret; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d4625c9b8e82..55826dff5595 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -269,6 +269,10 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); */ Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config); +Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config, + bool hcm_stream_error_set, + const Protobuf::BoolValue& hcm_stream_error); + struct EncodeFunctions { // Function to rewrite locally generated response. std::function streamErrorOnInvalidHttpMessage() const 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 476b7272053b..dfd412cff487 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -817,8 +817,7 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { data.drain(4); return Http::okStatus(); })); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(true))); + 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(); @@ -2134,12 +2133,9 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest { public: - void sendInvalidRequestAndVerifyConnectionState( - bool hcm_stream_error_on_invalid_http_message, - absl::optional stream_error_on_invalid_http_message, bool is_connection_terminated) { + void sendInvalidRequestAndVerifyConnectionState(bool codec_stream_error, + bool is_connection_terminated) { setup(false, ""); - // HCM stream error - stream_error_on_invalid_http_messaging_ = hcm_stream_error_on_invalid_http_message; EXPECT_CALL(*codec_, dispatch(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { @@ -2163,7 +2159,7 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest // codec stream error EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(stream_error_on_invalid_http_message)); + .WillOnce(Return(codec_stream_error)); EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { @@ -2184,24 +2180,12 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest } }; -TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecOverridesStreamError) { - // codec stream error, if set, drives behavior (irrespective of HCM stream error) - sendInvalidRequestAndVerifyConnectionState(false, false, true); +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecStreamErrorIsFalse) { + sendInvalidRequestAndVerifyConnectionState(false, true); } -TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionOpenIfCodecOverridesStreamError) { - // codec stream error, if set, drives behavior (irrespective of HCM stream error) - sendInvalidRequestAndVerifyConnectionState(false, true, false); -} - -TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecDoesntOverrideStreamError) { - // codec stream error not set, so fall back to HCM stream error - sendInvalidRequestAndVerifyConnectionState(false, absl::nullopt, true); -} - -TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionLeftOpenIfCodecDoesntOverrideStreamError) { - // codec stream error not set, so fall back to HCM stream error - sendInvalidRequestAndVerifyConnectionState(true, absl::nullopt, false); +TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionOpenIfCodecStreamErrorIsTrue) { + sendInvalidRequestAndVerifyConnectionState(true, false); } TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) { @@ -5042,8 +5026,7 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { return FilterHeadersStatus::Continue; })); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(true))); + 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()); @@ -5085,8 +5068,7 @@ TEST_F(HttpConnectionManagerImplTest, ResetWithStoppedFilter) { return FilterHeadersStatus::Continue; })); - EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(absl::optional(true))); + 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/mocks/http/stream_encoder.h b/test/mocks/http/stream_encoder.h index 01a7402e4de6..d2c682889ea3 100644 --- a/test/mocks/http/stream_encoder.h +++ b/test/mocks/http/stream_encoder.h @@ -49,7 +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(absl::optional, streamErrorOnInvalidHttpMessage, (), (const)); + MOCK_METHOD(bool, streamErrorOnInvalidHttpMessage, (), (const)); MOCK_METHOD(Stream&, getStream, (), ()); testing::NiceMock stream_; From 4bb2a868c4d67834a6e00935c60af0471eaa5fac Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 26 Aug 2020 20:42:08 +0200 Subject: [PATCH 35/40] Unit test for init logic of stream_error Signed-off-by: Erica Manno --- test/common/http/utility_test.cc | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 687c5255ee36..96a10f9db12f 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; + + // 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, true, 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, true, hcm_value) + .stream_error_on_invalid_http_message_); + + http1_options.clear_override_stream_error_on_invalid_http_message(); + + // HCM.stream_error used if present + hcm_value.set_value(true); + EXPECT_TRUE(Utility::parseHttp1Settings(http1_options, true, hcm_value) + .stream_error_on_invalid_http_message_); + + // HCM.stream_error used if present (flip boolean value) + hcm_value.set_value(false); + EXPECT_FALSE(Utility::parseHttp1Settings(http1_options, true, hcm_value) + .stream_error_on_invalid_http_message_); + + // nothing explicitly configured, default to false + EXPECT_FALSE(Utility::parseHttp1Settings(http1_options, false, hcm_value) + .stream_error_on_invalid_http_message_); +} + TEST(HttpUtility, getLastAddressFromXFF) { { const std::string first_address = "192.0.2.10"; From 14109720d593b55199f2799a4a5534c2e888fe69 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Wed, 26 Aug 2020 20:55:21 +0200 Subject: [PATCH 36/40] Update doc Signed-off-by: Erica Manno --- api/envoy/config/core/v3/protocol.proto | 2 +- api/envoy/config/core/v4alpha/protocol.proto | 2 +- generated_api_shadow/envoy/config/core/v3/protocol.proto | 2 +- generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index f9231df90da5..064c863a8c2b 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // open where possible. // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging // `; - // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index e9bc66111115..d4f6b7b68d53 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // open where possible. // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging // `; - // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index f9231df90da5..064c863a8c2b 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // open where possible. // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging // `; - // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 1e28829e1e88..553714c2d664 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // open where possible. // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging // `; - // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging + // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging // `, which defaults to false. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } From 5e63612db20965555f1f1e17fdacafa394510cac Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Thu, 27 Aug 2020 11:05:53 +0200 Subject: [PATCH 37/40] Address feedback and fix CI Signed-off-by: Erica Manno --- api/envoy/config/core/v3/protocol.proto | 4 +--- api/envoy/config/core/v4alpha/protocol.proto | 4 +--- generated_api_shadow/envoy/config/core/v3/protocol.proto | 4 +--- .../envoy/config/core/v4alpha/protocol.proto | 4 +--- include/envoy/http/codec.h | 2 +- test/common/http/http2/codec_impl_test.cc | 8 ++------ 6 files changed, 7 insertions(+), 19 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 064c863a8c2b..6af4c34331f0 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -163,9 +163,7 @@ message Http1ProtocolOptions { // 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 - // `; - // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging - // `, which defaults to false. + // `. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index d4f6b7b68d53..86f9293a2212 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -163,9 +163,7 @@ message Http1ProtocolOptions { // 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 - // `; - // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging - // `, which defaults to false. + // `. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 064c863a8c2b..6af4c34331f0 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -163,9 +163,7 @@ message Http1ProtocolOptions { // 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 - // `; - // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging - // `, which defaults to false. + // `. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 553714c2d664..5fe019ac086d 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -163,9 +163,7 @@ message Http1ProtocolOptions { // 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 - // `; - // this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging - // `, which defaults to false. + // `. google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 6; } diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 96272f53f618..59ad68799a6f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -145,7 +145,7 @@ class ResponseEncoder : public virtual StreamEncoder { /** * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection - * error; if value is missing, then HCM drives behaviour on invalid HTTP messaging. + * error. */ virtual bool streamErrorOnInvalidHttpMessage() const PURE; }; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 8867a91ff4f1..9b4fdeccc1b9 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -501,9 +501,7 @@ TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfFalse) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); request_encoder_->encodeHeaders(request_headers, true); - auto stream_error = response_encoder_->streamErrorOnInvalidHttpMessage(); - EXPECT_TRUE(stream_error.has_value()); - EXPECT_FALSE(stream_error.value()); + EXPECT_FALSE(response_encoder_->streamErrorOnInvalidHttpMessage()); } TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfTrue) { @@ -515,9 +513,7 @@ TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfTrue) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); request_encoder_->encodeHeaders(request_headers, true); - auto stream_error = response_encoder_->streamErrorOnInvalidHttpMessage(); - EXPECT_TRUE(stream_error.has_value()); - EXPECT_TRUE(stream_error.value()); + EXPECT_TRUE(response_encoder_->streamErrorOnInvalidHttpMessage()); } TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { From 85e57c4c0560e8c70eb9dc52966cfe788e538232 Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 28 Aug 2020 12:28:40 +0200 Subject: [PATCH 38/40] Address feedback Signed-off-by: Erica Manno --- source/common/http/utility.cc | 5 +- source/common/http/utility.h | 1 - .../network/http_connection_manager/config.cc | 3 +- test/common/http/conn_manager_impl_test.cc | 7 +- test/common/http/utility_test.cc | 28 +++---- test/integration/integration_test.cc | 64 +++++++++++++++ test/integration/protocol_integration_test.cc | 79 ------------------- 7 files changed, 84 insertions(+), 103 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index a31dd583e045..ef5232d53f90 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -416,7 +416,6 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& Http1Settings Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config, - bool hcm_stream_error_set, const Protobuf::BoolValue& hcm_stream_error) { Http1Settings ret = parseHttp1Settings(config); @@ -425,8 +424,8 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& // stream_error_on_invalid_http_message ret.stream_error_on_invalid_http_message_ = config.override_stream_error_on_invalid_http_message().value(); - } else if (hcm_stream_error_set) { - // HCM, if set, overrides override_stream_error_on_invalid_http_message + } else { + // fallback to HCM value ret.stream_error_on_invalid_http_message_ = hcm_stream_error.value(); } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 55826dff5595..0d69cf42bcdb 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -270,7 +270,6 @@ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config); Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config, - bool hcm_stream_error_set, const Protobuf::BoolValue& hcm_stream_error); struct EncodeFunctions { diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 809aa4b4b0aa..82a65448222f 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -208,8 +208,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( 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(), config.has_stream_error_on_invalid_http_message(), - config.stream_error_on_invalid_http_message())), + 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/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index dfd412cff487..1e9ad26415bb 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2133,8 +2133,7 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest { public: - void sendInvalidRequestAndVerifyConnectionState(bool codec_stream_error, - bool is_connection_terminated) { + void sendInvalidRequestAndVerifyConnectionState(bool stream_error_on_invalid_http_message) { setup(false, ""); EXPECT_CALL(*codec_, dispatch(_)) @@ -2159,14 +2158,14 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest // codec stream error EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()) - .WillOnce(Return(codec_stream_error)); + .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 (is_connection_terminated) { + if (!stream_error_on_invalid_http_message) { EXPECT_NE(nullptr, headers.Connection()); EXPECT_EQ("close", headers.getConnectionValue()); } else { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 96a10f9db12f..ec31fee046a0 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -425,33 +425,33 @@ 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, true, hcm_value) - .stream_error_on_invalid_http_message_); + 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, true, hcm_value) - .stream_error_on_invalid_http_message_); + EXPECT_FALSE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); http1_options.clear_override_stream_error_on_invalid_http_message(); - // HCM.stream_error used if present + // fallback to HCM.stream_error hcm_value.set_value(true); - EXPECT_TRUE(Utility::parseHttp1Settings(http1_options, true, hcm_value) - .stream_error_on_invalid_http_message_); + EXPECT_TRUE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); - // HCM.stream_error used if present (flip boolean value) + // fallback to HCM.stream_error (flip boolean value) hcm_value.set_value(false); - EXPECT_FALSE(Utility::parseHttp1Settings(http1_options, true, hcm_value) - .stream_error_on_invalid_http_message_); - - // nothing explicitly configured, default to false - EXPECT_FALSE(Utility::parseHttp1Settings(http1_options, false, hcm_value) - .stream_error_on_invalid_http_message_); + EXPECT_FALSE( + Utility::parseHttp1Settings(http1_options, hcm_value).stream_error_on_invalid_http_message_); } TEST(HttpUtility, getLastAddressFromXFF) { diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 6c526fbb7425..15796acdbbe4 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1447,4 +1447,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 e5e85475adad..77b282b20be1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1192,85 +1192,6 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { } } -// 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(DownstreamProtocolIntegrationTest, - Http1ConnectionIsLeftOpenIfHCMStreamErrorIsFalseAndOverrideIsTrue) { - if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { - return; - } - - 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(DownstreamProtocolIntegrationTest, - Http1ConnectionIsLeftOpenIfHCMStreamErrorIsTrueAndOverrideNotSet) { - if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { - return; - } - - 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(DownstreamProtocolIntegrationTest, - Http1ConnectionIsTerminatedIfHCMStreamErrorIsFalseAndOverrideNotSet) { - if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) { - return; - } - - 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()); -} - // TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc. TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { config_helper_.addConfigModifier( From 464a5c728b988434144b469a057ceebaf7ced74f Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 28 Aug 2020 14:42:48 +0200 Subject: [PATCH 39/40] Fix unit test Signed-off-by: Erica Manno --- test/common/http/http1/codec_impl_test.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 4f60f50f0e04..d9065d9359f9 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -632,9 +632,7 @@ TEST_P(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfTrue) { })); auto status = codec_->dispatch(buffer); - auto stream_error = response_encoder->streamErrorOnInvalidHttpMessage(); - EXPECT_TRUE(stream_error.has_value()); - EXPECT_TRUE(stream_error.value()); + EXPECT_TRUE(response_encoder->streamErrorOnInvalidHttpMessage()); } TEST_P(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfFalse) { @@ -659,12 +657,10 @@ TEST_P(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfFalse) { })); auto status = codec_->dispatch(buffer); - auto stream_error = response_encoder->streamErrorOnInvalidHttpMessage(); - EXPECT_TRUE(stream_error.has_value()); - EXPECT_FALSE(stream_error.value()); + EXPECT_FALSE(response_encoder->streamErrorOnInvalidHttpMessage()); } -TEST_P(Http1ServerConnectionImplTest, CodecHasNoStreamErrorIfNotSet) { +TEST_P(Http1ServerConnectionImplTest, CodecHasDefaultStreamErrorIfNotSet) { initialize(); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n"); From e663b45f0667f73c1563f06caf51e97cc35b834f Mon Sep 17 00:00:00 2001 From: Erica Manno Date: Fri, 28 Aug 2020 15:25:08 +0200 Subject: [PATCH 40/40] Fix test Signed-off-by: Erica Manno --- test/common/http/conn_manager_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index f6978d9075bb..0e30795f0e93 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2240,11 +2240,11 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest }; TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionTerminatedIfCodecStreamErrorIsFalse) { - sendInvalidRequestAndVerifyConnectionState(false, true); + sendInvalidRequestAndVerifyConnectionState(false); } TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionOpenIfCodecStreamErrorIsTrue) { - sendInvalidRequestAndVerifyConnectionState(true, false); + sendInvalidRequestAndVerifyConnectionState(true); } TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) {