codec: Raise max_request_headers_kb limit to 96 KiB

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -134,13 +134,15 @@ message HttpConnectionManager {
   string server_name = 10;
 
   // 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 64K in order to
+  // is 60 KiB, based on default settings for http codecs. For HTTP1, the currentB
+  // limit set by http_parser is 80 KiB. for HTTP2, the default allowed header
+  // block in nghttp2 is 64 KiB. The max configurable setting is 64K in order to
   // stay under both codec limits.
+  // The maximum per-header limit, i.e. for each key-value pair, is 64 KiB in
+  // http2.
   // 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 = 64];
+      [(validate.rules).uint32.gt = 0, (validate.rules).uint32.lte = 96];
 
   // The idle timeout for connections managed by the connection manager. The
   // idle timeout is defined as the period in which there are no active

bazel/external/http-parser.BUILD

@@ -7,6 +7,7 @@ cc_library(
         "http_parser.h",
     ],
     hdrs = ["http_parser.h"],
+    copts = ["-DHTTP_MAX_HEADER_SIZE=0x7fffffff"],
     includes = ["."],
     visibility = ["//visibility:public"],
 )

source/common/http/http2/codec_impl.cc

@@ -867,6 +867,8 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings)
   // of kept alive HTTP/2 connections.
   nghttp2_option_set_no_closed_streams(options_, 1);
   nghttp2_option_set_no_auto_window_update(options_, 1);
+  nghttp2_option_set_max_send_header_block_length(options_,
+                                                  NGHTTP2_MAX_SEND_HEADER_BLOCK_LENGTH * 1024);
 
   if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) {
     nghttp2_option_set_max_deflate_dynamic_table_size(options_, http2_settings.hpack_table_size_);

source/common/http/http2/codec_impl.h

@@ -34,6 +34,11 @@ const std::string ALPN_STRING = "h2";
 // differentiate between HTTP/1 and HTTP/2.
 const std::string CLIENT_MAGIC_PREFIX = "PRI * HTTP/2";
 
+// This setting will be passed to the nghttp2 library's max request headers check. It is set to an
+// arbitrarily high number so as to never trigger the check, as we check request headers length in
+// codec_impl::saveHeader.
+static const uint32_t NGHTTP2_MAX_SEND_HEADER_BLOCK_LENGTH = 0x7fffffff;
+
 /**
  * All stats for the HTTP/2 codec. @see stats_macros.h
  */

test/common/http/http1/codec_impl_test.cc

@@ -1007,11 +1007,9 @@ TEST_F(Http1ClientConnectionImplTest, HighwatermarkMultipleResponses) {
       ->onUnderlyingConnectionBelowWriteBufferLowWatermark();
 }
 
-// For issue #1421 regression test that Envoy's HTTP parser applies header limits early.
-TEST_F(Http1ServerConnectionImplTest, TestCodecHeaderLimits) {
+TEST_F(Http1ServerConnectionImplTest, TestLargeRequestHeadersAccepted) {
   initialize();
 
-  std::string exception_reason;
   NiceMock<Http::MockStreamDecoder> decoder;
   Http::StreamEncoder* response_encoder = nullptr;
   EXPECT_CALL(callbacks_, newStream(_, _))
@@ -1022,14 +1020,27 @@ TEST_F(Http1ServerConnectionImplTest, TestCodecHeaderLimits) {
 
   Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n");
   codec_->dispatch(buffer);
-  std::string long_string = "foo: " + std::string(1024, 'q') + "\r\n";
-  for (int i = 0; i < 79; ++i) {
-    buffer = Buffer::OwnedImpl(long_string);
-    codec_->dispatch(buffer);
-  }
+  std::string long_string = "big: " + std::string(64 * 1024, 'q') + "\r\n";
+  buffer = Buffer::OwnedImpl(long_string);
+  codec_->dispatch(buffer);
+}
+
+TEST_F(Http1ServerConnectionImplTest, TestLargeRequestHeadersAcceptedMaxConfigurable) {
+  initialize();
+
+  NiceMock<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("GET / HTTP/1.1\r\n");
+  codec_->dispatch(buffer);
+  std::string long_string = "big: " + std::string(95 * 1024, 'q') + "\r\n";
   buffer = Buffer::OwnedImpl(long_string);
-  EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException,
-                            "http/1.1 protocol error: HPE_HEADER_OVERFLOW");
+  codec_->dispatch(buffer);
 }
 
 } // namespace Http1

test/common/http/http2/codec_impl_test.cc

@@ -66,7 +66,7 @@ class Http2CodecImplTest : public TestBaseWithParam<Http2SettingsTestParam> {
     setupDefaultConnectionMocks();
 
     EXPECT_CALL(server_callbacks_, newStream(_, _))
-        .WillOnce(Invoke([&](StreamEncoder& encoder, bool) -> StreamDecoder& {
+        .WillRepeatedly(Invoke([&](StreamEncoder& encoder, bool) -> StreamDecoder& {
           response_encoder_ = &encoder;
           encoder.getStream().addCallbacks(server_stream_callbacks_);
           return request_decoder_;
@@ -865,7 +865,7 @@ TEST(Http2CodecUtility, reconstituteCrumbledCookies) {
   }
 }
 
-TEST_P(Http2CodecImplTest, TestLargeHeadersInvokeResetStream) {
+TEST_P(Http2CodecImplTest, TestLargeRequestHeadersInvokeResetStream) {
   initialize();
 
   TestHeaderMapImpl request_headers;
@@ -876,7 +876,7 @@ TEST_P(Http2CodecImplTest, TestLargeHeadersInvokeResetStream) {
   request_encoder_->encodeHeaders(request_headers, false);
 }
 
-TEST_P(Http2CodecImplTest, TestLargeHeadersAcceptedIfConfigured) {
+TEST_P(Http2CodecImplTest, TestLargeRequestHeadersAccepted) {
   max_request_headers_kb_ = 64;
   initialize();
 
@@ -890,7 +890,7 @@ TEST_P(Http2CodecImplTest, TestLargeHeadersAcceptedIfConfigured) {
   request_encoder_->encodeHeaders(request_headers, false);
 }
 
-TEST_P(Http2CodecImplTest, TestLargeHeadersAtLimitAccepted) {
+TEST_P(Http2CodecImplTest, TestLargeRequestHeadersAtLimitAccepted) {
   uint32_t codec_limit_kb = 64;
   max_request_headers_kb_ = codec_limit_kb;
   initialize();
@@ -913,6 +913,72 @@ TEST_P(Http2CodecImplTest, TestLargeHeadersAtLimitAccepted) {
   request_encoder_->encodeHeaders(request_headers, true);
 }
 
+TEST_P(Http2CodecImplTest, TestLargeRequestHeadersOverDefaultCodecLibraryLimit) {
+  max_request_headers_kb_ = 66;
+  initialize();
+
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  std::string long_string = std::string(65 * 1024, 'q');
+  request_headers.addCopy("big", long_string);
+
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(1);
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(_)).Times(0);
+  request_encoder_->encodeHeaders(request_headers, true);
+}
+
+TEST_P(Http2CodecImplTest, TestLargeRequestHeadersExceedPerHeaderLimit) {
+  // The name-value pair max is set by NGHTTP2_HD_MAX_NV in lib/nghttp2_hd.h to 64KB, and
+  // creates a per-request header limit for us in h2. Note that the nghttp2
+  // calculated byte size will differ from envoy due to H2 compression and frames.
+
+  max_request_headers_kb_ = 81;
+  initialize();
+
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  std::string long_string = std::string(80 * 1024, 'q');
+  request_headers.addCopy("big", long_string);
+
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(0);
+  EXPECT_CALL(client_callbacks_, onGoAway());
+  server_->shutdownNotice();
+  server_->goAway();
+  request_encoder_->encodeHeaders(request_headers, true);
+}
+
+TEST_P(Http2CodecImplTest, TestManyLargeRequestHeadersUnderPerHeaderLimit) {
+  max_request_headers_kb_ = 81;
+  initialize();
+
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  std::string long_string = std::string(1024, 'q');
+  for (int i = 0; i < 80; i++) {
+    request_headers.addCopy(fmt::format("{}", i), long_string);
+  }
+
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(1);
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(_)).Times(0);
+  request_encoder_->encodeHeaders(request_headers, true);
+}
+
+TEST_P(Http2CodecImplTest, TestLargeRequestHeadersAtMaxConfigurable) {
+  max_request_headers_kb_ = 96;
+  initialize();
+
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  std::string long_string = std::string(1024, 'q');
+  for (int i = 0; i < 95; i++) {
+    request_headers.addCopy(fmt::format("{}", i), long_string);
+  }
+
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(1);
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(_)).Times(0);
+  request_encoder_->encodeHeaders(request_headers, true);
+}
+
 TEST_P(Http2CodecImplTest, TestCodecHeaderCompression) {
   initialize();
 

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -152,7 +152,7 @@ TEST_F(HttpConnectionManagerConfigTest, UnixSocketInternalAddress) {
   EXPECT_FALSE(config.internalAddressConfig().isInternalAddress(externalIpAddress));
 }
 
-TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeDefault) {
+TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbDefault) {
   const std::string yaml_string = R"EOF(
   stat_prefix: ingress_http
   route_config:
@@ -166,7 +166,7 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeDefault) {
   EXPECT_EQ(60, config.maxRequestHeadersKb());
 }
 
-TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeConfigured) {
+TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbConfigured) {
   const std::string yaml_string = R"EOF(
   stat_prefix: ingress_http
   max_request_headers_kb: 16
@@ -181,6 +181,21 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersSizeConfigured) {
   EXPECT_EQ(16, config.maxRequestHeadersKb());
 }
 
+TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbMaxConfigurable) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  max_request_headers_kb: 96
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_);
+  EXPECT_EQ(96, config.maxRequestHeadersKb());
+}
+
 // Validated that an explicit zero stream idle timeout disables.
 TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) {
   const std::string yaml_string = R"EOF(

test/integration/http2_integration_test.cc

@@ -430,10 +430,6 @@ TEST_P(Http2IntegrationTest, GrpcRouterNotFound) {
 
 TEST_P(Http2IntegrationTest, GrpcRetry) { testGrpcRetry(); }
 
-TEST_P(Http2IntegrationTest, LargeHeadersInvokeResetStream) { testLargeRequestHeaders(62, 60); }
-
-TEST_P(Http2IntegrationTest, LargeHeadersAcceptedIfConfigured) { testLargeRequestHeaders(62, 63); }
-
 TEST_P(Http2IntegrationTest, BadMagic) {
   initialize();
   Buffer::OwnedImpl buffer("hello");

test/integration/http_protocol_integration.h

@@ -17,7 +17,7 @@ struct HttpProtocolTestParams {
 //
 // typedef HttpProtocolIntegrationTest MyTest
 //
-// INSTANTIATE_TEST_SUITE_P(Protocols, BufferIntegrationTest,
+// INSTANTIATE_TEST_SUITE_P(Protocols, MyTest,
 //                         testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()),
 //                         HttpProtocolIntegrationTest::protocolTestParamsToString);
 //

test/integration/integration_test.cc

@@ -468,10 +468,6 @@ TEST_P(IntegrationTest, Connect) {
   EXPECT_EQ(normalizeDate(response1), normalizeDate(response2));
 }
 
-TEST_P(IntegrationTest, LargeHeadersRejected) { testLargeRequestHeaders(62, 60); }
-
-TEST_P(IntegrationTest, LargeHeadersAccepted) { testLargeRequestHeaders(62, 63); }
-
 TEST_P(IntegrationTest, UpstreamProtocolError) {
   initialize();
   codec_client_ = makeHttpConnection(lookupPort("http"));

test/integration/protocol_integration_test.cc

@@ -719,6 +719,14 @@ name: decode-headers-only
   EXPECT_EQ(0, upstream_request_->body().length());
 }
 
+TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) {
+  testLargeRequestHeaders(95, 60);
+}
+
+TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersAccepted) {
+  testLargeRequestHeaders(95, 96);
+}
+
 // For tests which focus on downstream-to-Envoy behavior, and don't need to be
 // run with both HTTP/1 and HTTP/2 upstreams.
 INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest,

tools/spelling_dictionary.txt

@@ -433,6 +433,7 @@ gregs
 gzip
 hacky
 handshaker
+hd
 hdr
 healths
 healthz