From 14525f8074eeca5f4232933f8e910ff61b9876e0 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 19 Sep 2024 13:47:11 -0700 Subject: [PATCH] http: add config for max response header size Signed-off-by: Greg Greenway --- api/envoy/config/core/v3/protocol.proto | 11 +++- .../v3/http_connection_manager.proto | 3 ++ changelogs/current.yaml | 4 ++ envoy/upstream/upstream.h | 5 ++ source/common/http/codec_client.cc | 8 +-- source/common/http/http1/codec_impl.cc | 4 +- source/common/http/http1/codec_impl.h | 1 + source/common/protobuf/utility.h | 6 +++ source/common/upstream/upstream_impl.cc | 2 + source/common/upstream/upstream_impl.h | 4 ++ .../network/http_connection_manager/config.cc | 16 ++++-- test/common/http/codec_impl_fuzz_test.cc | 2 +- test/common/http/http1/codec_impl_test.cc | 37 ++++++++++++-- .../http/http1/http1_connection_fuzz_test.cc | 2 +- .../http_connection_manager/config_test.cc | 50 +++++++++++++++++++ .../multiplexed_upstream_integration_test.cc | 27 ++++++++++ test/mocks/upstream/cluster_info.h | 1 + 17 files changed, 168 insertions(+), 15 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 6dbff8c48f03..138d704d9eaa 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -205,7 +205,7 @@ message AlternateProtocolsCacheOptions { repeated string canonical_suffixes = 5; } -// [#next-free-field: 7] +// [#next-free-field: 8] message HttpProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.HttpProtocolOptions"; @@ -260,6 +260,15 @@ message HttpProtocolOptions { // 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}]; + // The maximum combined size of headers. + // If unconfigured, the default is 60 KiB, except for HTTP/1 response headers which have a default + // of 80KiB. + // Requests that exceed this limit will receive a 431 response. + // If both this setting and :ref:`max_request_headers_kb + // ` + // are set, this setting is used. + google.protobuf.UInt32Value max_headers_kb = 7 [(validate.rules).uint32 = {lte: 8192 gt: 0}]; + // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be // reset independent of any other timeouts. If not specified, this value is not set. google.protobuf.Duration max_stream_duration = 4; diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 4cbbbc20d3fb..0c0ad980d940 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -494,6 +494,9 @@ message HttpConnectionManager { // The maximum request headers size for incoming connections. // If unconfigured, the default max request headers allowed is 60 KiB. // Requests that exceed this limit will receive a 431 response. + // If both this setting and :ref:`max_headers_kb + // ` are set, + // that setting is used and this one is ignored. google.protobuf.UInt32Value max_request_headers_kb = 29 [(validate.rules).uint32 = {lte: 8192 gt: 0}]; diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 5fd9945a3ad6..a0ba00a5fa3f 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -264,6 +264,10 @@ new_features: change: | Added full feature absl::FormatTime() support to the DateFormatter. This allows the timepoint formatters (like ``%START_TIME%``) to use ``%E#S``, ``%E*S``, ``%E#f`` and ``%E*f`` to format the subsecond part of the timepoint. +- area: http + change: | + Added configuration setting for the :ref:`maximum size of headers + ` in responses. - area: http_11_proxy change: | Added the option to configure the transport socket via locality or endpoint metadata. diff --git a/envoy/upstream/upstream.h b/envoy/upstream/upstream.h index e307de9471f7..ca704d02fdb6 100644 --- a/envoy/upstream/upstream.h +++ b/envoy/upstream/upstream.h @@ -1068,6 +1068,11 @@ class ClusterInfo : public Http::FilterChainFactory { */ virtual uint32_t maxResponseHeadersCount() const PURE; + /** + * @return uint32_t the maximum total size of response headers in KB. + */ + virtual absl::optional maxResponseHeadersKb() const PURE; + /** * @return the human readable name of the cluster. */ diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 4c0cee082880..fb86c869f300 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -277,13 +277,14 @@ CodecClientProd::CodecClientProd(CodecType type, Network::ClientConnectionPtr&& } codec_ = std::make_unique( *connection_, host->cluster().http1CodecStats(), *this, host->cluster().http1Settings(), - host->cluster().maxResponseHeadersCount(), proxied); + host->cluster().maxResponseHeadersKb(), host->cluster().maxResponseHeadersCount(), proxied); break; } case CodecType::HTTP2: codec_ = std::make_unique( *connection_, *this, host->cluster().http2CodecStats(), random_generator, - host->cluster().http2Options(), Http::DEFAULT_MAX_REQUEST_HEADERS_KB, + host->cluster().http2Options(), + host->cluster().maxResponseHeadersKb().value_or(Http::DEFAULT_MAX_REQUEST_HEADERS_KB), host->cluster().maxResponseHeadersCount(), Http2::ProdNghttp2SessionFactory::get()); break; case CodecType::HTTP3: { @@ -291,7 +292,8 @@ CodecClientProd::CodecClientProd(CodecType type, Network::ClientConnectionPtr&& auto& quic_session = dynamic_cast(*connection_); codec_ = std::make_unique( quic_session, *this, host->cluster().http3CodecStats(), host->cluster().http3Options(), - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount()); + host->cluster().maxResponseHeadersKb().value_or(Http::DEFAULT_MAX_REQUEST_HEADERS_KB), + host->cluster().maxResponseHeadersCount()); // Initialize the session after max request header size is changed in above http client // connection creation. quic_session.Initialize(); diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 5e054058a996..92fb58bb1a40 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1428,9 +1428,11 @@ void ServerConnectionImpl::ActiveRequest::dumpState(std::ostream& os, int indent ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks&, const Http1Settings& settings, + absl::optional max_response_headers_kb, const uint32_t max_response_headers_count, bool passing_through_proxy) - : ConnectionImpl(connection, stats, settings, MessageType::Response, MAX_RESPONSE_HEADERS_KB, + : ConnectionImpl(connection, stats, settings, MessageType::Response, + max_response_headers_kb.value_or(MAX_RESPONSE_HEADERS_KB), max_response_headers_count), owned_output_buffer_(connection.dispatcher().getWatermarkFactory().createBuffer( [&]() -> void { this->onBelowLowWatermark(); }, diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 26e3bfe82d9f..875a9c6d99d6 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -590,6 +590,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { public: ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks& callbacks, const Http1Settings& settings, + absl::optional max_response_headers_kb, const uint32_t max_response_headers_count, bool passing_through_proxy = false); // Http::ClientConnection diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 546fbcd22783..ffd19ad89e78 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -23,6 +23,12 @@ #define PROTOBUF_GET_WRAPPED_OR_DEFAULT(message, field_name, default_value) \ ((message).has_##field_name() ? (message).field_name().value() : (default_value)) +// Obtain the value of a wrapped field (e.g. google.protobuf.UInt32Value) if set. Otherwise, return +// absl::nullopt. +#define PROTOBUF_GET_OPTIONAL_WRAPPED(message, field_name) \ + ((message).has_##field_name() ? absl::make_optional((message).field_name().value()) \ + : absl::nullopt) + // Obtain the value of a wrapped field (e.g. google.protobuf.UInt32Value) if set. Otherwise, throw // a EnvoyException. diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index bfee8e6b2baa..1f01a0ff5b2e 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1229,6 +1229,8 @@ ClusterInfoImpl::ClusterInfoImpl( http_protocol_options_->common_http_protocol_options_, max_headers_count, runtime_.snapshot().getInteger(Http::MaxResponseHeadersCountOverrideKey, Http::DEFAULT_MAX_HEADERS_COUNT))), + max_response_headers_kb_(PROTOBUF_GET_OPTIONAL_WRAPPED( + http_protocol_options_->common_http_protocol_options_, max_headers_kb)), type_(config.type()), drain_connections_on_host_removal_(config.ignore_health_on_host_removal()), connection_pool_per_downstream_connection_( diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 47d32468571d..a3d43f7b969e 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -913,6 +913,9 @@ class ClusterInfoImpl : public ClusterInfo, bool maintenanceMode() const override; uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; } uint32_t maxResponseHeadersCount() const override { return max_response_headers_count_; } + absl::optional maxResponseHeadersKb() const override { + return max_response_headers_kb_; + } const std::string& name() const override { return name_; } const std::string& observabilityName() const override { if (observability_name_ != nullptr) { @@ -1126,6 +1129,7 @@ class ClusterInfoImpl : public ClusterInfo, // overhead via alignment const uint32_t per_connection_buffer_limit_bytes_; const uint32_t max_response_headers_count_; + const absl::optional max_response_headers_kb_; const envoy::config::cluster::v3::Cluster::DiscoveryType type_; const bool drain_connections_on_host_removal_ : 1; const bool connection_pool_per_downstream_connection_ : 1; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 563ece44c917..eb587a82507b 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -369,9 +369,11 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( config.stream_error_on_invalid_http_message(), xff_num_trusted_hops_ == 0 && use_remote_address_)), max_request_headers_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, max_request_headers_kb, - context.serverFactoryContext().runtime().snapshot().getInteger( - Http::MaxRequestHeadersSizeOverrideKey, Http::DEFAULT_MAX_REQUEST_HEADERS_KB))), + config.common_http_protocol_options(), max_headers_kb, + PROTOBUF_GET_WRAPPED_OR_DEFAULT( + config, max_request_headers_kb, + context.serverFactoryContext().runtime().snapshot().getInteger( + Http::MaxRequestHeadersSizeOverrideKey, Http::DEFAULT_MAX_REQUEST_HEADERS_KB)))), max_request_headers_count_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( config.common_http_protocol_options(), max_headers_count, context.serverFactoryContext().runtime().snapshot().getInteger( @@ -432,6 +434,14 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( return; } + if (config.has_max_request_headers_kb() && + config.common_http_protocol_options().has_max_headers_kb() && + config.max_request_headers_kb().value() != + config.common_http_protocol_options().max_headers_kb().value()) { + ENVOY_LOG(warn, "Both `max_request_headers_kb` and `max_headers_kb` are configured. Ignoring " + "`max_request_headers_kb`."); + } + auto options_or_error = Http2::Utility::initializeAndValidateOptions( config.http2_protocol_options(), config.has_stream_error_on_invalid_http_message(), config.stream_error_on_invalid_http_message()); diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index 8554be417b01..bca32e84542e 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -616,7 +616,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi } else { client = std::make_unique( client_connection, Http1::CodecStats::atomicGet(http1_stats, scope), client_callbacks, - client_http1settings, max_response_headers_count); + client_http1settings, max_request_headers_kb, max_response_headers_count); } if (http2) { diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 1fca99b17e7b..0dca1c8abc16 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2544,7 +2544,8 @@ class Http1ClientConnectionImplTest : public Http1CodecTestBase { public: void initialize() { codec_ = std::make_unique( - connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_, + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_kb_, + max_response_headers_count_, /* passing_through_proxy=*/false); } @@ -2564,6 +2565,7 @@ class Http1ClientConnectionImplTest : public Http1CodecTestBase { protected: Stats::TestUtil::TestStore store_; uint32_t max_response_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; + uint32_t max_response_headers_kb_{Http::Http1::MAX_RESPONSE_HEADERS_KB}; }; void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength( @@ -2571,8 +2573,9 @@ void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength( // Response validation is not implemented in UHV yet #ifndef ENVOY_ENABLE_UHV codec_settings_.allow_chunked_length_ = allow_chunked_length; - codec_ = std::make_unique( - connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + codec_ = std::make_unique(connection_, http1CodecStats(), callbacks_, + codec_settings_, max_response_headers_kb_, + max_response_headers_count_); NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); @@ -3706,6 +3709,28 @@ TEST_P(Http1ClientConnectionImplTest, LargeResponseHeadersAccepted) { std::string long_header = "big: " + std::string(79 * 1024, 'q') + "\r\n"; buffer = Buffer::OwnedImpl(long_header); status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); +} + +// Tests that the size of response headers for HTTP/1 can be configured higher than the default of +// 80kB. +TEST_P(Http1ClientConnectionImplTest, LargeResponseHeadersAcceptedConfigurable) { + constexpr uint32_t size_limit_kb = 85; + max_response_headers_kb_ = size_limit_kb; + initialize(); + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); + + Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); + std::string long_header = "big: " + std::string((size_limit_kb - 1) * 1024, 'q') + "\r\n"; + buffer = Buffer::OwnedImpl(long_header); + status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); } // Regression test for CVE-2019-18801. Large method headers should not trigger @@ -3792,6 +3817,7 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersAccepted) { // Response already contains one header. buffer = Buffer::OwnedImpl(createHeaderOrTrailerFragment(150) + "\r\n"); status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); } TEST_P(Http1ClientConnectionImplTest, TestResponseSplit0) { @@ -3821,8 +3847,9 @@ TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength100) { TEST_P(Http1ClientConnectionImplTest, VerifyResponseHeaderTrailerMapMaxLimits) { codec_settings_.allow_chunked_length_ = true; codec_settings_.enable_trailers_ = true; - codec_ = std::make_unique( - connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + codec_ = std::make_unique(connection_, http1CodecStats(), callbacks_, + codec_settings_, max_response_headers_kb_, + max_response_headers_count_); NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); diff --git a/test/common/http/http1/http1_connection_fuzz_test.cc b/test/common/http/http1/http1_connection_fuzz_test.cc index 9dae7af6c7b9..37eb6fc64e4a 100644 --- a/test/common/http/http1/http1_connection_fuzz_test.cc +++ b/test/common/http/http1/http1_connection_fuzz_test.cc @@ -45,7 +45,7 @@ class Http1Harness { client_ = std::make_unique( mock_client_connection_, Http1::CodecStats::atomicGet(http1_stats_, *stats_store_.rootScope()), - mock_client_callbacks_, client_settings_, Http::DEFAULT_MAX_HEADERS_COUNT); + mock_client_callbacks_, client_settings_, absl::nullopt, Http::DEFAULT_MAX_HEADERS_COUNT); Status status = client_->dispatch(payload); } 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 5f39f9935a21..599dada4bf20 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -826,6 +826,56 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbConfigured) { EXPECT_EQ(16, config.maxRequestHeadersKb()); } +TEST_F(HttpConnectionManagerConfigTest, MaxHeadersKbConfigured) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + common_http_protocol_options: + max_headers_kb: 16 + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + EXPECT_EQ(16, config.maxRequestHeadersKb()); +} + +TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbAndMaxHeadersKbConfigured) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + common_http_protocol_options: + max_headers_kb: 16 + max_request_headers_kb: 32 + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + EXPECT_LOG_CONTAINS("warn", + "Both `max_request_headers_kb` and `max_headers_kb` are configured. Ignoring " + "`max_request_headers_kb`.", + { + HttpConnectionManagerConfig config( + parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + + ASSERT_TRUE(creation_status_.ok()); + EXPECT_EQ(16, config.maxRequestHeadersKb()); + }); +} + TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbMaxConfigurable) { const std::string yaml_string = R"EOF( stat_prefix: ingress_http diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index e2270be533a8..0afab76a0f34 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -573,6 +573,33 @@ TEST_P(MultiplexedUpstreamIntegrationTest, LargeResponseHeadersRejected) { EXPECT_EQ("503", response->headers().getStatusValue()); } +// Tests configuration of max response headers size. +TEST_P(MultiplexedUpstreamIntegrationTest, LargeResponseHeadersAccepted) { + constexpr uint32_t limit_kb = 150; + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + ConfigHelper::HttpProtocolOptions protocol_options; + auto* http_protocol_options = protocol_options.mutable_common_http_protocol_options(); + http_protocol_options->mutable_max_headers_kb()->set_value(limit_kb); + ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0), + protocol_options); + }); + Http::TestResponseHeaderMapImpl large_headers(default_response_headers_); + large_headers.addCopy("large", std::string((limit_kb - 1) * 1024, 'a')); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeRequestWithBody(default_request_headers_, 1024); + waitForNextUpstreamRequest(); + + upstream_request_->encodeHeaders(response_headers, false); + upstream_request_->encodeData(512, true); + ASSERT_TRUE(response->waitForEndStream()); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_TRUE(response->complete()); +} + TEST_P(MultiplexedUpstreamIntegrationTest, NoInitialStreams) { // Set the fake upstream to start with 0 streams available. upstreamConfig().http2_options_.mutable_max_concurrent_streams()->set_value(0); diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 8f5a72973f49..978f9c5826aa 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -128,6 +128,7 @@ class MockClusterInfo : public ClusterInfo { (const)); MOCK_METHOD(bool, maintenanceMode, (), (const)); MOCK_METHOD(uint32_t, maxResponseHeadersCount, (), (const)); + MOCK_METHOD(absl::optional, maxResponseHeadersKb, (), (const)); MOCK_METHOD(uint64_t, maxRequestsPerConnection, (), (const)); MOCK_METHOD(const std::string&, name, (), (const)); MOCK_METHOD(const std::string&, observabilityName, (), (const));