From 886c5f0dcaceca8d35351bdcd90bb90f5aba803d Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Sat, 25 Jul 2020 20:21:32 -0700 Subject: [PATCH 01/40] [http1 codec] remove Content-Length is Transfer-Encoding is chunked Signed-off-by: Oleg Guba --- bazel/repository_locations.bzl | 6 +++--- include/envoy/http/codec.h | 4 ++++ source/common/http/http1/codec_impl.cc | 25 +++++++++++++++++++++---- source/common/http/http1/codec_impl.h | 3 ++- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 7e1f4d8c6141..92885dc45686 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -283,9 +283,9 @@ DEPENDENCY_REPOSITORIES = dict( cpe = "N/A", ), com_github_nodejs_http_parser = dict( - sha256 = "8fa0ab8770fd8425a9b431fdbf91623c4d7a9cdb842b9339289bd2b0b01b0d3d", - strip_prefix = "http-parser-2.9.3", - urls = ["https://github.com/nodejs/http-parser/archive/v2.9.3.tar.gz"], + sha256 = "6a12896313ce1ca630cf516a0ee43a79b5f13f5a5d8143f56560ac0b21c98fac", + strip_prefix = "http-parser-4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878", + urls = ["https://github.com/nodejs/http-parser/archive/4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878.tar.gz"], use_category = ["dataplane"], cpe = "cpe:2.3:a:nodejs:node.js:*", ), diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 46a9bf4e4f2b..ab2b33e7db69 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -376,6 +376,10 @@ struct Http1Settings { // - Is neither a HEAD only request nor a HTTP Upgrade // - Not a HEAD request bool enable_trailers_{false}; + // Allow serving requests with both Content-Length and Transfer-Encoding: chunked. + // In enabled and such request served - Content-Length is ignored and removed from request + // headers. + bool allow_chunked_length{false}; enum class HeaderKeyFormat { // By default no formatting is performed, presenting all headers in lowercase (as Envoy diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 596540e56a66..6b02f5e25eac 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -444,7 +444,8 @@ http_parser_settings ConnectionImpl::settings_{ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, http_parser_type type, uint32_t max_headers_kb, const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) + HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers, + bool allow_chunked_length) : connection_(connection), stats_(stats), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), @@ -459,6 +460,9 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_.setWatermarks(connection.bufferLimit()); http_parser_init(&parser_, type); + if (allow_chunked_length) { + parser_.allow_chunked_length = 1; + } parser_.data = this; } @@ -763,7 +767,8 @@ ServerConnectionImpl::ServerConnectionImpl( envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings.enable_trailers_), + max_request_headers_count, formatter(settings), settings.enable_trailers_, + settings.allow_chunked_length), callbacks_(callbacks), codec_settings_(settings), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); @@ -878,6 +883,18 @@ int ServerConnectionImpl::onHeadersComplete() { "http/1.1 protocol error: request headers failed spec compliance checks"); } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message downstream. + if (parser_.flags & F_CHUNKED && headers->ContentLength()) { + headers->removeContentLength(); + } + // Determine here whether we have a body or not. This uses the new RFC semantics where the // presence of content-length or chunked transfer-encoding indicates a body vs. a particular // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed @@ -887,7 +904,6 @@ int ServerConnectionImpl::onHeadersComplete() { if (parser_.flags & F_CHUNKED || (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX) || handling_upgrade_) { active_request.request_decoder_->decodeHeaders(std::move(headers), false); - // If the connection has been closed (or is closing) after decoding headers, pause the parser // so we return control to the caller. if (connection_.state() != Network::Connection::State::Open) { @@ -1057,7 +1073,8 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_) {} + max_response_headers_count, formatter(settings), settings.enable_trailers_, + settings.allow_chunked_length) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index c74c0adae87c..1c0839479688 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -220,7 +220,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Tue, 28 Jul 2020 22:48:16 -0700 Subject: [PATCH 02/40] enable http parser option by default and do all validation in Envoy Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 29 ++++++++++++++++---------- source/common/http/http1/codec_impl.h | 5 +++-- test/integration/integration_test.cc | 3 ++- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 6b02f5e25eac..9237538422f5 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -444,8 +444,7 @@ http_parser_settings ConnectionImpl::settings_{ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, http_parser_type type, uint32_t max_headers_kb, const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers, - bool allow_chunked_length) + HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) : connection_(connection), stats_(stats), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), @@ -460,9 +459,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_.setWatermarks(connection.bufferLimit()); http_parser_init(&parser_, type); - if (allow_chunked_length) { - parser_.allow_chunked_length = 1; - } + parser_.allow_chunked_length = 1; parser_.data = this; } @@ -767,8 +764,7 @@ ServerConnectionImpl::ServerConnectionImpl( envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings.enable_trailers_, - settings.allow_chunked_length), + max_request_headers_count, formatter(settings), settings.enable_trailers_), callbacks_(callbacks), codec_settings_(settings), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); @@ -891,8 +887,20 @@ int ServerConnectionImpl::onHeadersComplete() { // (Section 9.4) and ought to be handled as an error. A sender MUST // remove the received Content-Length field prior to forwarding such // a message downstream. - if (parser_.flags & F_CHUNKED && headers->ContentLength()) { - headers->removeContentLength(); + + // Reject request with Http::Code::BadRequest by default or remove Content-Length header + // and serve request if allowed by http1 codec settings. + if (parser_.uses_transfer_encoding != 0 && + (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX)) { + if ((parser_.flags & F_CHUNKED) && allowChunkedLength()) { + headers->removeContentLength(); + } else { + ENVOY_CONN_LOG(error, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", + connection_); + error_code_ = Http::Code::BadRequest; + sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + throw CodecProtocolException("Both Content-Length and Transfer-Encdoding headers are set."); + } } // Determine here whether we have a body or not. This uses the new RFC semantics where the @@ -1073,8 +1081,7 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_, - settings.allow_chunked_length) {} + max_response_headers_count, formatter(settings), settings.enable_trailers_) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 1c0839479688..d7898a9aab0c 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -201,6 +201,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Wed, 29 Jul 2020 11:46:48 -0700 Subject: [PATCH 03/40] address comments: typo/spelling/comments Signed-off-by: Oleg Guba --- bazel/repository_locations.bzl | 3 +++ include/envoy/http/codec.h | 4 ++-- source/common/http/http1/codec_impl.cc | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 92885dc45686..90800f2dd4b0 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -285,6 +285,9 @@ DEPENDENCY_REPOSITORIES = dict( com_github_nodejs_http_parser = dict( sha256 = "6a12896313ce1ca630cf516a0ee43a79b5f13f5a5d8143f56560ac0b21c98fac", strip_prefix = "http-parser-4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878", + # 2020-07-10 + # This SHA includes fix for https://github.com/nodejs/http-parser/issues/517 which allows (opt-in) to serve + # requests with both Content-Legth and Transfer-Encoding: chunked headers set. urls = ["https://github.com/nodejs/http-parser/archive/4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878.tar.gz"], use_category = ["dataplane"], cpe = "cpe:2.3:a:nodejs:node.js:*", diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index ab2b33e7db69..a92e26d5a9af 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -377,8 +377,8 @@ struct Http1Settings { // - Not a HEAD request bool enable_trailers_{false}; // Allow serving requests with both Content-Length and Transfer-Encoding: chunked. - // In enabled and such request served - Content-Length is ignored and removed from request - // headers. + // If enabled and such a request is served - Content-Length is ignored and removed from + // the request headers. bool allow_chunked_length{false}; enum class HeaderKeyFormat { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 9237538422f5..0319274ea2cc 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -895,7 +895,7 @@ int ServerConnectionImpl::onHeadersComplete() { if ((parser_.flags & F_CHUNKED) && allowChunkedLength()) { headers->removeContentLength(); } else { - ENVOY_CONN_LOG(error, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", + ENVOY_CONN_LOG(debug, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", connection_); error_code_ = Http::Code::BadRequest; sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); From 542e6e3e0b2b684f9b3b286597d7495df6e3edd3 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 29 Jul 2020 16:25:14 -0700 Subject: [PATCH 04/40] add allow_chunked_length to Http1ProtocolOoptions Signed-off-by: Oleg Guba --- api/envoy/config/core/v3/protocol.proto | 8 +++++++- api/envoy/config/core/v4alpha/protocol.proto | 8 +++++++- generated_api_shadow/envoy/config/core/v3/protocol.proto | 8 +++++++- .../envoy/config/core/v4alpha/protocol.proto | 8 +++++++- include/envoy/http/codec.h | 2 +- source/common/http/http1/codec_impl.h | 2 +- source/common/http/utility.cc | 1 + 7 files changed, 31 insertions(+), 6 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 0ab6289e9659..7b39f0c501c3 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 955c29335a3f..8459b031942c 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 0ab6289e9659..7b39f0c501c3 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index cc1b99d0a048..83cb50ff4f78 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index a92e26d5a9af..3aec31296708 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -379,7 +379,7 @@ struct Http1Settings { // Allow serving requests with both Content-Length and Transfer-Encoding: chunked. // If enabled and such a request is served - Content-Length is ignored and removed from // the request headers. - bool allow_chunked_length{false}; + bool allow_chunked_length_{false}; enum class HeaderKeyFormat { // By default no formatting is performed, presenting all headers in lowercase (as Envoy diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index d7898a9aab0c..5e7bdc6a4eae 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -427,7 +427,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action); bool supportsHttp10() override { return codec_settings_.accept_http_10_; } - bool allowChunkedLength() override { return codec_settings_.allow_chunked_length; } + bool allowChunkedLength() override { return codec_settings_.allow_chunked_length_; } protected: /** diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index cac031e3b2a0..91b09d4a1d4f 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -403,6 +403,7 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& ret.accept_http_10_ = config.accept_http_10(); ret.default_host_for_http_10_ = config.default_host_for_http_10(); ret.enable_trailers_ = config.enable_trailers(); + ret.allow_chunked_length_ = config.allow_chunked_length(); if (config.header_key_format().has_proper_case_words()) { ret.header_key_format_ = Http1Settings::HeaderKeyFormat::ProperCase; From b59bceaf3ffa4ca2a075f4be42f9be22173aa1a2 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 29 Jul 2020 16:25:14 -0700 Subject: [PATCH 05/40] add allow_chunked_length to Http1ProtocolOoptions Signed-off-by: Oleg Guba --- api/BUILD | 1 - api/envoy/config/core/v3/protocol.proto | 8 +++++++- api/envoy/config/core/v4alpha/protocol.proto | 8 +++++++- generated_api_shadow/envoy/config/core/v3/protocol.proto | 8 +++++++- .../envoy/config/core/v4alpha/protocol.proto | 8 +++++++- include/envoy/http/codec.h | 2 +- source/common/http/http1/codec_impl.h | 2 +- source/common/http/utility.cc | 1 + 8 files changed, 31 insertions(+), 7 deletions(-) diff --git a/api/BUILD b/api/BUILD index 50835fb0b1c4..9d4f802dfe5f 100644 --- a/api/BUILD +++ b/api/BUILD @@ -245,7 +245,6 @@ proto_library( "//envoy/service/discovery/v3:pkg", "//envoy/service/endpoint/v3:pkg", "//envoy/service/event_reporting/v3:pkg", - "//envoy/service/extension/v3:pkg", "//envoy/service/health/v3:pkg", "//envoy/service/listener/v3:pkg", "//envoy/service/load_stats/v3:pkg", diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 0ab6289e9659..7b39f0c501c3 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 955c29335a3f..8459b031942c 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 0ab6289e9659..7b39f0c501c3 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index cc1b99d0a048..83cb50ff4f78 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,12 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // By default such requests are rejected with `400 Bad Request`, but if option is enabled - + // Envoy will remove Content-Length header and pass request to upstream. + // See `RFC7230, sec. 3.3.3 ` for details. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index a92e26d5a9af..3aec31296708 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -379,7 +379,7 @@ struct Http1Settings { // Allow serving requests with both Content-Length and Transfer-Encoding: chunked. // If enabled and such a request is served - Content-Length is ignored and removed from // the request headers. - bool allow_chunked_length{false}; + bool allow_chunked_length_{false}; enum class HeaderKeyFormat { // By default no formatting is performed, presenting all headers in lowercase (as Envoy diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index d7898a9aab0c..5e7bdc6a4eae 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -427,7 +427,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action); bool supportsHttp10() override { return codec_settings_.accept_http_10_; } - bool allowChunkedLength() override { return codec_settings_.allow_chunked_length; } + bool allowChunkedLength() override { return codec_settings_.allow_chunked_length_; } protected: /** diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index cac031e3b2a0..91b09d4a1d4f 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -403,6 +403,7 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& ret.accept_http_10_ = config.accept_http_10(); ret.default_host_for_http_10_ = config.default_host_for_http_10(); ret.enable_trailers_ = config.enable_trailers(); + ret.allow_chunked_length_ = config.allow_chunked_length(); if (config.header_key_format().has_proper_case_words()) { ret.header_key_format_ = Http1Settings::HeaderKeyFormat::ProperCase; From 7c91d036c53acdc99b0219b552354afe24a478e3 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 29 Jul 2020 22:04:00 -0700 Subject: [PATCH 06/40] add unit tests Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 67 +++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 9f21744cdd0a..76823bfecfe6 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -95,6 +95,8 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase, // Used to test if trailers are decoded/encoded void expectTrailersTest(bool enable_trailers); + void testAllowChunkedContentLength(bool allow_chunked_length); + // Send the request, and validate the received request headers. // Then send a response just to clean up. void @@ -326,6 +328,63 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade EXPECT_TRUE(status.ok()); } +void Http1ServerConnectionImplTest::testAllowChunkedContentLength(bool allow_chunked_length) { + // initialize(); + // Make a new 'codec' with the right settings + codec_settings_.allow_chunked_length_ = allow_chunked_length; + // 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); + // } + + MockRequestDecoder decoder; + Http::ResponseEncoder* response_encoder = nullptr; + + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + TestRequestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "POST"}, + {"transfer-encoding", "chunked"}, + }; + Buffer::OwnedImpl expected_data("Hello World"); + + if (allow_chunked_length) { + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(decoder, decodeData(_, true)); + } else { + EXPECT_CALL(decoder, decodeHeaders_(_, _)).Times(0); + EXPECT_CALL(decoder, decodeData(_, _)).Times(0); + EXPECT_CALL(decoder, sendLocalReply(false, Http::Code::BadRequest, "Bad Request", _, _, _, _)); + } + + Buffer::OwnedImpl buffer( + "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\ncontent-length: 1\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n"); + + auto status = codec_->dispatch(buffer); + + if (allow_chunked_length) { + EXPECT_TRUE(status.ok()); + } else { + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), "Both Content-Length and Transfer-Encdoding headers are set."); + EXPECT_EQ("http1.content_length_not_allowed", response_encoder->getStream().responseDetails()); + } +} + INSTANTIATE_TEST_SUITE_P(Codecs, Http1ServerConnectionImplTest, testing::Bool(), [](const testing::TestParamInfo& param) { return param.param ? "New" : "Legacy"; @@ -2866,5 +2925,13 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersAccepted) { status = codec_->dispatch(buffer); } +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength) { + testAllowChunkedContentLength(false); +} + +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength) { + testAllowChunkedContentLength(true); +} + } // namespace Http } // namespace Envoy From 0cd4cb7ad008ce3412c3d42ae365b462cb684733 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 29 Jul 2020 22:09:50 -0700 Subject: [PATCH 07/40] typo fix Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 76823bfecfe6..ef3db92cf3ac 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -329,18 +329,11 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade } void Http1ServerConnectionImplTest::testAllowChunkedContentLength(bool allow_chunked_length) { - // initialize(); - // Make a new 'codec' with the right settings + codec_settings_.allow_chunked_length_ = allow_chunked_length; - // 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); - // } MockRequestDecoder decoder; Http::ResponseEncoder* response_encoder = nullptr; From 4a88b624cd3fcaa1a3169f7295c59af216b9f106 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 30 Jul 2020 12:30:07 -0700 Subject: [PATCH 08/40] release notes update Signed-off-by: Oleg Guba --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index de0307a2c4bf..ee79f5792ab1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -44,6 +44,7 @@ New Features * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP ` and :ref:`network ` filters. * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message. * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as custom header. +* http: added :ref:`allow_chunked_length` configuration option for HTTP/1 codec to allow processing requessts with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is deprecated, but can be used during the removal period by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to false. The removal period will be one month. * load balancer: added a :ref:`configuration` option to specify the active request bias used by the least request load balancer. * redis: added fault injection support :ref:`fault injection for redis proxy `, described further in :ref:`configuration documentation `. From 4119c782887d3a12166f4b33e37a34e619de96c4 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 30 Jul 2020 13:25:14 -0700 Subject: [PATCH 09/40] typo fix Signed-off-by: Oleg Guba --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ee79f5792ab1..94fff33ad7e8 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -44,7 +44,7 @@ New Features * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP ` and :ref:`network ` filters. * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message. * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as custom header. -* http: added :ref:`allow_chunked_length` configuration option for HTTP/1 codec to allow processing requessts with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. +* http: added :ref:`allow_chunked_length` configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is deprecated, but can be used during the removal period by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to false. The removal period will be one month. * load balancer: added a :ref:`configuration` option to specify the active request bias used by the least request load balancer. * redis: added fault injection support :ref:`fault injection for redis proxy `, described further in :ref:`configuration documentation `. From f4776d6a64bac60eb8e9c3eb7ca77c5620e208b9 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 30 Jul 2020 20:39:22 -0700 Subject: [PATCH 10/40] fix version_histroy after merge Signed-off-by: Oleg Guba --- docs/root/version_history/current.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 422d8695b11e..aa3d85e43718 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -48,7 +48,6 @@ New Features * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message. * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as custom header. * http: added :ref:`allow_chunked_length` configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. -* http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is deprecated, but can be used during the removal period by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to false. The removal period will be one month. * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is used by default, but the new codecs can be enabled for testing by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to true. The new codecs will be in development for one month, and then enabled by default while the old codecs are deprecated. * load balancer: added a :ref:`configuration` option to specify the active request bias used by the least request load balancer. * lua: added Lua APIs to access :ref:`SSL connection info ` object. From db56f664cbf27d32aab51dcc50edb1e21fe6a3bf Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 30 Jul 2020 21:42:35 -0700 Subject: [PATCH 11/40] typo fix & proto gen Signed-off-by: Oleg Guba --- api/BUILD | 1 + api/envoy/config/core/v3/protocol.proto | 2 +- api/envoy/config/core/v4alpha/protocol.proto | 2 +- docs/root/version_history/current.rst | 2 +- generated_api_shadow/envoy/config/core/v3/protocol.proto | 2 +- generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 2 +- 6 files changed, 6 insertions(+), 5 deletions(-) diff --git a/api/BUILD b/api/BUILD index 8ec38a529521..3ac2738ebc3e 100644 --- a/api/BUILD +++ b/api/BUILD @@ -249,6 +249,7 @@ proto_library( "//envoy/service/discovery/v3:pkg", "//envoy/service/endpoint/v3:pkg", "//envoy/service/event_reporting/v3:pkg", + "//envoy/service/extension/v3:pkg", "//envoy/service/health/v3:pkg", "//envoy/service/listener/v3:pkg", "//envoy/service/load_stats/v3:pkg", diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 7b39f0c501c3..c7690d8374be 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -158,7 +158,7 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // Allows Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 8459b031942c..e12392fa5782 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -158,7 +158,7 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // Allows Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index aa3d85e43718..c80419ccde39 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -47,7 +47,7 @@ New Features * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP ` and :ref:`network ` filters. * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message. * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as custom header. -* http: added :ref:`allow_chunked_length` configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. +* http: added :ref:`allow_chunked_length ` configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is used by default, but the new codecs can be enabled for testing by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to true. The new codecs will be in development for one month, and then enabled by default while the old codecs are deprecated. * load balancer: added a :ref:`configuration` option to specify the active request bias used by the least request load balancer. * lua: added Lua APIs to access :ref:`SSL connection info ` object. diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 7b39f0c501c3..c7690d8374be 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -158,7 +158,7 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // Allows Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 83cb50ff4f78..fd217a92211f 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -158,7 +158,7 @@ message Http1ProtocolOptions { // - The content length header is not present. bool enable_trailers = 5; - // Allows Envoy to process requests with `Content-Lenght` and `Transfer-Encoding: chunked`. + // Allows Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. From e9f2f1f6ee738f545695de51bbd84ee31b1d841b Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 30 Jul 2020 22:46:13 -0700 Subject: [PATCH 12/40] sendLocalReply signature was changed, update test Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 41568fffef2a..ec68318ee1de 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -358,7 +358,7 @@ void Http1ServerConnectionImplTest::testAllowChunkedContentLength(bool allow_chu } else { EXPECT_CALL(decoder, decodeHeaders_(_, _)).Times(0); EXPECT_CALL(decoder, decodeData(_, _)).Times(0); - EXPECT_CALL(decoder, sendLocalReply(false, Http::Code::BadRequest, "Bad Request", _, _, _, _)); + EXPECT_CALL(decoder, sendLocalReply(false, Http::Code::BadRequest, "Bad Request", _, _, _)); } Buffer::OwnedImpl buffer( From c7424f0a4eddf8ac63930d46a771854fbdf59d38 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 31 Jul 2020 01:56:12 -0700 Subject: [PATCH 13/40] implementation for legacy codec, update tests Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl_legacy.cc | 25 +++++++++++++++++++ source/common/http/http1/codec_impl_legacy.h | 2 ++ test/common/http/http1/codec_impl_test.cc | 14 ++++++++--- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index a8829d2182e2..9f2407b499eb 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -464,6 +464,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_.setWatermarks(connection.bufferLimit()); http_parser_init(&parser_, type); + parser_.allow_chunked_length = 1; parser_.data = this; } @@ -883,6 +884,30 @@ int ServerConnectionImpl::onHeadersComplete() { "http/1.1 protocol error: request headers failed spec compliance checks"); } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message downstream. + + // Reject request with Http::Code::BadRequest by default or remove Content-Length header + // and serve request if allowed by http1 codec settings. + if (parser_.uses_transfer_encoding != 0 && + (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX)) { + if ((parser_.flags & F_CHUNKED) && allowChunkedLength()) { + headers->removeContentLength(); + } else { + ENVOY_CONN_LOG(debug, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", + connection_); + error_code_ = Http::Code::BadRequest; + sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + throw CodecProtocolException("Both Content-Length and Transfer-Encdoding headers are set."); + } + } + // Determine here whether we have a body or not. This uses the new RFC semantics where the // presence of content-length or chunked transfer-encoding indicates a body vs. a particular // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 622d9441459b..08888eb6a1cb 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -205,6 +205,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable( - connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + + 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); + } MockRequestDecoder decoder; Http::ResponseEncoder* response_encoder = nullptr; From 9815afbbebf89c643824c3143d2e4b01f8143523 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 31 Jul 2020 12:26:04 -0700 Subject: [PATCH 14/40] address comments Signed-off-by: Oleg Guba --- api/envoy/config/core/v3/protocol.proto | 4 ++++ source/common/http/http1/codec_impl.cc | 2 +- source/common/http/http1/codec_impl_legacy.cc | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index c7690d8374be..f10e30d63a23 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -162,6 +162,10 @@ message Http1ProtocolOptions { // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead request smuggling vlnarability, especially if traffic + // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index f13eb1eff367..57f7f66bcb1d 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -886,7 +886,7 @@ int ServerConnectionImpl::onHeadersComplete() { // perform request smuggling (Section 9.5) or response splitting // (Section 9.4) and ought to be handled as an error. A sender MUST // remove the received Content-Length field prior to forwarding such - // a message downstream. + // a message. // Reject request with Http::Code::BadRequest by default or remove Content-Length header // and serve request if allowed by http1 codec settings. diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 9f2407b499eb..446f1f71294a 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -891,7 +891,7 @@ int ServerConnectionImpl::onHeadersComplete() { // perform request smuggling (Section 9.5) or response splitting // (Section 9.4) and ought to be handled as an error. A sender MUST // remove the received Content-Length field prior to forwarding such - // a message downstream. + // a message. // Reject request with Http::Code::BadRequest by default or remove Content-Length header // and serve request if allowed by http1 codec settings. From e6c38f500a2176bfbbccbd0581b8c2cd03608001 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 31 Jul 2020 16:00:02 -0700 Subject: [PATCH 15/40] update comment in proto Signed-off-by: Oleg Guba --- api/envoy/config/core/v3/protocol.proto | 2 +- api/envoy/config/core/v4alpha/protocol.proto | 4 ++++ generated_api_shadow/envoy/config/core/v3/protocol.proto | 4 ++++ generated_api_shadow/envoy/config/core/v4alpha/protocol.proto | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index f10e30d63a23..62e6ebce0192 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: - // Enabling this option might lead request smuggling vlnarability, especially if traffic + // Enabling this option might lead request smuggling vlnarability, especially if traffic // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index e12392fa5782..1736f750ce01 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -162,6 +162,10 @@ message Http1ProtocolOptions { // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead request smuggling vlnarability, especially if traffic + // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index c7690d8374be..62e6ebce0192 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -162,6 +162,10 @@ message Http1ProtocolOptions { // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead request smuggling vlnarability, especially if traffic + // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index fd217a92211f..7c6474a08be0 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -162,6 +162,10 @@ message Http1ProtocolOptions { // By default such requests are rejected with `400 Bad Request`, but if option is enabled - // Envoy will remove Content-Length header and pass request to upstream. // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead request smuggling vlnarability, especially if traffic + // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } From 004dc2ad2b46a49834b9d87185273500f9ffd3df Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 31 Jul 2020 16:00:17 -0700 Subject: [PATCH 16/40] use configuration option directly Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 2 +- source/common/http/http1/codec_impl.h | 2 -- source/common/http/http1/codec_impl_legacy.cc | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 57f7f66bcb1d..126c48153c6c 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -892,7 +892,7 @@ int ServerConnectionImpl::onHeadersComplete() { // and serve request if allowed by http1 codec settings. if (parser_.uses_transfer_encoding != 0 && (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX)) { - if ((parser_.flags & F_CHUNKED) && allowChunkedLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { headers->removeContentLength(); } else { ENVOY_CONN_LOG(debug, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 5e7bdc6a4eae..c74c0adae87c 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -201,7 +201,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable 0 && parser_.content_length != ULLONG_MAX)) { - if ((parser_.flags & F_CHUNKED) && allowChunkedLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { headers->removeContentLength(); } else { ENVOY_CONN_LOG(debug, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", From de444cc9f3a1612c149ddf87c88fe47fdee46e12 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 31 Jul 2020 16:34:59 -0700 Subject: [PATCH 17/40] fix typo Signed-off-by: Oleg Guba --- 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 62e6ebce0192..36038e1ff09c 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: - // Enabling this option might lead request smuggling vlnarability, especially if traffic + // Enabling this option might lead to request smuggling vulnerability, especially if traffic // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 1736f750ce01..3c779d6031e8 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -164,7 +164,7 @@ message Http1ProtocolOptions { // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: - // Enabling this option might lead request smuggling vlnarability, especially if traffic + // Enabling this option might lead to request smuggling vulnerability, especially if traffic // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 62e6ebce0192..36038e1ff09c 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 { // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: - // Enabling this option might lead request smuggling vlnarability, especially if traffic + // Enabling this option might lead to request smuggling vulnerability, especially if traffic // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 7c6474a08be0..ba3b92d8ef96 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 { // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: - // Enabling this option might lead request smuggling vlnarability, especially if traffic + // Enabling this option might lead to request smuggling vulnerability, especially if traffic // is proxied via multiple layers of proxies. bool allow_chunked_length = 6; } From af435ca343eb41c7aa0292f4c55f054f49475d13 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 31 Jul 2020 17:24:17 -0700 Subject: [PATCH 18/40] cleanup legacy codec Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl_legacy.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 08888eb6a1cb..622d9441459b 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -205,7 +205,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Fri, 31 Jul 2020 18:48:03 -0700 Subject: [PATCH 19/40] blankline Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 126c48153c6c..2419e2eae42d 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -912,6 +912,7 @@ int ServerConnectionImpl::onHeadersComplete() { if (parser_.flags & F_CHUNKED || (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX) || handling_upgrade_) { active_request.request_decoder_->decodeHeaders(std::move(headers), false); + // If the connection has been closed (or is closing) after decoding headers, pause the parser // so we return control to the caller. if (connection_.state() != Network::Connection::State::Open) { From 9bbc9641a39d8b6a239cfee1aaf4bd9a22d8e344 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 5 Aug 2020 13:18:31 -0700 Subject: [PATCH 20/40] check for Content-Length header, not for parser_.content_length Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 3 +-- source/common/http/http1/codec_impl_legacy.cc | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 2419e2eae42d..85863370d90c 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -890,8 +890,7 @@ int ServerConnectionImpl::onHeadersComplete() { // Reject request with Http::Code::BadRequest by default or remove Content-Length header // and serve request if allowed by http1 codec settings. - if (parser_.uses_transfer_encoding != 0 && - (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX)) { + if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { headers->removeContentLength(); } else { diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 923bc109a6ab..63e3fe89ca6d 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -895,8 +895,7 @@ int ServerConnectionImpl::onHeadersComplete() { // Reject request with Http::Code::BadRequest by default or remove Content-Length header // and serve request if allowed by http1 codec settings. - if (parser_.uses_transfer_encoding != 0 && - (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX)) { + if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { headers->removeContentLength(); } else { From 43e18a1b760c2b9dc0403e3fdb1c718986994622 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 5 Aug 2020 18:23:09 -0700 Subject: [PATCH 21/40] add testcases for different content-length value Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 36 ++++++++++++++++------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index dcd061d9f253..2bccad684a12 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -95,7 +95,7 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase, // Used to test if trailers are decoded/encoded void expectTrailersTest(bool enable_trailers); - void testAllowChunkedContentLength(bool allow_chunked_length); + void testAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); // Send the request, and validate the received request headers. // Then send a response just to clean up. @@ -328,9 +328,8 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade EXPECT_TRUE(status.ok()); } -void Http1ServerConnectionImplTest::testAllowChunkedContentLength(bool allow_chunked_length) { +void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length) { codec_settings_.allow_chunked_length_ = allow_chunked_length; - if (GetParam()) { codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, @@ -367,11 +366,12 @@ void Http1ServerConnectionImplTest::testAllowChunkedContentLength(bool allow_chu EXPECT_CALL(decoder, sendLocalReply(false, Http::Code::BadRequest, "Bad Request", _, _, _)); } - Buffer::OwnedImpl buffer( - "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\ncontent-length: 1\r\n\r\n" + Buffer::OwnedImpl buffer(fmt::format( + "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\ncontent-length: {}\r\n\r\n" "6\r\nHello \r\n" "5\r\nWorld\r\n" - "0\r\n\r\n"); + "0\r\n\r\n", content_length) + ); auto status = codec_->dispatch(buffer); @@ -2884,12 +2884,28 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersAccepted) { status = codec_->dispatch(buffer); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength) { - testAllowChunkedContentLength(false); +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength0) { + testAllowChunkedContentLength(0, false); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength1) { + // content-length less than POST body size + testAllowChunkedContentLength(1, false); +} + TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength100) { + // content-length greater than POST body size + testAllowChunkedContentLength(100, false); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength) { - testAllowChunkedContentLength(true); +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength0) { + testAllowChunkedContentLength(0, true); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength1) { + // content-length less than POST body size + testAllowChunkedContentLength(1, true); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength100) { + // content-length greater than POST body size + testAllowChunkedContentLength(100, true); } } // namespace Http From 09b9b11c4ad31f3a3da44818727180014934b9a2 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 5 Aug 2020 18:37:21 -0700 Subject: [PATCH 22/40] add minor behavior change note Signed-off-by: Oleg Guba --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c80419ccde39..4f5744b32933 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,7 @@ Minor Behavior Changes * 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: clarified and enforced 1xx handling. Multiple 100-continue headers are coalesced when proxying. 1xx headers other than {100, 101} are dropped. * http: fixed the 100-continue response path to properly handle upstream failure by sending 5xx responses. This behavior can be temporarily reverted by setting `envoy.reloadable_features.allow_500_after_100` to false. +* http: http1 codec returns `500 Non Implemented` instead of `400 Bad Request` for request having both Content-Length and Transfer-Encoding headers set and Transfer-Encoding contains unsupported encoding. * 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 From 1fbb0f8a12aa41720d7fb2601026c96ef766fe65 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 5 Aug 2020 21:35:35 -0700 Subject: [PATCH 23/40] update proto comment Signed-off-by: Oleg Guba --- api/envoy/config/core/v3/protocol.proto | 6 +++--- api/envoy/config/core/v4alpha/protocol.proto | 6 +++--- generated_api_shadow/envoy/config/core/v3/protocol.proto | 6 +++--- .../envoy/config/core/v4alpha/protocol.proto | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 36038e1ff09c..f397882e3199 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 Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. - // By default such requests are rejected with `400 Bad Request`, but if option is enabled - - // Envoy will remove Content-Length header and pass request to upstream. + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 3c779d6031e8..746d53c557ee 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 Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. - // By default such requests are rejected with `400 Bad Request`, but if option is enabled - - // Envoy will remove Content-Length header and pass request to upstream. + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 36038e1ff09c..f397882e3199 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 Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. - // By default such requests are rejected with `400 Bad Request`, but if option is enabled - - // Envoy will remove Content-Length header and pass request to upstream. + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index ba3b92d8ef96..5afb71210fbb 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 Envoy to process requests with `Content-Length` and `Transfer-Encoding: chunked`. - // By default such requests are rejected with `400 Bad Request`, but if option is enabled - - // Envoy will remove Content-Length header and pass request to upstream. + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. // See `RFC7230, sec. 3.3.3 ` for details. // // .. attention:: From 02a98d45ffd541a56eefa89c77f92050171cc0de Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 5 Aug 2020 21:46:56 -0700 Subject: [PATCH 24/40] add ClientConnectionImpl checks Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 33 +++++++++++++++---- source/common/http/http1/codec_impl.h | 2 +- source/common/http/http1/codec_impl_legacy.cc | 33 +++++++++++++++---- source/common/http/http1/codec_impl_legacy.h | 1 + test/common/http/http1/codec_impl_test.cc | 20 ++++++----- 5 files changed, 67 insertions(+), 22 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 85863370d90c..3098ee14b52c 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -888,17 +888,16 @@ int ServerConnectionImpl::onHeadersComplete() { // remove the received Content-Length field prior to forwarding such // a message. - // Reject request with Http::Code::BadRequest by default or remove Content-Length header - // and serve request if allowed by http1 codec settings. + // Reject request with Http::Code::BadRequest if Transfer-Encoding and Content-Length headers + // are present or remove Content-Length and serve request if allowed by http1 codec settings. if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { headers->removeContentLength(); } else { - ENVOY_CONN_LOG(debug, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", - connection_); error_code_ = Http::Code::BadRequest; sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); - throw CodecProtocolException("Both Content-Length and Transfer-Encdoding headers are set."); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); } } @@ -1080,7 +1079,8 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_) {} + max_response_headers_count, formatter(settings), settings.enable_trailers_), + codec_settings_(settings) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { @@ -1139,6 +1139,27 @@ int ClientConnectionImpl::onHeadersComplete() { } } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message. + + // Reject response if Transfer-Encoding and Content-Length headers are present + // or remove Content-Length and serve response if allowed by http1 codec settings. + if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { + headers->removeContentLength(); + } else { + sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + } + } + if (strict_1xx_and_204_headers_ && (parser_.status_code < 200 || parser_.status_code == 204)) { if (headers->TransferEncoding()) { sendProtocolError(Http1ResponseCodeDetails::get().TransferEncodingNotAllowed); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index c74c0adae87c..ae72f230a9ab 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -527,7 +527,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks& callbacks, const Http1Settings& settings, const uint32_t max_response_headers_count); - // Http::ClientConnection RequestEncoder& newStream(ResponseDecoder& response_decoder) override; @@ -577,6 +576,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } } + Http1Settings codec_settings_; absl::optional pending_response_; // TODO(mattklein123): The following bool tracks whether a pending response is complete before // dispatching callbacks. This is needed so that pending_response_ stays valid during callbacks diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 63e3fe89ca6d..a9aaadcc6ad0 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -893,17 +893,16 @@ int ServerConnectionImpl::onHeadersComplete() { // remove the received Content-Length field prior to forwarding such // a message. - // Reject request with Http::Code::BadRequest by default or remove Content-Length header - // and serve request if allowed by http1 codec settings. + // Reject request with Http::Code::BadRequest if Transfer-Encoding and Content-Length headers + // are present or remove Content-Length and serve request if allowed by http1 codec settings. if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { headers->removeContentLength(); } else { - ENVOY_CONN_LOG(debug, "Both 'Content-Length' and 'Transfer-Encdoding' are set.", - connection_); error_code_ = Http::Code::BadRequest; sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); - throw CodecProtocolException("Both Content-Length and Transfer-Encdoding headers are set."); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); } } @@ -1085,7 +1084,8 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_) {} + max_response_headers_count, formatter(settings), settings.enable_trailers_), + codec_settings_(settings) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { @@ -1144,6 +1144,27 @@ int ClientConnectionImpl::onHeadersComplete() { } } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message. + + // Reject response if Transfer-Encoding and Content-Length headers are present + // or remove Content-Length and serve response if allowed by http1 codec settings. + if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { + headers->removeContentLength(); + } else { + sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + } + } + if (strict_1xx_and_204_headers_ && (parser_.status_code < 200 || parser_.status_code == 204)) { if (headers->TransferEncoding()) { sendProtocolError(Http1ResponseCodeDetails::get().TransferEncodingNotAllowed); diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 622d9441459b..9b06db1eeecb 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -581,6 +581,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } } + Http1Settings codec_settings_; absl::optional pending_response_; // TODO(mattklein123): The following bool tracks whether a pending response is complete before // dispatching callbacks. This is needed so that pending_response_ stays valid during callbacks diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 2bccad684a12..2e89d568018a 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -328,7 +328,8 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade EXPECT_TRUE(status.ok()); } -void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length) { +void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t content_length, + bool allow_chunked_length) { codec_settings_.allow_chunked_length_ = allow_chunked_length; if (GetParam()) { codec_ = std::make_unique( @@ -366,12 +367,12 @@ void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t conte EXPECT_CALL(decoder, sendLocalReply(false, Http::Code::BadRequest, "Bad Request", _, _, _)); } - Buffer::OwnedImpl buffer(fmt::format( - "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\ncontent-length: {}\r\n\r\n" - "6\r\nHello \r\n" - "5\r\nWorld\r\n" - "0\r\n\r\n", content_length) - ); + Buffer::OwnedImpl buffer( + fmt::format("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\ncontent-length: {}\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n", + content_length)); auto status = codec_->dispatch(buffer); @@ -379,7 +380,8 @@ void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t conte EXPECT_TRUE(status.ok()); } else { EXPECT_TRUE(isCodecProtocolError(status)); - EXPECT_EQ(status.message(), "Both Content-Length and Transfer-Encdoding headers are set."); + EXPECT_EQ(status.message(), + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); EXPECT_EQ("http1.content_length_not_allowed", response_encoder->getStream().responseDetails()); } } @@ -2891,7 +2893,7 @@ TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength1 // content-length less than POST body size testAllowChunkedContentLength(1, false); } - TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength100) { +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength100) { // content-length greater than POST body size testAllowChunkedContentLength(100, false); } From 57a8b1f154d83904f0f1eb02cbd5573671943a62 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 6 Aug 2020 13:22:13 -0700 Subject: [PATCH 25/40] move settings to ConnectionImpl Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 15 +++++++-------- source/common/http/http1/codec_impl.h | 8 +++----- source/common/http/http1/codec_impl_legacy.cc | 15 +++++++-------- source/common/http/http1/codec_impl_legacy.h | 8 +++----- 4 files changed, 20 insertions(+), 26 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 3098ee14b52c..971baa53c5f4 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -444,13 +444,13 @@ http_parser_settings ConnectionImpl::settings_{ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, http_parser_type type, uint32_t max_headers_kb, const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) + HeaderKeyFormatterPtr&& header_key_formatter, Http1Settings settings) : connection_(connection), stats_(stats), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), connection_header_sanitization_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.connection_header_sanitization")), - enable_trailers_(enable_trailers), + codec_settings_(settings), strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.strict_1xx_and_204_response_headers")), output_buffer_([&]() -> void { this->onBelowLowWatermark(); }, @@ -581,7 +581,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { // We previously already finished up the headers, these headers are // now trailers. if (header_parsing_state_ == HeaderParsingState::Done) { - if (!enable_trailers_) { + if (!enableTrailers()) { // Ignore trailers. return; } @@ -599,7 +599,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { } void ConnectionImpl::onHeaderValue(const char* data, size_t length) { - if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) { + if (header_parsing_state_ == HeaderParsingState::Done && !enableTrailers()) { // Ignore trailers. return; } @@ -764,8 +764,8 @@ ServerConnectionImpl::ServerConnectionImpl( envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings.enable_trailers_), - callbacks_(callbacks), codec_settings_(settings), + max_request_headers_count, formatter(settings), settings), + callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); }), @@ -1079,8 +1079,7 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_), - codec_settings_(settings) {} + max_response_headers_count, formatter(settings), settings) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index ae72f230a9ab..4f76fa6c795c 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -204,7 +204,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable active_request_; - Http1Settings codec_settings_; const Buffer::OwnedBufferFragmentImpl::Releasor response_buffer_releasor_; uint32_t outbound_responses_{}; // This defaults to 2, which functionally disables pipelining. If any users @@ -576,7 +575,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } } - Http1Settings codec_settings_; absl::optional pending_response_; // TODO(mattklein123): The following bool tracks whether a pending response is complete before // dispatching callbacks. This is needed so that pending_response_ stays valid during callbacks diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index a9aaadcc6ad0..7eb85301b432 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -449,13 +449,13 @@ http_parser_settings ConnectionImpl::settings_{ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, http_parser_type type, uint32_t max_headers_kb, const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) + HeaderKeyFormatterPtr&& header_key_formatter, Http1Settings settings) : connection_(connection), stats_(stats), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), connection_header_sanitization_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.connection_header_sanitization")), - enable_trailers_(enable_trailers), + codec_settings_(settings), strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.strict_1xx_and_204_response_headers")), output_buffer_([&]() -> void { this->onBelowLowWatermark(); }, @@ -586,7 +586,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { // We previously already finished up the headers, these headers are // now trailers. if (header_parsing_state_ == HeaderParsingState::Done) { - if (!enable_trailers_) { + if (!enableTrailers()) { // Ignore trailers. return; } @@ -604,7 +604,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { } void ConnectionImpl::onHeaderValue(const char* data, size_t length) { - if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) { + if (header_parsing_state_ == HeaderParsingState::Done && !enableTrailers()) { // Ignore trailers. return; } @@ -769,8 +769,8 @@ ServerConnectionImpl::ServerConnectionImpl( envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings.enable_trailers_), - callbacks_(callbacks), codec_settings_(settings), + max_request_headers_count, formatter(settings), settings), + callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); }), @@ -1084,8 +1084,7 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_), - codec_settings_(settings) {} + max_response_headers_count, formatter(settings), settings) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 9b06db1eeecb..2587951cc372 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -208,7 +208,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable active_request_; - Http1Settings codec_settings_; const Buffer::OwnedBufferFragmentImpl::Releasor response_buffer_releasor_; uint32_t outbound_responses_{}; // This defaults to 2, which functionally disables pipelining. If any users @@ -581,7 +580,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } } - Http1Settings codec_settings_; absl::optional pending_response_; // TODO(mattklein123): The following bool tracks whether a pending response is complete before // dispatching callbacks. This is needed so that pending_response_ stays valid during callbacks From 5f13f375403d46c66a289a31e0a18208f3e5f2a6 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 6 Aug 2020 15:53:55 -0700 Subject: [PATCH 26/40] update field description with requests/responses Signed-off-by: Oleg Guba --- include/envoy/http/codec.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 6c1d0e225c4c..23d94197585a 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -374,9 +374,9 @@ struct Http1Settings { // - Is neither a HEAD only request nor a HTTP Upgrade // - Not a HEAD request bool enable_trailers_{false}; - // Allow serving requests with both Content-Length and Transfer-Encoding: chunked. - // If enabled and such a request is served - Content-Length is ignored and removed from - // the request headers. + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. bool allow_chunked_length_{false}; enum class HeaderKeyFormat { From b082ee4d13022e1f216c6207d9d04ca1b0cf2fe0 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 6 Aug 2020 15:56:53 -0700 Subject: [PATCH 27/40] fix typo in Transfer-Encdoding Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 4 ++-- source/common/http/http1/codec_impl_legacy.cc | 4 ++-- test/common/http/http1/codec_impl_test.cc | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 971baa53c5f4..fd4afe24c09f 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -897,7 +897,7 @@ int ServerConnectionImpl::onHeadersComplete() { error_code_ = Http::Code::BadRequest; sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); } } @@ -1155,7 +1155,7 @@ int ClientConnectionImpl::onHeadersComplete() { } else { sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); } } diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 7eb85301b432..c3a850c76582 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -902,7 +902,7 @@ int ServerConnectionImpl::onHeadersComplete() { error_code_ = Http::Code::BadRequest; sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); } } @@ -1160,7 +1160,7 @@ int ClientConnectionImpl::onHeadersComplete() { } else { sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); } } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 2e89d568018a..cade5eafd8b7 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -381,7 +381,7 @@ void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t conte } else { EXPECT_TRUE(isCodecProtocolError(status)); EXPECT_EQ(status.message(), - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encdoding' are set."); + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); EXPECT_EQ("http1.content_length_not_allowed", response_encoder->getStream().responseDetails()); } } From 4102c142a02103462d638f7616881e8672a50072 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Thu, 6 Aug 2020 21:28:53 -0700 Subject: [PATCH 28/40] move validation logic to ConnectionImpl::onHeadersCompleteBase() to avoid duplication Signed-off-by: Oleg Guba --- docs/root/version_history/current.rst | 3 +- source/common/http/http1/codec_impl.cc | 66 +++++++----------- source/common/http/http1/codec_impl_legacy.cc | 68 +++++++------------ test/integration/integration_test.cc | 3 +- 4 files changed, 49 insertions(+), 91 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 4f5744b32933..2ea85cb346f0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,7 +15,6 @@ Minor Behavior Changes * 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: clarified and enforced 1xx handling. Multiple 100-continue headers are coalesced when proxying. 1xx headers other than {100, 101} are dropped. * http: fixed the 100-continue response path to properly handle upstream failure by sending 5xx responses. This behavior can be temporarily reverted by setting `envoy.reloadable_features.allow_500_after_100` to false. -* http: http1 codec returns `500 Non Implemented` instead of `400 Bad Request` for request having both Content-Length and Transfer-Encoding headers set and Transfer-Encoding contains unsupported encoding. * 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 @@ -48,7 +47,7 @@ New Features * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP ` and :ref:`network ` filters. * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message. * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as custom header. -* http: added :ref:`allow_chunked_length ` configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream. +* http: added :ref:`allow_chunked_length ` configuration option for HTTP/1 codec to allow processing requests/responses with both Content-Length and Transfer-Encoding: chunked headers. If such message is served and option is enabled - per RFC Content-Length is ignored and removed. * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is used by default, but the new codecs can be enabled for testing by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to true. The new codecs will be in development for one month, and then enabled by default while the old codecs are deprecated. * load balancer: added a :ref:`configuration` option to specify the active request bias used by the least request load balancer. * lua: added Lua APIs to access :ref:`SSL connection info ` object. diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index fd4afe24c09f..433e5266d2c9 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -689,6 +689,29 @@ int ConnectionImpl::onHeadersCompleteBase() { } } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message. + + // Reject message with Http::Code::BadRequest if both Transfer-Encoding and Content-Length + // headers are present or if allowed by http1 codec settings and 'Transfer-Encoding' + // is chunked - remove Content-Length and serve request. + if (parser_.uses_transfer_encoding != 0 && request_or_response_headers.ContentLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { + request_or_response_headers.removeContentLength(); + } else { + error_code_ = Http::Code::BadRequest; + sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + } + } + int rc = onHeadersComplete(); header_parsing_state_ = HeaderParsingState::Done; @@ -879,28 +902,6 @@ int ServerConnectionImpl::onHeadersComplete() { "http/1.1 protocol error: request headers failed spec compliance checks"); } - // https://tools.ietf.org/html/rfc7230#section-3.3.3 - // If a message is received with both a Transfer-Encoding and a - // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message. - - // Reject request with Http::Code::BadRequest if Transfer-Encoding and Content-Length headers - // are present or remove Content-Length and serve request if allowed by http1 codec settings. - if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { - if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { - headers->removeContentLength(); - } else { - error_code_ = Http::Code::BadRequest; - sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); - throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); - } - } - // Determine here whether we have a body or not. This uses the new RFC semantics where the // presence of content-length or chunked transfer-encoding indicates a body vs. a particular // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed @@ -1138,27 +1139,6 @@ int ClientConnectionImpl::onHeadersComplete() { } } - // https://tools.ietf.org/html/rfc7230#section-3.3.3 - // If a message is received with both a Transfer-Encoding and a - // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message. - - // Reject response if Transfer-Encoding and Content-Length headers are present - // or remove Content-Length and serve response if allowed by http1 codec settings. - if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { - if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { - headers->removeContentLength(); - } else { - sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); - throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); - } - } - if (strict_1xx_and_204_headers_ && (parser_.status_code < 200 || parser_.status_code == 204)) { if (headers->TransferEncoding()) { sendProtocolError(Http1ResponseCodeDetails::get().TransferEncodingNotAllowed); diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index c3a850c76582..2ac7ca47555a 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -694,6 +694,29 @@ int ConnectionImpl::onHeadersCompleteBase() { } } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message. + + // Reject message with Http::Code::BadRequest if both Transfer-Encoding and Content-Length + // headers are present or if allowed by http1 codec settings and 'Transfer-Encoding' + // is chunked - remove Content-Length and serve request. + if (parser_.uses_transfer_encoding != 0 && request_or_response_headers.ContentLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { + request_or_response_headers.removeContentLength(); + } else { + error_code_ = Http::Code::BadRequest; + sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + } + } + int rc = onHeadersComplete(); header_parsing_state_ = HeaderParsingState::Done; @@ -770,7 +793,7 @@ ServerConnectionImpl::ServerConnectionImpl( headers_with_underscores_action) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, max_request_headers_count, formatter(settings), settings), - callbacks_(callbacks), + callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); }), @@ -884,28 +907,6 @@ int ServerConnectionImpl::onHeadersComplete() { "http/1.1 protocol error: request headers failed spec compliance checks"); } - // https://tools.ietf.org/html/rfc7230#section-3.3.3 - // If a message is received with both a Transfer-Encoding and a - // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message. - - // Reject request with Http::Code::BadRequest if Transfer-Encoding and Content-Length headers - // are present or remove Content-Length and serve request if allowed by http1 codec settings. - if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { - if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { - headers->removeContentLength(); - } else { - error_code_ = Http::Code::BadRequest; - sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); - throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); - } - } - // Determine here whether we have a body or not. This uses the new RFC semantics where the // presence of content-length or chunked transfer-encoding indicates a body vs. a particular // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed @@ -1143,27 +1144,6 @@ int ClientConnectionImpl::onHeadersComplete() { } } - // https://tools.ietf.org/html/rfc7230#section-3.3.3 - // If a message is received with both a Transfer-Encoding and a - // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message. - - // Reject response if Transfer-Encoding and Content-Length headers are present - // or remove Content-Length and serve response if allowed by http1 codec settings. - if (parser_.uses_transfer_encoding != 0 && headers->ContentLength()) { - if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { - headers->removeContentLength(); - } else { - sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); - throw CodecProtocolException( - "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); - } - } - if (strict_1xx_and_204_headers_ && (parser_.status_code < 200 || parser_.status_code == 204)) { if (headers->TransferEncoding()) { sendProtocolError(Http1ResponseCodeDetails::get().TransferEncodingNotAllowed); diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index c6d93fcf1adf..081bcd2681ba 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -425,8 +425,7 @@ TEST_P(IntegrationTest, TestSmuggling) { "identity,chunked \r\ncontent-length: 36\r\n\r\n" + smuggled_request; sendRawHttpAndWaitForResponse(lookupPort("http"), request.c_str(), &response, false); - // 'identity' encoding is not supported - EXPECT_THAT(response, HasSubstr("HTTP/1.1 501 Not Implemented\r\n")); + EXPECT_THAT(response, HasSubstr("HTTP/1.1 400 Bad Request\r\n")); } } From 467e7d0bcc8a8ec9a54a92edc174b21da42d7dec Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Fri, 7 Aug 2020 10:38:00 -0700 Subject: [PATCH 29/40] change check order Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 28 +++++++++---------- source/common/http/http1/codec_impl_legacy.cc | 28 +++++++++---------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 433e5266d2c9..dbb860e3b15b 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -675,20 +675,6 @@ int ConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } - // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject - // transfer-codings it does not understand. - // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a - // CONNECT request has no defined semantics, and may be rejected. - if (request_or_response_headers.TransferEncoding()) { - const absl::string_view encoding = request_or_response_headers.getTransferEncodingValue(); - if (!absl::EqualsIgnoreCase(encoding, Headers::get().TransferEncodingValues.Chunked) || - parser_.method == HTTP_CONNECT) { - error_code_ = Http::Code::NotImplemented; - sendProtocolError(Http1ResponseCodeDetails::get().InvalidTransferEncoding); - throw CodecProtocolException("http/1.1 protocol error: unsupported transfer encoding"); - } - } - // https://tools.ietf.org/html/rfc7230#section-3.3.3 // If a message is received with both a Transfer-Encoding and a // Content-Length header field, the Transfer-Encoding overrides the @@ -712,6 +698,20 @@ int ConnectionImpl::onHeadersCompleteBase() { } } + // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject + // transfer-codings it does not understand. + // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a + // CONNECT request has no defined semantics, and may be rejected. + if (request_or_response_headers.TransferEncoding()) { + const absl::string_view encoding = request_or_response_headers.getTransferEncodingValue(); + if (!absl::EqualsIgnoreCase(encoding, Headers::get().TransferEncodingValues.Chunked) || + parser_.method == HTTP_CONNECT) { + error_code_ = Http::Code::NotImplemented; + sendProtocolError(Http1ResponseCodeDetails::get().InvalidTransferEncoding); + throw CodecProtocolException("http/1.1 protocol error: unsupported transfer encoding"); + } + } + int rc = onHeadersComplete(); header_parsing_state_ = HeaderParsingState::Done; diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 2ac7ca47555a..227a518f3f82 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -680,20 +680,6 @@ int ConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } - // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject - // transfer-codings it does not understand. - // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a - // CONNECT request has no defined semantics, and may be rejected. - if (request_or_response_headers.TransferEncoding()) { - const absl::string_view encoding = request_or_response_headers.getTransferEncodingValue(); - if (!absl::EqualsIgnoreCase(encoding, Headers::get().TransferEncodingValues.Chunked) || - parser_.method == HTTP_CONNECT) { - error_code_ = Http::Code::NotImplemented; - sendProtocolError(Http1ResponseCodeDetails::get().InvalidTransferEncoding); - throw CodecProtocolException("http/1.1 protocol error: unsupported transfer encoding"); - } - } - // https://tools.ietf.org/html/rfc7230#section-3.3.3 // If a message is received with both a Transfer-Encoding and a // Content-Length header field, the Transfer-Encoding overrides the @@ -717,6 +703,20 @@ int ConnectionImpl::onHeadersCompleteBase() { } } + // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject + // transfer-codings it does not understand. + // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a + // CONNECT request has no defined semantics, and may be rejected. + if (request_or_response_headers.TransferEncoding()) { + const absl::string_view encoding = request_or_response_headers.getTransferEncodingValue(); + if (!absl::EqualsIgnoreCase(encoding, Headers::get().TransferEncodingValues.Chunked) || + parser_.method == HTTP_CONNECT) { + error_code_ = Http::Code::NotImplemented; + sendProtocolError(Http1ResponseCodeDetails::get().InvalidTransferEncoding); + throw CodecProtocolException("http/1.1 protocol error: unsupported transfer encoding"); + } + } + int rc = onHeadersComplete(); header_parsing_state_ = HeaderParsingState::Done; From 984b394c46fb09555f129fdc861085f93c8dff69 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Mon, 10 Aug 2020 20:08:06 -0700 Subject: [PATCH 30/40] change field order in ConnectionImpl Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 17 ++++++++--------- source/common/http/http1/codec_impl.h | 8 ++++---- source/common/http/http1/codec_impl_legacy.cc | 17 ++++++++--------- source/common/http/http1/codec_impl_legacy.h | 7 ++++--- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index dbb860e3b15b..5a00ba2fc3ca 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -442,15 +442,14 @@ http_parser_settings ConnectionImpl::settings_{ }; ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, - http_parser_type type, uint32_t max_headers_kb, - const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, Http1Settings settings) - : connection_(connection), stats_(stats), + const Http1Settings& settings, http_parser_type type, + uint32_t max_headers_kb, const uint32_t max_headers_count, + HeaderKeyFormatterPtr&& header_key_formatter) + : connection_(connection), stats_(stats), codec_settings_(settings), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), connection_header_sanitization_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.connection_header_sanitization")), - codec_settings_(settings), strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.strict_1xx_and_204_response_headers")), output_buffer_([&]() -> void { this->onBelowLowWatermark(); }, @@ -786,8 +785,8 @@ ServerConnectionImpl::ServerConnectionImpl( const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) - : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings), + : ConnectionImpl(connection, stats, settings, HTTP_REQUEST, max_request_headers_kb, + max_request_headers_count, formatter(settings)), callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); @@ -1079,8 +1078,8 @@ void ServerConnectionImpl::checkHeaderNameForUnderscores() { ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) - : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings) {} + : ConnectionImpl(connection, stats, settings, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, + max_response_headers_count, formatter(settings)) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 4f76fa6c795c..0478905ee169 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -218,9 +218,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable void { this->onBelowLowWatermark(); }, @@ -791,8 +790,8 @@ ServerConnectionImpl::ServerConnectionImpl( const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) - : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings), + : ConnectionImpl(connection, stats, settings, HTTP_REQUEST, max_request_headers_kb, + max_request_headers_count, formatter(settings)), callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); @@ -1084,8 +1083,8 @@ void ServerConnectionImpl::checkHeaderNameForUnderscores() { ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) - : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings) {} + : ConnectionImpl(connection, stats, settings, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, + max_response_headers_count, formatter(settings)) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 2587951cc372..59de84e67921 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -223,8 +223,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Wed, 12 Aug 2020 17:25:34 -0700 Subject: [PATCH 31/40] add response test Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 141 +++++++++++++++++++--- 1 file changed, 122 insertions(+), 19 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index cade5eafd8b7..6730693bfd63 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -95,7 +95,7 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase, // Used to test if trailers are decoded/encoded void expectTrailersTest(bool enable_trailers); - void testAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); + void testServerAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); // Send the request, and validate the received request headers. // Then send a response just to clean up. @@ -328,8 +328,8 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade EXPECT_TRUE(status.ok()); } -void Http1ServerConnectionImplTest::testAllowChunkedContentLength(uint32_t content_length, - bool allow_chunked_length) { +void Http1ServerConnectionImplTest::testServerAllowChunkedContentLength(uint32_t content_length, + bool allow_chunked_length) { codec_settings_.allow_chunked_length_ = allow_chunked_length; if (GetParam()) { codec_ = std::make_unique( @@ -1898,6 +1898,30 @@ TEST_P(Http1ServerConnectionImplTest, WatermarkTest) { ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); } +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength0) { + testServerAllowChunkedContentLength(0, false); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength1) { + // content-length less than POST body size + testServerAllowChunkedContentLength(1, false); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength100) { + // content-length greater than POST body size + testServerAllowChunkedContentLength(100, false); +} + +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength0) { + testServerAllowChunkedContentLength(0, true); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength1) { + // content-length less than POST body size + testServerAllowChunkedContentLength(1, true); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength100) { + // content-length greater than POST body size + testServerAllowChunkedContentLength(100, true); +} + class Http1ClientConnectionImplTest : public Http1CodecTestBase, public testing::TestWithParam { public: @@ -1924,6 +1948,8 @@ class Http1ClientConnectionImplTest : public Http1CodecTestBase, NiceMock codec_settings_; Http::ClientConnectionPtr codec_; + void testClientAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); + protected: Stats::TestUtil::TestStore store_; uint32_t max_response_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; @@ -1934,6 +1960,52 @@ INSTANTIATE_TEST_SUITE_P(Codecs, Http1ClientConnectionImplTest, testing::Bool(), return param.param ? "New" : "Legacy"; }); +void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength(uint32_t content_length, + bool allow_chunked_length) { + codec_settings_.allow_chunked_length_ = allow_chunked_length; + if (GetParam()) { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + } else { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + } + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + TestResponseHeaderMapImpl expected_headers{{":status", "200"}, {"transfer-encoding", "chunked"}}; + Buffer::OwnedImpl expected_data("Hello World"); + + if (allow_chunked_length) { + EXPECT_CALL(response_decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(response_decoder, decodeData(_, true)); + } else { + EXPECT_CALL(response_decoder, decodeHeaders_(_, _)).Times(0); + EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0); + } + + Buffer::OwnedImpl buffer( + fmt::format("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\ncontent-length: {}\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n", + content_length)); + auto status = codec_->dispatch(buffer); + + if (allow_chunked_length) { + EXPECT_TRUE(status.ok()); + } else { + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + }; +} + TEST_P(Http1ClientConnectionImplTest, SimpleGet) { initialize(); @@ -2886,28 +2958,59 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersAccepted) { status = codec_->dispatch(buffer); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength0) { - testAllowChunkedContentLength(0, false); +TEST_P(Http1ClientConnectionImplTest, TestResponseSplit0) { + testClientAllowChunkedContentLength(0, false); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength1) { - // content-length less than POST body size - testAllowChunkedContentLength(1, false); + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplit1) { + testClientAllowChunkedContentLength(1, false); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength100) { - // content-length greater than POST body size - testAllowChunkedContentLength(100, false); + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplit100) { + testClientAllowChunkedContentLength(100, false); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength0) { - testAllowChunkedContentLength(0, true); +TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength0) { + testClientAllowChunkedContentLength(0, true); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength1) { - // content-length less than POST body size - testAllowChunkedContentLength(1, true); + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength1) { + testClientAllowChunkedContentLength(1, true); } -TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength100) { - // content-length greater than POST body size - testAllowChunkedContentLength(100, true); + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength100) { + testClientAllowChunkedContentLength(100, true); +} + +TEST_P(Http1ClientConnectionImplTest, TestUnknown) { + codec_settings_.allow_chunked_length_ = true; + initialize(); + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + TestResponseHeaderMapImpl expected_headers{{":status", "200"}, {"transfer-encoding", "chunked"}}; + Buffer::OwnedImpl expected_data("Hello World"); + + EXPECT_CALL(response_decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(response_decoder, decodeData(_, true)); + + Buffer::OwnedImpl buffer( + "HTTP/1.1 200 OK\r\n" + "Content-Length: 0\r\n" + "Transfer-Encoding: chunked\r\n" + "\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n"); + auto status = codec_->dispatch(buffer); + + EXPECT_EQ(status.message(), "NONE"); + // EXPECT_TRUE(status.ok()); } } // namespace Http From ac60a7006c80b5570d7ef82138c2e53b7524dc58 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 12 Aug 2020 18:20:33 -0700 Subject: [PATCH 32/40] implement response tests, fix response codec for content-length=0 case Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 3 +- source/common/http/http1/codec_impl_legacy.cc | 3 +- test/common/http/http1/codec_impl_test.cc | 31 ------------------- 3 files changed, 4 insertions(+), 33 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 5a00ba2fc3ca..e2b7e9f7d479 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1086,7 +1086,8 @@ bool ClientConnectionImpl::cannotHaveBody() { ASSERT(!pending_response_done_); return true; } else if (parser_.status_code == 204 || parser_.status_code == 304 || - (parser_.status_code >= 200 && parser_.content_length == 0)) { + (parser_.status_code >= 200 && parser_.content_length == 0 && + !(parser_.flags & F_CHUNKED))) { return true; } else { return false; diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 88956c00c9a6..e419f22fbdbb 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -1091,7 +1091,8 @@ bool ClientConnectionImpl::cannotHaveBody() { ASSERT(!pending_response_done_); return true; } else if (parser_.status_code == 204 || parser_.status_code == 304 || - (parser_.status_code >= 200 && parser_.content_length == 0)) { + (parser_.status_code >= 200 && parser_.content_length == 0 && + !(parser_.flags & F_CHUNKED))) { return true; } else { return false; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 6730693bfd63..ac20a594d5f9 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2982,36 +2982,5 @@ TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength100) { testClientAllowChunkedContentLength(100, true); } -TEST_P(Http1ClientConnectionImplTest, TestUnknown) { - codec_settings_.allow_chunked_length_ = true; - initialize(); - - NiceMock response_decoder; - Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - - TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); - - TestResponseHeaderMapImpl expected_headers{{":status", "200"}, {"transfer-encoding", "chunked"}}; - Buffer::OwnedImpl expected_data("Hello World"); - - EXPECT_CALL(response_decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)); - EXPECT_CALL(response_decoder, decodeData(_, true)); - - Buffer::OwnedImpl buffer( - "HTTP/1.1 200 OK\r\n" - "Content-Length: 0\r\n" - "Transfer-Encoding: chunked\r\n" - "\r\n" - "6\r\nHello \r\n" - "5\r\nWorld\r\n" - "0\r\n\r\n"); - auto status = codec_->dispatch(buffer); - - EXPECT_EQ(status.message(), "NONE"); - // EXPECT_TRUE(status.ok()); -} - } // namespace Http } // namespace Envoy From 97e7828ad07754f5a082ad7934bdb59d92669849 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 12 Aug 2020 19:17:47 -0700 Subject: [PATCH 33/40] use testingNewCodec() instead of GetParam() Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 1ca4a84a4696..8bf91543a661 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -333,7 +333,7 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade void Http1ServerConnectionImplTest::testServerAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length) { codec_settings_.allow_chunked_length_ = allow_chunked_length; - if (GetParam()) { + if (testingNewCodec()) { codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); @@ -1967,7 +1967,7 @@ INSTANTIATE_TEST_SUITE_P(Codecs, Http1ClientConnectionImplTest, testing::Bool(), void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length) { codec_settings_.allow_chunked_length_ = allow_chunked_length; - if (GetParam()) { + if (testingNewCodec()) { codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); } else { From 536f724a6833be653ebb719181745f7601b19d25 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Mon, 24 Aug 2020 12:17:54 -0700 Subject: [PATCH 34/40] define Http1ResponseCodeDetails::get().ChunkedContentLength and update code to use it Signed-off-by: Oleg Guba --- source/common/http/http1/codec_impl.cc | 3 ++- source/common/http/http1/codec_impl_legacy.cc | 3 ++- test/common/http/http1/codec_impl_test.cc | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 453f5d3cd2a6..5a3f87340239 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -42,6 +42,7 @@ struct Http1ResponseCodeDetailValues { const absl::string_view TransferEncodingNotAllowed = "http1.transfer_encoding_not_allowed"; const absl::string_view ContentLengthNotAllowed = "http1.content_length_not_allowed"; const absl::string_view InvalidUnderscore = "http1.unexpected_underscore"; + const absl::string_view ChunkedContentLength = "http1.content_length_and_chunked_not_allowed"; }; struct Http1HeaderTypesValues { @@ -747,7 +748,7 @@ Envoy::StatusOr ConnectionImpl::onHeadersCompleteBase() { request_or_response_headers.removeContentLength(); } else { error_code_ = Http::Code::BadRequest; - RETURN_IF_ERROR(sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed)); + RETURN_IF_ERROR(sendProtocolError(Http1ResponseCodeDetails::get().ChunkedContentLength)); return codecProtocolError( "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); } diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index e419f22fbdbb..1ecccdbad4ba 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -41,6 +41,7 @@ struct Http1ResponseCodeDetailValues { const absl::string_view TransferEncodingNotAllowed = "http1.transfer_encoding_not_allowed"; const absl::string_view ContentLengthNotAllowed = "http1.content_length_not_allowed"; const absl::string_view InvalidUnderscore = "http1.unexpected_underscore"; + const absl::string_view ChunkedContentLength = "http1.content_length_and_chunked_not_allowed"; }; struct Http1HeaderTypesValues { @@ -696,7 +697,7 @@ int ConnectionImpl::onHeadersCompleteBase() { request_or_response_headers.removeContentLength(); } else { error_code_ = Http::Code::BadRequest; - sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed); + sendProtocolError(Http1ResponseCodeDetails::get().ChunkedContentLength); throw CodecProtocolException( "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 8bf91543a661..b6911d30897d 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -384,7 +384,7 @@ void Http1ServerConnectionImplTest::testServerAllowChunkedContentLength(uint32_t EXPECT_TRUE(isCodecProtocolError(status)); EXPECT_EQ(status.message(), "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); - EXPECT_EQ("http1.content_length_not_allowed", response_encoder->getStream().responseDetails()); + EXPECT_EQ("http1.content_length_and_chunked_not_allowed", response_encoder->getStream().responseDetails()); } } From ac1d0e12d4f2bb26cb81d247379cf10dc5cf4689 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Tue, 25 Aug 2020 17:34:22 -0700 Subject: [PATCH 35/40] simple integration test Signed-off-by: Oleg Guba --- test/integration/integration_test.cc | 32 ++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 6c526fbb7425..8212b536adeb 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -48,6 +48,11 @@ void setAllowHttp10WithDefaultHost( hcm.mutable_http_protocol_options()->set_default_host_for_http_10("default.com"); } +void setAllowChunkedLength( + envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) { + hcm.mutable_http_protocol_options()->set_allow_chunked_length(true); +} + } // namespace INSTANTIATE_TEST_SUITE_P(IpVersions, IntegrationTest, @@ -429,6 +434,33 @@ TEST_P(IntegrationTest, TestSmuggling) { } } +TEST_P(IntegrationTest, TestAllowChunkedLength) { + autonomous_upstream_ = true; + config_helper_.addConfigModifier(&setAllowChunkedLength); + initialize(); + const std::string smuggled_request = "GET / HTTP/1.1\r\nHost: disallowed\r\n\r\n"; + ASSERT_EQ(smuggled_request.length(), 36); + + { + std::string response; + const std::string full_request = + "POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Content-length: 36\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\nbody\r\n" + "0\r\n\r\n" + + smuggled_request; + sendRawHttpAndWaitForResponse(lookupPort("http"), full_request.c_str(), &response, true); + EXPECT_THAT(response, HasSubstr("HTTP/1.1 200 OK\r\n")); + + std::unique_ptr upstream_headers = + reinterpret_cast(fake_upstreams_.front().get())->lastRequestHeaders(); + EXPECT_EQ(upstream_headers->Host()->value(), "host"); + EXPECT_EQ(upstream_headers->ContentLength(), nullptr); + } +} + TEST_P(IntegrationTest, BadFirstline) { initialize(); std::string response; From 2549c628ea7c7f29dfc8176a3a900a5543f778b9 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Tue, 25 Aug 2020 17:51:33 -0700 Subject: [PATCH 36/40] typo fix Signed-off-by: Oleg Guba --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index d53d95c81dc9..7a8b7061c2c8 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -385,7 +385,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( version = "4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878", sha256 = "6a12896313ce1ca630cf516a0ee43a79b5f13f5a5d8143f56560ac0b21c98fac", strip_prefix = "http-parser-{version}", - urls = ["https://github.com/nodejs/http-parser/archive/v{version}.tar.gz"], + urls = ["https://github.com/nodejs/http-parser/archive/{version}.tar.gz"], use_category = ["dataplane"], cpe = "cpe:2.3:a:nodejs:node.js:*", ), From 62ef94a2867fb147c17f2a5cd81ccf49097b5e9f Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Tue, 25 Aug 2020 20:03:38 -0700 Subject: [PATCH 37/40] server/client integration tests Signed-off-by: Oleg Guba --- test/integration/integration_test.cc | 94 ++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 27 deletions(-) diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 8212b536adeb..a3ddd04f8427 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -48,11 +48,6 @@ void setAllowHttp10WithDefaultHost( hcm.mutable_http_protocol_options()->set_default_host_for_http_10("default.com"); } -void setAllowChunkedLength( - envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) { - hcm.mutable_http_protocol_options()->set_allow_chunked_length(true); -} - } // namespace INSTANTIATE_TEST_SUITE_P(IpVersions, IntegrationTest, @@ -434,31 +429,76 @@ TEST_P(IntegrationTest, TestSmuggling) { } } -TEST_P(IntegrationTest, TestAllowChunkedLength) { - autonomous_upstream_ = true; - config_helper_.addConfigModifier(&setAllowChunkedLength); +TEST_P(IntegrationTest, TestServerAllowChunkedLength) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_http_protocol_options()->set_allow_chunked_length(true); + }); initialize(); - const std::string smuggled_request = "GET / HTTP/1.1\r\nHost: disallowed\r\n\r\n"; - ASSERT_EQ(smuggled_request.length(), 36); - { - std::string response; - const std::string full_request = - "POST / HTTP/1.1\r\n" - "Host: host\r\n" - "Content-length: 36\r\n" - "Transfer-Encoding: chunked\r\n\r\n" - "4\r\nbody\r\n" - "0\r\n\r\n" + - smuggled_request; - sendRawHttpAndWaitForResponse(lookupPort("http"), full_request.c_str(), &response, true); - EXPECT_THAT(response, HasSubstr("HTTP/1.1 200 OK\r\n")); + auto tcp_client = makeTcpConnection(lookupPort("http")); + ASSERT_TRUE(tcp_client->write("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Content-length: 100\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\nbody\r\n" + "0\r\n\r\n")); - std::unique_ptr upstream_headers = - reinterpret_cast(fake_upstreams_.front().get())->lastRequestHeaders(); - EXPECT_EQ(upstream_headers->Host()->value(), "host"); - EXPECT_EQ(upstream_headers->ContentLength(), nullptr); - } + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + + ASSERT_THAT(data, HasSubstr("POST / HTTP/1.1")); + ASSERT_THAT(data, HasSubstr("transfer-encoding: chunked")); + // verify no {Cc}ontent-length header + ASSERT_THAT(data, Not(HasSubstr("ontent-length"))); + + ASSERT_TRUE( + fake_upstream_connection->write("HTTP/1.1 200 OK\r\nTransfer-encoding: chunked\r\n\r\n")); + ASSERT_TRUE(fake_upstream_connection->close()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + tcp_client->close(); +} + +TEST_P(IntegrationTest, TestClientAllowChunkedLength) { + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() == 1, ""); + if (fake_upstreams_[0]->httpType() == FakeHttpConnection::Type::HTTP1) { + auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); + cluster->mutable_http_protocol_options()->set_allow_chunked_length(true); + } + }); + + initialize(); + + auto tcp_client = makeTcpConnection(lookupPort("http")); + ASSERT_TRUE(tcp_client->write("GET / HTTP/1.1\r\nHost: host\r\n\r\n")); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + + ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 200 OK\r\n" + "Transfer-encoding: chunked\r\n" + "Content-Length: 100\r\n\r\n" + "4\r\nbody\r\n" + "0\r\n\r\n")); + tcp_client->waitForData("\r\n\r\n", false); + std::string response = tcp_client->data(); + + EXPECT_THAT(response, HasSubstr("HTTP/1.1 200 OK\r\n")); + EXPECT_THAT(response, Not(HasSubstr("content-length"))); + EXPECT_THAT(response, HasSubstr("transfer-encoding: chunked\r\n")); + EXPECT_THAT(response, EndsWith("\r\n\r\n")); + + ASSERT_TRUE(fake_upstream_connection->close()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + tcp_client->close(); } TEST_P(IntegrationTest, BadFirstline) { From 85864ec328b4d453c4277e80df6a70243a75313d Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Tue, 25 Aug 2020 20:05:27 -0700 Subject: [PATCH 38/40] format fix Signed-off-by: Oleg Guba --- test/common/http/http1/codec_impl_test.cc | 3 ++- test/integration/integration_test.cc | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index b6911d30897d..9cd59c74c5c0 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -384,7 +384,8 @@ void Http1ServerConnectionImplTest::testServerAllowChunkedContentLength(uint32_t EXPECT_TRUE(isCodecProtocolError(status)); EXPECT_EQ(status.message(), "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); - EXPECT_EQ("http1.content_length_and_chunked_not_allowed", response_encoder->getStream().responseDetails()); + EXPECT_EQ("http1.content_length_and_chunked_not_allowed", + response_encoder->getStream().responseDetails()); } } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index a3ddd04f8427..55255675d7cb 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -453,7 +453,7 @@ TEST_P(IntegrationTest, TestServerAllowChunkedLength) { ASSERT_THAT(data, HasSubstr("POST / HTTP/1.1")); ASSERT_THAT(data, HasSubstr("transfer-encoding: chunked")); - // verify no {Cc}ontent-length header + // verify no 'content-length' header ASSERT_THAT(data, Not(HasSubstr("ontent-length"))); ASSERT_TRUE( From bd9d2359cbc67cf795a4f481b48deb98eab471f0 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 26 Aug 2020 11:15:31 -0700 Subject: [PATCH 39/40] Kick CI Signed-off-by: Oleg Guba From c959d584ee1038376005179ca744371b01f67987 Mon Sep 17 00:00:00 2001 From: Oleg Guba Date: Wed, 26 Aug 2020 12:20:15 -0700 Subject: [PATCH 40/40] Kick CI Signed-off-by: Oleg Guba