diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index c8b8cdc6dd354..d7f50d60f9ba1 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -26,7 +26,28 @@ message UpstreamHttpProtocolOptions { bool auto_sni = 1; } +// [#next-free-field: 6] message HttpProtocolOptions { + // Action to take when Envoy receives client request with header names containing underscore + // characters. + // Underscore character is allowed in header names by the RFC-7230 and this behavior is implemented + // as a security measure due to systems that treat '_' and '-' as interchangeable. Envoy by default allows client request headers with underscore + // characters. + enum HeadersWithUnderscoresAction { + // Allow headers with underscores. This is the default behavior. + ALLOW = 0; + + // Reject client request. HTTP/1 requests are rejected with the 400 status. HTTP/2 requests + // end with the stream reset. The "httpN.requests_rejected_with_underscores_in_headers" counter + // is incremented for each rejected request. + REJECT_REQUEST = 1; + + // Drop the header with name containing underscores. The header is dropped before the filter chain is + // invoked and as such filters will not see dropped headers. The + // "httpN.dropped_headers_with_underscores" is incremented for each dropped header. + DROP_HEADER = 2; + } + // The idle timeout for connections. The idle timeout is defined as the // period in which there are no active requests. If not set, there is no idle timeout. When the // idle timeout is reached the connection will be closed. If the connection is an HTTP/2 @@ -53,6 +74,11 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Action to take when a client request with a header name containing underscore characters is received. + // If this setting is not specified, the value defaults to ALLOW. + // Note: upstream responses are not affected by this setting. + HeadersWithUnderscoresAction headers_with_underscores_action = 5; } // [#next-free-field: 6] diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index dbbb4318e0cfe..07af0d729acd7 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -31,10 +31,31 @@ message UpstreamHttpProtocolOptions { bool auto_sni = 1; } +// [#next-free-field: 6] message HttpProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.HttpProtocolOptions"; + // Action to take when Envoy receives client request with header names containing underscore + // characters. + // Underscore character is allowed in header names by the RFC-7230 and this behavior is implemented + // as a security measure due to systems that treat '_' and '-' as interchangeable. Envoy by default allows client request headers with underscore + // characters. + enum HeadersWithUnderscoresAction { + // Allow headers with underscores. This is the default behavior. + ALLOW = 0; + + // Reject client request. HTTP/1 requests are rejected with the 400 status. HTTP/2 requests + // end with the stream reset. The "httpN.requests_rejected_with_underscores_in_headers" counter + // is incremented for each rejected request. + REJECT_REQUEST = 1; + + // Drop the header with name containing underscores. The header is dropped before the filter chain is + // invoked and as such filters will not see dropped headers. The + // "httpN.dropped_headers_with_underscores" is incremented for each dropped header. + DROP_HEADER = 2; + } + // The idle timeout for connections. The idle timeout is defined as the // period in which there are no active requests. If not set, there is no idle timeout. When the // idle timeout is reached the connection will be closed. If the connection is an HTTP/2 @@ -61,6 +82,11 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Action to take when a client request with a header name containing underscore characters is received. + // If this setting is not specified, the value defaults to ALLOW. + // Note: upstream responses are not affected by this setting. + HeadersWithUnderscoresAction headers_with_underscores_action = 5; } // [#next-free-field: 6] diff --git a/docs/root/configuration/http/http_conn_man/stats.rst b/docs/root/configuration/http/http_conn_man/stats.rst index 6d46fb937bd0b..74ce491f39a68 100644 --- a/docs/root/configuration/http/http_conn_man/stats.rst +++ b/docs/root/configuration/http/http_conn_man/stats.rst @@ -110,8 +110,10 @@ All http1 statistics are rooted at *http1.* :header: Name, Type, Description :widths: 1, 1, 2 + dropped_headers_with_underscores, Counter, Total number of dropped headers with names containing underscores. This action is configured by setting the :ref:`headers_with_underscores_action config setting `. metadata_not_supported_error, Counter, Total number of metadata dropped during HTTP/1 encoding response_flood, Counter, Total number of connections closed due to response flooding + requests_rejected_with_underscores_in_headers, Counter, Total numbers of rejected requests due to header names containing underscores. This action is configured by setting the :ref:`headers_with_underscores_action config setting `. Http2 codec statistics ~~~~~~~~~~~~~~~~~~~~~~ @@ -122,6 +124,7 @@ All http2 statistics are rooted at *http2.* :header: Name, Type, Description :widths: 1, 1, 2 + dropped_headers_with_underscores, Counter, Total number of dropped headers with names containing underscores. This action is configured by setting the :ref:`headers_with_underscores_action config setting `. header_overflow, Counter, Total number of connections reset due to the headers being larger than the :ref:`configured value `. headers_cb_no_stream, Counter, Total number of errors where a header callback is called without an associated stream. This tracks an unexpected occurrence due to an as yet undiagnosed bug inbound_empty_frames_flood, Counter, Total number of connections terminated for exceeding the limit on consecutive inbound frames with an empty payload and no end stream flag. The limit is configured by setting the :ref:`max_consecutive_inbound_frames_with_empty_payload config setting `. @@ -129,6 +132,7 @@ All http2 statistics are rooted at *http2.* inbound_window_update_frames_flood, Counter, Total number of connections terminated for exceeding the limit on inbound frames of type WINDOW_UPDATE. The limit is configured by setting the :ref:`max_inbound_window_updateframes_per_data_frame_sent config setting `. outbound_flood, Counter, Total number of connections terminated for exceeding the limit on outbound frames of all types. The limit is configured by setting the :ref:`max_outbound_frames config setting `. outbound_control_flood, Counter, "Total number of connections terminated for exceeding the limit on outbound frames of types PING, SETTINGS and RST_STREAM. The limit is configured by setting the :ref:`max_outbound_control_frames config setting `." + requests_rejected_with_underscores_in_headers, Counter, Total numbers of rejected requests due to header names containing underscores. This action is configured by setting the :ref:`headers_with_underscores_action config setting `. rx_messaging_error, Counter, Total number of invalid received frames that violated `section 8 `_ of the HTTP/2 spec. This will result in a *tx_reset* rx_reset, Counter, Total number of reset stream frames received by Envoy too_many_header_frames, Counter, Total number of times an HTTP2 connection is reset due to receiving too many headers frames. Envoy currently supports proxying at most one header frame for 100-Continue one non-100 response code header frame and one frame with trailers diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 45dfcbe06ffee..a84b982167337 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -1,6 +1,10 @@ Version history --------------- +1.13.2 (Pending) +================ +* http: added :ref:`headers_with_underscores_action setting ` to control how client requests with header names containing underscore characters are handled. The options are to allow such headers, reject request or drop headers. The default is to allow headers, preserving existing behavior. + 1.13.1 (March 3, 2020) ====================== * buffer: force copy when appending small slices to OwnedImpl buffer to avoid fragmentation. diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index c8b8cdc6dd354..d7f50d60f9ba1 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -26,7 +26,28 @@ message UpstreamHttpProtocolOptions { bool auto_sni = 1; } +// [#next-free-field: 6] message HttpProtocolOptions { + // Action to take when Envoy receives client request with header names containing underscore + // characters. + // Underscore character is allowed in header names by the RFC-7230 and this behavior is implemented + // as a security measure due to systems that treat '_' and '-' as interchangeable. Envoy by default allows client request headers with underscore + // characters. + enum HeadersWithUnderscoresAction { + // Allow headers with underscores. This is the default behavior. + ALLOW = 0; + + // Reject client request. HTTP/1 requests are rejected with the 400 status. HTTP/2 requests + // end with the stream reset. The "httpN.requests_rejected_with_underscores_in_headers" counter + // is incremented for each rejected request. + REJECT_REQUEST = 1; + + // Drop the header with name containing underscores. The header is dropped before the filter chain is + // invoked and as such filters will not see dropped headers. The + // "httpN.dropped_headers_with_underscores" is incremented for each dropped header. + DROP_HEADER = 2; + } + // The idle timeout for connections. The idle timeout is defined as the // period in which there are no active requests. If not set, there is no idle timeout. When the // idle timeout is reached the connection will be closed. If the connection is an HTTP/2 @@ -53,6 +74,11 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Action to take when a client request with a header name containing underscore characters is received. + // If this setting is not specified, the value defaults to ALLOW. + // Note: upstream responses are not affected by this setting. + HeadersWithUnderscoresAction headers_with_underscores_action = 5; } // [#next-free-field: 6] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index dbbb4318e0cfe..07af0d729acd7 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -31,10 +31,31 @@ message UpstreamHttpProtocolOptions { bool auto_sni = 1; } +// [#next-free-field: 6] message HttpProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.HttpProtocolOptions"; + // Action to take when Envoy receives client request with header names containing underscore + // characters. + // Underscore character is allowed in header names by the RFC-7230 and this behavior is implemented + // as a security measure due to systems that treat '_' and '-' as interchangeable. Envoy by default allows client request headers with underscore + // characters. + enum HeadersWithUnderscoresAction { + // Allow headers with underscores. This is the default behavior. + ALLOW = 0; + + // Reject client request. HTTP/1 requests are rejected with the 400 status. HTTP/2 requests + // end with the stream reset. The "httpN.requests_rejected_with_underscores_in_headers" counter + // is incremented for each rejected request. + REJECT_REQUEST = 1; + + // Drop the header with name containing underscores. The header is dropped before the filter chain is + // invoked and as such filters will not see dropped headers. The + // "httpN.dropped_headers_with_underscores" is incremented for each dropped header. + DROP_HEADER = 2; + } + // The idle timeout for connections. The idle timeout is defined as the // period in which there are no active requests. If not set, there is no idle timeout. When the // idle timeout is reached the connection will be closed. If the connection is an HTTP/2 @@ -61,6 +82,11 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Action to take when a client request with a header name containing underscore characters is received. + // If this setting is not specified, the value defaults to ALLOW. + // Note: upstream responses are not affected by this setting. + HeadersWithUnderscoresAction headers_with_underscores_action = 5; } // [#next-free-field: 6] diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index f81b67f976e12..1127a0c932176 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -391,6 +391,13 @@ class ConnectionManagerConfig { * one. */ virtual bool shouldMergeSlashes() const PURE; + + /** + * @return the action HttpConnectionManager should take when receiving client request + * headers containing underscore characters. + */ + virtual envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headersWithUnderscoresAction() const PURE; }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index ce09baabd0e32..97bca6a7ef696 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -44,15 +44,17 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings, const Http2Settings& http2_settings, uint32_t max_request_headers_kb, - uint32_t max_request_headers_count) { + uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) { if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) { - return std::make_unique(connection, callbacks, scope, - http2_settings, max_request_headers_kb, - max_request_headers_count); + return std::make_unique( + connection, callbacks, scope, http2_settings, max_request_headers_kb, + max_request_headers_count, headers_with_underscores_action); } else { - return std::make_unique(connection, scope, callbacks, - http1_settings, max_request_headers_kb, - max_request_headers_count); + return std::make_unique( + connection, scope, callbacks, http1_settings, max_request_headers_kb, + max_request_headers_count, headers_with_underscores_action); } } diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index ee67111c8de8f..31edbc1a4e944 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -37,7 +37,9 @@ class ConnectionManagerUtility { autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings, const Http2Settings& http2_settings, - uint32_t max_request_headers_kb, uint32_t max_request_headers_count); + uint32_t max_request_headers_kb, uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); /** * Mutates request headers in various ways. This functionality is broken out because of its diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 0062a35993e49..2c19042936032 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -143,11 +143,15 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderD return match != header_data.invert_match_; } -bool HeaderUtility::headerIsValid(const absl::string_view header_value) { +bool HeaderUtility::headerValueIsValid(const absl::string_view header_value) { return nghttp2_check_header_value(reinterpret_cast(header_value.data()), header_value.size()) != 0; } +bool HeaderUtility::headerNameContainsUnderscore(const absl::string_view header_name) { + return header_name.find('_') != absl::string_view::npos; +} + bool HeaderUtility::authorityIsValid(const absl::string_view header_value) { return nghttp2_check_authority(reinterpret_cast(header_value.data()), header_value.size()) != 0; diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index fd0687ec59105..ce7a633b4e72c 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -94,7 +94,16 @@ class HeaderUtility { * http://tools.ietf.org/html/rfc7230#section-3.2 * @return bool true if the header values are valid, according to the aforementioned RFC. */ - static bool headerIsValid(const absl::string_view header_value); + static bool headerValueIsValid(const absl::string_view header_value); + + /** + * Checks if header name contains underscore characters. + * Underscore character is allowed in header names by the RFC-7230 and this check is implemented + * as a security measure due to systems that treat '_' and '-' as interchangeable. Envoy by + * default allows headers with underscore characters. + * @return bool true if header name contains underscore characters. + */ + static bool headerNameContainsUnderscore(const absl::string_view header_name); /** * Validates that the characters in the authority are valid. diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index aca8bd1d8d0e0..e981e7915598d 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -40,6 +40,7 @@ envoy_cc_library( "//source/common/http:utility_lib", "//source/common/http/http1:header_formatter_lib", "//source/common/runtime:runtime_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 299f7c517a469..233721cb86664 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -458,6 +458,7 @@ void ConnectionImpl::completeLastHeader() { ENVOY_CONN_LOG(trace, "completed header: key={} value={}", connection_, current_header_field_.getStringView(), current_header_value_.getStringView()); + checkHeaderNameForUnderscores(); if (!current_header_field_.empty()) { toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size()); current_header_map_->addViaMove(std::move(current_header_field_), @@ -570,7 +571,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); if (strict_header_validation_) { - if (!Http::HeaderUtility::headerIsValid(header_value)) { + if (!Http::HeaderUtility::headerValueIsValid(header_value)) { ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value); error_code_ = Http::Code::BadRequest; sendProtocolError(Http1ResponseCodeDetails::get().InvalidCharacters); @@ -706,11 +707,12 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { onResetStream(reason); } -ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Stats::Scope& stats, - ServerConnectionCallbacks& callbacks, - const Http1Settings& settings, - uint32_t max_request_headers_kb, - const uint32_t max_request_headers_count) +ServerConnectionImpl::ServerConnectionImpl( + Network::Connection& connection, Stats::Scope& stats, ServerConnectionCallbacks& callbacks, + const Http1Settings& settings, uint32_t max_request_headers_kb, + 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.enable_trailers_), callbacks_(callbacks), codec_settings_(settings), @@ -724,7 +726,8 @@ ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Stat max_outbound_responses_( Runtime::getInteger("envoy.do_not_use_going_away_max_http2_outbound_responses", 2)), flood_protection_( - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_flood_protection")) {} + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_flood_protection")), + headers_with_underscores_action_(headers_with_underscores_action) {} void ServerConnectionImpl::onEncodeComplete() { ASSERT(active_request_); @@ -917,6 +920,27 @@ void ServerConnectionImpl::releaseOutboundResponse( delete fragment; } +void ServerConnectionImpl::checkHeaderNameForUnderscores() { + if (headers_with_underscores_action_ != envoy::config::core::v3::HttpProtocolOptions::ALLOW && + Http::HeaderUtility::headerNameContainsUnderscore(current_header_field_.getStringView())) { + if (headers_with_underscores_action_ == + envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { + ENVOY_CONN_LOG(debug, "Dropping header with invalid characters in its name: {}", connection_, + current_header_field_.getStringView()); + stats_.dropped_headers_with_underscores_.inc(); + current_header_field_.clear(); + current_header_value_.clear(); + } else { + ENVOY_CONN_LOG(debug, "Rejecting request due to header name with underscores: {}", + connection_, current_header_field_.getStringView()); + error_code_ = Http::Code::BadRequest; + sendProtocolError(Http1ResponseCodeDetails::get().InvalidCharacters); + stats_.requests_rejected_with_underscores_in_headers_.inc(); + throw CodecProtocolException("http/1.1 protocol error: header name contains underscores"); + } + } +} + ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Stats::Scope& stats, ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 344c32696c3a8..a089f8e02fd3b 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -8,6 +8,7 @@ #include #include +#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/http/codec.h" #include "envoy/network/connection.h" #include "envoy/stats/scope.h" @@ -28,7 +29,9 @@ namespace Http1 { * All stats for the HTTP/1 codec. @see stats_macros.h */ #define ALL_HTTP1_CODEC_STATS(COUNTER) \ + COUNTER(dropped_headers_with_underscores) \ COUNTER(metadata_not_supported_error) \ + COUNTER(requests_rejected_with_underscores_in_headers) \ COUNTER(response_flood) /** @@ -220,6 +223,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable; ServerConnectionImpl(Network::Connection& connection, Stats::Scope& stats, ServerConnectionCallbacks& callbacks, const Http1Settings& settings, - uint32_t max_request_headers_kb, const uint32_t max_request_headers_count); + uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); bool supports_http_10() override { return codec_settings_.accept_http_10_; } @@ -382,6 +405,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { void releaseOutboundResponse(const Buffer::OwnedBufferFragmentImpl* fragment); void maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer& output_buffer) override; void doFloodProtectionChecks() const; + void checkHeaderNameForUnderscores() override; ServerConnectionCallbacks& callbacks_; std::function flood_checks_{[&]() { this->doFloodProtectionChecks(); }}; @@ -394,6 +418,9 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { // we could make this configurable. uint32_t max_outbound_responses_{}; bool flood_protection_{}; + // The action to take when a request header name contains underscore characters. + const envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_; }; /** diff --git a/source/common/http/http2/BUILD b/source/common/http/http2/BUILD index ba48dc67903fb..62afeab699105 100644 --- a/source/common/http/http2/BUILD +++ b/source/common/http/http2/BUILD @@ -38,9 +38,11 @@ envoy_cc_library( "//source/common/http:codes_lib", "//source/common/http:exception_lib", "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/http:headers_lib", "//source/common/http:utility_lib", "//source/common/runtime:runtime_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 794eb437073a2..90ce5bff7abc8 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -18,6 +18,7 @@ #include "common/common/utility.h" #include "common/http/codes.h" #include "common/http/exception.h" +#include "common/http/header_utility.h" #include "common/http/headers.h" namespace Envoy { @@ -808,6 +809,14 @@ int ConnectionImpl::saveHeader(const nghttp2_frame* frame, HeaderString&& name, stats_.headers_cb_no_stream_.inc(); return 0; } + + auto should_return = checkHeaderNameForUnderscores(name.getStringView()); + if (should_return) { + name.clear(); + value.clear(); + return should_return.value(); + } + stream->saveHeader(std::move(name), std::move(value)); if (stream->headers_->byteSize() > max_headers_kb_ * 1024 || @@ -1126,14 +1135,15 @@ int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na return saveHeader(frame, std::move(name), std::move(value)); } -ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, - Http::ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings, - const uint32_t max_request_headers_kb, - const uint32_t max_request_headers_count) +ServerConnectionImpl::ServerConnectionImpl( + Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks, + Stats::Scope& scope, const Http2Settings& http2_settings, const uint32_t max_request_headers_kb, + const uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) : ConnectionImpl(connection, scope, http2_settings, max_request_headers_kb, max_request_headers_count), - callbacks_(callbacks) { + callbacks_(callbacks), headers_with_underscores_action_(headers_with_underscores_action) { Http2Options http2_options(http2_settings); nghttp2_session_server_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options.options()); @@ -1280,6 +1290,25 @@ void ServerConnectionImpl::dispatch(Buffer::Instance& data) { ConnectionImpl::dispatch(data); } +absl::optional +ServerConnectionImpl::checkHeaderNameForUnderscores(absl::string_view header_name) { + if (headers_with_underscores_action_ != envoy::config::core::v3::HttpProtocolOptions::ALLOW && + Http::HeaderUtility::headerNameContainsUnderscore(header_name)) { + if (headers_with_underscores_action_ == + envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { + ENVOY_CONN_LOG(debug, "Dropping header with invalid characters in its name: {}", connection_, + header_name); + stats_.dropped_headers_with_underscores_.inc(); + return 0; + } + ENVOY_CONN_LOG(debug, "Rejecting request due to header name with underscores: {}", connection_, + header_name); + stats_.requests_rejected_with_underscores_in_headers_.inc(); + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; + } + return absl::nullopt; +} + } // namespace Http2 } // namespace Http } // namespace Envoy diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index c5b8dc1ac8f4f..9fe7185a12ded 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -7,6 +7,7 @@ #include #include +#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/event/deferred_deletable.h" #include "envoy/http/codec.h" #include "envoy/network/connection.h" @@ -40,6 +41,7 @@ const std::string CLIENT_MAGIC_PREFIX = "PRI * HTTP/2"; * All stats for the HTTP/2 codec. @see stats_macros.h */ #define ALL_HTTP2_CODEC_STATS(COUNTER) \ + COUNTER(dropped_headers_with_underscores) \ COUNTER(header_overflow) \ COUNTER(headers_cb_no_stream) \ COUNTER(inbound_empty_frames_flood) \ @@ -47,6 +49,7 @@ const std::string CLIENT_MAGIC_PREFIX = "PRI * HTTP/2"; COUNTER(inbound_window_update_frames_flood) \ COUNTER(outbound_control_flood) \ COUNTER(outbound_flood) \ + COUNTER(requests_rejected_with_underscores_in_headers) \ COUNTER(rx_messaging_error) \ COUNTER(rx_reset) \ COUNTER(too_many_header_frames) \ @@ -286,6 +289,18 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable checkHeaderNameForUnderscores(absl::string_view /* header_name */) { + return absl::nullopt; + } + static Http2Callbacks http2_callbacks_; std::list active_streams_; @@ -433,7 +448,9 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http2Settings& http2_settings, const uint32_t max_request_headers_kb, - const uint32_t max_request_headers_count); + const uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); private: // ConnectionImpl @@ -443,6 +460,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { void checkOutboundQueueLimits() override; bool trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) override; bool checkInboundFrameLimits() override; + absl::optional checkHeaderNameForUnderscores(absl::string_view header_name) override; // Http::Connection // The reason for overriding the dispatch method is to do flood mitigation only when @@ -458,6 +476,10 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { // This flag indicates that downstream data is being dispatched and turns on flood mitigation // in the checkMaxOutbound*Framed methods. bool dispatching_downstream_data_{false}; + + // The action to take when a request header name contains underscore characters. + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_; }; } // namespace Http2 diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index ba46e92c99f3b..bf3b331c858f3 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -196,7 +196,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( context.runtime().snapshot().featureEnabled("http_connection_manager.normalize_path", 0))), #endif - merge_slashes_(config.merge_slashes()) { + merge_slashes_(config.merge_slashes()), + headers_with_underscores_action_( + config.common_http_protocol_options().headers_with_underscores_action()) { // If idle_timeout_ was not configured in common_http_protocol_options, use value in deprecated // idle_timeout field. // TODO(asraa): Remove when idle_timeout is removed. @@ -446,11 +448,11 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, case CodecType::HTTP1: return std::make_unique( connection, context_.scope(), callbacks, http1_settings_, maxRequestHeadersKb(), - maxRequestHeadersCount()); + maxRequestHeadersCount(), headersWithUnderscoresAction()); case CodecType::HTTP2: return std::make_unique( connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb(), - maxRequestHeadersCount()); + maxRequestHeadersCount(), headersWithUnderscoresAction()); case CodecType::HTTP3: // Hard code Quiche factory name here to instantiate a QUIC codec implemented. // TODO(danzh) Add support to get the factory name from config, possibly @@ -463,7 +465,7 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, case CodecType::AUTO: return Http::ConnectionManagerUtility::autoCreateCodec( connection, data, callbacks, context_.scope(), http1_settings_, http2_settings_, - maxRequestHeadersKb(), maxRequestHeadersCount()); + maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction()); } NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 56993962cf2aa..e177cc1b59286 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -147,6 +147,10 @@ class HttpConnectionManagerConfig : Logger::Loggable, const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headersWithUnderscoresAction() const override { + return headers_with_underscores_action_; + } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } private: @@ -198,6 +202,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, std::chrono::milliseconds delayed_close_timeout_; const bool normalize_path_; const bool merge_slashes_; + const envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_; // Default idle timeout is 5 minutes if nothing is specified in the HCM config. static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index cf8e4a89b4834..9479bcf6bb4aa 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -1443,7 +1443,7 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection Http::ServerConnectionCallbacks& callbacks) { return Http::ConnectionManagerUtility::autoCreateCodec( connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings(), - maxRequestHeadersKb(), maxRequestHeadersCount()); + maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction()); } bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index f14389026c7b8..ed14dc49bf42f 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -155,6 +155,10 @@ class AdminImpl : public Admin, const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return true; } bool shouldMergeSlashes() const override { return true; } + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headersWithUnderscoresAction() const override { + return envoy::config::core::v3::HttpProtocolOptions::ALLOW; + } Http::Code request(absl::string_view path_and_query, absl::string_view method, Http::HeaderMap& response_headers, std::string& body) override; void closeSocket(); diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index ed592b66dc790..5063793ba9d09 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -353,6 +353,8 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi uint32_t max_request_headers_kb = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; uint32_t max_request_headers_count = Http::DEFAULT_MAX_HEADERS_COUNT; uint32_t max_response_headers_count = Http::DEFAULT_MAX_HEADERS_COUNT; + const envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action = envoy::config::core::v3::HttpProtocolOptions::ALLOW; ClientConnectionPtr client; ServerConnectionPtr server; const bool http2 = http_version == HttpVersion::Http2; @@ -373,12 +375,12 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi const Http2Settings server_http2settings{fromHttp2Settings(input.h2_settings().server())}; server = std::make_unique( server_connection, server_callbacks, stats_store, server_http2settings, - max_request_headers_kb, max_request_headers_count); + max_request_headers_kb, max_request_headers_count, headers_with_underscores_action); } else { const Http1Settings server_http1settings{fromHttp1Settings(input.h1_settings().server())}; server = std::make_unique( server_connection, stats_store, server_callbacks, server_http1settings, - max_request_headers_kb, max_request_headers_count); + max_request_headers_kb, max_request_headers_count, headers_with_underscores_action); } ReorderBuffer client_write_buf{*server}; diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 0318637f2016a..d804a36ed22ed 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -146,6 +146,10 @@ class FuzzConfig : public ConnectionManagerConfig { const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return false; } bool shouldMergeSlashes() const override { return false; } + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headersWithUnderscoresAction() const override { + return envoy::config::core::v3::HttpProtocolOptions::ALLOW; + } const envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager config_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 2d63ba04fc4bf..960a3ca8defe2 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -343,6 +343,10 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headersWithUnderscoresAction() const override { + return headers_with_underscores_action_; + } DangerousDeprecatedTestTime test_time_; NiceMock route_config_provider_; @@ -397,6 +401,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan Http::Http1Settings http1_settings_; bool normalize_path_ = false; bool merge_slashes_ = false; + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::ALLOW; NiceMock upstream_conn_; // for websocket tests NiceMock conn_pool_; // for websocket tests diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 2459bd0bbc6b9..3d1da316c3b22 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -94,6 +94,8 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD(const Http::Http1Settings&, http1Settings, (), (const)); MOCK_METHOD(bool, shouldNormalizePath, (), (const)); MOCK_METHOD(bool, shouldMergeSlashes, (), (const)); + MOCK_METHOD(envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction, + headersWithUnderscoresAction, (), (const)); std::unique_ptr internal_address_config_ = std::make_unique(); diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index d9039a7301e34..55d2056aab78f 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -455,13 +455,13 @@ TEST(HeaderIsValidTest, InvalidHeaderValuesAreRejected) { continue; } - EXPECT_FALSE(HeaderUtility::headerIsValid(std::string(1, i))); + EXPECT_FALSE(HeaderUtility::headerValueIsValid(std::string(1, i))); } } TEST(HeaderIsValidTest, ValidHeaderValuesAreAccepted) { - EXPECT_TRUE(HeaderUtility::headerIsValid("some-value")); - EXPECT_TRUE(HeaderUtility::headerIsValid("Some Other Value")); + EXPECT_TRUE(HeaderUtility::headerValueIsValid("some-value")); + EXPECT_TRUE(HeaderUtility::headerValueIsValid("Some Other Value")); } TEST(HeaderIsValidTest, AuthorityIsValid) { @@ -485,5 +485,13 @@ TEST(HeaderAddTest, HeaderAdd) { &headers); } +TEST(HeaderIsValidTest, HeaderNameContainsUnderscore) { + EXPECT_FALSE(HeaderUtility::headerNameContainsUnderscore("cookie")); + EXPECT_FALSE(HeaderUtility::headerNameContainsUnderscore("x-something")); + EXPECT_TRUE(HeaderUtility::headerNameContainsUnderscore("_cookie")); + EXPECT_TRUE(HeaderUtility::headerNameContainsUnderscore("cookie_")); + EXPECT_TRUE(HeaderUtility::headerNameContainsUnderscore("x_something")); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 85857378d0f3e..d83be0dad0401 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -48,9 +48,9 @@ std::string createHeaderFragment(int num_headers) { class Http1ServerConnectionImplTest : public testing::Test { public: void initialize() { - codec_ = - std::make_unique(connection_, store_, callbacks_, codec_settings_, - max_request_headers_kb_, max_request_headers_count_); + codec_ = std::make_unique( + connection_, store_, callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, headers_with_underscores_action_); } NiceMock connection_; @@ -90,6 +90,8 @@ class Http1ServerConnectionImplTest : public testing::Test { protected: uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_{envoy::config::core::v3::HttpProtocolOptions::ALLOW}; Stats::IsolatedStoreImpl store_; }; @@ -103,9 +105,9 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur if (allow_absolute_url) { codec_settings_.allow_absolute_url_ = allow_absolute_url; - codec_ = - std::make_unique(connection_, store_, callbacks_, codec_settings_, - max_request_headers_kb_, max_request_headers_count_); + codec_ = std::make_unique( + connection_, store_, callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); } Http::MockStreamDecoder decoder; @@ -132,9 +134,9 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs // Make a new 'codec' with the right settings if (allow_absolute_url) { codec_settings_.allow_absolute_url_ = allow_absolute_url; - codec_ = - std::make_unique(connection_, store_, callbacks_, codec_settings_, - max_request_headers_kb_, max_request_headers_count_); + codec_ = std::make_unique( + connection_, store_, callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); } Http::MockStreamDecoder decoder; @@ -152,9 +154,9 @@ void Http1ServerConnectionImplTest::expectTrailersTest(bool enable_trailers) { // Make a new 'codec' with the right settings if (enable_trailers) { codec_settings_.enable_trailers_ = enable_trailers; - codec_ = - std::make_unique(connection_, store_, callbacks_, codec_settings_, - max_request_headers_kb_, max_request_headers_count_); + codec_ = std::make_unique( + connection_, store_, callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); } StrictMock decoder; @@ -183,9 +185,9 @@ void Http1ServerConnectionImplTest::testTrailersExceedLimit(std::string trailer_ initialize(); // Make a new 'codec' with the right settings codec_settings_.enable_trailers_ = enable_trailers; - codec_ = - std::make_unique(connection_, store_, callbacks_, codec_settings_, - max_request_headers_kb_, max_request_headers_count_); + codec_ = std::make_unique( + connection_, store_, callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); std::string exception_reason; NiceMock decoder; EXPECT_CALL(callbacks_, newStream(_, _)) @@ -753,6 +755,72 @@ TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) { EXPECT_EQ("http1.invalid_characters", response_encoder->getStream().responseDetails()); } +// Ensures that request headers with names containing the underscore character are allowed +// when the option is set to allow. +TEST_F(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAllowed) { + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::ALLOW; + initialize(); + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":authority", "h.com"}, + {":path", "/"}, + {":method", "GET"}, + {"foo_bar", "bar"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + + Buffer::OwnedImpl buffer(absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo_bar: bar\r\n\r\n")); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + EXPECT_EQ(0, store_.counter("http1.dropped_headers_with_underscores").value()); +} + +// Ensures that request headers with names containing the underscore character are dropped +// when the option is set to drop headers. +TEST_F(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAreDropped) { + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER; + initialize(); + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":authority", "h.com"}, + {":path", "/"}, + {":method", "GET"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + + Buffer::OwnedImpl buffer(absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo_bar: bar\r\n\r\n")); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + EXPECT_EQ(1, store_.counter("http1.dropped_headers_with_underscores").value()); +} + +// Ensures that request with header names containing the underscore character are rejected +// when the option is set to reject request. +TEST_F(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreCauseRequestRejected) { + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST; + initialize(); + + Http::MockStreamDecoder decoder; + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer(absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo_bar: bar\r\n\r\n")); + EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, + "http/1.1 protocol error: header name contains underscores"); + EXPECT_EQ("http1.invalid_characters", response_encoder->getStream().responseDetails()); + EXPECT_EQ(1, store_.counter("http1.requests_rejected_with_underscores_in_headers").value()); +} + TEST_F(Http1ServerConnectionImplTest, HeaderInvalidAuthority) { TestScopedRuntime scoped_runtime; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 87e6995d99751..0b7c3cc80ec74 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -70,7 +70,7 @@ class Http2CodecImplTestFixture { max_request_headers_kb_, max_response_headers_count_); server_ = std::make_unique( server_connection_, server_callbacks_, stats_store_, server_http2settings_, - max_request_headers_kb_, max_request_headers_count_); + max_request_headers_kb_, max_request_headers_count_, headers_with_underscores_action_); request_encoder_ = &client_->newStream(response_decoder_); setupDefaultConnectionMocks(); @@ -166,6 +166,8 @@ class Http2CodecImplTestFixture { Http2Settings::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM; uint32_t max_inbound_window_update_frames_per_data_frame_sent_ = Http2Settings::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT; + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_{envoy::config::core::v3::HttpProtocolOptions::ALLOW}; }; class Http2CodecImplTest : public ::testing::TestWithParam, @@ -955,7 +957,7 @@ TEST_P(Http2CodecImplStreamLimitTest, MaxClientStreams) { max_request_headers_kb_, max_response_headers_count_); server_ = std::make_unique( server_connection_, server_callbacks_, stats_store_, server_http2settings_, - max_request_headers_kb_, max_request_headers_count_); + max_request_headers_kb_, max_request_headers_count_, headers_with_underscores_action_); for (int i = 0; i < 101; ++i) { request_encoder_ = &client_->newStream(response_decoder_); @@ -1090,6 +1092,51 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAccepted) { request_encoder_->encodeHeaders(request_headers, false); } +// Tests request headers with name containing underscore are dropped when the option is set to drop +// header. +TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAreDropped) { + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER; + initialize(); + + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + TestHeaderMapImpl expected_headers(request_headers); + request_headers.addCopy("bad_header", "something"); + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), _)); + request_encoder_->encodeHeaders(request_headers, false); + EXPECT_EQ(1, stats_store_.counter("http2.dropped_headers_with_underscores").value()); +} + +// Tests that request with header names containing underscore are rejected when the option is set to +// reject request. +TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAreRejectedByDefault) { + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST; + initialize(); + + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + request_headers.addCopy("bad_header", "something"); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(1); + request_encoder_->encodeHeaders(request_headers, false); + EXPECT_EQ(1, stats_store_.counter("http2.requests_rejected_with_underscores_in_headers").value()); +} + +// Tests request headers with name containing underscore are allowed when the option is set to +// allow. +TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAllowed) { + headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::ALLOW; + initialize(); + + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + request_headers.addCopy("bad_header", "something"); + TestHeaderMapImpl expected_headers(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), _)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); + request_encoder_->encodeHeaders(request_headers, false); + EXPECT_EQ(0, stats_store_.counter("http2.dropped_headers_with_underscores").value()); +} + // This is the HTTP/2 variant of the HTTP/1 regression test for CVE-2019-18801. // Large method headers should not trigger ASSERTs or ASAN. The underlying issue // in CVE-2019-18801 only affected the HTTP/1 encoder, but we include a test diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index 67521db0fec86..ff3e539fbe809 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -8,11 +8,14 @@ namespace Http2 { class TestServerConnectionImpl : public ServerConnectionImpl { public: - TestServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings, - uint32_t max_request_headers_kb, uint32_t max_request_headers_count) + TestServerConnectionImpl( + Network::Connection& connection, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, + const Http2Settings& http2_settings, uint32_t max_request_headers_kb, + uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) : ServerConnectionImpl(connection, callbacks, scope, http2_settings, max_request_headers_kb, - max_request_headers_count) {} + max_request_headers_count, headers_with_underscores_action) {} nghttp2_session* session() { return session_; } using ServerConnectionImpl::getStream; }; diff --git a/test/common/http/http2/frame_replay_test.cc b/test/common/http/http2/frame_replay_test.cc index e9869b2963b7b..4c2ad252c3baf 100644 --- a/test/common/http/http2/frame_replay_test.cc +++ b/test/common/http/http2/frame_replay_test.cc @@ -58,7 +58,8 @@ TEST_F(RequestFrameCommentTest, SimpleExampleHuffman) { ServerCodecFrameInjector codec; TestServerConnectionImpl connection( codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); + Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); codec.write(WellKnownFrames::initialWindowUpdateFrame(), connection); @@ -134,7 +135,8 @@ TEST_F(RequestFrameCommentTest, SimpleExamplePlain) { ServerCodecFrameInjector codec; TestServerConnectionImpl connection( codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); + Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); codec.write(WellKnownFrames::initialWindowUpdateFrame(), connection); @@ -197,7 +199,8 @@ TEST_F(RequestFrameCommentTest, SingleByteNulCrLfInHeaderFrame) { ServerCodecFrameInjector codec; TestServerConnectionImpl connection( codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); + Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); codec.write(WellKnownFrames::initialWindowUpdateFrame(), connection); @@ -265,7 +268,8 @@ TEST_F(RequestFrameCommentTest, SingleByteNulCrLfInHeaderField) { ServerCodecFrameInjector codec; TestServerConnectionImpl connection( codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); + Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); codec.write(WellKnownFrames::initialWindowUpdateFrame(), connection); diff --git a/test/common/http/http2/request_header_fuzz_test.cc b/test/common/http/http2/request_header_fuzz_test.cc index 440014afb648b..e7a94e8c73bd5 100644 --- a/test/common/http/http2/request_header_fuzz_test.cc +++ b/test/common/http/http2/request_header_fuzz_test.cc @@ -18,7 +18,8 @@ void Replay(const Frame& frame, ServerCodecFrameInjector& codec) { // Create the server connection containing the nghttp2 session. TestServerConnectionImpl connection( codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); + Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); codec.write(WellKnownFrames::initialWindowUpdateFrame(), connection); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 1b26306abc70c..556bd7f46fd41 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -761,6 +761,61 @@ TEST_F(HttpConnectionManagerConfigTest, MergeSlashesFalse) { EXPECT_FALSE(config.shouldMergeSlashes()); } +// Validated that by default we allow requests with header names containing underscores. +TEST_F(HttpConnectionManagerConfigTest, HeadersWithUnderscoresAllowedByDefault) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + http_filters: + - name: envoy.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); + EXPECT_EQ(envoy::config::core::v3::HttpProtocolOptions::ALLOW, + config.headersWithUnderscoresAction()); +} + +// Validated that when configured, we drop headers with underscores. +TEST_F(HttpConnectionManagerConfigTest, HeadersWithUnderscoresDroppedByConfig) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + common_http_protocol_options: + headers_with_underscores_action: DROP_HEADER + route_config: + name: local_route + http_filters: + - name: envoy.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); + EXPECT_EQ(envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER, + config.headersWithUnderscoresAction()); +} + +// Validated that when configured, we reject requests with header names containing underscores. +TEST_F(HttpConnectionManagerConfigTest, HeadersWithUnderscoresRequestRejectedByConfig) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + common_http_protocol_options: + headers_with_underscores_action: REJECT_REQUEST + route_config: + name: local_route + http_filters: + - name: envoy.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); + EXPECT_EQ(envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST, + config.headersWithUnderscoresAction()); +} + TEST_F(HttpConnectionManagerConfigTest, ConfiguredRequestTimeout) { const std::string yaml_string = R"EOF( stat_prefix: ingress_http diff --git a/test/integration/autonomous_upstream.cc b/test/integration/autonomous_upstream.cc index 997f28cad759e..465c17768f2f2 100644 --- a/test/integration/autonomous_upstream.cc +++ b/test/integration/autonomous_upstream.cc @@ -66,7 +66,8 @@ AutonomousHttpConnection::AutonomousHttpConnection(SharedConnectionWrapper& shar Stats::Store& store, Type type, AutonomousUpstream& upstream) : FakeHttpConnection(shared_connection, store, type, upstream.timeSystem(), - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT), + Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::ALLOW), upstream_(upstream) {} Http::StreamDecoder& AutonomousHttpConnection::newStream(Http::StreamEncoder& response_encoder, diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 1a2b7c6807716..e2425e48d5df3 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -215,11 +215,12 @@ void FakeStream::finishGrpcStream(Grpc::Status::GrpcStatus status) { Http::TestHeaderMapImpl{{"grpc-status", std::to_string(static_cast(status))}}); } -FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connection, - Stats::Store& store, Type type, - Event::TestTimeSystem& time_system, - uint32_t max_request_headers_kb, - uint32_t max_request_headers_count) +FakeHttpConnection::FakeHttpConnection( + SharedConnectionWrapper& shared_connection, Stats::Store& store, Type type, + Event::TestTimeSystem& time_system, uint32_t max_request_headers_kb, + uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) : FakeConnectionBase(shared_connection, time_system) { if (type == Type::HTTP1) { Http::Http1Settings http1_settings; @@ -227,14 +228,14 @@ FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connectio http1_settings.enable_trailers_ = true; codec_ = std::make_unique( shared_connection_.connection(), store, *this, http1_settings, max_request_headers_kb, - max_request_headers_count); + max_request_headers_count, headers_with_underscores_action); } else { auto settings = Http::Http2Settings(); settings.allow_connect_ = true; settings.allow_metadata_ = true; codec_ = std::make_unique( shared_connection_.connection(), *this, store, settings, max_request_headers_kb, - max_request_headers_count); + max_request_headers_count, headers_with_underscores_action); ASSERT(type == Type::HTTP2); } @@ -465,11 +466,11 @@ void FakeUpstream::threadRoutine() { } } -AssertionResult FakeUpstream::waitForHttpConnection(Event::Dispatcher& client_dispatcher, - FakeHttpConnectionPtr& connection, - milliseconds timeout, - uint32_t max_request_headers_kb, - uint32_t max_request_headers_count) { +AssertionResult FakeUpstream::waitForHttpConnection( + Event::Dispatcher& client_dispatcher, FakeHttpConnectionPtr& connection, milliseconds timeout, + uint32_t max_request_headers_kb, uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) { Event::TestTimeSystem& time_system = timeSystem(); auto end_time = time_system.monotonicTime() + timeout; { @@ -488,9 +489,9 @@ AssertionResult FakeUpstream::waitForHttpConnection(Event::Dispatcher& client_di if (new_connections_.empty()) { return AssertionFailure() << "Got a new connection event, but didn't create a connection."; } - connection = std::make_unique(consumeConnection(), stats_store_, http_type_, - time_system, max_request_headers_kb, - max_request_headers_count); + connection = std::make_unique( + consumeConnection(), stats_store_, http_type_, time_system, max_request_headers_kb, + max_request_headers_count, headers_with_underscores_action); } VERIFY_ASSERTION(connection->initialize()); VERIFY_ASSERTION(connection->readDisable(false)); @@ -521,7 +522,7 @@ FakeUpstream::waitForHttpConnection(Event::Dispatcher& client_dispatcher, connection = std::make_unique( upstream.consumeConnection(), upstream.stats_store_, upstream.http_type_, upstream.timeSystem(), Http::DEFAULT_MAX_REQUEST_HEADERS_KB, - Http::DEFAULT_MAX_HEADERS_COUNT); + Http::DEFAULT_MAX_HEADERS_COUNT, envoy::config::core::v3::HttpProtocolOptions::ALLOW); lock.release(); VERIFY_ASSERTION(connection->initialize()); VERIFY_ASSERTION(connection->readDisable(false)); diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 2bcd5b40cacea..551aa25aef619 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -417,7 +417,9 @@ class FakeHttpConnection : public Http::ServerConnectionCallbacks, public FakeCo FakeHttpConnection(SharedConnectionWrapper& shared_connection, Stats::Store& store, Type type, Event::TestTimeSystem& time_system, uint32_t max_request_headers_kb, - uint32_t max_request_headers_count); + uint32_t max_request_headers_count, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); // By default waitForNewStream assumes the next event is a new stream and // returns AssertionFailure if an unexpected event occurs. If a caller truly @@ -558,11 +560,13 @@ class FakeUpstream : Logger::Loggable, // Returns the new connection via the connection argument. ABSL_MUST_USE_RESULT - testing::AssertionResult - waitForHttpConnection(Event::Dispatcher& client_dispatcher, FakeHttpConnectionPtr& connection, - std::chrono::milliseconds timeout = TestUtility::DefaultTimeout, - uint32_t max_request_headers_kb = Http::DEFAULT_MAX_REQUEST_HEADERS_KB, - uint32_t max_request_headers_count = Http::DEFAULT_MAX_HEADERS_COUNT); + testing::AssertionResult waitForHttpConnection( + Event::Dispatcher& client_dispatcher, FakeHttpConnectionPtr& connection, + std::chrono::milliseconds timeout = TestUtility::DefaultTimeout, + uint32_t max_request_headers_kb = Http::DEFAULT_MAX_REQUEST_HEADERS_KB, + uint32_t max_request_headers_count = Http::DEFAULT_MAX_HEADERS_COUNT, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action = envoy::config::core::v3::HttpProtocolOptions::ALLOW); ABSL_MUST_USE_RESULT testing::AssertionResult diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 50ad6b4df639a..bf2fd9fd66aba 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -266,14 +266,17 @@ TEST_P(IntegrationAdminTest, Admin) { switch (GetParam().downstream_protocol) { case Http::CodecClient::Type::HTTP1: EXPECT_EQ(" Count Lookup\n" + " 1 http1.dropped_headers_with_underscores\n" " 1 http1.metadata_not_supported_error\n" + " 1 http1.requests_rejected_with_underscores_in_headers\n" " 1 http1.response_flood\n" "\n" - "total: 2\n", + "total: 4\n", response->body()); break; case Http::CodecClient::Type::HTTP2: EXPECT_EQ(" Count Lookup\n" + " 1 http2.dropped_headers_with_underscores\n" " 1 http2.header_overflow\n" " 1 http2.headers_cb_no_stream\n" " 1 http2.inbound_empty_frames_flood\n" @@ -281,13 +284,14 @@ TEST_P(IntegrationAdminTest, Admin) { " 1 http2.inbound_window_update_frames_flood\n" " 1 http2.outbound_control_flood\n" " 1 http2.outbound_flood\n" + " 1 http2.requests_rejected_with_underscores_in_headers\n" " 1 http2.rx_messaging_error\n" " 1 http2.rx_reset\n" " 1 http2.too_many_header_frames\n" " 1 http2.trailers\n" " 1 http2.tx_reset\n" "\n" - "total: 12\n", + "total: 14\n", response->body()); break; case Http::CodecClient::Type::HTTP3: diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 7ba5b38064f9a..e9045002d4f5f 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -38,6 +38,7 @@ #include "gtest/gtest.h" using testing::HasSubstr; +using testing::Not; namespace Envoy { @@ -602,6 +603,85 @@ TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); } TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) { testTwoRequests(true); } +// Verify that headers with underscores in their names are dropped from client requests +// but remain in upstream responses. +TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresDropped) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_common_http_protocol_options()->set_headers_with_underscores_action( + envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER); + }); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = + codec_client_->makeHeaderOnlyRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"foo_bar", "baz"}}); + waitForNextUpstreamRequest(); + + EXPECT_THAT(upstream_request_->headers(), Not(HeaderHasValueRef("foo_bar", "baz"))); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {"bar_baz", "fooz"}}, + true); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_THAT(response->headers(), HeaderHasValueRef("bar_baz", "fooz")); +} + +// Verify that by default headers with underscores in their names remain in both requests and +// responses when allowed in configuration. +TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresRemainByDefault) { + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = + codec_client_->makeHeaderOnlyRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"foo_bar", "baz"}}); + waitForNextUpstreamRequest(); + + EXPECT_THAT(upstream_request_->headers(), HeaderHasValueRef("foo_bar", "baz")); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {"bar_baz", "fooz"}}, + true); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_THAT(response->headers(), HeaderHasValueRef("bar_baz", "fooz")); +} + +// Verify that request with headers containing underscores is rejected when configured. +TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresCauseRequestRejectedByDefault) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_common_http_protocol_options()->set_headers_with_underscores_action( + envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST); + }); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = + codec_client_->makeHeaderOnlyRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"foo_bar", "baz"}}); + + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + codec_client_->waitForDisconnect(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + response->waitForReset(); + codec_client_->close(); + ASSERT_TRUE(response->reset()); + EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->reset_reason()); + } +} + TEST_P(DownstreamProtocolIntegrationTest, ValidZeroLengthContent) { initialize();