diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index fbfd421b0220a..6c5488ef78836 100644 --- a/source/common/http/codec_client.cc +++ b/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(*connection_, *this, - strict_header_validation); + codec_ = std::make_unique(*connection_, *this); break; } case Type::HTTP2: { diff --git a/source/common/http/codec_client.h b/source/common/http/codec_client.h index cc27050e8fbe9..b9cf3482dce5d 100644 --- a/source/common/http/codec_client.h +++ b/source/common/http/codec_client.h @@ -241,8 +241,7 @@ using CodecClientPtr = std::unique_ptr; 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 diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 78ebc7a19161c..5e08e267d5b92 100644 --- a/source/common/http/conn_manager_config.h +++ b/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 diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 998642cd2cd6f..1e9bbbfedc581 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/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(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 7b3a650d26dde..a72dab9069cda 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -679,8 +679,6 @@ class ConnectionManagerImpl : Logger::Loggable, 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 diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 6ec1bd5b91161..acc258499e569 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/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(connection, callbacks, scope, http2_settings, max_request_headers_kb); } else { - return std::make_unique( - connection, callbacks, http1_settings, max_request_headers_kb, strict_header_validation); + return std::make_unique(connection, callbacks, http1_settings, + max_request_headers_kb); } } diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index c203fb154717a..802d0ef07addb 100644 --- a/source/common/http/conn_manager_utility.h +++ b/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 diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index 22f47ba2f31f0..98ddb6edb4849 100644 --- a/source/common/http/http1/BUILD +++ b/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", ], ) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 863606b32c6c2..255de29a6a1e4 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/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_) || diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index a15bdbe913ff4..c1a9fa5d7e93c 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -174,8 +174,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(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) { diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index f5596e83bfe0a..04edfdba79c44 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/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( - connection, callbacks, http1_settings_, maxRequestHeadersKb(), strict_header_validation); + connection, callbacks, http1_settings_, maxRequestHeadersKb()); case CodecType::HTTP2: return std::make_unique( 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; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index eb97a9bf21dbe..796b67fc97286 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -99,8 +99,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const std::list& 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; } diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 17e655af186f6..b7e8a22cebbbe 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -1242,11 +1242,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, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 9d1da1e8700da..ac0f6af807bb0 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -105,8 +105,7 @@ class AdminImpl : public Admin, const std::list& 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; } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index e15e88e97473b..3c11424b59648 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -72,7 +72,7 @@ class FuzzConfig : public ConnectionManagerConfig { // Http::ConnectionManagerConfig const std::list& 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_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 7f2abde9ba580..c635e59c69282 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -241,7 +241,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan // Http::ConnectionManagerConfig const std::list& 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_; } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 48fa93d141743..46937cf17ab0b 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -43,15 +43,13 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { // Http::ConnectionManagerConfig ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& instance, - ServerConnectionCallbacks& callbacks, - const bool strict_header_validation) override { - return ServerConnectionPtr{ - createCodec_(connection, instance, callbacks, strict_header_validation)}; + ServerConnectionCallbacks& callbacks) override { + return ServerConnectionPtr{createCodec_(connection, instance, callbacks)}; } MOCK_METHOD0(accessLogs, const std::list&()); - MOCK_METHOD4(createCodec_, ServerConnection*(Network::Connection&, const Buffer::Instance&, - ServerConnectionCallbacks&, const bool)); + MOCK_METHOD3(createCodec_, ServerConnection*(Network::Connection&, const Buffer::Instance&, + ServerConnectionCallbacks&)); MOCK_METHOD0(dateProvider, DateProvider&()); MOCK_METHOD0(drainTimeout, std::chrono::milliseconds()); MOCK_METHOD0(filterFactory, FilterChainFactory&()); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index a4ceb41c5836e..fec552d6f3f84 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -49,11 +49,8 @@ class Http1ServerConnectionImplTest : public testing::Test { } void initialize() { - strict_header_validation_ = - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation"); - codec_ = - std::make_unique(connection_, callbacks_, codec_settings_, - max_request_headers_kb_, strict_header_validation_); + codec_ = std::make_unique(connection_, callbacks_, codec_settings_, + max_request_headers_kb_); } NiceMock connection_; @@ -67,7 +64,6 @@ class Http1ServerConnectionImplTest : public testing::Test { protected: uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; - bool strict_header_validation_; Event::MockDispatcher dispatcher_; NiceMock tls_; Stats::IsolatedStoreImpl store_; @@ -88,9 +84,8 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur if (allow_absolute_url) { codec_settings_.allow_absolute_url_ = allow_absolute_url; - codec_ = - std::make_unique(connection_, callbacks_, codec_settings_, - max_request_headers_kb_, strict_header_validation_); + codec_ = std::make_unique(connection_, callbacks_, codec_settings_, + max_request_headers_kb_); } Http::MockStreamDecoder decoder; @@ -109,9 +104,8 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs // Make a new 'codec' with the right settings if (allow_absolute_url) { codec_settings_.allow_absolute_url_ = allow_absolute_url; - codec_ = - std::make_unique(connection_, callbacks_, codec_settings_, - max_request_headers_kb_, strict_header_validation_); + codec_ = std::make_unique(connection_, callbacks_, codec_settings_, + max_request_headers_kb_); } Http::MockStreamDecoder decoder; @@ -866,12 +860,7 @@ class Http1ClientConnectionImplTest : public testing::Test { generator_, validation_visitor_, *api_)}); } - void initialize() { - strict_header_validation_ = - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation"); - codec_ = - std::make_unique(connection_, callbacks_, strict_header_validation_); - } + void initialize() { codec_ = std::make_unique(connection_, callbacks_); } NiceMock connection_; NiceMock callbacks_; @@ -887,7 +876,6 @@ class Http1ClientConnectionImplTest : public testing::Test { Init::MockManager init_manager_; NiceMock validation_visitor_; std::unique_ptr loader_; - bool strict_header_validation_; }; TEST_F(Http1ClientConnectionImplTest, SimpleGet) { diff --git a/test/integration/BUILD b/test/integration/BUILD index 64949483c84ea..702ed04a4fa49 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -418,6 +418,7 @@ envoy_cc_test_library( "//source/common/network:filter_lib", "//source/common/network:listen_socket_lib", "//source/common/network:utility_lib", + "//source/common/runtime:runtime_lib", "//source/common/stats:isolated_store_lib", "//source/common/stats:thread_local_store_lib", "//source/common/thread_local:thread_local_lib", diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index eec74c73d4523..fbd7308fd4a64 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -62,8 +62,8 @@ typeToCodecType(Http::CodecClient::Type type) { IntegrationCodecClient::IntegrationCodecClient( Event::Dispatcher& dispatcher, Network::ClientConnectionPtr&& conn, Upstream::HostDescriptionConstSharedPtr host_description, CodecClient::Type type) - : CodecClientProd(type, std::move(conn), host_description, dispatcher, false), - dispatcher_(dispatcher), callbacks_(*this), codec_callbacks_(*this) { + : CodecClientProd(type, std::move(conn), host_description, dispatcher), dispatcher_(dispatcher), + callbacks_(*this), codec_callbacks_(*this) { connection_->addConnectionCallbacks(callbacks_); setCodecConnectionCallbacks(codec_callbacks_); dispatcher.run(Event::Dispatcher::RunType::Block); diff --git a/test/integration/utility.cc b/test/integration/utility.cc index 59b336b38d6d9..e0953f3540015 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -76,7 +76,7 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt type, dispatcher->createClientConnection(addr, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr), - host_description, *dispatcher, false); + host_description, *dispatcher); BufferingStreamDecoderPtr response(new BufferingStreamDecoder([&]() -> void { client.close(); dispatcher->exit();