backport to rel1.32: http_inspector dynamic buffering #37002 (#39065)

Author: cpakulski

source/extensions/filters/listener/http_inspector/config.cc

@@ -1,4 +1,3 @@
-#include "envoy/extensions/filters/listener/http_inspector/v3/http_inspector.pb.h"
 #include "envoy/extensions/filters/listener/http_inspector/v3/http_inspector.pb.validate.h"
 #include "envoy/registry/registry.h"
 #include "envoy/server/filter_config.h"

source/extensions/filters/listener/http_inspector/http_inspector.cc

@@ -23,7 +23,8 @@ Config::Config(Stats::Scope& scope)
 
 const absl::string_view Filter::HTTP2_CONNECTION_PREFACE = "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n";
 
-Filter::Filter(const ConfigSharedPtr config) : config_(config) {
+Filter::Filter(const ConfigSharedPtr config)
+    : config_(config), requested_read_bytes_(Config::DEFAULT_INITIAL_BUFFER_SIZE) {
   http_parser_init(&parser_, HTTP_REQUEST);
 }
 
@@ -44,6 +45,24 @@ Network::FilterStatus Filter::onData(Network::ListenerFilterBuffer& buffer) {
     done(true);
     return Network::FilterStatus::Continue;
   case ParseState::Continue:
+    ENVOY_LOG(trace, "http inspector: need more bytes");
+
+    // If we have requested the maximum amount of data, then close the connection
+    // the request line is too large to determine the http version.
+    if (static_cast<size_t>(parser_.nread) >= Config::MAX_INSPECT_SIZE) {
+      ENVOY_LOG(warn, "http inspector: reached max buffer without determining HTTP version, "
+                      "dropping connection");
+      config_->stats().read_error_.inc();
+      cb_->socket().ioHandle().close();
+      return Network::FilterStatus::StopIteration;
+    }
+
+    // Otherwise, double the buffer size and try again
+    if (static_cast<size_t>(parser_.nread) >= requested_read_bytes_) {
+      requested_read_bytes_ =
+          std::min<uint32_t>(2 * requested_read_bytes_, Config::MAX_INSPECT_SIZE);
+    }
+
     return Network::FilterStatus::StopIteration;
   }
   PANIC_DUE_TO_CORRUPT_ENUM

source/extensions/filters/listener/http_inspector/http_inspector.h

@@ -52,7 +52,8 @@ class Config {
 
   const HttpInspectorStats& stats() const { return stats_; }
 
-  static constexpr uint32_t MAX_INSPECT_SIZE = 8192;
+  static constexpr uint32_t DEFAULT_INITIAL_BUFFER_SIZE = 8 * 1024;
+  static constexpr uint32_t MAX_INSPECT_SIZE = 64 * 1024;
 
 private:
   HttpInspectorStats stats_;
@@ -71,7 +72,7 @@ class Filter : public Network::ListenerFilter, Logger::Loggable<Logger::Id::filt
   Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override;
   Network::FilterStatus onData(Network::ListenerFilterBuffer& buffer) override;
 
-  size_t maxReadBytes() const override { return Config::MAX_INSPECT_SIZE; }
+  size_t maxReadBytes() const override { return requested_read_bytes_; }
 
 private:
   static const absl::string_view HTTP2_CONNECTION_PREFACE;
@@ -87,6 +88,7 @@ class Filter : public Network::ListenerFilter, Logger::Loggable<Logger::Id::filt
   absl::string_view protocol_;
   http_parser parser_;
   static http_parser_settings settings_;
+  size_t requested_read_bytes_ = 0;
 };
 
 } // namespace HttpInspector

test/extensions/filters/listener/http_inspector/http_inspector_test.cc

@@ -1,4 +1,5 @@
 #include "source/common/common/hex.h"
+#include "source/common/common/logger.h"
 #include "source/common/http/utility.h"
 #include "source/common/network/io_socket_handle_impl.h"
 #include "source/common/network/listener_filter_buffer_impl.h"
@@ -33,8 +34,6 @@ class HttpInspectorTest : public testing::Test {
       : cfg_(std::make_shared<Config>(*store_.rootScope())),
         io_handle_(Network::SocketInterfaceImpl::makePlatformSpecificSocket(42, false,
                                                                             absl::nullopt, {})) {}
-  ~HttpInspectorTest() override { io_handle_->close(); }
-
   void init() {
     filter_ = std::make_unique<Filter>(cfg_);
 
@@ -50,7 +49,7 @@ class HttpInspectorTest : public testing::Test {
             DoAll(SaveArg<1>(&file_event_callback_), ReturnNew<NiceMock<Event::MockFileEvent>>()));
     buffer_ = std::make_unique<Network::ListenerFilterBufferImpl>(
         *io_handle_, dispatcher_, [](bool) {}, [](Network::ListenerFilterBuffer&) {},
-        filter_->maxReadBytes() == 0, filter_->maxReadBytes());
+        Config::DEFAULT_INITIAL_BUFFER_SIZE == 0, Config::DEFAULT_INITIAL_BUFFER_SIZE);
   }
 
   void testHttpInspectMultipleReadsNotFound(absl::string_view header, bool http2 = false) {
@@ -516,7 +515,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) {
   // multiple recv calls.
   init();
   absl::string_view method = "GET", http = "/index HTTP/1.0\r";
-  std::string spaces(Config::MAX_INSPECT_SIZE - method.size() - http.size(), ' ');
+  std::string spaces(Config::DEFAULT_INITIAL_BUFFER_SIZE - method.size() - http.size(), ' ');
   const std::string data = absl::StrCat(method, spaces, http);
   {
     InSequence s;
@@ -529,7 +528,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) {
     }));
 #endif
 
-    uint64_t num_loops = Config::MAX_INSPECT_SIZE;
+    uint64_t num_loops = Config::DEFAULT_INITIAL_BUFFER_SIZE;
 #if defined(__has_feature) &&                                                                      \
     ((__has_feature(thread_sanitizer)) || (__has_feature(address_sanitizer)))
     num_loops = 2;
@@ -556,7 +555,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) {
           size_t len = (*ctr);
           if (num_loops == 2) {
             ASSERT(*ctr != 3);
-            len = size_t(Config::MAX_INSPECT_SIZE / (3 - (*ctr)));
+            len = size_t(Config::DEFAULT_INITIAL_BUFFER_SIZE / (3 - (*ctr)));
           }
           ASSERT(length >= len);
           memcpy(buffer, data.data(), len);
@@ -584,8 +583,8 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) {
 TEST_F(HttpInspectorTest, Http1WithLargeHeader) {
   init();
   absl::string_view request = "GET /index HTTP/1.0\rfield: ";
-  //                           0                  20
-  std::string value(Config::MAX_INSPECT_SIZE - request.size(), 'a');
+  //                           0                    20
+  std::string value(Config::DEFAULT_INITIAL_BUFFER_SIZE - request.size(), 'a');
   const std::string data = absl::StrCat(request, value);
 
   {
@@ -635,6 +634,96 @@ TEST_F(HttpInspectorTest, Http1WithLargeHeader) {
   EXPECT_EQ(1, cfg_->stats().http10_found_.value());
 }
 
+TEST_F(HttpInspectorTest, HttpExceedMaxBufferSize) {
+  absl::string_view method = "GET", http = "/index HTTP/1.0\r\n";
+  std::string spaces(Config::MAX_INSPECT_SIZE, ' ');
+  const std::string data = absl::StrCat(method, spaces, http);
+
+  cfg_ = std::make_shared<Config>(*store_.rootScope());
+
+  init();
+  buffer_->resetCapacity(Config::MAX_INSPECT_SIZE);
+
+  EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
+      .WillOnce(
+          Invoke([&data](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult {
+            const size_t amount_to_copy = std::min(length, data.size());
+            memcpy(buffer, data.data(), amount_to_copy);
+            return Api::SysCallSizeResult{ssize_t(amount_to_copy), 0};
+          }));
+
+  EXPECT_TRUE(file_event_callback_(Event::FileReadyType::Read).ok());
+
+  auto accepted = filter_->onAccept(cb_);
+  EXPECT_EQ(accepted, Network::FilterStatus::StopIteration);
+  auto status = filter_->onData(*buffer_);
+  EXPECT_EQ(Network::FilterStatus::StopIteration, status);
+  EXPECT_EQ(1, cfg_->stats().read_error_.value());
+  EXPECT_FALSE(io_handle_->isOpen());
+}
+
+TEST_F(HttpInspectorTest, HttpExceedInitialBufferSize) {
+  uint32_t buffer_size = Config::DEFAULT_INITIAL_BUFFER_SIZE;
+
+  absl::string_view method = "GET", http = "/index HTTP/1.0\r\n";
+
+  // Want to test doubling a couple times.
+  std::string spaces(buffer_size * 3, ' ');
+
+  const std::string data = absl::StrCat(method, spaces, http);
+
+  init();
+
+  EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
+      .WillOnce(
+          Invoke([&data](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult {
+            const size_t amount_to_copy = std::min(length, data.size());
+            memcpy(buffer, data.data(), amount_to_copy);
+            return Api::SysCallSizeResult{ssize_t(amount_to_copy), 0};
+          }));
+  EXPECT_TRUE(file_event_callback_(Event::FileReadyType::Read).ok());
+
+  auto accepted = filter_->onAccept(cb_);
+  EXPECT_EQ(accepted, Network::FilterStatus::StopIteration);
+  auto status = filter_->onData(*buffer_);
+  EXPECT_EQ(status, Network::FilterStatus::StopIteration);
+  EXPECT_EQ(filter_->maxReadBytes(), 2 * buffer_size);
+  buffer_size *= 2;
+  buffer_->resetCapacity(buffer_size);
+
+  EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
+      .WillOnce(
+          Invoke([&data](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult {
+            const size_t amount_to_copy = std::min(length, data.size());
+            memcpy(buffer, data.data(), amount_to_copy);
+            return Api::SysCallSizeResult{ssize_t(amount_to_copy), 0};
+          }));
+  EXPECT_TRUE(file_event_callback_(Event::FileReadyType::Read).ok());
+
+  status = filter_->onData(*buffer_);
+  EXPECT_EQ(status, Network::FilterStatus::StopIteration);
+  EXPECT_EQ(filter_->maxReadBytes(), 2 * buffer_size);
+  buffer_size *= 2;
+  buffer_->resetCapacity(buffer_size);
+
+  EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
+      .WillOnce(
+          Invoke([&data](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult {
+            const size_t amount_to_copy = std::min(length, data.size());
+            memcpy(buffer, data.data(), amount_to_copy);
+            return Api::SysCallSizeResult{ssize_t(amount_to_copy), 0};
+          }));
+  EXPECT_TRUE(file_event_callback_(Event::FileReadyType::Read).ok());
+
+  const std::vector<absl::string_view> alpn_protos{Http::Utility::AlpnNames::get().Http10};
+  EXPECT_CALL(socket_, setRequestedApplicationProtocols(alpn_protos));
+
+  status = filter_->onData(*buffer_);
+  EXPECT_EQ(status, Network::FilterStatus::Continue);
+
+  EXPECT_EQ(1, cfg_->stats().http10_found_.value());
+}
+
 } // namespace
 } // namespace HttpInspector
 } // namespace ListenerFilters