diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 5fb9f24cc7c38..e8198fc8a64ca 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -37,7 +37,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // HTTP connection manager :ref:`configuration overview `. // [#extension: envoy.filters.network.http_connection_manager] -// [#next-free-field: 59] +// [#next-free-field: 60] message HttpConnectionManager { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager"; @@ -289,11 +289,18 @@ message HttpConnectionManager { // HTTP connections will be used for this upgrade type. repeated HttpFilter filters = 2; - // Determines if upgrades are enabled or disabled by default. Defaults to true. + // Determines if upgrades are enabled or disabled by default. Defaults to true, unless + // :ref:`ignore ` + // is enabled. It is invalid to set both this option and ``disabled`` to true. // This can be overridden on a per-route basis with :ref:`cluster // ` as documented in the // :ref:`upgrade documentation `. google.protobuf.BoolValue enabled = 3; + + // If enabled, Envoy will ignore upgrade requests of this type if the request is HTTP/1.1. It is invalid to set both this option and + // :ref:`enabled ` + // to true. + google.protobuf.BoolValue ignore_on_http11 = 4; } // [#not-implemented-hide:] Transformations that apply to path headers. Transformations are applied @@ -787,6 +794,13 @@ message HttpConnectionManager { repeated UpgradeConfig upgrade_configs = 23; + // If enabled, if Envoy receives an Upgrade request from a client on an HTTP/1.1 connection and no configuration for + // the upgrade type is found in + // :ref:`upgrade_configs `, + // Envoy will ignore the upgrade request. If disabled, Envoy will respond with an error and close the connection. + // The default is true. + google.protobuf.BoolValue ignore_unconfigured_http11_upgrades = 59; + // Should paths be normalized according to RFC 3986 before any processing of // requests by HTTP filters or routing? This affects the upstream ``:path`` header // as well. For paths that fail this check, Envoy will respond with 400 to diff --git a/envoy/http/BUILD b/envoy/http/BUILD index 0d4854c722b43..2da3060ec806e 100644 --- a/envoy/http/BUILD +++ b/envoy/http/BUILD @@ -93,6 +93,7 @@ envoy_cc_library( hdrs = ["filter_factory.h"], deps = [ ":header_map_interface", + ":protocol_interface", "//envoy/access_log:access_log_interface", "//envoy/grpc:status", "@com_google_absl//absl/types:optional", diff --git a/envoy/http/filter_factory.h b/envoy/http/filter_factory.h index a53e8977a9955..4b57e67e61b87 100644 --- a/envoy/http/filter_factory.h +++ b/envoy/http/filter_factory.h @@ -4,6 +4,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/http/protocol.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -100,19 +101,26 @@ class FilterChainFactory { createFilterChain(FilterChainManager& manager, bool only_create_if_configured = false, const FilterChainOptions& options = EmptyFilterChainOptions{}) const PURE; + enum class UpgradeAction { + Rejected, + Accepted, + Ignored, + }; + /** * Called when a new upgrade stream is created on the connection. * @param upgrade supplies the upgrade header from downstream * @param per_route_upgrade_map supplies the upgrade map, if any, for this route. * @param manager supplies the "sink" that is used for actually creating the filter chain. @see * FilterChainManager. - * @return true if upgrades of this type are allowed and the filter chain has been created. - * returns false if this upgrade type is not configured, and no filter chain is created. + * @return ACCEPTED if upgrades of this type are allowed and the filter chain has been created. + * returns REJECTED if this upgrade type is not allowed, and no filter chain is created. + * returns IGNORED if the upgrade type should be ignored and no upgrade will take place. */ using UpgradeMap = std::map; - virtual bool createUpgradeFilterChain( + virtual UpgradeAction createUpgradeFilterChain( absl::string_view upgrade, const UpgradeMap* per_route_upgrade_map, - FilterChainManager& manager, + absl::optional http_version, FilterChainManager& manager, const FilterChainOptions& options = EmptyFilterChainOptions{}) const PURE; }; diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index d158ea8088047..b55f36cdd670b 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1623,6 +1623,14 @@ void FilterManager::contextOnContinue(ScopeTrackedObjectStack& tracked_object_st tracked_object_stack.add(filter_manager_callbacks_.scope()); } +namespace { +const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgrade() { + CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet, + Http::Headers::get().ConnectionValues.Upgrade); +} + +} // namespace + bool FilterManager::createFilterChain() { if (state_.created_filter_chain_) { return false; @@ -1647,14 +1655,26 @@ bool FilterManager::createFilterChain() { if (upgrade != nullptr) { const Router::RouteEntry::UpgradeMap* upgrade_map = filter_manager_callbacks_.upgradeMap(); - if (filter_chain_factory_.createUpgradeFilterChain(upgrade->value().getStringView(), - upgrade_map, *this, options)) { + auto result = filter_chain_factory_.createUpgradeFilterChain( + upgrade->value().getStringView(), upgrade_map, streamInfo().protocol(), *this, options); + switch (result) { + case FilterChainFactory::UpgradeAction::Accepted: filter_manager_callbacks_.upgradeFilterChainCreated(); return true; - } else { + case FilterChainFactory::UpgradeAction::Rejected: upgrade_rejected = true; // Fall through to the default filter chain. The function calling this // will send a local reply indicating that the upgrade failed. + break; + case FilterChainFactory::UpgradeAction::Ignored: + // Upgrades can only be ignored for HTTP/1.1. + ASSERT(streamInfo().protocol().has_value() && + *streamInfo().protocol() == Http::Protocol::Http11); + + filter_manager_callbacks_.requestHeaders()->removeUpgrade(); + Http::Utility::removeConnectionUpgrade(filter_manager_callbacks_.requestHeaders().value(), + caseUnorderdSetContainingUpgrade()); + break; } } @@ -1714,11 +1734,11 @@ bool ActiveStreamDecoderFilter::recreateStream(const ResponseHeaderMap* headers) if (headers != nullptr) { // The call to setResponseHeaders is needed to ensure that the headers are properly logged in - // access logs before the stream is destroyed. Since the function expects a ResponseHeaderPtr&&, - // ownership of the headers must be passed. This cannot happen earlier in the flow (such as in - // the call to setupRedirect) because at that point it is still possible for the headers to be - // used in a different logical branch. We work around this by creating a copy and passing - // ownership of the copy instead. + // access logs before the stream is destroyed. Since the function expects a + // ResponseHeaderPtr&&, ownership of the headers must be passed. This cannot happen earlier in + // the flow (such as in the call to setupRedirect) because at that point it is still possible + // for the headers to be used in a different logical branch. We work around this by creating a + // copy and passing ownership of the copy instead. ResponseHeaderMapPtr headers_copy = createHeaderMap(*headers); parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers_copy)); parent_.filter_manager_callbacks_.chargeStats(*headers); diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index aea743c5f67f7..17466bbcf1f37 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -852,16 +852,8 @@ StatusOr ConnectionImpl::onHeadersCompleteImpl() { header_values.UpgradeValues.H2c)) { ENVOY_CONN_LOG(trace, "removing unsupported h2c upgrade headers.", connection_); request_or_response_headers.removeUpgrade(); - if (request_or_response_headers.Connection()) { - const auto& tokens_to_remove = caseUnorderdSetContainingUpgradeAndHttp2Settings(); - std::string new_value = StringUtil::removeTokens( - request_or_response_headers.getConnectionValue(), ",", tokens_to_remove, ","); - if (new_value.empty()) { - request_or_response_headers.removeConnection(); - } else { - request_or_response_headers.setConnection(new_value); - } - } + Utility::removeConnectionUpgrade(request_or_response_headers, + caseUnorderdSetContainingUpgradeAndHttp2Settings()); request_or_response_headers.remove(header_values.Http2Settings); } else { ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 4b37d158f5789..23195ac5cfeb4 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -649,6 +649,19 @@ bool Utility::isWebSocketUpgradeRequest(const RequestHeaderMap& headers) { Http::Headers::get().UpgradeValues.WebSocket)); } +void Utility::removeConnectionUpgrade(RequestOrResponseHeaderMap& headers, + StringUtil::CaseUnorderedSet tokens_to_remove) { + if (headers.Connection()) { + std::string new_value = + StringUtil::removeTokens(headers.getConnectionValue(), ",", tokens_to_remove, ","); + if (new_value.empty()) { + headers.removeConnection(); + } else { + headers.setConnection(new_value); + } + } +} + Utility::PreparedLocalReplyPtr Utility::prepareLocalReply(const EncodeFunctions& encode_functions, const LocalReplyData& local_reply_data) { Code response_code = local_reply_data.response_code_; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index eaee0c34fae7c..374050ec4b2cc 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -315,6 +315,13 @@ bool isH3UpgradeRequest(const RequestHeaderMap& headers); */ bool isWebSocketUpgradeRequest(const RequestHeaderMap& headers); +/** + * Removes `tokens_to_remove` from the `Connection` header, if present and part of a comma separated + * set of values. Removes the `Connection` header if it only contains `tokens_to_remove`. + */ +void removeConnectionUpgrade(RequestOrResponseHeaderMap& headers, + StringUtil::CaseUnorderedSet tokens_to_remove); + struct EncodeFunctions { // Function to modify locally generated response headers. std::function modify_headers_; diff --git a/source/common/router/router.h b/source/common/router/router.h index 730c3a4068c1a..7ea468eb4fe41 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -248,10 +248,12 @@ class FilterConfig : Http::FilterChainFactory { return true; } - bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&, - const Http::FilterChainOptions&) const override { + Http::FilterChainFactory::UpgradeAction + createUpgradeFilterChain(absl::string_view, const UpgradeMap*, absl::optional, + Http::FilterChainManager&, + const Http::FilterChainOptions&) const override { // Upgrade filter chains not yet supported for upstream HTTP filters. - return false; + return FilterChainFactory::UpgradeAction::Rejected; } using HeaderVector = std::vector; diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index f448ece400a97..92dcf0577b2b4 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -1029,10 +1029,12 @@ class ClusterInfoImpl : public ClusterInfo, manager, Http::EmptyFilterChainOptions{}, http_filter_factories_); return true; } - bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&, - const Http::FilterChainOptions&) const override { + Http::FilterChainFactory::UpgradeAction + createUpgradeFilterChain(absl::string_view, const UpgradeMap*, absl::optional, + Http::FilterChainManager&, + const Http::FilterChainOptions&) const override { // Upgrade filter chains not yet supported for upstream HTTP filters. - return false; + return Http::FilterChainFactory::UpgradeAction::Rejected; } Http::Http1::CodecStats& http1CodecStats() const override; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index dbd320f173a60..b421a1989711f 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -364,6 +364,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( internal_address_config_(createInternalAddressConfig(config, creation_status)), xff_num_trusted_hops_(config.xff_num_trusted_hops()), skip_xff_append_(config.skip_xff_append()), via_(config.via()), + ignore_unconfigured_http11_upgrades_( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, ignore_unconfigured_http11_upgrades, true)), scoped_routes_config_provider_manager_(scoped_routes_config_provider_manager), filter_config_provider_manager_(filter_config_provider_manager), http3_options_(Http3::Utility::initializeAndValidateOptions( @@ -729,13 +731,20 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( for (const auto& upgrade_config : config.upgrade_configs()) { const std::string& name = upgrade_config.upgrade_type(); - const bool enabled = upgrade_config.has_enabled() ? upgrade_config.enabled().value() : true; + const bool ignored = PROTOBUF_GET_WRAPPED_OR_DEFAULT(upgrade_config, ignore_on_http11, false); + const bool enabled = PROTOBUF_GET_WRAPPED_OR_DEFAULT(upgrade_config, enabled, !ignored); if (findUpgradeCaseInsensitive(upgrade_filter_factories_, name) != upgrade_filter_factories_.end()) { creation_status = absl::InvalidArgumentError( fmt::format("Error: multiple upgrade configs with the same name: '{}'", name)); return; } + if (enabled && ignored) { + creation_status = absl::InvalidArgumentError(fmt::format( + "Error: upgrade config set both `ignore_on_http11` and `enabled` for upgrade type: '{}'", + name)); + return; + } if (!upgrade_config.filters().empty()) { std::unique_ptr factories = std::make_unique(); Http::DependencyManager upgrade_dependency_manager; @@ -747,11 +756,11 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( SET_AND_RETURN_IF_NOT_OK(status, creation_status); upgrade_filter_factories_.emplace( - std::make_pair(name, FilterConfig{std::move(factories), enabled})); + std::make_pair(name, FilterConfig{std::move(factories), enabled, ignored})); } else { std::unique_ptr factories(nullptr); upgrade_filter_factories_.emplace( - std::make_pair(name, FilterConfig{std::move(factories), enabled})); + std::make_pair(name, FilterConfig{std::move(factories), enabled, ignored})); } } } @@ -796,17 +805,18 @@ bool HttpConnectionManagerConfig::createFilterChain(Http::FilterChainManager& ma return true; } -bool HttpConnectionManagerConfig::createUpgradeFilterChain( +Http::FilterChainFactory::UpgradeAction HttpConnectionManagerConfig::createUpgradeFilterChain( absl::string_view upgrade_type, const Http::FilterChainFactory::UpgradeMap* per_route_upgrade_map, - Http::FilterChainManager& callbacks, const Http::FilterChainOptions& options) const { + absl::optional http_version, Http::FilterChainManager& callbacks, + const Http::FilterChainOptions& options) const { bool route_enabled = false; if (per_route_upgrade_map) { auto route_it = findUpgradeBoolCaseInsensitive(*per_route_upgrade_map, upgrade_type); if (route_it != per_route_upgrade_map->end()) { // Upgrades explicitly not allowed on this route. if (route_it->second == false) { - return false; + return Http::FilterChainFactory::UpgradeAction::Rejected; } // Upgrades explicitly enabled on this route. route_enabled = true; @@ -814,18 +824,40 @@ bool HttpConnectionManagerConfig::createUpgradeFilterChain( } auto it = findUpgradeCaseInsensitive(upgrade_filter_factories_, upgrade_type); - if ((it == upgrade_filter_factories_.end() || !it->second.allow_upgrade) && !route_enabled) { - // Either the HCM disables upgrades and the route-config does not override, - // or neither is configured for this upgrade. - return false; + if (route_enabled || (it != upgrade_filter_factories_.end() && it->second.allow_upgrade)) { + // Either the per-route config enables this upgrade, or the HCM config allows the upgrade. + const FilterFactoriesList* filters_to_use = &filter_factories_; + if (it != upgrade_filter_factories_.end() && it->second.filter_factories != nullptr) { + filters_to_use = it->second.filter_factories.get(); + } + + Http::FilterChainUtility::createFilterChainForFactories(callbacks, options, *filters_to_use); + return Http::FilterChainFactory::UpgradeAction::Accepted; } - const FilterFactoriesList* filters_to_use = &filter_factories_; - if (it != upgrade_filter_factories_.end() && it->second.filter_factories != nullptr) { - filters_to_use = it->second.filter_factories.get(); + + // Upgrades are handled differently depending on the HTTP version. In version 1.1 only, upgrades + // can be ignored by the server and the request processed as if there had not been an upgrade + // request. + const bool can_ignore = (http_version.has_value() && *http_version == Http::Protocol::Http11); + + const bool ignore_specific_upgrade_type = + (it != upgrade_filter_factories_.end() && it->second.ignore_on_http11); + const bool no_specific_config_and_ignore_unknown = + (it == upgrade_filter_factories_.end() && ignore_unconfigured_http11_upgrades_); + + if (can_ignore && (ignore_specific_upgrade_type || no_specific_config_and_ignore_unknown)) { + // Either the HCM ignores all unconfigured upgrades, or this upgrade type is configured to be + // ignored. + ENVOY_LOG(trace, "Ignoring upgrade for requested type: {}", upgrade_type); + return Http::FilterChainFactory::UpgradeAction::Ignored; } - Http::FilterChainUtility::createFilterChainForFactories(callbacks, options, *filters_to_use); - return true; + // Fall-through cases: the HCM does not ignore unconfigured upgrades, or this upgrade type is + // configured to be disallowed. + ASSERT(!can_ignore || !ignore_unconfigured_http11_upgrades_ || + (it != upgrade_filter_factories_.end() && !it->second.ignore_on_http11 && + !it->second.allow_upgrade)); + return Http::FilterChainFactory::UpgradeAction::Rejected; } const Network::Address::Instance& HttpConnectionManagerConfig::localAddress() { diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 31ce08a817280..211a35f206caf 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -144,12 +144,15 @@ class HttpConnectionManagerConfig : Logger::Loggable, using FilterFactoriesList = Envoy::Http::FilterChainUtility::FilterFactoriesList; struct FilterConfig { std::unique_ptr filter_factories; - bool allow_upgrade; + const bool allow_upgrade; + const bool ignore_on_http11; }; - bool createUpgradeFilterChain(absl::string_view upgrade_type, - const Http::FilterChainFactory::UpgradeMap* per_route_upgrade_map, - Http::FilterChainManager& manager, - const Http::FilterChainOptions& options) const override; + Http::FilterChainFactory::UpgradeAction + createUpgradeFilterChain(absl::string_view upgrade_type, + const Http::FilterChainFactory::UpgradeMap* per_route_upgrade_map, + absl::optional http_version, + Http::FilterChainManager& manager, + const Http::FilterChainOptions& options) const override; // Http::ConnectionManagerConfig const Http::RequestIDExtensionSharedPtr& requestIDExtension() override { @@ -307,6 +310,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const uint32_t xff_num_trusted_hops_; const bool skip_xff_append_; const std::string via_; + const bool ignore_unconfigured_http11_upgrades_; Http::ForwardClientCertType forward_client_cert_; std::vector set_current_client_cert_details_; Config::ConfigProviderManager* scoped_routes_config_provider_manager_; diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index dcf07ad8eeccb..7bde1080867f2 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -117,10 +117,11 @@ class AdminImpl : public Admin, // Http::FilterChainFactory bool createFilterChain(Http::FilterChainManager& manager, bool, const Http::FilterChainOptions&) const override; - bool createUpgradeFilterChain(absl::string_view, const Http::FilterChainFactory::UpgradeMap*, - Http::FilterChainManager&, - const Http::FilterChainOptions&) const override { - return false; + Http::FilterChainFactory::UpgradeAction + createUpgradeFilterChain(absl::string_view, const Http::FilterChainFactory::UpgradeMap*, + absl::optional, Http::FilterChainManager&, + const Http::FilterChainOptions&) const override { + return Http::FilterChainFactory::UpgradeAction::Ignored; } // Http::ConnectionManagerConfig diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 2a5b9da2cfc50..5837b101d5804 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -94,12 +94,14 @@ class FuzzConfig : public ConnectionManagerConfig { callbacks.streamInfo().setResponseCodeDetails(""); })); EXPECT_CALL(*encoder_filter_, setEncoderFilterCallbacks(_)); - EXPECT_CALL(filter_factory_, createUpgradeFilterChain(_, _, _, _)) - .WillRepeatedly( - Invoke([&](absl::string_view, const Http::FilterChainFactory::UpgradeMap*, - FilterChainManager& manager, const Http::FilterChainOptions&) -> bool { - return filter_factory_.createFilterChain(manager); - })); + EXPECT_CALL(filter_factory_, createUpgradeFilterChain(_, _, _, _, _)) + .WillRepeatedly(Invoke([&](absl::string_view, const Http::FilterChainFactory::UpgradeMap*, + absl::optional, FilterChainManager& manager, + const Http::FilterChainOptions&) { + return filter_factory_.createFilterChain(manager) + ? Http::FilterChainFactory::UpgradeAction::Accepted + : Http::FilterChainFactory::UpgradeAction::Rejected; + })); } Http::ForwardClientCertType diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 44a1dacf48253..96c9df1930cb4 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2570,14 +2570,15 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogOnTunnelEstablished) { std::shared_ptr filter(new NiceMock()); std::shared_ptr handler(new NiceMock()); - EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _)) + EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _, _)) .WillOnce(Invoke([&](absl::string_view, const FilterChainFactory::UpgradeMap*, - FilterChainManager& manager, const Http::FilterChainOptions&) -> bool { + absl::optional, FilterChainManager& manager, + const Http::FilterChainOptions&) { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); - return true; + return Http::FilterChainFactory::UpgradeAction::Accepted; })); flush_log_on_tunnel_successfully_established_ = true; @@ -4081,14 +4082,14 @@ TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); - EXPECT_CALL(filter_factory_, createUpgradeFilterChain(_, _, _, _)) - .WillRepeatedly( - Invoke([&](absl::string_view, const Http::FilterChainFactory::UpgradeMap*, - FilterChainManager& manager, const Http::FilterChainOptions&) -> bool { - auto factory = createStreamFilterFactoryCb(StreamFilterSharedPtr{filter}); - manager.applyFilterFactoryCb({}, factory); - return true; - })); + EXPECT_CALL(filter_factory_, createUpgradeFilterChain(_, _, _, _, _)) + .WillRepeatedly(Invoke([&](absl::string_view, const Http::FilterChainFactory::UpgradeMap*, + absl::optional, FilterChainManager& manager, + const Http::FilterChainOptions&) { + auto factory = createStreamFilterFactoryCb(StreamFilterSharedPtr{filter}); + manager.applyFilterFactoryCb({}, factory); + return Http::FilterChainFactory::UpgradeAction::Accepted; + })); // When dispatch is called on the codec, we pretend to get a new stream and then fire a headers // only request into it. Then we respond into the filter. @@ -4125,8 +4126,8 @@ TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) { TEST_F(HttpConnectionManagerImplTest, ConnectAsUpgrade) { setup(SetupOpts().setTracing(false)); - EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _)) - .WillRepeatedly(Return(true)); + EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _, _)) + .WillRepeatedly(Return(Http::FilterChainFactory::UpgradeAction::Accepted)); EXPECT_CALL(*codec_, dispatch(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { @@ -4149,8 +4150,8 @@ TEST_F(HttpConnectionManagerImplTest, ConnectAsUpgrade) { TEST_F(HttpConnectionManagerImplTest, ConnectWithEmptyPath) { setup(SetupOpts().setTracing(false)); - EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _)) - .WillRepeatedly(Return(true)); + EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _, _)) + .WillRepeatedly(Return(Http::FilterChainFactory::UpgradeAction::Accepted)); EXPECT_CALL(*codec_, dispatch(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index a48830886f146..66e3f59efffc0 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -6251,7 +6251,9 @@ TEST_F(ClusterInfoImplTest, FilterChain) { auto cluster = makeCluster(yaml); Http::MockFilterChainManager manager; const Http::EmptyFilterChainOptions options; - EXPECT_FALSE(cluster->info()->createUpgradeFilterChain("foo", nullptr, manager, options)); + EXPECT_EQ( + cluster->info()->createUpgradeFilterChain("foo", nullptr, absl::nullopt, manager, options), + Http::FilterChainFactory::UpgradeAction::Rejected); EXPECT_CALL(manager, applyFilterFactoryCb(_, _)); cluster->info()->createFilterChain(manager); diff --git a/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc b/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc index 29048d827af6f..99d2be487bcf9 100644 --- a/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc @@ -194,7 +194,9 @@ TEST_F(FilterChainTest, CreateUpgradeFilterChain) { { EXPECT_CALL(manager.callbacks_, addStreamFilter(_)); // Buffer EXPECT_CALL(manager.callbacks_, addStreamDecoderFilter(_)); // Router - EXPECT_TRUE(config.createUpgradeFilterChain("WEBSOCKET", nullptr, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Accepted, + config.createUpgradeFilterChain("WEBSOCKET", nullptr, absl::nullopt, manager, options)); } // Check the case where WebSockets are configured in the HCM, and no router @@ -202,7 +204,8 @@ TEST_F(FilterChainTest, CreateUpgradeFilterChain) { { EXPECT_CALL(manager.callbacks_, addStreamFilter(_)).Times(0); EXPECT_CALL(manager.callbacks_, addStreamDecoderFilter(_)).Times(0); - EXPECT_FALSE(config.createUpgradeFilterChain("foo", nullptr, manager, options)); + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("foo", nullptr, absl::nullopt, manager, options)); } // Now override the HCM with a route-specific disabling of WebSocket to @@ -210,7 +213,9 @@ TEST_F(FilterChainTest, CreateUpgradeFilterChain) { { std::map upgrade_map; upgrade_map.emplace(std::make_pair("WebSocket", false)); - EXPECT_FALSE(config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, manager, options)); + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, absl::nullopt, manager, + options)); } // For paranoia's sake make sure route-specific enabling doesn't break @@ -220,7 +225,9 @@ TEST_F(FilterChainTest, CreateUpgradeFilterChain) { EXPECT_CALL(manager.callbacks_, addStreamDecoderFilter(_)); // Router std::map upgrade_map; upgrade_map.emplace(std::make_pair("WebSocket", true)); - EXPECT_TRUE(config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, manager, options)); + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Accepted, + config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, absl::nullopt, manager, + options)); } } @@ -240,20 +247,28 @@ TEST_F(FilterChainTest, CreateUpgradeFilterChainHCMDisabled) { const Http::EmptyFilterChainOptions options; // Check the case where WebSockets are off in the HCM, and no router config is present. - { EXPECT_FALSE(config.createUpgradeFilterChain("WEBSOCKET", nullptr, manager, options)); } + { + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("WEBSOCKET", nullptr, absl::nullopt, manager, options)); + } // Check the case where WebSockets are off in the HCM and in router config. { std::map upgrade_map; upgrade_map.emplace(std::make_pair("WebSocket", false)); - EXPECT_FALSE(config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, manager, options)); + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, absl::nullopt, manager, + options)); } // With a route-specific enabling for WebSocket, WebSocket should work. { std::map upgrade_map; upgrade_map.emplace(std::make_pair("WebSocket", true)); - EXPECT_TRUE(config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, manager, options)); + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Accepted, + config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, absl::nullopt, manager, + options)); } // With only a route-config we should do what the route config says. @@ -261,10 +276,111 @@ TEST_F(FilterChainTest, CreateUpgradeFilterChainHCMDisabled) { std::map upgrade_map; upgrade_map.emplace(std::make_pair("foo", true)); upgrade_map.emplace(std::make_pair("bar", false)); - EXPECT_TRUE(config.createUpgradeFilterChain("foo", &upgrade_map, manager, options)); - EXPECT_FALSE(config.createUpgradeFilterChain("bar", &upgrade_map, manager, options)); - EXPECT_FALSE(config.createUpgradeFilterChain("eep", &upgrade_map, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Accepted, + config.createUpgradeFilterChain("foo", &upgrade_map, absl::nullopt, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("bar", &upgrade_map, absl::nullopt, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("eep", &upgrade_map, absl::nullopt, manager, options)); + } +} + +TEST_F(FilterChainTest, CreateUpgradeFilterChainIgnoreUpgrades) { + auto hcm_config = parseHttpConnectionManagerFromYaml(basic_config_); + auto* upgrade_ignore = hcm_config.add_upgrade_configs(); + upgrade_ignore->set_upgrade_type("ignore"); + upgrade_ignore->mutable_ignore_on_http11()->set_value(true); + auto* upgrade_reject = hcm_config.add_upgrade_configs(); + upgrade_reject->set_upgrade_type("reject"); + upgrade_reject->mutable_enabled()->set_value(false); + + HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, + route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + + NiceMock manager; + const Http::EmptyFilterChainOptions options; + + // Reject configured upgrade when protocol isn't HTTP/1.1. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("ignore", nullptr, Http::Protocol::Http10, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("ignore", nullptr, Http::Protocol::Http2, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("ignore", nullptr, Http::Protocol::Http3, manager, options)); + + // Ignore configured upgrade when protocol is HTTP/1.1. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Ignored, + config.createUpgradeFilterChain("ignore", nullptr, Http::Protocol::Http11, manager, options)); + + // A route-specific override causes the type to be rejected instead of ignored. + { + std::map upgrade_map{{"ignore", false}}; + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("ignore", &upgrade_map, Http::Protocol::Http11, + manager, options)); } + + // Reject unknown upgrade when protocol isn't HTTP/1.1. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("unknown", nullptr, Http::Protocol::Http2, manager, options)); + + // Ignore unknown upgrade when protocol is HTTP/1.1. + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Ignored, + config.createUpgradeFilterChain("unknown", nullptr, Http::Protocol::Http11, manager, + options)); + + // Reject a disabled upgrade type when unknown types are ignored. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("reject", nullptr, Http::Protocol::Http11, manager, options)); +} + +TEST_F(FilterChainTest, CreateUpgradeFilterChainIgnoreUpgradesUnknownDisabled) { + auto hcm_config = parseHttpConnectionManagerFromYaml(basic_config_); + hcm_config.mutable_ignore_unconfigured_http11_upgrades()->set_value(false); + auto* upgrade_ignore = hcm_config.add_upgrade_configs(); + upgrade_ignore->set_upgrade_type("ignore"); + upgrade_ignore->mutable_ignore_on_http11()->set_value(true); + + HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, + route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + + NiceMock manager; + const Http::EmptyFilterChainOptions options; + + // Reject configured upgrade when protocol isn't HTTP/1.1. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("ignore", nullptr, Http::Protocol::Http2, manager, options)); + + // Ignore configured upgrade when protocol is HTTP/1.1. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Ignored, + config.createUpgradeFilterChain("ignore", nullptr, Http::Protocol::Http11, manager, options)); + + // Reject unknown upgrade when protocol isn't HTTP/1.1. + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("unknown", nullptr, Http::Protocol::Http2, manager, options)); + + // Reject unknown upgrade when protocol is HTTP/1.1. + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Rejected, + config.createUpgradeFilterChain("unknown", nullptr, Http::Protocol::Http11, manager, + options)); } TEST_F(FilterChainTest, CreateCustomUpgradeFilterChain) { @@ -306,14 +422,17 @@ TEST_F(FilterChainTest, CreateCustomUpgradeFilterChain) { { NiceMock manager; EXPECT_CALL(manager.callbacks_, addStreamDecoderFilter(_)); - EXPECT_TRUE(config.createUpgradeFilterChain("websocket", nullptr, manager, options)); + EXPECT_EQ( + Http::FilterChainFactory::UpgradeAction::Accepted, + config.createUpgradeFilterChain("websocket", nullptr, absl::nullopt, manager, options)); } { NiceMock manager; EXPECT_CALL(manager.callbacks_, addStreamDecoderFilter(_)); EXPECT_CALL(manager.callbacks_, addStreamFilter(_)).Times(2); // Buffer - EXPECT_TRUE(config.createUpgradeFilterChain("Foo", nullptr, manager, options)); + EXPECT_EQ(Http::FilterChainFactory::UpgradeAction::Accepted, + config.createUpgradeFilterChain("Foo", nullptr, absl::nullopt, manager, options)); } } @@ -358,6 +477,21 @@ TEST_F(FilterChainTest, InvalidConfig) { "Error: multiple upgrade configs with the same name: 'websocket'"); } +TEST_F(FilterChainTest, UpgradeSettingsConflict) { + auto hcm_config = parseHttpConnectionManagerFromYaml(basic_config_); + auto* upgrade = hcm_config.add_upgrade_configs(); + upgrade->set_upgrade_type("websocket"); + upgrade->mutable_ignore_on_http11()->set_value(true); + upgrade->mutable_enabled()->set_value(true); + + HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, + route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + EXPECT_EQ(creation_status_.message(), "Error: upgrade config set both `ignore_on_http11` and " + "`enabled` for upgrade type: 'websocket'"); +} + } // namespace } // namespace HttpConnectionManager } // namespace NetworkFilters diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index a6119814f2be4..ec97470ec07e1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -5485,4 +5485,30 @@ TEST_P(DownstreamProtocolIntegrationTest, UnknownPseudoHeader) { } } +TEST_P(DownstreamProtocolIntegrationTest, UnknownHttpUpgrade) { + disable_client_header_validation_ = true; + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + Envoy::LogLevelSetter save_levels(spdlog::level::trace); + + // Start the request. + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"connection", "upgrade"}, + {"upgrade", "unknown-type"}}); + + if (downstream_protocol_ == Http::CodecType::HTTP1) { + waitForNextUpstreamRequest(); + EXPECT_EQ(nullptr, upstream_request_->headers().Upgrade()); + EXPECT_EQ(nullptr, upstream_request_->headers().Connection()); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + EXPECT_TRUE(response->waitForEndStream()); + } else { + EXPECT_TRUE(response->waitForReset()); + } +} + } // namespace Envoy diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 63b2163fbb89f..2d64521267723 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -207,9 +207,10 @@ class MockFilterChainFactory : public FilterChainFactory { return createFilterChain(manager); } MOCK_METHOD(bool, createFilterChain, (FilterChainManager & manager), (const)); - MOCK_METHOD(bool, createUpgradeFilterChain, + MOCK_METHOD(Http::FilterChainFactory::UpgradeAction, createUpgradeFilterChain, (absl::string_view upgrade_type, const FilterChainFactory::UpgradeMap* upgrade_map, - FilterChainManager& manager, const FilterChainOptions&), + absl::optional, FilterChainManager& manager, + const FilterChainOptions&), (const)); }; diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 978f9c5826aa2..7a3b571961fa9 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -164,10 +164,11 @@ class MockClusterInfo : public ClusterInfo { (Http::FilterChainManager & manager, bool only_create_if_configured, const Http::FilterChainOptions& options), (const, override)); - MOCK_METHOD(bool, createUpgradeFilterChain, + MOCK_METHOD(Http::FilterChainFactory::UpgradeAction, createUpgradeFilterChain, (absl::string_view upgrade_type, const Http::FilterChainFactory::UpgradeMap* upgrade_map, - Http::FilterChainManager& manager, const Http::FilterChainOptions&), + absl::optional http_version, Http::FilterChainManager& manager, + const Http::FilterChainOptions&), (const)); MOCK_METHOD(Http::ClientHeaderValidatorPtr, makeHeaderValidator, (Http::Protocol), (const)); MOCK_METHOD(