diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index 60e7728a86e0c..be519e7da07f0 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -133,11 +133,11 @@ message HttpConnectionManager { // The maximum request headers size for incoming connections. The default max // is 60K, based on default settings for http codecs. For HTTP1, the current // limit set by http_parser is 80K. for HTTP2, the default allowed header - // block in nghttp2 is 64K. The max configurable setting is 63K in order to + // block in nghttp2 is 64K. The max configurable setting is 64K in order to // stay under both codec limits. // Requests that exceed this size will receive a 431 response. google.protobuf.UInt32Value max_request_headers_kb = 29 - [(validate.rules).uint32.gt = 0, (validate.rules).uint32.lte = 63]; + [(validate.rules).uint32.gt = 0, (validate.rules).uint32.lte = 64]; // The idle timeout for connections managed by the connection manager. The // idle timeout is defined as the period in which there are no active diff --git a/docs/root/configuration/http_conn_man/stats.rst b/docs/root/configuration/http_conn_man/stats.rst index 188b79d1873cb..174fa99bdb105 100644 --- a/docs/root/configuration/http_conn_man/stats.rst +++ b/docs/root/configuration/http_conn_man/stats.rst @@ -109,7 +109,7 @@ All http2 statistics are rooted at *http2.* :header: Name, Type, Description :widths: 1, 1, 2 - header_overflow, Counter, Total number of connections reset due to the headers being larger than `Envoy::Http::Http2::ConnectionImpl::StreamImpl::MAX_HEADER_SIZE` (63k) + 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 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 diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 7f4701a63ea21..568a735339e7f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -14,7 +14,7 @@ namespace Envoy { namespace Http { // Legacy default value of 60K is safely under both codec default limits. -static const uint32_t DEFAULT_MAX_REQUEST_HEADERS_SIZE_KB = 60; +static const uint32_t DEFAULT_MAX_REQUEST_HEADERS_KB = 60; class Stream; diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 43ae66e859d8a..3f8defb71ee6d 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -145,7 +145,8 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne } case Type::HTTP2: { codec_ = std::make_unique( - *connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings()); + *connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings(), + Http::DEFAULT_MAX_REQUEST_HEADERS_KB); break; } } diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index ee7b486aaf973..32d7d8b7d2f69 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -231,7 +231,7 @@ class ConnectionManagerConfig { /** * @return maximum request headers size the connection manager will accept. */ - virtual uint32_t maxRequestHeadersSizeKb() const PURE; + virtual uint32_t maxRequestHeadersKb() const PURE; /** * @return per-stream idle timeout for incoming connection manager connections. Zero indicates a diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index c941694efb99d..8805eacffe33e 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -617,9 +617,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } } - ASSERT(connection_manager_.config_.maxRequestHeadersSizeKb() > 0); - if (request_headers_->byteSize() > - (connection_manager_.config_.maxRequestHeadersSizeKb() * 1024)) { + ASSERT(connection_manager_.config_.maxRequestHeadersKb() > 0); + if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_, absl::nullopt); return; diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 7e28efd675f7f..2ebfe881d4d79 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -36,15 +36,13 @@ std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection& return ""; } -ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(Network::Connection& connection, - const Buffer::Instance& data, - ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, - const Http1Settings& http1_settings, - const Http2Settings& http2_settings) { +ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( + Network::Connection& connection, const Buffer::Instance& data, + ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings, + const Http2Settings& http2_settings, const uint32_t max_request_headers_kb) { if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) { - return ServerConnectionPtr{ - new Http2::ServerConnectionImpl(connection, callbacks, scope, http2_settings)}; + return ServerConnectionPtr{new Http2::ServerConnectionImpl( + connection, callbacks, scope, http2_settings, max_request_headers_kb)}; } else { return ServerConnectionPtr{ new Http1::ServerConnectionImpl(connection, callbacks, http1_settings)}; diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index 7ba81d8eeb52e..0d313f185e649 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -36,7 +36,8 @@ class ConnectionManagerUtility { static ServerConnectionPtr autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, - const Http1Settings& http1_settings, const Http2Settings& http2_settings); + const Http1Settings& http1_settings, const Http2Settings& http2_settings, + const uint32_t max_request_headers_kb); /** * Mutates request headers in various ways. This functionality is broken out because of its diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index a3ad27a815120..c7dd029d8ce0c 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -641,7 +641,7 @@ int ConnectionImpl::saveHeader(const nghttp2_frame* frame, HeaderString&& name, } stream->saveHeader(std::move(name), std::move(value)); - if (stream->headers_->byteSize() > StreamImpl::MAX_HEADER_SIZE) { + if (stream->headers_->byteSize() > max_request_headers_kb_ * 1024) { // This will cause the library to reset/close the stream. stats_.header_overflow_.inc(); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; @@ -877,8 +877,10 @@ ConnectionImpl::ClientHttp2Options::ClientHttp2Options(const Http2Settings& http ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, - Stats::Scope& stats, const Http2Settings& http2_settings) - : ConnectionImpl(connection, stats, http2_settings), callbacks_(callbacks) { + Stats::Scope& stats, const Http2Settings& http2_settings, + const uint32_t max_request_headers_kb) + : ConnectionImpl(connection, stats, http2_settings, max_request_headers_kb), + callbacks_(callbacks) { ClientHttp2Options client_http2_options(http2_settings); nghttp2_session_client_new2(&session_, http2_callbacks_.callbacks(), base(), client_http2_options.options()); @@ -924,8 +926,10 @@ int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings) - : ConnectionImpl(connection, scope, http2_settings), callbacks_(callbacks) { + Stats::Scope& scope, const Http2Settings& http2_settings, + const uint32_t max_request_headers_kb) + : ConnectionImpl(connection, scope, http2_settings, max_request_headers_kb), + callbacks_(callbacks) { Http2Options http2_options(http2_settings); nghttp2_session_server_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options.options()); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 143b306d6ce87..713abb886aefc 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -74,9 +74,9 @@ class Utility { class ConnectionImpl : public virtual Connection, protected Logger::Loggable { public: ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, - const Http2Settings& http2_settings) + const Http2Settings& http2_settings, const uint32_t max_request_headers_kb) : stats_{ALL_HTTP2_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http2."))}, - connection_(connection), + connection_(connection), max_request_headers_kb_(max_request_headers_kb), per_stream_buffer_limit_(http2_settings.initial_stream_window_size_), dispatching_(false), raised_goaway_(false), pending_deferred_reset_(false) {} @@ -185,11 +185,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable, FilterChainFactory& filterFactory() override { return *this; } bool reverseEncodeOrder() override { return reverse_encode_order_; } bool generateRequestId() override { return generate_request_id_; } - uint32_t maxRequestHeadersSizeKb() const override { return max_request_headers_size_kb_; } + uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } @@ -158,7 +158,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, std::string server_name_; Http::TracingConnectionManagerConfigPtr tracing_config_; absl::optional user_agent_; - uint32_t max_request_headers_size_kb_; + const uint32_t max_request_headers_kb_; absl::optional idle_timeout_; std::chrono::milliseconds stream_idle_timeout_; std::chrono::milliseconds request_timeout_; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index c16b7ae5396ba..ade0137129bfc 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -1106,7 +1106,8 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection const Buffer::Instance& data, Http::ServerConnectionCallbacks& callbacks) { return Http::ConnectionManagerUtility::autoCreateCodec( - connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings()); + connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings(), + maxRequestHeadersKb()); } bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 05b3c1fa840c5..8587c9ec52df9 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -100,7 +100,7 @@ class AdminImpl : public Admin, bool reverseEncodeOrder() override { return false; } bool generateRequestId() override { return false; } absl::optional idleTimeout() const override { return idle_timeout_; } - uint32_t maxRequestHeadersSizeKb() const override { return max_request_headers_size_kb_; } + uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; } std::chrono::milliseconds streamIdleTimeout() const override { return {}; } std::chrono::milliseconds requestTimeout() const override { return {}; } std::chrono::milliseconds delayedCloseTimeout() const override { return {}; } @@ -305,7 +305,7 @@ class AdminImpl : public Admin, Http::ConnectionManagerTracingStats tracing_stats_; NullRouteConfigProvider route_config_provider_; std::list handlers_; - uint32_t max_request_headers_size_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_SIZE_KB}; + const uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; absl::optional idle_timeout_; absl::optional user_agent_; Http::SlowDateProviderImpl date_provider_; diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index d2c53be04898b..40e85bfc23cbf 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -91,7 +91,7 @@ class FuzzConfig : public ConnectionManagerConfig { FilterChainFactory& filterFactory() override { return filter_factory_; } bool reverseEncodeOrder() override { return true; } bool generateRequestId() override { return true; } - uint32_t maxRequestHeadersSizeKb() const override { return max_request_headers_size_kb_; } + uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } @@ -132,7 +132,7 @@ class FuzzConfig : public ConnectionManagerConfig { ConnectionManagerStats stats_; ConnectionManagerTracingStats tracing_stats_; ConnectionManagerListenerStats listener_stats_; - uint32_t max_request_headers_size_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_SIZE_KB}; + uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; absl::optional idle_timeout_; std::chrono::milliseconds stream_idle_timeout_{}; std::chrono::milliseconds request_timeout_{}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 527bb7b86578f..39885de7c4e0c 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -230,7 +230,7 @@ class HttpConnectionManagerImplTest : public TestBase, public ConnectionManagerC FilterChainFactory& filterFactory() override { return filter_factory_; } bool reverseEncodeOrder() override { return reverse_encode_order_; } bool generateRequestId() override { return true; } - uint32_t maxRequestHeadersSizeKb() const override { return max_request_headers_size_kb_; } + uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } @@ -281,7 +281,7 @@ class HttpConnectionManagerImplTest : public TestBase, public ConnectionManagerC Http::ForwardClientCertType forward_client_cert_{Http::ForwardClientCertType::Sanitize}; std::vector set_current_client_cert_details_; absl::optional user_agent_; - uint32_t max_request_headers_size_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_SIZE_KB}; + uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; absl::optional idle_timeout_; std::chrono::milliseconds stream_idle_timeout_{}; std::chrono::milliseconds request_timeout_{}; @@ -3624,7 +3624,7 @@ TEST_F(HttpConnectionManagerImplTest, OverlyLongHeadersRejected) { } TEST_F(HttpConnectionManagerImplTest, OverlyLongHeadersAcceptedIfConfigured) { - max_request_headers_size_kb_ = 62; + max_request_headers_kb_ = 62; setup(false, ""); EXPECT_CALL(*codec_, dispatch(_)).Times(1).WillOnce(Invoke([&](Buffer::Instance&) -> void { diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 6ede90b769f41..6fa6608d219da 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -51,7 +51,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(filterFactory, FilterChainFactory&()); MOCK_METHOD0(reverseEncodeOrder, bool()); MOCK_METHOD0(generateRequestId, bool()); - MOCK_CONST_METHOD0(maxRequestHeadersSizeKb, uint32_t()); + MOCK_CONST_METHOD0(maxRequestHeadersKb, uint32_t()); MOCK_CONST_METHOD0(idleTimeout, absl::optional()); MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds()); MOCK_CONST_METHOD0(requestTimeout, std::chrono::milliseconds()); diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index 27677cfd9b454..436b29f83860d 100644 --- a/test/common/http/http2/BUILD +++ b/test/common/http/http2/BUILD @@ -4,6 +4,7 @@ load( "//bazel:envoy_build_system.bzl", "envoy_cc_fuzz_test", "envoy_cc_test", + "envoy_cc_test_library", "envoy_package", "envoy_proto_library", ) @@ -22,6 +23,7 @@ envoy_cc_fuzz_test( corpus = "codec_impl_corpus", deps = [ ":codec_impl_fuzz_proto_cc", + ":codec_impl_test_util", "//source/common/http:header_map_lib", "//source/common/http/http2:codec_lib", "//test/fuzz:utility_lib", @@ -35,6 +37,7 @@ envoy_cc_test( srcs = ["codec_impl_test.cc"], shard_count = 5, deps = [ + ":codec_impl_test_util", "//source/common/event:dispatcher_lib", "//source/common/http:exception_lib", "//source/common/http:header_map_lib", @@ -48,6 +51,14 @@ envoy_cc_test( ], ) +envoy_cc_test_library( + name = "codec_impl_test_util", + hdrs = ["codec_impl_test_util.h"], + deps = [ + "//source/common/http/http2:codec_lib", + ], +) + envoy_cc_test( name = "conn_pool_test", srcs = ["conn_pool_test.cc"], diff --git a/test/common/http/http2/codec_impl_fuzz_test.cc b/test/common/http/http2/codec_impl_fuzz_test.cc index c023d11207420..32f5af6e981c3 100644 --- a/test/common/http/http2/codec_impl_fuzz_test.cc +++ b/test/common/http/http2/codec_impl_fuzz_test.cc @@ -9,6 +9,8 @@ #include +#include "codec_impl_test_util.h" + #include "common/common/assert.h" #include "common/common/logger.h" #include "common/http/header_map_impl.h" @@ -30,24 +32,6 @@ namespace Envoy { namespace Http { namespace Http2 { -class TestServerConnectionImpl : public ServerConnectionImpl { -public: - TestServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings) - : ServerConnectionImpl(connection, callbacks, scope, http2_settings) {} - nghttp2_session* session() { return session_; } - using ServerConnectionImpl::getStream; -}; - -class TestClientConnectionImpl : public ClientConnectionImpl { -public: - TestClientConnectionImpl(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings) - : ClientConnectionImpl(connection, callbacks, scope, http2_settings) {} - nghttp2_session* session() { return session_; } - using ClientConnectionImpl::getStream; -}; - // Convert from test proto Http2Settings to Http2Settings. Http2Settings fromHttp2Settings(const test::common::http::http2::Http2Settings& settings) { Http2Settings h2_settings; @@ -260,14 +244,16 @@ DEFINE_PROTO_FUZZER(const test::common::http::http2::CodecImplFuzzTestCase& inpu NiceMock client_connection; const Http2Settings client_http2settings{fromHttp2Settings(input.client_settings())}; NiceMock client_callbacks; + uint32_t max_request_headers_kb = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; + TestClientConnectionImpl client(client_connection, client_callbacks, stats_store, - client_http2settings); + client_http2settings, max_request_headers_kb); NiceMock server_connection; const Http2Settings server_http2settings{fromHttp2Settings(input.server_settings())}; NiceMock server_callbacks; TestServerConnectionImpl server(server_connection, server_callbacks, stats_store, - server_http2settings); + server_http2settings, max_request_headers_kb); ReorderBuffer client_write_buf{server}; ReorderBuffer server_write_buf{client}; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index a20bfda08d15f..2399511a129e3 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -15,6 +15,7 @@ #include "test/test_common/test_base.h" #include "test/test_common/utility.h" +#include "codec_impl_test_util.h" #include "gmock/gmock.h" using testing::_; @@ -33,24 +34,6 @@ namespace Http2 { using Http2SettingsTuple = ::testing::tuple; using Http2SettingsTestParam = ::testing::tuple; -class TestServerConnectionImpl : public ServerConnectionImpl { -public: - TestServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings) - : ServerConnectionImpl(connection, callbacks, scope, http2_settings) {} - nghttp2_session* session() { return session_; } - using ServerConnectionImpl::getStream; -}; - -class TestClientConnectionImpl : public ClientConnectionImpl { -public: - TestClientConnectionImpl(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings) - : ClientConnectionImpl(connection, callbacks, scope, http2_settings) {} - nghttp2_session* session() { return session_; } - using ClientConnectionImpl::getStream; -}; - class Http2CodecImplTest : public TestBaseWithParam { public: struct ConnectionWrapper { @@ -73,9 +56,11 @@ class Http2CodecImplTest : public TestBaseWithParam { Http2SettingsFromTuple(client_http2settings_, ::testing::get<0>(GetParam())); Http2SettingsFromTuple(server_http2settings_, ::testing::get<1>(GetParam())); client_ = std::make_unique(client_connection_, client_callbacks_, - stats_store_, client_http2settings_); + stats_store_, client_http2settings_, + max_request_headers_kb_); server_ = std::make_unique(server_connection_, server_callbacks_, - stats_store_, server_http2settings_); + stats_store_, server_http2settings_, + max_request_headers_kb_); request_encoder_ = &client_->newStream(response_decoder_); setupDefaultConnectionMocks(); @@ -144,6 +129,7 @@ class Http2CodecImplTest : public TestBaseWithParam { StreamEncoder* response_encoder_{}; MockStreamCallbacks server_stream_callbacks_; bool corrupt_data_ = false; + uint32_t max_request_headers_kb_ = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; }; TEST_P(Http2CodecImplTest, ShutdownNotice) { @@ -775,9 +761,11 @@ TEST_P(Http2CodecImplStreamLimitTest, MaxClientStreams) { Http2SettingsFromTuple(client_http2settings_, ::testing::get<0>(GetParam())); Http2SettingsFromTuple(server_http2settings_, ::testing::get<1>(GetParam())); client_ = std::make_unique(client_connection_, client_callbacks_, - stats_store_, client_http2settings_); + stats_store_, client_http2settings_, + max_request_headers_kb_); server_ = std::make_unique(server_connection_, server_callbacks_, - stats_store_, server_http2settings_); + stats_store_, server_http2settings_, + max_request_headers_kb_); for (int i = 0; i < 101; ++i) { request_encoder_ = &client_->newStream(response_decoder_); @@ -877,20 +865,54 @@ TEST(Http2CodecUtility, reconstituteCrumbledCookies) { } } -// For issue #1421 regression test that Envoy's H2 codec applies header limits early. -TEST_P(Http2CodecImplTest, TestCodecHeaderLimits) { +TEST_P(Http2CodecImplTest, TestLargeHeadersInvokeResetStream) { initialize(); TestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); - std::string long_string = std::string(1024, 'q'); - for (int i = 0; i < 63; ++i) { - request_headers.addCopy(fmt::format("{}", i), long_string); - } - EXPECT_CALL(server_stream_callbacks_, onResetStream(_)); + std::string long_string = std::string(63 * 1024, 'q'); + request_headers.addCopy("big", long_string); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_)).Times(1); + request_encoder_->encodeHeaders(request_headers, false); +} + +TEST_P(Http2CodecImplTest, TestLargeHeadersAcceptedIfConfigured) { + max_request_headers_kb_ = 64; + initialize(); + + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + std::string long_string = std::string(63 * 1024, 'q'); + request_headers.addCopy("big", long_string); + + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_)).Times(0); request_encoder_->encodeHeaders(request_headers, false); } +TEST_P(Http2CodecImplTest, TestLargeHeadersAtLimitAccepted) { + uint32_t codec_limit_kb = 64; + max_request_headers_kb_ = codec_limit_kb; + initialize(); + + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + std::string key = "big"; + uint32_t head_room = 77; + uint32_t long_string_length = + codec_limit_kb * 1024 - request_headers.byteSize() - key.length() - head_room; + std::string long_string = std::string(long_string_length, 'q'); + request_headers.addCopy(key, long_string); + + // The amount of data sent to the codec is not equivalent to the size of the + // request headers that Envoy computes, as the codec limits based on the + // entire http2 frame. The exact head room needed (76) was found through iteration. + ASSERT_EQ(request_headers.byteSize() + head_room, codec_limit_kb * 1024); + + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); + request_encoder_->encodeHeaders(request_headers, true); +} + TEST_P(Http2CodecImplTest, TestCodecHeaderCompression) { initialize(); diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h new file mode 100644 index 0000000000000..b642701265c44 --- /dev/null +++ b/test/common/http/http2/codec_impl_test_util.h @@ -0,0 +1,33 @@ +#include "envoy/http/codec.h" + +#include "common/http/http2/codec_impl.h" + +namespace Envoy { +namespace Http { +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) + : ServerConnectionImpl(connection, callbacks, scope, http2_settings, max_request_headers_kb) { + } + nghttp2_session* session() { return session_; } + using ServerConnectionImpl::getStream; +}; + +class TestClientConnectionImpl : public ClientConnectionImpl { +public: + TestClientConnectionImpl(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + Stats::Scope& scope, const Http2Settings& http2_settings, + uint32_t max_request_headers_kb) + : ClientConnectionImpl(connection, callbacks, scope, http2_settings, max_request_headers_kb) { + } + nghttp2_session* session() { return session_; } + using ClientConnectionImpl::getStream; +}; + +} // namespace Http2 +} // namespace Http +} // namespace Envoy 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 4fe6b5d7cc92d..2e42f07f03dcb 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -163,7 +163,7 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeDefault) { HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_); - EXPECT_EQ(60, config.maxRequestHeadersSizeKb()); + EXPECT_EQ(60, config.maxRequestHeadersKb()); } TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeConfigured) { @@ -178,7 +178,7 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeConfigured) { HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_); - EXPECT_EQ(16, config.maxRequestHeadersSizeKb()); + EXPECT_EQ(16, config.maxRequestHeadersKb()); } // Validated that an explicit zero stream idle timeout disables. diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index d97f56258b3fb..0a48b7e0c526f 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -217,8 +217,9 @@ FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connectio auto settings = Http::Http2Settings(); settings.allow_connect_ = true; settings.allow_metadata_ = true; - codec_ = std::make_unique(shared_connection_.connection(), - *this, store, settings); + codec_ = std::make_unique( + shared_connection_.connection(), *this, store, settings, + Http::DEFAULT_MAX_REQUEST_HEADERS_KB); ASSERT(type == Type::HTTP2); } diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 4287216c1392c..666abf707efc7 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -467,9 +467,9 @@ TEST_P(Http2IntegrationTest, RetryPriority) { testRetryPriority(); } TEST_P(Http2IntegrationTest, GrpcRetry) { testGrpcRetry(); } -TEST_P(Http2IntegrationTest, LargeHeadersInvokeResetStream) { testLargeRequestHeaders(62, 60); } +TEST_P(Http2IntegrationTest, LargeHeadersInvokeResetStream) { testLargeRequestHeaders(63, 60); } -TEST_P(Http2IntegrationTest, LargeHeadersAcceptedIfConfigured) { testLargeRequestHeaders(62, 63); } +TEST_P(Http2IntegrationTest, LargeHeadersAcceptedIfConfigured) { testLargeRequestHeaders(63, 64); } TEST_P(Http2IntegrationTest, EncodingHeaderOnlyResponse) { testHeadersOnlyFilterEncoding(); } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index a2eac922b4a2b..8b2be331f9f68 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -490,9 +490,9 @@ TEST_P(IntegrationTest, InvalidContentLength) { testInvalidContentLength(); } TEST_P(IntegrationTest, MultipleContentLengths) { testMultipleContentLengths(); } -TEST_P(IntegrationTest, LargeHeadersRejected) { testLargeRequestHeaders(62, 60); } +TEST_P(IntegrationTest, LargeHeadersRejected) { testLargeRequestHeaders(63, 60); } -TEST_P(IntegrationTest, LargeHeadersAccepted) { testLargeRequestHeaders(62, 63); } +TEST_P(IntegrationTest, LargeHeadersAcceptedIfConfigured) { testLargeRequestHeaders(63, 64); } TEST_P(IntegrationTest, UpstreamProtocolError) { initialize();