http: moving strict header checking into the http/1.1 codec

include/envoy/thread_local/thread_local.h

@@ -28,6 +28,14 @@ class Slot {
 public:
   virtual ~Slot() = default;
 
+  /**
+   * This is a helper, mainly used for test code, to determine if a given thread
+   * has a valid slot (i.e. if it is a worker thread) or not.
+   *
+   * @return true if this is a valid slot (called from a worker thread).
+   */
+  virtual bool valid() PURE;
+
   /**
    * @return ThreadLocalObjectSharedPtr a thread local object stored in the slot.
    */

source/common/http/codec_client.cc

@@ -136,12 +136,11 @@ void CodecClient::onData(Buffer::Instance& data) {
 
 CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& connection,
                                  Upstream::HostDescriptionConstSharedPtr host,
-                                 Event::Dispatcher& dispatcher, bool strict_header_validation)
+                                 Event::Dispatcher& dispatcher)
     : CodecClient(type, std::move(connection), host, dispatcher) {
   switch (type) {
   case Type::HTTP1: {
-    codec_ = std::make_unique<Http1::ClientConnectionImpl>(*connection_, *this,
-                                                           strict_header_validation);
+    codec_ = std::make_unique<Http1::ClientConnectionImpl>(*connection_, *this);
     break;
   }
   case Type::HTTP2: {

source/common/http/codec_client.h

@@ -241,8 +241,7 @@ using CodecClientPtr = std::unique_ptr<CodecClient>;
 class CodecClientProd : public CodecClient {
 public:
   CodecClientProd(Type type, Network::ClientConnectionPtr&& connection,
-                  Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher,
-                  bool strict_header_validation);
+                  Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);
 };
 
 } // namespace Http

source/common/http/conn_manager_config.h

@@ -184,15 +184,11 @@ class ConnectionManagerConfig {
    * @param connection supplies the owning connection.
    * @param data supplies the currently available read data.
    * @param callbacks supplies the callbacks to install into the codec.
-   * @param strict_header_validation indicates whether or not the codec should validate the values
-   * of each HTTP header (NOTE: this argument only affects the H/1.1 codec; the H/2 codec always
-   * does this)
    * @return a codec or nullptr if no codec can be created.
    */
   virtual ServerConnectionPtr createCodec(Network::Connection& connection,
                                           const Buffer::Instance& data,
-                                          ServerConnectionCallbacks& callbacks,
-                                          const bool strict_header_validation) PURE;
+                                          ServerConnectionCallbacks& callbacks) PURE;
 
   /**
    * @return DateProvider& the date provider to use for

source/common/http/conn_manager_impl.cc

@@ -112,8 +112,7 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config,
           overload_manager ? overload_manager->getThreadLocalOverloadState().getState(
                                  Server::OverloadActionNames::get().DisableHttpKeepAlive)
                            : Server::OverloadManager::getInactiveState()),
-      time_source_(time_source), strict_header_validation_(Runtime::runtimeFeatureEnabled(
-                                     "envoy.reloadable_features.strict_header_validation")) {}
+      time_source_(time_source) {}
 
 const HeaderMapImpl& ConnectionManagerImpl::continueHeader() {
   CONSTRUCT_ON_FIRST_USE(HeaderMapImpl,
@@ -261,8 +260,7 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder,
 
 Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) {
   if (!codec_) {
-    codec_ =
-        config_.createCodec(read_callbacks_->connection(), data, *this, strict_header_validation_);
+    codec_ = config_.createCodec(read_callbacks_->connection(), data, *this);
     if (codec_->protocol() == Protocol::Http2) {
       stats_.named_.downstream_cx_http2_total_.inc();
       stats_.named_.downstream_cx_http2_active_.inc();

source/common/http/conn_manager_impl.h

@@ -678,8 +678,6 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   const Server::OverloadActionState& overload_stop_accepting_requests_ref_;
   const Server::OverloadActionState& overload_disable_keepalive_ref_;
   TimeSource& time_source_;
-
-  const bool strict_header_validation_;
 };
 
 } // namespace Http

source/common/http/conn_manager_utility.cc

@@ -41,14 +41,13 @@ std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection&
 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,
-    bool strict_header_validation) {
+    const Http2Settings& http2_settings, const uint32_t max_request_headers_kb) {
   if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) {
     return std::make_unique<Http2::ServerConnectionImpl>(connection, callbacks, scope,
                                                          http2_settings, max_request_headers_kb);
   } else {
-    return std::make_unique<Http1::ServerConnectionImpl>(
-        connection, callbacks, http1_settings, max_request_headers_kb, strict_header_validation);
+    return std::make_unique<Http1::ServerConnectionImpl>(connection, callbacks, http1_settings,
+                                                         max_request_headers_kb);
   }
 }
 

source/common/http/conn_manager_utility.h

@@ -37,7 +37,7 @@ class 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, bool strict_header_validation);
+                  const uint32_t max_request_headers_kb);
 
   /**
    * Mutates request headers in various ways. This functionality is broken out because of its

source/common/http/http1/BUILD

@@ -30,6 +30,7 @@ envoy_cc_library(
         "//source/common/http:header_utility_lib",
         "//source/common/http:headers_lib",
         "//source/common/http:utility_lib",
+        "//source/common/runtime:runtime_lib",
     ],
 )
 

source/common/http/http1/codec_impl.cc

@@ -16,6 +16,7 @@
 #include "common/http/header_utility.h"
 #include "common/http/headers.h"
 #include "common/http/utility.h"
+#include "common/runtime/runtime_impl.h"
 
 namespace Envoy {
 namespace Http {
@@ -320,20 +321,12 @@ const ToLowerTable& ConnectionImpl::toLowerTable() {
   return *table;
 }
 
-// TODO(alyssawilk) The overloaded constructors here and on {Client,Server}ConnectionImpl
-// can be cleaned up once "strict_header_validation" becomes the default behavior, rather
-// than a runtime-guarded one. The overloads were a workaround for the fact that Runtime
-// doesn't work from integration test call sites in scenarios where the required
-// thread-local storage is not available.
 ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type,
                                uint32_t max_headers_kb)
-    : ConnectionImpl::ConnectionImpl(connection, type, max_headers_kb, false) {}
-
-ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type,
-                               uint32_t max_headers_kb, bool strict_header_validation)
     : connection_(connection), output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
                                               [&]() -> void { this->onAboveHighWatermark(); }),
-      max_headers_kb_(max_headers_kb), strict_header_validation_(strict_header_validation) {
+      max_headers_kb_(max_headers_kb), strict_header_validation_(Runtime::runtimeFeatureEnabled(
+                                           "envoy.reloadable_features.strict_header_validation")) {
   output_buffer_.setWatermarks(connection.bufferLimit());
   http_parser_init(&parser_, type);
   parser_.data = this;
@@ -516,15 +509,8 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) {
 ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection,
                                            ServerConnectionCallbacks& callbacks,
                                            Http1Settings settings, uint32_t max_request_headers_kb)
-    : ServerConnectionImpl::ServerConnectionImpl(connection, callbacks, settings,
-                                                 max_request_headers_kb, false) {}
-
-ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection,
-                                           ServerConnectionCallbacks& callbacks,
-                                           Http1Settings settings, uint32_t max_request_headers_kb,
-                                           bool strict_header_validation)
-    : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb, strict_header_validation),
-      callbacks_(callbacks), codec_settings_(settings) {}
+    : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb), callbacks_(callbacks),
+      codec_settings_(settings) {}
 
 void ServerConnectionImpl::onEncodeComplete() {
   ASSERT(active_request_);
@@ -695,14 +681,8 @@ void ServerConnectionImpl::onBelowLowWatermark() {
   }
 }
 
-ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection,
-                                           ConnectionCallbacks& callbacks)
-    : ClientConnectionImpl::ClientConnectionImpl(connection, callbacks, false) {}
-
-ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&,
-                                           bool strict_header_validation)
-    : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, strict_header_validation) {
-}
+ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&)
+    : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {}
 
 bool ClientConnectionImpl::cannotHaveBody() {
   if ((!pending_responses_.empty() && pending_responses_.front().head_request_) ||

source/common/http/http1/codec_impl.h

@@ -174,8 +174,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 protected:
   ConnectionImpl(Network::Connection& connection, http_parser_type type,
                  uint32_t max_request_headers_kb);
-  ConnectionImpl(Network::Connection& connection, http_parser_type type,
-                 uint32_t max_request_headers_kb, bool strict_header_validation);
 
   bool resetStreamCalled() { return reset_stream_called_; }
 
@@ -296,10 +294,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
   ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
                        Http1Settings settings, uint32_t max_request_headers_kb);
 
-  ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
-                       Http1Settings settings, uint32_t max_request_headers_kb,
-                       bool strict_header_validation);
-
   virtual bool supports_http_10() override { return codec_settings_.accept_http_10_; }
 
 private:
@@ -350,9 +344,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
 public:
   ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks);
 
-  ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks,
-                       bool strict_header_validation);
-
   // Http::ClientConnection
   StreamEncoder& newStream(StreamDecoder& response_decoder) override;
 

source/common/http/http1/conn_pool.cc

@@ -342,13 +342,8 @@ void ConnPoolImpl::ActiveClient::onConnectTimeout() {
 }
 
 CodecClientPtr ProdConnPoolImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
-
-  const bool strict_header_validation =
-      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation");
-
   CodecClientPtr codec{new CodecClientProd(CodecClient::Type::HTTP1, std::move(data.connection_),
-                                           data.host_description_, dispatcher_,
-                                           strict_header_validation)};
+                                           data.host_description_, dispatcher_)};
   return codec;
 }
 

source/common/http/http2/conn_pool.cc

@@ -304,7 +304,7 @@ ConnPoolImpl::ActiveClient::~ActiveClient() {
 
 CodecClientPtr ProdConnPoolImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
   CodecClientPtr codec{new CodecClientProd(CodecClient::Type::HTTP2, std::move(data.connection_),
-                                           data.host_description_, dispatcher_, false)};
+                                           data.host_description_, dispatcher_)};
   return codec;
 }
 

source/common/runtime/runtime_impl.h

@@ -246,11 +246,14 @@ class LoaderImpl : public Loader, Logger::Loggable<Logger::Id::runtime> {
   Snapshot& snapshot() override;
   void mergeValues(const std::unordered_map<std::string, std::string>& values) override;
 
+protected:
+  ThreadLocal::SlotPtr& tls() { return tls_; }
+  virtual std::unique_ptr<SnapshotImpl> createNewSnapshot();
+
 private:
   friend RtdsSubscription;
 
   // Create a new Snapshot
-  virtual std::unique_ptr<SnapshotImpl> createNewSnapshot();
   // Load a new Snapshot into TLS
   void loadNewSnapshot();
   RuntimeStats generateStats(Stats::Store& store);

source/common/thread_local/thread_local_impl.cc

@@ -37,8 +37,10 @@ SlotPtr InstanceImpl::allocateSlot() {
   return slot;
 }
 
+bool InstanceImpl::SlotImpl::valid() { return thread_local_data_.data_.size() > index_; }
+
 ThreadLocalObjectSharedPtr InstanceImpl::SlotImpl::get() {
-  ASSERT(thread_local_data_.data_.size() > index_);
+  ASSERT(valid());
   return thread_local_data_.data_[index_];
 }
 

source/common/thread_local/thread_local_impl.h

@@ -34,6 +34,7 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
 
     // ThreadLocal::Slot
     ThreadLocalObjectSharedPtr get() override;
+    bool valid() override;
     void runOnAllThreads(Event::PostCb cb) override { parent_.runOnAllThreads(cb); }
     void runOnAllThreads(Event::PostCb cb, Event::PostCb main_callback) override {
       parent_.runOnAllThreads(cb, main_callback);

source/common/upstream/health_checker_impl.cc

@@ -336,10 +336,8 @@ Http::CodecClient::Type HttpHealthCheckerImpl::codecClientType(bool use_http2) {
 
 Http::CodecClient*
 ProdHttpHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
-  const bool strict_header_validation =
-      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation");
   return new Http::CodecClientProd(codec_client_type_, std::move(data.connection_),
-                                   data.host_description_, dispatcher_, strict_header_validation);
+                                   data.host_description_, dispatcher_);
 }
 
 TcpHealthCheckMatcher::MatchSegments TcpHealthCheckMatcher::loadProtoBytes(
@@ -747,7 +745,7 @@ Http::CodecClientPtr
 ProdGrpcHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
   return std::make_unique<Http::CodecClientProd>(Http::CodecClient::Type::HTTP2,
                                                  std::move(data.connection_),
-                                                 data.host_description_, dispatcher_, false);
+                                                 data.host_description_, dispatcher_);
 }
 
 std::ostream& operator<<(std::ostream& out, HealthState state) {

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

@@ -360,20 +360,21 @@ void HttpConnectionManagerConfig::processFilter(
   filter_factories.push_back(callback);
 }
 
-Http::ServerConnectionPtr HttpConnectionManagerConfig::createCodec(
-    Network::Connection& connection, const Buffer::Instance& data,
-    Http::ServerConnectionCallbacks& callbacks, const bool strict_header_validation) {
+Http::ServerConnectionPtr
+HttpConnectionManagerConfig::createCodec(Network::Connection& connection,
+                                         const Buffer::Instance& data,
+                                         Http::ServerConnectionCallbacks& callbacks) {
   switch (codec_type_) {
   case CodecType::HTTP1:
     return std::make_unique<Http::Http1::ServerConnectionImpl>(
-        connection, callbacks, http1_settings_, maxRequestHeadersKb(), strict_header_validation);
+        connection, callbacks, http1_settings_, maxRequestHeadersKb());
   case CodecType::HTTP2:
     return std::make_unique<Http::Http2::ServerConnectionImpl>(
         connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb());
   case CodecType::AUTO:
-    return Http::ConnectionManagerUtility::autoCreateCodec(
-        connection, data, callbacks, context_.scope(), http1_settings_, http2_settings_,
-        maxRequestHeadersKb(), strict_header_validation);
+    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

@@ -99,8 +99,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   const std::list<AccessLog::InstanceSharedPtr>& accessLogs() override { return access_logs_; }
   Http::ServerConnectionPtr createCodec(Network::Connection& connection,
                                         const Buffer::Instance& data,
-                                        Http::ServerConnectionCallbacks& callbacks,
-                                        const bool strict_header_validation) override;
+                                        Http::ServerConnectionCallbacks& callbacks) override;
   Http::DateProvider& dateProvider() override { return date_provider_; }
   std::chrono::milliseconds drainTimeout() override { return drain_timeout_; }
   FilterChainFactory& filterFactory() override { return *this; }

source/server/http/admin.cc

@@ -1241,11 +1241,10 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server)
 
 Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection,
                                                  const Buffer::Instance& data,
-                                                 Http::ServerConnectionCallbacks& callbacks,
-                                                 const bool strict_header_validation) {
+                                                 Http::ServerConnectionCallbacks& callbacks) {
   return Http::ConnectionManagerUtility::autoCreateCodec(
       connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings(),
-      maxRequestHeadersKb(), strict_header_validation);
+      maxRequestHeadersKb());
 }
 
 bool AdminImpl::createNetworkFilterChain(Network::Connection& connection,

source/server/http/admin.h

@@ -104,8 +104,7 @@ class AdminImpl : public Admin,
   const std::list<AccessLog::InstanceSharedPtr>& accessLogs() override { return access_logs_; }
   Http::ServerConnectionPtr createCodec(Network::Connection& connection,
                                         const Buffer::Instance& data,
-                                        Http::ServerConnectionCallbacks& callbacks,
-                                        const bool strict_header_validation) override;
+                                        Http::ServerConnectionCallbacks& callbacks) override;
   Http::DateProvider& dateProvider() override { return date_provider_; }
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   Http::FilterChainFactory& filterFactory() override { return *this; }

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -72,7 +72,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   // Http::ConnectionManagerConfig
   const std::list<AccessLog::InstanceSharedPtr>& accessLogs() override { return access_logs_; }
   ServerConnectionPtr createCodec(Network::Connection&, const Buffer::Instance&,
-                                  ServerConnectionCallbacks&, const bool) override {
+                                  ServerConnectionCallbacks&) override {
     return ServerConnectionPtr{codec_};
   }
   DateProvider& dateProvider() override { return date_provider_; }