(codec) Increase max_request_headers_kb limit to 64 and add http2 codec checks

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

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 <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.max_request_headers_kb>`.
    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 <https://tools.ietf.org/html/rfc7540#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

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;
 

source/common/http/codec_client.cc

@@ -145,7 +145,8 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
   }
   case Type::HTTP2: {
     codec_ = std::make_unique<Http2::ClientConnectionImpl>(
-        *connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings());
+        *connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings(),
+        Http::DEFAULT_MAX_REQUEST_HEADERS_KB);
     break;
   }
   }

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

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;

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)};

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

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());

source/common/http/http2/codec_impl.h

@@ -74,9 +74,9 @@ class Utility {
 class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Logger::Id::http2> {
 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<Log
     void pendingSendBufferHighWatermark();
     void pendingSendBufferLowWatermark();
 
-    // Max header size of 63K. This is arbitrary but makes it easier to test since nghttp2 doesn't
-    // appear to transmit headers greater than approximately 64K (NGHTTP2_MAX_HEADERSLEN) for
-    // reasons I don't fully understand.
-    static const uint64_t MAX_HEADER_SIZE = 63 * 1024;
-
     // Does any necessary WebSocket/Upgrade conversion, then passes the headers
     // to the decoder_.
     void decodeHeaders();
@@ -291,6 +286,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   nghttp2_session* session_{};
   CodecStats stats_;
   Network::Connection& connection_;
+  const uint32_t max_request_headers_kb_;
   uint32_t per_stream_buffer_limit_;
   bool allow_metadata_;
 
@@ -319,7 +315,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
 public:
   ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks,
-                       Stats::Scope& stats, const Http2Settings& http2_settings);
+                       Stats::Scope& stats, const Http2Settings& http2_settings,
+                       const uint32_t max_request_headers_kb);
 
   // Http::ClientConnection
   Http::StreamEncoder& newStream(StreamDecoder& response_decoder) override;
@@ -339,7 +336,8 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
 class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
 public:
   ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
-                       Stats::Scope& scope, const Http2Settings& http2_settings);
+                       Stats::Scope& scope, const Http2Settings& http2_settings,
+                       const uint32_t max_request_headers_kb);
 
 private:
   // ConnectionImpl

source/extensions/filters/network/http_connection_manager/config.cc

@@ -140,8 +140,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
       route_config_provider_manager_(route_config_provider_manager),
       http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())),
       http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())),
-      max_request_headers_size_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
-          config, max_request_headers_kb, Http::DEFAULT_MAX_REQUEST_HEADERS_SIZE_KB)),
+      max_request_headers_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
+          config, max_request_headers_kb, Http::DEFAULT_MAX_REQUEST_HEADERS_KB)),
       idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(config, idle_timeout)),
       stream_idle_timeout_(
           PROTOBUF_GET_MS_OR_DEFAULT(config, stream_idle_timeout, StreamIdleTimeoutMs)),
@@ -326,10 +326,11 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection,
         new Http::Http1::ServerConnectionImpl(connection, callbacks, http1_settings_)};
   case CodecType::HTTP2:
     return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl(
-        connection, callbacks, context_.scope(), http2_settings_)};
+        connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb())};
   case CodecType::AUTO:
-    return Http::ConnectionManagerUtility::autoCreateCodec(
-        connection, data, callbacks, context_.scope(), http1_settings_, http2_settings_);
+    return Http::ConnectionManagerUtility::autoCreateCodec(connection, data, callbacks,
+                                                           context_.scope(), http1_settings_,
+                                                           http2_settings_, maxRequestHeadersKb());
   }
 
   NOT_REACHED_GCOVR_EXCL_LINE;

source/extensions/filters/network/http_connection_manager/config.h

@@ -101,7 +101,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   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<std::chrono::milliseconds> 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<Logger::Id::config>,
   std::string server_name_;
   Http::TracingConnectionManagerConfigPtr tracing_config_;
   absl::optional<std::string> user_agent_;
-  uint32_t max_request_headers_size_kb_;
+  const uint32_t max_request_headers_kb_;
   absl::optional<std::chrono::milliseconds> idle_timeout_;
   std::chrono::milliseconds stream_idle_timeout_;
   std::chrono::milliseconds request_timeout_;

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,

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<std::chrono::milliseconds> 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<UrlHandler> 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<std::chrono::milliseconds> idle_timeout_;
   absl::optional<std::string> user_agent_;
   Http::SlowDateProviderImpl date_provider_;

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<std::chrono::milliseconds> 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<std::chrono::milliseconds> idle_timeout_;
   std::chrono::milliseconds stream_idle_timeout_{};
   std::chrono::milliseconds request_timeout_{};

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<std::chrono::milliseconds> 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<Http::ClientCertDetailsType> set_current_client_cert_details_;
   absl::optional<std::string> 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<std::chrono::milliseconds> 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 {

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<std::chrono::milliseconds>());
   MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds());
   MOCK_CONST_METHOD0(requestTimeout, std::chrono::milliseconds());

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"],