From 0898c3e187f16e8ec297e23808a86268c40d568e Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 29 Mar 2021 00:26:05 -0400 Subject: [PATCH 01/13] make test pass Signed-off-by: Dan Zhang --- api/envoy/config/core/v3/protocol.proto | 12 +++- api/envoy/config/core/v4alpha/protocol.proto | 12 +++- .../v3/http_connection_manager.proto | 6 +- .../v4alpha/http_connection_manager.proto | 6 +- .../envoy/config/core/v3/protocol.proto | 12 +++- .../envoy/config/core/v4alpha/protocol.proto | 12 +++- .../v3/http_connection_manager.proto | 6 +- .../v4alpha/http_connection_manager.proto | 6 +- source/common/http/codec_client.cc | 3 +- source/common/http/http3/BUILD | 1 + source/common/http/http3/quic_codec_factory.h | 13 +++- source/common/http/utility.cc | 18 ++++++ source/common/http/utility.h | 8 +++ .../network/http_connection_manager/config.cc | 6 +- .../network/http_connection_manager/config.h | 1 + source/extensions/quic_listeners/quiche/BUILD | 3 + .../quic_listeners/quiche/codec_impl.cc | 31 +++++++--- .../quic_listeners/quiche/codec_impl.h | 26 +++++--- .../quiche/envoy_quic_client_session.cc | 3 +- .../quiche/envoy_quic_client_stream.cc | 18 +++--- .../quiche/envoy_quic_client_stream.h | 6 +- .../quiche/envoy_quic_server_session.cc | 8 ++- .../quiche/envoy_quic_server_session.h | 9 +++ .../quiche/envoy_quic_server_stream.cc | 61 ++++++++++++++++--- .../quiche/envoy_quic_server_stream.h | 24 +++++++- .../quic_listeners/quiche/envoy_quic_stream.h | 57 ++++++++++++++++- .../quic_listeners/quiche/envoy_quic_utils.h | 39 +++++++++--- .../quic_filter_manager_connection_impl.h | 11 +++- test/integration/fake_upstream.cc | 6 +- test/integration/fake_upstream.h | 3 + test/integration/protocol_integration_test.cc | 9 +++ 31 files changed, 362 insertions(+), 74 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 8c7693c8ab17..d9bbd47c7c6d 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -409,9 +409,15 @@ message GrpcProtocolOptions { // [#not-implemented-hide:] // -// A message which allows using HTTP/3 as an upstream protocol. -// -// Eventually this will include configuration for tuning HTTP/3. +// A message which allows using HTTP/3. message Http3ProtocolOptions { QuicProtocolOptions quic_protocol_options = 1; + + // Allows invalid HTTP messaging and headers. When this option is disabled (default), then + // the whole HTTP/3 connection is terminated upon receiving invalid HEADERS frame. However, + // when this option is enabled, only the offending stream is terminated. + // + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2; } diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 1f0af4d12922..99c681701d11 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -405,12 +405,18 @@ message GrpcProtocolOptions { // [#not-implemented-hide:] // -// A message which allows using HTTP/3 as an upstream protocol. -// -// Eventually this will include configuration for tuning HTTP/3. +// A message which allows using HTTP/3. message Http3ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.Http3ProtocolOptions"; QuicProtocolOptions quic_protocol_options = 1; + + // Allows invalid HTTP messaging and headers. When this option is disabled (default), then + // the whole HTTP/3 connection is terminated upon receiving invalid HEADERS frame. However, + // when this option is enabled, only the offending stream is terminated. + // + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2; } 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 fc062b6dcaa7..cac1353b5a5b 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 @@ -34,7 +34,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: 44] +// [#next-free-field: 45] message HttpConnectionManager { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager"; @@ -325,6 +325,10 @@ message HttpConnectionManager { config.core.v3.Http2ProtocolOptions http2_protocol_options = 9 [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + config.core.v3.Http3ProtocolOptions http3_protocol_options = 44 + [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. string server_name = 10 diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 0c6ee412317c..a95e20a39025 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -33,7 +33,7 @@ option (udpa.annotations.file_status).package_version_status = NEXT_MAJOR_VERSIO // HTTP connection manager :ref:`configuration overview `. // [#extension: envoy.filters.network.http_connection_manager] -// [#next-free-field: 44] +// [#next-free-field: 45] message HttpConnectionManager { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager"; @@ -328,6 +328,10 @@ message HttpConnectionManager { config.core.v4alpha.Http2ProtocolOptions http2_protocol_options = 9 [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44 + [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. string server_name = 10 diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 8c7693c8ab17..d9bbd47c7c6d 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -409,9 +409,15 @@ message GrpcProtocolOptions { // [#not-implemented-hide:] // -// A message which allows using HTTP/3 as an upstream protocol. -// -// Eventually this will include configuration for tuning HTTP/3. +// A message which allows using HTTP/3. message Http3ProtocolOptions { QuicProtocolOptions quic_protocol_options = 1; + + // Allows invalid HTTP messaging and headers. When this option is disabled (default), then + // the whole HTTP/3 connection is terminated upon receiving invalid HEADERS frame. However, + // when this option is enabled, only the offending stream is terminated. + // + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2; } diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index 131553b755df..ac74ccfe0217 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -415,12 +415,18 @@ message GrpcProtocolOptions { // [#not-implemented-hide:] // -// A message which allows using HTTP/3 as an upstream protocol. -// -// Eventually this will include configuration for tuning HTTP/3. +// A message which allows using HTTP/3. message Http3ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.Http3ProtocolOptions"; QuicProtocolOptions quic_protocol_options = 1; + + // Allows invalid HTTP messaging and headers. When this option is disabled (default), then + // the whole HTTP/3 connection is terminated upon receiving invalid HEADERS frame. However, + // when this option is enabled, only the offending stream is terminated. + // + // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging + // `. + google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2; } diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 5253b167c924..9cb416365056 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -36,7 +36,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: 44] +// [#next-free-field: 45] message HttpConnectionManager { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager"; @@ -329,6 +329,10 @@ message HttpConnectionManager { config.core.v3.Http2ProtocolOptions http2_protocol_options = 9 [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + config.core.v3.Http3ProtocolOptions http3_protocol_options = 44 + [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. string server_name = 10 diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 0c6ee412317c..a95e20a39025 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -33,7 +33,7 @@ option (udpa.annotations.file_status).package_version_status = NEXT_MAJOR_VERSIO // HTTP connection manager :ref:`configuration overview `. // [#extension: envoy.filters.network.http_connection_manager] -// [#next-free-field: 44] +// [#next-free-field: 45] message HttpConnectionManager { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager"; @@ -328,6 +328,10 @@ message HttpConnectionManager { config.core.v4alpha.Http2ProtocolOptions http2_protocol_options = 9 [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44 + [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. string server_name = 10 diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index f6d457403cb7..c41826701d9b 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -185,7 +185,8 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne codec_ = std::unique_ptr( Config::Utility::getAndCheckFactoryByName( Http::QuicCodecNames::get().Quiche) - .createQuicClientConnection(*connection_, *this)); + .createQuicClientConnection(*connection_, *this, host->cluster().http3Options(), + Http::DEFAULT_MAX_REQUEST_HEADERS_KB)); break; } } diff --git a/source/common/http/http3/BUILD b/source/common/http/http3/BUILD index 112ca100d629..8a2dd3fc89d8 100644 --- a/source/common/http/http3/BUILD +++ b/source/common/http/http3/BUILD @@ -28,6 +28,7 @@ envoy_cc_library( "//include/envoy/config:typed_config_interface", "//include/envoy/http:codec_interface", "//include/envoy/network:connection_interface", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/source/common/http/http3/quic_codec_factory.h b/source/common/http/http3/quic_codec_factory.h index 0b4a72404200..07a8357088e9 100644 --- a/source/common/http/http3/quic_codec_factory.h +++ b/source/common/http/http3/quic_codec_factory.h @@ -2,6 +2,7 @@ #include +#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/config/typed_config.h" #include "envoy/http/codec.h" #include "envoy/network/connection.h" @@ -14,8 +15,12 @@ class QuicHttpServerConnectionFactory : public Config::UntypedFactory { public: ~QuicHttpServerConnectionFactory() override = default; - virtual std::unique_ptr - createQuicServerConnection(Network::Connection& connection, ConnectionCallbacks& callbacks) PURE; + virtual std::unique_ptr createQuicServerConnection( + Network::Connection& connection, ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) PURE; std::string category() const override { return "envoy.quic_client_codec"; } }; @@ -26,7 +31,9 @@ class QuicHttpClientConnectionFactory : public Config::UntypedFactory { ~QuicHttpClientConnectionFactory() override = default; virtual std::unique_ptr - createQuicClientConnection(Network::Connection& connection, ConnectionCallbacks& callbacks) PURE; + createQuicClientConnection(Network::Connection& connection, ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb) PURE; std::string category() const override { return "envoy.quic_server_codec"; } }; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e2a39c3aaf51..d83c6d36f9c8 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -223,6 +223,24 @@ initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions } // namespace Utility } // namespace Http2 +namespace Http3 { +namespace Utility { +envoy::config::core::v3::Http3ProtocolOptions +initializeAndValidateOptions(const envoy::config::core::v3::Http3ProtocolOptions& options, + bool hcm_stream_error_set, + const Protobuf::BoolValue& hcm_stream_error) { + envoy::config::core::v3::Http3ProtocolOptions options_clone(options); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.hcm_stream_error_on_invalid_message") && + !options.has_override_stream_error_on_invalid_http_message() && hcm_stream_error_set) { + options_clone.mutable_override_stream_error_on_invalid_http_message()->set_value( + hcm_stream_error.value()); + } + return options_clone; +} + +} // namespace Utility +} // namespace Http3 namespace Http { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 7d2a065bcb37..adf48e2ad1e4 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -122,7 +122,15 @@ initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions const Protobuf::BoolValue& hcm_stream_error); } // namespace Utility } // namespace Http2 +namespace Http3 { +namespace Utility { +envoy::config::core::v3::Http3ProtocolOptions +initializeAndValidateOptions(const envoy::config::core::v3::Http3ProtocolOptions& options, + bool hcm_stream_error_set, + const Protobuf::BoolValue& hcm_stream_error); +} // namespace Utility +} // namespace Http3 namespace Http { namespace Utility { diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 6475de5f7ce1..3a0fb5b91fd3 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -204,6 +204,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( route_config_provider_manager_(route_config_provider_manager), scoped_routes_config_provider_manager_(scoped_routes_config_provider_manager), filter_config_provider_manager_(filter_config_provider_manager), + http3_options_(Http3::Utility::initializeAndValidateOptions( + config.http3_protocol_options(), config.has_stream_error_on_invalid_http_message(), + config.stream_error_on_invalid_http_message())), http2_options_(Http2::Utility::initializeAndValidateOptions( config.http2_protocol_options(), config.has_stream_error_on_invalid_http_message(), config.stream_error_on_invalid_http_message())), @@ -582,7 +585,8 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, return std::unique_ptr( Config::Utility::getAndCheckFactoryByName( Http::QuicCodecNames::get().Quiche) - .createQuicServerConnection(connection, callbacks)); + .createQuicServerConnection(connection, callbacks, http3_options_, + maxRequestHeadersKb(), headersWithUnderscoresAction())); case CodecType::AUTO: return Http::ConnectionManagerUtility::autoCreateCodec( connection, data, callbacks, context_.scope(), context_.api().randomGenerator(), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 639e3f6ac9a5..d6aeae55208a 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -224,6 +224,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Config::ConfigProviderManager& scoped_routes_config_provider_manager_; Filter::Http::FilterConfigProviderManager& filter_config_provider_manager_; CodecType codec_type_; + envoy::config::core::v3::Http3ProtocolOptions http3_options_; envoy::config::core::v3::Http2ProtocolOptions http2_options_; const Http::Http1Settings http1_settings_; HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{ diff --git a/source/extensions/quic_listeners/quiche/BUILD b/source/extensions/quic_listeners/quiche/BUILD index f7bd827f8af7..d05863023757 100644 --- a/source/extensions/quic_listeners/quiche/BUILD +++ b/source/extensions/quic_listeners/quiche/BUILD @@ -127,10 +127,12 @@ envoy_cc_library( tags = ["nofips"], deps = [ ":envoy_quic_simulated_watermark_buffer_lib", + ":envoy_quic_utils_lib", ":quic_filter_manager_connection_lib", "//include/envoy/event:dispatcher_interface", "//include/envoy/http:codec_interface", "//source/common/http:codec_helper_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) @@ -191,6 +193,7 @@ envoy_cc_library( "//source/common/http:header_map_lib", "//source/common/network:connection_base_lib", "//source/common/stream_info:stream_info_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/source/extensions/quic_listeners/quiche/codec_impl.cc b/source/extensions/quic_listeners/quiche/codec_impl.cc index 950d914c755b..7467d401e115 100644 --- a/source/extensions/quic_listeners/quiche/codec_impl.cc +++ b/source/extensions/quic_listeners/quiche/codec_impl.cc @@ -19,8 +19,14 @@ EnvoyQuicClientStream* quicStreamToEnvoyClientStream(quic::QuicStream* stream) { bool QuicHttpConnectionImplBase::wantsToWrite() { return quic_session_.bytesToSend() > 0; } QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl( - EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks) + EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t /*max_request_headers_kb*/, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) : QuicHttpConnectionImplBase(quic_session), quic_server_session_(quic_session) { + quic_session.setHttp3Options(http3_options); + quic_session.setHeadersWithUnderscoreAction(headers_with_underscores_action); quic_session.setHttpConnectionCallbacks(callbacks); } @@ -56,9 +62,12 @@ void QuicHttpServerConnectionImpl::goAway() { } } -QuicHttpClientConnectionImpl::QuicHttpClientConnectionImpl(EnvoyQuicClientSession& session, - Http::ConnectionCallbacks& callbacks) +QuicHttpClientConnectionImpl::QuicHttpClientConnectionImpl( + EnvoyQuicClientSession& session, Http::ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t /*max_request_headers_kb*/) : QuicHttpConnectionImplBase(session), quic_client_session_(session) { + session.setHttp3Options(http3_options); session.setHttpConnectionCallbacks(callbacks); } @@ -95,17 +104,25 @@ void QuicHttpClientConnectionImpl::onUnderlyingConnectionBelowWriteBufferLowWate std::unique_ptr QuicHttpClientConnectionFactoryImpl::createQuicClientConnection( - Network::Connection& connection, Http::ConnectionCallbacks& callbacks) { + Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb) { return std::make_unique( - dynamic_cast(connection), callbacks); + dynamic_cast(connection), callbacks, http3_options, + max_request_headers_kb); } std::unique_ptr QuicHttpServerConnectionFactoryImpl::createQuicServerConnection( - Network::Connection& connection, Http::ConnectionCallbacks& callbacks) { + Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) { return std::make_unique( dynamic_cast(connection), - dynamic_cast(callbacks)); + dynamic_cast(callbacks), http3_options, + max_request_headers_kb, headers_with_underscores_action); } REGISTER_FACTORY(QuicHttpClientConnectionFactoryImpl, Http::QuicHttpClientConnectionFactory); diff --git a/source/extensions/quic_listeners/quiche/codec_impl.h b/source/extensions/quic_listeners/quiche/codec_impl.h index b13f85b3d341..57723b011b1a 100644 --- a/source/extensions/quic_listeners/quiche/codec_impl.h +++ b/source/extensions/quic_listeners/quiche/codec_impl.h @@ -40,8 +40,12 @@ class QuicHttpConnectionImplBase : public virtual Http::Connection, class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase, public Http::ServerConnection { public: - QuicHttpServerConnectionImpl(EnvoyQuicServerSession& quic_session, - Http::ServerConnectionCallbacks& callbacks); + QuicHttpServerConnectionImpl( + EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); // Http::Connection void goAway() override; @@ -57,7 +61,9 @@ class QuicHttpClientConnectionImpl : public QuicHttpConnectionImplBase, public Http::ClientConnection { public: QuicHttpClientConnectionImpl(EnvoyQuicClientSession& session, - Http::ConnectionCallbacks& callbacks); + Http::ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb); // Http::ClientConnection Http::RequestEncoder& newStream(Http::ResponseDecoder& response_decoder) override; @@ -76,8 +82,9 @@ class QuicHttpClientConnectionImpl : public QuicHttpConnectionImplBase, class QuicHttpClientConnectionFactoryImpl : public Http::QuicHttpClientConnectionFactory { public: std::unique_ptr - createQuicClientConnection(Network::Connection& connection, - Http::ConnectionCallbacks& callbacks) override; + createQuicClientConnection(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb) override; std::string name() const override { return Http::QuicCodecNames::get().Quiche; } }; @@ -85,9 +92,12 @@ class QuicHttpClientConnectionFactoryImpl : public Http::QuicHttpClientConnectio // A factory to create QuicHttpServerConnection. class QuicHttpServerConnectionFactoryImpl : public Http::QuicHttpServerConnectionFactory { public: - std::unique_ptr - createQuicServerConnection(Network::Connection& connection, - Http::ConnectionCallbacks& callbacks) override; + std::unique_ptr createQuicServerConnection( + Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + const uint32_t max_request_headers_kb, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) override; std::string name() const override { return Http::QuicCodecNames::get().Quiche; } }; diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc b/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc index c37d0ceb3dbf..174292b1fff7 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc @@ -84,8 +84,9 @@ void EnvoyQuicClientSession::SetDefaultEncryptionLevel(quic::EncryptionLevel lev } std::unique_ptr EnvoyQuicClientSession::CreateClientStream() { + ASSERT(http3_options_.has_value()); return std::make_unique(GetNextOutgoingBidirectionalStreamId(), this, - quic::BIDIRECTIONAL); + quic::BIDIRECTIONAL, http3_options_.value()); } quic::QuicSpdyStream* EnvoyQuicClientSession::CreateIncomingStream(quic::QuicStreamId /*id*/) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc index 1664e68ca0ed..ff0fdd63a6df 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc @@ -29,9 +29,9 @@ namespace Envoy { namespace Quic { -EnvoyQuicClientStream::EnvoyQuicClientStream(quic::QuicStreamId id, - quic::QuicSpdyClientSession* client_session, - quic::StreamType type) +EnvoyQuicClientStream::EnvoyQuicClientStream( + quic::QuicStreamId id, quic::QuicSpdyClientSession* client_session, quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options) : quic::QuicSpdyClientStream(id, client_session, type), EnvoyQuicStream( // This should be larger than 8k to fully utilize congestion control @@ -40,15 +40,15 @@ EnvoyQuicClientStream::EnvoyQuicClientStream(quic::QuicStreamId id, // Ideally this limit should also correlate to peer's receive window // but not fully depends on that. 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }) {} + [this]() { runHighWatermarkCallbacks(); }, http3_options) {} -EnvoyQuicClientStream::EnvoyQuicClientStream(quic::PendingStream* pending, - quic::QuicSpdyClientSession* client_session, - quic::StreamType type) +EnvoyQuicClientStream::EnvoyQuicClientStream( + quic::PendingStream* pending, quic::QuicSpdyClientSession* client_session, + quic::StreamType type, const envoy::config::core::v3::Http3ProtocolOptions& http3_options) : quic::QuicSpdyClientStream(pending, client_session, type), EnvoyQuicStream( 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }) {} + [this]() { runHighWatermarkCallbacks(); }, http3_options) {} Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) { @@ -158,7 +158,7 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, end_stream_decoded_ = true; } std::unique_ptr headers = - quicHeadersToEnvoyHeaders(header_list); + quicHeadersToEnvoyHeaders(header_list, *this); const uint64_t status = Http::Utility::getResponseStatus(*headers); if (Http::CodeUtility::is1xx(status)) { if (status == enumToInt(Http::Code::SwitchingProtocols)) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h index 3d7943bb0c3e..dcab38bddda7 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h @@ -23,9 +23,11 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, public Http::RequestEncoder { public: EnvoyQuicClientStream(quic::QuicStreamId id, quic::QuicSpdyClientSession* client_session, - quic::StreamType type); + quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options); EnvoyQuicClientStream(quic::PendingStream* pending, quic::QuicSpdyClientSession* client_session, - quic::StreamType type); + quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options); void setResponseDecoder(Http::ResponseDecoder& decoder) { response_decoder_ = &decoder; } diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc b/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc index bdfb2315b172..2143ac82c054 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc @@ -44,7 +44,13 @@ quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::QuicStr if (!ShouldCreateIncomingStream(id)) { return nullptr; } - auto stream = new EnvoyQuicServerStream(id, this, quic::BIDIRECTIONAL); + if (!http3_options_.has_value()) { + ENVOY_CONN_LOG(error, "Attempt to create stream {} before HCM filter is initialized.", *this, + id); + return nullptr; + } + auto stream = new EnvoyQuicServerStream(id, this, quic::BIDIRECTIONAL, http3_options_.value(), + headers_with_underscores_action_); ActivateStream(absl::WrapUnique(stream)); setUpRequestDecoder(*stream); if (aboveHighWatermark()) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_session.h b/source/extensions/quic_listeners/quiche/envoy_quic_server_session.h index 7a6f51b1db1c..bcc857816ac6 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_session.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_session.h @@ -59,6 +59,12 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, // quic::QuicSpdySession void SetDefaultEncryptionLevel(quic::EncryptionLevel level) override; + void setHeadersWithUnderscoreAction( + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) { + headers_with_underscores_action_ = headers_with_underscores_action; + } + using quic::QuicSession::PerformActionOnActiveStreams; protected: @@ -86,6 +92,9 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, // These callbacks are owned by network filters and quic session should out live // them. Http::ServerConnectionCallbacks* http_connection_callbacks_{nullptr}; + + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_; }; } // namespace Quic diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc index 38c1c756d8c4..4471141f7d64 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc @@ -31,8 +31,11 @@ namespace Envoy { namespace Quic { -EnvoyQuicServerStream::EnvoyQuicServerStream(quic::QuicStreamId id, quic::QuicSpdySession* session, - quic::StreamType type) +EnvoyQuicServerStream::EnvoyQuicServerStream( + quic::QuicStreamId id, quic::QuicSpdySession* session, quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) : quic::QuicSpdyServerStreamBase(id, session, type), EnvoyQuicStream( // This should be larger than 8k to fully utilize congestion control @@ -41,17 +44,22 @@ EnvoyQuicServerStream::EnvoyQuicServerStream(quic::QuicStreamId id, quic::QuicSp // Ideally this limit should also correlate to peer's receive window // but not fully depends on that. 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }) {} - -EnvoyQuicServerStream::EnvoyQuicServerStream(quic::PendingStream* pending, - quic::QuicSpdySession* session, quic::StreamType type) + [this]() { runHighWatermarkCallbacks(); }, http3_options), + headers_with_underscores_action_(headers_with_underscores_action) {} + +EnvoyQuicServerStream::EnvoyQuicServerStream( + quic::PendingStream* pending, quic::QuicSpdySession* session, quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action) : quic::QuicSpdyServerStreamBase(pending, session, type), EnvoyQuicStream( // This should be larger than 8k to fully utilize congestion control // window. And no larger than the max stream flow control window for // the stream to buffer all the data. 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }) {} + [this]() { runHighWatermarkCallbacks(); }, http3_options), + headers_with_underscores_action_(headers_with_underscores_action) {} void EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) { ASSERT(headers.Status()->value() == "100"); @@ -162,7 +170,15 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, end_stream_decoded_ = true; } std::unique_ptr headers = - quicHeadersToEnvoyHeaders(header_list); + quicHeadersToEnvoyHeaders(header_list, *this); + if (headers == nullptr) { + if (close_connection_upon_invalid_header_) { + stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); + } else { + Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + } + return; + } if (!Http::HeaderUtility::authorityIsValid(headers->Host()->value().getStringView())) { stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); return; @@ -306,5 +322,34 @@ QuicFilterManagerConnectionImpl* EnvoyQuicServerStream::filterManagerConnection( return dynamic_cast(session()); } +HeaderValidationResult EnvoyQuicServerStream::validateHeader(const std::string& header_name, + absl::string_view header_value) { + HeaderValidationResult result = EnvoyQuicStream::validateHeader(header_name, header_value); + if (result != HeaderValidationResult::ACCEPT) { + return result; + } + return checkHeaderNameForUnderscores(header_name); +} + +HeaderValidationResult +EnvoyQuicServerStream::checkHeaderNameForUnderscores(const std::string& header_name) { + if (headers_with_underscores_action_ == envoy::config::core::v3::HttpProtocolOptions::ALLOW || + !Http::HeaderUtility::headerNameContainsUnderscore(header_name)) { + return HeaderValidationResult::ACCEPT; + } + if (headers_with_underscores_action_ == + envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { + ENVOY_STREAM_LOG(debug, "Dropping header with invalid characters in its name: {}", *this, + header_name); + // TODO(danzh) Increment dropped_headers_with_underscores_ once http3 stats is propogated; + return HeaderValidationResult::DROP; + } + ENVOY_STREAM_LOG(debug, "Rejecting request due to header name with underscores: {}", *this, + header_name); + // TODO(danzh) Increment requests_rejected_with_underscores_in_headers_ once http3 stats is + // propogated; + return HeaderValidationResult::INVALID; +} + } // namespace Quic } // namespace Envoy diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h index f6796dfa7b9b..cc91fe6d292f 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h @@ -23,10 +23,16 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, public Http::ResponseEncoder { public: EnvoyQuicServerStream(quic::QuicStreamId id, quic::QuicSpdySession* session, - quic::StreamType type); + quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); EnvoyQuicServerStream(quic::PendingStream* pending, quic::QuicSpdySession* session, - quic::StreamType type); + quic::StreamType type, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action); void setRequestDecoder(Http::RequestDecoder& decoder) { request_decoder_ = &decoder; } @@ -39,7 +45,9 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { return absl::nullopt; } - bool streamErrorOnInvalidHttpMessage() const override { return false; } + bool streamErrorOnInvalidHttpMessage() const override { + return http3_options_.override_stream_error_on_invalid_http_message().value(); + } // Http::Stream void resetStream(Http::StreamResetReason reason) override; @@ -56,6 +64,12 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, // quic::QuicSpdyServerStreamBase void OnConnectionClosed(quic::QuicErrorCode error, quic::ConnectionCloseSource source) override; + void clearWatermarkBuffer(); + + // EnvoyQuicStream + HeaderValidationResult validateHeader(const std::string& header_name, + absl::string_view header_value) override; + protected: // EnvoyQuicStream void switchStreamBlockState(bool should_block) override; @@ -75,7 +89,11 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, // Deliver awaiting trailers if body has been delivered. void maybeDecodeTrailers(); + HeaderValidationResult checkHeaderNameForUnderscores(const std::string& header_name); + Http::RequestDecoder* request_decoder_{nullptr}; + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action_; }; } // namespace Quic diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_stream.h index 287878b0f96b..28ff3e16f9ad 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_stream.h @@ -1,11 +1,13 @@ #pragma once +#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/event/dispatcher.h" #include "envoy/http/codec.h" #include "common/http/codec_helper.h" #include "extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h" +#include "extensions/quic_listeners/quiche/envoy_quic_utils.h" #include "extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h" namespace Envoy { @@ -15,13 +17,16 @@ namespace Quic { class EnvoyQuicStream : public virtual Http::StreamEncoder, public Http::Stream, public Http::StreamCallbackHelper, + public HeaderValidator, protected Logger::Loggable { public: // |buffer_limit| is the high watermark of the stream send buffer, and the low // watermark will be half of it. EnvoyQuicStream(uint32_t buffer_limit, std::function below_low_watermark, - std::function above_high_watermark) - : send_buffer_simulation_(buffer_limit / 2, buffer_limit, std::move(below_low_watermark), + std::function above_high_watermark, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options) + : http3_options_(http3_options), + send_buffer_simulation_(buffer_limit / 2, buffer_limit, std::move(below_low_watermark), std::move(above_high_watermark), ENVOY_LOGGER()) {} // Http::StreamEncoder @@ -97,6 +102,51 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, connection.adjustBytesToSend(buffered_data_new - buffered_data_old); } + HeaderValidationResult validateHeader(const std::string& header_name, + absl::string_view header_value) override { + bool override_stream_error_on_invalid_http_message = + http3_options_.has_override_stream_error_on_invalid_http_message() && + http3_options_.override_stream_error_on_invalid_http_message().value(); + if (header_name == "content-length") { + return validateContentLength(header_value, override_stream_error_on_invalid_http_message); + } + return HeaderValidationResult::ACCEPT; + } + + HeaderValidationResult validateContentLength(absl::string_view header_value, + bool override_stream_error_on_invalid_http_message) { + std::vector values = absl::StrSplit(header_value, ','); + absl::optional content_length; + for (const absl::string_view& value : values) { + uint64_t new_value; + if (!absl::SimpleAtoi(value, &new_value) || + !std::all_of(value.begin(), value.end(), absl::ascii_isdigit)) { + ENVOY_STREAM_LOG(debug, "Content length was either unparseable or negative", *this); + // TODO(danzh) set value according to override_stream_error_on_invalid_http_message from + // configured http3 options. + if (!override_stream_error_on_invalid_http_message) { + close_connection_upon_invalid_header_ = true; + } + return HeaderValidationResult::INVALID; + } + if (!content_length.has_value()) { + content_length = new_value; + continue; + } + if (new_value != content_length.value()) { + ENVOY_STREAM_LOG( + debug, + "Parsed content length {} is inconsistent with previously detected content length {}", + *this, new_value, content_length.value()); + if (!override_stream_error_on_invalid_http_message) { + close_connection_upon_invalid_header_ = true; + } + return HeaderValidationResult::INVALID; + } + } + return HeaderValidationResult::ACCEPT; + } + protected: virtual void switchStreamBlockState(bool should_block) PURE; @@ -113,6 +163,9 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, // becomes false. bool in_decode_data_callstack_{false}; + const envoy::config::core::v3::Http3ProtocolOptions& http3_options_; + bool close_connection_upon_invalid_header_{false}; + private: // Keeps track of bytes buffered in the stream send buffer in QUICHE and reacts // upon crossing high and low watermarks. diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h index 209b1b0c21ae..ffeefcd0f356 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h @@ -37,19 +37,40 @@ quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address); quic::QuicSocketAddress envoyIpAddressToQuicSocketAddress(const Network::Address::Ip* envoy_ip); +enum class HeaderValidationResult { + ACCEPT = 0, + DROP, + INVALID, +}; + +class HeaderValidator { +public: + virtual ~HeaderValidator() = default; + virtual HeaderValidationResult validateHeader(const std::string& header_name, + absl::string_view header_value) = 0; +}; + // The returned header map has all keys in lower case. template -std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list) { +std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, + HeaderValidator& validator) { auto headers = T::create(); for (const auto& entry : header_list) { - auto key = Http::LowerCaseString(entry.first); - if (key != Http::Headers::get().Cookie) { - // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. - headers->addCopy(key, entry.second); - } else { - // QUICHE breaks "cookie" header into crumbs. Coalesce them by appending current one to - // existing one if there is any. - headers->appendCopy(key, entry.second); + HeaderValidationResult result = validator.validateHeader(entry.first, entry.second); + if (result == HeaderValidationResult::INVALID) { + return nullptr; + } + + if (result == HeaderValidationResult::ACCEPT) { + auto key = Http::LowerCaseString(entry.first); + if (key != Http::Headers::get().Cookie) { + // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. + headers->addCopy(key, entry.second); + } else { + // QUICHE breaks "cookie" header into crumbs. Coalesce them by appending current one to + // existing one if there is any. + headers->appendCopy(key, entry.second); + } } } return headers; diff --git a/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h b/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h index 4be7a776f068..0b701172a2fb 100644 --- a/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h +++ b/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/event/dispatcher.h" #include "envoy/network/connection.h" @@ -114,6 +115,11 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase { uint32_t bytesToSend() { return bytes_to_send_; } + void setHttp3Options(const envoy::config::core::v3::Http3ProtocolOptions& http3_options) { + http3_options_ = + std::reference_wrapper(http3_options); + } + protected: // Propagate connection close to network_connection_callbacks_. void onConnectionCloseEvent(const quic::QuicConnectionCloseFrame& frame, @@ -125,6 +131,9 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase { EnvoyQuicConnection* quic_connection_{nullptr}; + absl::optional> + http3_options_; + private: friend class Envoy::TestPauseFilterForQuic; @@ -145,7 +154,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase { // Keeps the buffer state of the connection, and react upon the changes of how many bytes are // buffered cross all streams' send buffer. The state is evaluated and may be changed upon each // stream write. QUICHE doesn't buffer data in connection, all the data is buffered in stream's - // send buffer. + // send buffer.` EnvoyQuicSimulatedWatermarkBuffer write_buffer_watermark_simulation_; Buffer::OwnedImpl empty_buffer_; }; diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 2869c8c9c4f0..f555595e6e6f 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -338,11 +338,12 @@ FakeHttpConnection::FakeHttpConnection( max_request_headers_kb, max_request_headers_count, headers_with_underscores_action); } else { ASSERT(type == Type::HTTP3); - envoy::config::core::v3::Http3ProtocolOptions http3_options; codec_ = std::unique_ptr( Config::Utility::getAndCheckFactoryByName( Http::QuicCodecNames::get().Quiche) - .createQuicServerConnection(shared_connection_.connection(), *this)); + .createQuicServerConnection(shared_connection_.connection(), *this, + fake_upstream.http3Options(), max_request_headers_kb, + headers_with_underscores_action)); } shared_connection_.connection().addReadFilter( Network::ReadFilterSharedPtr{new ReadFilter(*this)}); @@ -513,6 +514,7 @@ FakeUpstream::FakeUpstream(Network::TransportSocketFactoryPtr&& transport_socket FakeUpstream::FakeUpstream(Network::TransportSocketFactoryPtr&& transport_socket_factory, Network::SocketPtr&& listen_socket, const FakeUpstreamConfig& config) : http_type_(config.upstream_protocol_), http2_options_(config.http2_options_), + http3_options_(config.http3_options_), socket_(Network::SocketSharedPtr(listen_socket.release())), socket_factory_(std::make_shared(socket_)), api_(Api::createApiForTest(stats_store_)), time_system_(config.time_system_), diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 008c3fbcd67d..7f54c3b906ea 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -549,6 +549,7 @@ struct FakeUpstreamConfig { bool enable_half_close_{}; absl::optional udp_fake_upstream_; envoy::config::core::v3::Http2ProtocolOptions http2_options_; + envoy::config::core::v3::Http3ProtocolOptions http3_options_; uint32_t max_request_headers_kb_ = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; uint32_t max_request_headers_count_ = Http::DEFAULT_MAX_HEADERS_COUNT; envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction @@ -657,6 +658,7 @@ class FakeUpstream : Logger::Loggable, std::chrono::milliseconds timeout = TestUtility::DefaultTimeout); const envoy::config::core::v3::Http2ProtocolOptions& http2Options() { return http2_options_; } + const envoy::config::core::v3::Http3ProtocolOptions& http3Options() { return http3_options_; } protected: Stats::IsolatedStoreImpl stats_store_; @@ -783,6 +785,7 @@ class FakeUpstream : Logger::Loggable, std::chrono::milliseconds timeout = TestUtility::DefaultTimeout); const envoy::config::core::v3::Http2ProtocolOptions http2_options_; + const envoy::config::core::v3::Http3ProtocolOptions http3_options_; Network::SocketSharedPtr socket_; Network::ListenSocketFactorySharedPtr socket_factory_; ConditionalInitializer server_initialized_; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 4b8055873826..07d5d594b8c1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1396,6 +1396,9 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { + hcm.mutable_http3_protocol_options() + ->mutable_override_stream_error_on_invalid_http_message() + ->set_value(true); hcm.mutable_http2_protocol_options() ->mutable_override_stream_error_on_invalid_http_message() ->set_value(true); @@ -1461,6 +1464,9 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { + hcm.mutable_http3_protocol_options() + ->mutable_override_stream_error_on_invalid_http_message() + ->set_value(true); hcm.mutable_http2_protocol_options() ->mutable_override_stream_error_on_invalid_http_message() ->set_value(true); @@ -2254,6 +2260,9 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { + hcm.mutable_http3_protocol_options() + ->mutable_override_stream_error_on_invalid_http_message() + ->set_value(true); hcm.mutable_http2_protocol_options() ->mutable_override_stream_error_on_invalid_http_message() ->set_value(true); From c70d7ccc82a5cd67e654c2e10b400330b8e230c2 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 29 Mar 2021 18:52:12 -0400 Subject: [PATCH 02/13] plumb thru http3 options and hcm config Signed-off-by: Dan Zhang --- include/envoy/http/codec.h | 4 ++ include/envoy/upstream/upstream.h | 5 +++ source/common/http/codec_client.cc | 3 +- source/common/http/http3/BUILD | 11 ++++++ source/common/http/http3/quic_codec_factory.h | 4 ++ source/common/upstream/BUILD | 1 + source/common/upstream/upstream_impl.cc | 4 ++ source/common/upstream/upstream_impl.h | 3 ++ .../network/http_connection_manager/BUILD | 1 + .../network/http_connection_manager/config.cc | 6 ++- .../network/http_connection_manager/config.h | 2 + source/extensions/quic_listeners/quiche/BUILD | 1 + .../quic_listeners/quiche/codec_impl.cc | 14 +++++-- .../quic_listeners/quiche/codec_impl.h | 11 ++++-- .../quiche/envoy_quic_client_session.cc | 5 ++- .../quiche/envoy_quic_client_stream.cc | 8 ++-- .../quiche/envoy_quic_client_stream.h | 4 +- .../quiche/envoy_quic_server_session.cc | 6 +-- .../quiche/envoy_quic_server_stream.cc | 11 +++--- .../quiche/envoy_quic_server_stream.h | 4 +- .../quic_listeners/quiche/envoy_quic_stream.h | 5 ++- .../quic_filter_manager_connection_impl.h | 8 ++++ .../quiche/envoy_quic_client_session_test.cc | 8 +++- .../quiche/envoy_quic_client_stream_test.cc | 8 +++- .../quiche/envoy_quic_server_session_test.cc | 12 ++++-- .../quiche/envoy_quic_server_stream_test.cc | 9 ++++- .../quiche/envoy_quic_utils_test.cc | 38 ++++++++++++++++++- test/integration/fake_upstream.cc | 3 +- test/integration/fake_upstream.h | 6 +++ test/integration/http_integration.cc | 7 ++++ test/integration/protocol_integration_test.cc | 4 +- test/mocks/upstream/cluster_info.cc | 4 ++ test/mocks/upstream/cluster_info.h | 3 ++ 33 files changed, 182 insertions(+), 41 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 4ccdf2e3b357..d2f7113d0f73 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -26,6 +26,10 @@ namespace Http2 { struct CodecStats; } +namespace Http3 { +struct CodecStats; +} + // Legacy default value of 60K is safely under both codec default limits. static constexpr uint32_t DEFAULT_MAX_REQUEST_HEADERS_KB = 60; // Default maximum number of headers. diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 6a5cb1a151c2..f791e1c4911c 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -995,6 +995,11 @@ class ClusterInfo { */ virtual Http::Http2::CodecStats& http2CodecStats() const PURE; + /** + * @return the Http2 Codec Stats. + */ + virtual Http::Http3::CodecStats& http3CodecStats() const PURE; + protected: /** * Invoked by extensionProtocolOptionsTyped. diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index c41826701d9b..f1cc1f7344e1 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -185,7 +185,8 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne codec_ = std::unique_ptr( Config::Utility::getAndCheckFactoryByName( Http::QuicCodecNames::get().Quiche) - .createQuicClientConnection(*connection_, *this, host->cluster().http3Options(), + .createQuicClientConnection(*connection_, *this, host->cluster().http3CodecStats(), + host->cluster().http3Options(), Http::DEFAULT_MAX_REQUEST_HEADERS_KB)); break; } diff --git a/source/common/http/http3/BUILD b/source/common/http/http3/BUILD index 8a2dd3fc89d8..8fa859732922 100644 --- a/source/common/http/http3/BUILD +++ b/source/common/http/http3/BUILD @@ -25,6 +25,7 @@ envoy_cc_library( name = "quic_codec_factory_lib", hdrs = ["quic_codec_factory.h"], deps = [ + ":codec_stats_lib", "//include/envoy/config:typed_config_interface", "//include/envoy/http:codec_interface", "//include/envoy/network:connection_interface", @@ -48,3 +49,13 @@ envoy_cc_library( hdrs = ["well_known_names.h"], deps = ["//source/common/singleton:const_singleton"], ) + +envoy_cc_library( + name = "codec_stats_lib", + hdrs = ["codec_stats.h"], + deps = [ + "//include/envoy/stats:stats_interface", + "//include/envoy/stats:stats_macros", + "//source/common/common:thread_lib", + ], +) diff --git a/source/common/http/http3/quic_codec_factory.h b/source/common/http/http3/quic_codec_factory.h index 07a8357088e9..51daef5b484c 100644 --- a/source/common/http/http3/quic_codec_factory.h +++ b/source/common/http/http3/quic_codec_factory.h @@ -7,6 +7,8 @@ #include "envoy/http/codec.h" #include "envoy/network/connection.h" +#include "common/http/http3/codec_stats.h" + namespace Envoy { namespace Http { @@ -17,6 +19,7 @@ class QuicHttpServerConnectionFactory : public Config::UntypedFactory { virtual std::unique_ptr createQuicServerConnection( Network::Connection& connection, ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction @@ -32,6 +35,7 @@ class QuicHttpClientConnectionFactory : public Config::UntypedFactory { virtual std::unique_ptr createQuicClientConnection(Network::Connection& connection, ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb) PURE; diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index d69db58fb866..c7031fc561f9 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -539,6 +539,7 @@ envoy_cc_library( "//source/common/config:well_known_names", "//source/common/http/http1:codec_stats_lib", "//source/common/http/http2:codec_stats_lib", + "//source/common/http/http3:codec_stats_lib", "//source/common/init:manager_lib", "//source/common/shared_pool:shared_pool_lib", "//source/common/stats:isolated_store_lib", diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 4e4a52ecbe47..7f02e4337591 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1225,6 +1225,10 @@ Http::Http2::CodecStats& ClusterInfoImpl::http2CodecStats() const { return Http::Http2::CodecStats::atomicGet(http2_codec_stats_, *stats_scope_); } +Http::Http3::CodecStats& ClusterInfoImpl::http3CodecStats() const { + return Http::Http3::CodecStats::atomicGet(http3_codec_stats_, *stats_scope_); +} + std::pair, absl::optional> ClusterInfoImpl::getRetryBudgetParams( const envoy::config::cluster::v3::CircuitBreakers::Thresholds& thresholds) { constexpr double default_budget_percent = 20.0; diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index eb38961922c3..ec98ca4146d2 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -45,6 +45,7 @@ #include "common/config/well_known_names.h" #include "common/http/http1/codec_stats.h" #include "common/http/http2/codec_stats.h" +#include "common/http/http3/codec_stats.h" #include "common/init/manager_impl.h" #include "common/network/utility.h" #include "common/shared_pool/shared_pool.h" @@ -654,6 +655,7 @@ class ClusterInfoImpl : public ClusterInfo, Http::Http1::CodecStats& http1CodecStats() const override; Http::Http2::CodecStats& http2CodecStats() const override; + Http::Http3::CodecStats& http3CodecStats() const override; protected: // Gets the retry budget percent/concurrency from the circuit breaker thresholds. If the retry @@ -732,6 +734,7 @@ class ClusterInfoImpl : public ClusterInfo, std::vector filter_factories_; mutable Http::Http1::CodecStats::AtomicPtr http1_codec_stats_; mutable Http::Http2::CodecStats::AtomicPtr http2_codec_stats_; + mutable Http::Http3::CodecStats::AtomicPtr http3_codec_stats_; }; /** diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD index de7d4224163b..3f73add61b95 100644 --- a/source/extensions/filters/network/http_connection_manager/BUILD +++ b/source/extensions/filters/network/http_connection_manager/BUILD @@ -42,6 +42,7 @@ envoy_cc_extension( "//source/common/http:utility_lib", "//source/common/http/http1:codec_lib", "//source/common/http/http2:codec_lib", + "//source/common/http/http3:codec_stats_lib", "//source/common/json:json_loader_lib", "//source/common/local_reply:local_reply_lib", "//source/common/protobuf:utility_lib", diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 3a0fb5b91fd3..d50d5e30819b 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -585,8 +585,10 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, return std::unique_ptr( Config::Utility::getAndCheckFactoryByName( Http::QuicCodecNames::get().Quiche) - .createQuicServerConnection(connection, callbacks, http3_options_, - maxRequestHeadersKb(), headersWithUnderscoresAction())); + .createQuicServerConnection( + connection, callbacks, + Http::Http3::CodecStats::atomicGet(http3_codec_stats_, context_.scope()), + http3_options_, maxRequestHeadersKb(), headersWithUnderscoresAction())); case CodecType::AUTO: return Http::ConnectionManagerUtility::autoCreateCodec( connection, data, callbacks, context_.scope(), context_.api().randomGenerator(), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index d6aeae55208a..d2c889a12aa9 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -23,6 +23,7 @@ #include "common/http/date_provider_impl.h" #include "common/http/http1/codec_stats.h" #include "common/http/http2/codec_stats.h" +#include "common/http/http3/codec_stats.h" #include "common/json/json_loader.h" #include "common/local_reply/local_reply.h" #include "common/router/rds_impl.h" @@ -212,6 +213,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerStats stats_; mutable Http::Http1::CodecStats::AtomicPtr http1_codec_stats_; mutable Http::Http2::CodecStats::AtomicPtr http2_codec_stats_; + mutable Http::Http3::CodecStats::AtomicPtr http3_codec_stats_; Http::ConnectionManagerTracingStats tracing_stats_; const bool use_remote_address_{}; const std::unique_ptr internal_address_config_; diff --git a/source/extensions/quic_listeners/quiche/BUILD b/source/extensions/quic_listeners/quiche/BUILD index d05863023757..434bb8d64e74 100644 --- a/source/extensions/quic_listeners/quiche/BUILD +++ b/source/extensions/quic_listeners/quiche/BUILD @@ -191,6 +191,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:empty_string", "//source/common/http:header_map_lib", + "//source/common/http/http3:codec_stats_lib", "//source/common/network:connection_base_lib", "//source/common/stream_info:stream_info_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/source/extensions/quic_listeners/quiche/codec_impl.cc b/source/extensions/quic_listeners/quiche/codec_impl.cc index 7467d401e115..160447ff0d29 100644 --- a/source/extensions/quic_listeners/quiche/codec_impl.cc +++ b/source/extensions/quic_listeners/quiche/codec_impl.cc @@ -20,11 +20,13 @@ bool QuicHttpConnectionImplBase::wantsToWrite() { return quic_session_.bytesToSe QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl( EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t /*max_request_headers_kb*/, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) - : QuicHttpConnectionImplBase(quic_session), quic_server_session_(quic_session) { + : QuicHttpConnectionImplBase(quic_session, stats), quic_server_session_(quic_session) { + quic_session.setCodecStats(stats); quic_session.setHttp3Options(http3_options); quic_session.setHeadersWithUnderscoreAction(headers_with_underscores_action); quic_session.setHttpConnectionCallbacks(callbacks); @@ -64,9 +66,11 @@ void QuicHttpServerConnectionImpl::goAway() { QuicHttpClientConnectionImpl::QuicHttpClientConnectionImpl( EnvoyQuicClientSession& session, Http::ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t /*max_request_headers_kb*/) - : QuicHttpConnectionImplBase(session), quic_client_session_(session) { + : QuicHttpConnectionImplBase(session, stats), quic_client_session_(session) { + session.setCodecStats(stats); session.setHttp3Options(http3_options); session.setHttpConnectionCallbacks(callbacks); } @@ -105,23 +109,25 @@ void QuicHttpClientConnectionImpl::onUnderlyingConnectionBelowWriteBufferLowWate std::unique_ptr QuicHttpClientConnectionFactoryImpl::createQuicClientConnection( Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb) { return std::make_unique( - dynamic_cast(connection), callbacks, http3_options, + dynamic_cast(connection), callbacks, stats, http3_options, max_request_headers_kb); } std::unique_ptr QuicHttpServerConnectionFactoryImpl::createQuicServerConnection( Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) { return std::make_unique( dynamic_cast(connection), - dynamic_cast(callbacks), http3_options, + dynamic_cast(callbacks), stats, http3_options, max_request_headers_kb, headers_with_underscores_action); } diff --git a/source/extensions/quic_listeners/quiche/codec_impl.h b/source/extensions/quic_listeners/quiche/codec_impl.h index 57723b011b1a..bdeb0a0ec77e 100644 --- a/source/extensions/quic_listeners/quiche/codec_impl.h +++ b/source/extensions/quic_listeners/quiche/codec_impl.h @@ -20,8 +20,9 @@ namespace Quic { class QuicHttpConnectionImplBase : public virtual Http::Connection, protected Logger::Loggable { public: - QuicHttpConnectionImplBase(QuicFilterManagerConnectionImpl& quic_session) - : quic_session_(quic_session) {} + QuicHttpConnectionImplBase(QuicFilterManagerConnectionImpl& quic_session, + Http::Http3::CodecStats& stats) + : quic_session_(quic_session), stats_(stats) {} // Http::Connection Http::Status dispatch(Buffer::Instance& /*data*/) override { @@ -35,6 +36,7 @@ class QuicHttpConnectionImplBase : public virtual Http::Connection, protected: QuicFilterManagerConnectionImpl& quic_session_; + Http::Http3::CodecStats& stats_; }; class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase, @@ -42,6 +44,7 @@ class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase, public: QuicHttpServerConnectionImpl( EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction @@ -61,7 +64,7 @@ class QuicHttpClientConnectionImpl : public QuicHttpConnectionImplBase, public Http::ClientConnection { public: QuicHttpClientConnectionImpl(EnvoyQuicClientSession& session, - Http::ConnectionCallbacks& callbacks, + Http::ConnectionCallbacks& callbacks, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb); @@ -83,6 +86,7 @@ class QuicHttpClientConnectionFactoryImpl : public Http::QuicHttpClientConnectio public: std::unique_ptr createQuicClientConnection(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb) override; @@ -94,6 +98,7 @@ class QuicHttpServerConnectionFactoryImpl : public Http::QuicHttpServerConnectio public: std::unique_ptr createQuicServerConnection( Network::Connection& connection, Http::ConnectionCallbacks& callbacks, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, const uint32_t max_request_headers_kb, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc b/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc index 174292b1fff7..197a4e316acc 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc @@ -84,9 +84,10 @@ void EnvoyQuicClientSession::SetDefaultEncryptionLevel(quic::EncryptionLevel lev } std::unique_ptr EnvoyQuicClientSession::CreateClientStream() { - ASSERT(http3_options_.has_value()); + ASSERT(codec_stats_.has_value() && http3_options_.has_value()); return std::make_unique(GetNextOutgoingBidirectionalStreamId(), this, - quic::BIDIRECTIONAL, http3_options_.value()); + quic::BIDIRECTIONAL, codec_stats_.value(), + http3_options_.value()); } quic::QuicSpdyStream* EnvoyQuicClientSession::CreateIncomingStream(quic::QuicStreamId /*id*/) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc index ff0fdd63a6df..02c5ffc0ce32 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc @@ -31,6 +31,7 @@ namespace Quic { EnvoyQuicClientStream::EnvoyQuicClientStream( quic::QuicStreamId id, quic::QuicSpdyClientSession* client_session, quic::StreamType type, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options) : quic::QuicSpdyClientStream(id, client_session, type), EnvoyQuicStream( @@ -40,15 +41,16 @@ EnvoyQuicClientStream::EnvoyQuicClientStream( // Ideally this limit should also correlate to peer's receive window // but not fully depends on that. 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }, http3_options) {} + [this]() { runHighWatermarkCallbacks(); }, stats, http3_options) {} EnvoyQuicClientStream::EnvoyQuicClientStream( quic::PendingStream* pending, quic::QuicSpdyClientSession* client_session, - quic::StreamType type, const envoy::config::core::v3::Http3ProtocolOptions& http3_options) + quic::StreamType type, Http::Http3::CodecStats& stats, + const envoy::config::core::v3::Http3ProtocolOptions& http3_options) : quic::QuicSpdyClientStream(pending, client_session, type), EnvoyQuicStream( 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }, http3_options) {} + [this]() { runHighWatermarkCallbacks(); }, stats, http3_options) {} Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h index dcab38bddda7..6ed783f84158 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h @@ -23,10 +23,10 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, public Http::RequestEncoder { public: EnvoyQuicClientStream(quic::QuicStreamId id, quic::QuicSpdyClientSession* client_session, - quic::StreamType type, + quic::StreamType type, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options); EnvoyQuicClientStream(quic::PendingStream* pending, quic::QuicSpdyClientSession* client_session, - quic::StreamType type, + quic::StreamType type, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options); void setResponseDecoder(Http::ResponseDecoder& decoder) { response_decoder_ = &decoder; } diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc b/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc index 2143ac82c054..3796eeca3c10 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc @@ -44,13 +44,13 @@ quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::QuicStr if (!ShouldCreateIncomingStream(id)) { return nullptr; } - if (!http3_options_.has_value()) { + if (!codec_stats_.has_value() || !http3_options_.has_value()) { ENVOY_CONN_LOG(error, "Attempt to create stream {} before HCM filter is initialized.", *this, id); return nullptr; } - auto stream = new EnvoyQuicServerStream(id, this, quic::BIDIRECTIONAL, http3_options_.value(), - headers_with_underscores_action_); + auto stream = new EnvoyQuicServerStream(id, this, quic::BIDIRECTIONAL, codec_stats_.value(), + http3_options_.value(), headers_with_underscores_action_); ActivateStream(absl::WrapUnique(stream)); setUpRequestDecoder(*stream); if (aboveHighWatermark()) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc index 4471141f7d64..e9000e074ba5 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc @@ -33,6 +33,7 @@ namespace Quic { EnvoyQuicServerStream::EnvoyQuicServerStream( quic::QuicStreamId id, quic::QuicSpdySession* session, quic::StreamType type, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) @@ -44,11 +45,12 @@ EnvoyQuicServerStream::EnvoyQuicServerStream( // Ideally this limit should also correlate to peer's receive window // but not fully depends on that. 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }, http3_options), + [this]() { runHighWatermarkCallbacks(); }, stats, http3_options), headers_with_underscores_action_(headers_with_underscores_action) {} EnvoyQuicServerStream::EnvoyQuicServerStream( quic::PendingStream* pending, quic::QuicSpdySession* session, quic::StreamType type, + Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) @@ -58,7 +60,7 @@ EnvoyQuicServerStream::EnvoyQuicServerStream( // window. And no larger than the max stream flow control window for // the stream to buffer all the data. 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, - [this]() { runHighWatermarkCallbacks(); }, http3_options), + [this]() { runHighWatermarkCallbacks(); }, stats, http3_options), headers_with_underscores_action_(headers_with_underscores_action) {} void EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) { @@ -341,13 +343,12 @@ EnvoyQuicServerStream::checkHeaderNameForUnderscores(const std::string& header_n envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { ENVOY_STREAM_LOG(debug, "Dropping header with invalid characters in its name: {}", *this, header_name); - // TODO(danzh) Increment dropped_headers_with_underscores_ once http3 stats is propogated; + stats_.dropped_headers_with_underscores_.inc(); return HeaderValidationResult::DROP; } ENVOY_STREAM_LOG(debug, "Rejecting request due to header name with underscores: {}", *this, header_name); - // TODO(danzh) Increment requests_rejected_with_underscores_in_headers_ once http3 stats is - // propogated; + stats_.requests_rejected_with_underscores_in_headers_.inc(); return HeaderValidationResult::INVALID; } diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h index cc91fe6d292f..c31984defc83 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h @@ -23,13 +23,13 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, public Http::ResponseEncoder { public: EnvoyQuicServerStream(quic::QuicStreamId id, quic::QuicSpdySession* session, - quic::StreamType type, + quic::StreamType type, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action); EnvoyQuicServerStream(quic::PendingStream* pending, quic::QuicSpdySession* session, - quic::StreamType type, + quic::StreamType type, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action); diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_stream.h index 28ff3e16f9ad..3d1e943a707c 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_stream.h @@ -23,9 +23,9 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, // |buffer_limit| is the high watermark of the stream send buffer, and the low // watermark will be half of it. EnvoyQuicStream(uint32_t buffer_limit, std::function below_low_watermark, - std::function above_high_watermark, + std::function above_high_watermark, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options) - : http3_options_(http3_options), + : stats_(stats), http3_options_(http3_options), send_buffer_simulation_(buffer_limit / 2, buffer_limit, std::move(below_low_watermark), std::move(above_high_watermark), ENVOY_LOGGER()) {} @@ -163,6 +163,7 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, // becomes false. bool in_decode_data_callstack_{false}; + Http::Http3::CodecStats& stats_; const envoy::config::core::v3::Http3ProtocolOptions& http3_options_; bool close_connection_upon_invalid_header_{false}; diff --git a/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h b/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h index 0b701172a2fb..8544f53cb6cc 100644 --- a/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h +++ b/source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h @@ -1,11 +1,14 @@ #pragma once +#include + #include "envoy/config/core/v3/protocol.pb.h" #include "envoy/event/dispatcher.h" #include "envoy/network/connection.h" #include "common/common/empty_string.h" #include "common/common/logger.h" +#include "common/http/http3/codec_stats.h" #include "common/network/connection_impl_base.h" #include "common/stream_info/stream_info_impl.h" @@ -120,6 +123,10 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase { std::reference_wrapper(http3_options); } + void setCodecStats(Http::Http3::CodecStats& stats) { + codec_stats_ = std::reference_wrapper(stats); + } + protected: // Propagate connection close to network_connection_callbacks_. void onConnectionCloseEvent(const quic::QuicConnectionCloseFrame& frame, @@ -131,6 +138,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase { EnvoyQuicConnection* quic_connection_{nullptr}; + absl::optional> codec_stats_; absl::optional> http3_options_; diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc index 00501a568b52..4e529fe19ca3 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc @@ -115,7 +115,10 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { quic::QuicServerId("example.com", 443, false), &crypto_config_, nullptr, *dispatcher_, /*send_buffer_limit*/ 1024 * 1024), - http_connection_(envoy_quic_session_, http_connection_callbacks_) { + stats_({ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope_, "http3."), + POOL_GAUGE_PREFIX(scope_, "http3."))}), + http_connection_(envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_, + 64 * 1024) { EXPECT_EQ(time_system_.systemTime(), envoy_quic_session_.streamInfo().startTime()); EXPECT_EQ(EMPTY_STRING, envoy_quic_session_.nextProtocol()); EXPECT_EQ(Http::Protocol::Http3, http_connection_.protocol()); @@ -178,6 +181,9 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { testing::StrictMock read_current_; testing::StrictMock write_total_; testing::StrictMock write_current_; + Stats::IsolatedStoreImpl scope_; + Http::Http3::CodecStats stats_; + envoy::config::core::v3::Http3ProtocolOptions http3_options_; QuicHttpClientConnectionImpl http_connection_; }; diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc index 7ebb99df598e..b0fbca459eab 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc @@ -39,7 +39,10 @@ class EnvoyQuicClientStreamTest : public testing::TestWithParam { quic_session_(quic_config_, {quic_version_}, quic_connection_, *dispatcher_, quic_config_.GetInitialStreamFlowControlWindowToSend() * 2), stream_id_(quic::VersionUsesHttp3(quic_version_.transport_version) ? 4u : 5u), - quic_stream_(new EnvoyQuicClientStream(stream_id_, &quic_session_, quic::BIDIRECTIONAL)), + stats_({ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope_, "http3."), + POOL_GAUGE_PREFIX(scope_, "http3."))}), + quic_stream_(new EnvoyQuicClientStream(stream_id_, &quic_session_, quic::BIDIRECTIONAL, + stats_, http3_options_)), request_headers_{{":authority", host_}, {":method", "POST"}, {":path", "/"}}, request_trailers_{{"trailer-key", "trailer-value"}} { quic_stream_->setResponseDecoder(stream_decoder_); @@ -136,6 +139,9 @@ class EnvoyQuicClientStreamTest : public testing::TestWithParam { EnvoyQuicClientConnection* quic_connection_; MockEnvoyQuicClientSession quic_session_; quic::QuicStreamId stream_id_; + Stats::IsolatedStoreImpl scope_; + Http::Http3::CodecStats stats_; + envoy::config::core::v3::Http3ProtocolOptions http3_options_; EnvoyQuicClientStream* quic_stream_; Http::MockResponseDecoder stream_decoder_; Http::MockStreamCallbacks stream_callbacks_; diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc index 6615c64d53b5..91f1d43d566b 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc @@ -162,7 +162,10 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam { &compressed_certs_cache_, *dispatcher_, /*send_buffer_limit*/ quic::kDefaultFlowControlSendWindow * 1.5, listener_config_), - read_filter_(new Network::MockReadFilter()) { + read_filter_(new Network::MockReadFilter()), + stats_({ALL_HTTP3_CODEC_STATS( + POOL_COUNTER_PREFIX(listener_config_.listenerScope(), "http3."), + POOL_GAUGE_PREFIX(listener_config_.listenerScope(), "http3."))}) { EXPECT_EQ(time_system_.systemTime(), envoy_quic_session_.streamInfo().startTime()); EXPECT_EQ(EMPTY_STRING, envoy_quic_session_.nextProtocol()); @@ -221,8 +224,9 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam { EXPECT_EQ(&read_total_, &quic_connection_->connectionStats().read_total_); EXPECT_CALL(*read_filter_, onNewConnection()).WillOnce(Invoke([this]() { // Create ServerConnection instance and setup callbacks for it. - http_connection_ = std::make_unique(envoy_quic_session_, - http_connection_callbacks_); + http_connection_ = std::make_unique( + envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_, 64 * 1024, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); EXPECT_EQ(Http::Protocol::Http3, http_connection_->protocol()); // Stop iteration to avoid calling getRead/WriteBuffer(). return Network::FilterStatus::StopIteration; @@ -279,6 +283,8 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam { testing::StrictMock write_total_; testing::StrictMock write_current_; Http::ServerConnectionPtr http_connection_; + Http::Http3::CodecStats stats_; + envoy::config::core::v3::Http3ProtocolOptions http3_options_; }; INSTANTIATE_TEST_SUITE_P(EnvoyQuicServerSessionTests, EnvoyQuicServerSessionTest, diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc index d671dd56c1e9..9706ae585083 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc @@ -57,7 +57,12 @@ class EnvoyQuicServerStreamTest : public testing::TestWithParam { quic_session_(quic_config_, {quic_version_}, &quic_connection_, *dispatcher_, quic_config_.GetInitialStreamFlowControlWindowToSend() * 2), stream_id_(VersionUsesHttp3(quic_version_.transport_version) ? 4u : 5u), - quic_stream_(new EnvoyQuicServerStream(stream_id_, &quic_session_, quic::BIDIRECTIONAL)), + stats_( + {ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(listener_config_.listenerScope(), "http3."), + POOL_GAUGE_PREFIX(listener_config_.listenerScope(), "http3."))}), + quic_stream_(new EnvoyQuicServerStream( + stream_id_, &quic_session_, quic::BIDIRECTIONAL, stats_, http3_options_, + envoy::config::core::v3::HttpProtocolOptions::ALLOW)), response_headers_{{":status", "200"}, {"response-key", "response-value"}}, response_trailers_{{"trailer-key", "trailer-value"}} { quic_stream_->setRequestDecoder(stream_decoder_); @@ -163,6 +168,8 @@ class EnvoyQuicServerStreamTest : public testing::TestWithParam { EnvoyQuicServerConnection quic_connection_; MockEnvoyQuicSession quic_session_; quic::QuicStreamId stream_id_; + Http::Http3::CodecStats stats_; + envoy::config::core::v3::Http3ProtocolOptions http3_options_; EnvoyQuicServerStream* quic_stream_; Http::MockRequestDecoder stream_decoder_; Http::MockStreamCallbacks stream_callbacks_; diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_utils_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_utils_test.cc index 4ac8561f39c0..c31ec88457a2 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_utils_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_utils_test.cc @@ -45,13 +45,20 @@ TEST(EnvoyQuicUtilsTest, ConversionBetweenQuicAddressAndEnvoyAddress) { } } +class MockHeaderValidator : public HeaderValidator { +public: + ~MockHeaderValidator() override = default; + MOCK_METHOD(HeaderValidationResult, validateHeader, + (const std::string& header_name, absl::string_view header_value)); +}; + TEST(EnvoyQuicUtilsTest, HeadersConversion) { spdy::SpdyHeaderBlock headers_block; headers_block[":authority"] = "www.google.com"; headers_block[":path"] = "/index.hml"; headers_block[":scheme"] = "https"; // "value1" and "value2" should be coalesced into one header by QUICHE and split again while - // converting to Envoy headers.. + // converting to Envoy headers. headers_block.AppendValueOrAddHeader("key", "value1"); headers_block.AppendValueOrAddHeader("key", "value2"); auto envoy_headers = spdyHeaderBlockToEnvoyHeaders(headers_block); @@ -70,9 +77,36 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { quic_headers.OnHeader(":scheme", "https"); quic_headers.OnHeader("key", "value1"); quic_headers.OnHeader("key", "value2"); + quic_headers.OnHeader("key-to-drop", ""); quic_headers.OnHeaderBlockEnd(0, 0); - auto envoy_headers2 = quicHeadersToEnvoyHeaders(quic_headers); + MockHeaderValidator validator; + EXPECT_CALL(validator, validateHeader(_, _)) + .WillRepeatedly([](const std::string& header_name, absl::string_view) { + if (header_name == "key-to-drop") { + return HeaderValidationResult::DROP; + } + return HeaderValidationResult::ACCEPT; + }); + auto envoy_headers2 = + quicHeadersToEnvoyHeaders(quic_headers, validator); EXPECT_EQ(*envoy_headers, *envoy_headers2); + + quic::QuicHeaderList quic_headers2; + quic_headers2.OnHeaderBlockStart(); + quic_headers2.OnHeader(":authority", "www.google.com"); + quic_headers2.OnHeader(":path", "/index.hml"); + quic_headers2.OnHeader(":scheme", "https"); + quic_headers2.OnHeader("invalid_key", ""); + quic_headers2.OnHeaderBlockEnd(0, 0); + EXPECT_CALL(validator, validateHeader(_, _)) + .WillRepeatedly([](const std::string& header_name, absl::string_view) { + if (header_name == "invalid_key") { + return HeaderValidationResult::INVALID; + } + return HeaderValidationResult::ACCEPT; + }); + EXPECT_EQ(nullptr, + quicHeadersToEnvoyHeaders(quic_headers2, validator)); } } // namespace Quic diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index f555595e6e6f..c807303718d5 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -338,10 +338,11 @@ FakeHttpConnection::FakeHttpConnection( max_request_headers_kb, max_request_headers_count, headers_with_underscores_action); } else { ASSERT(type == Type::HTTP3); + Http::Http3::CodecStats& stats = fake_upstream.http3CodecStats(); codec_ = std::unique_ptr( Config::Utility::getAndCheckFactoryByName( Http::QuicCodecNames::get().Quiche) - .createQuicServerConnection(shared_connection_.connection(), *this, + .createQuicServerConnection(shared_connection_.connection(), *this, stats, fake_upstream.http3Options(), max_request_headers_kb, headers_with_underscores_action)); } diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 7f54c3b906ea..7c89965175e9 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -27,6 +27,7 @@ #include "common/grpc/common.h" #include "common/http/http1/codec_impl.h" #include "common/http/http2/codec_impl.h" +#include "common/http/http3/codec_stats.h" #include "common/http/http3/quic_codec_factory.h" #include "common/http/http3/well_known_names.h" #include "common/network/connection_balancer_impl.h" @@ -650,6 +651,10 @@ class FakeUpstream : Logger::Loggable, return Http::Http2::CodecStats::atomicGet(http2_codec_stats_, stats_store_); } + Http::Http3::CodecStats& http3CodecStats() { + return Http::Http3::CodecStats::atomicGet(http3_codec_stats_, stats_store_); + } + // Write into the outbound buffer of the network connection at the specified index. // Note: that this write bypasses any processing by the upstream codec. ABSL_MUST_USE_RESULT @@ -812,6 +817,7 @@ class FakeUpstream : Logger::Loggable, std::list received_datagrams_ ABSL_GUARDED_BY(lock_); Http::Http1::CodecStats::AtomicPtr http1_codec_stats_; Http::Http2::CodecStats::AtomicPtr http2_codec_stats_; + Http::Http3::CodecStats::AtomicPtr http3_codec_stats_; }; using FakeUpstreamPtr = std::unique_ptr; diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 25a9bf65b7b5..8c18c5753753 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -241,13 +241,20 @@ IntegrationCodecClientPtr HttpIntegrationTest::makeRawHttpConnection( absl::optional http2_options) { std::shared_ptr cluster{new NiceMock()}; cluster->max_response_headers_count_ = 200; + envoy::config::core::v3::Http3ProtocolOptions http3_options; if (!http2_options.has_value()) { http2_options = Http2::Utility::initializeAndValidateOptions( envoy::config::core::v3::Http2ProtocolOptions()); http2_options.value().set_allow_connect(true); http2_options.value().set_allow_metadata(true); + } else if (http2_options.value().has_override_stream_error_on_invalid_http_message()) { + http3_options.mutable_override_stream_error_on_invalid_http_message()->set_value( + http2_options.value().override_stream_error_on_invalid_http_message().value()); + } else if (http2_options.value().stream_error_on_invalid_http_messaging()) { + http3_options.mutable_override_stream_error_on_invalid_http_message()->set_value(true); } cluster->http2_options_ = http2_options.value(); + cluster->http3_options_ = http3_options; cluster->http1_settings_.enable_trailers_ = true; Upstream::HostDescriptionConstSharedPtr host_description{Upstream::makeTestHostDescription( cluster, fmt::format("tcp://{}:80", Network::Test::getLoopbackAddressUrlString(version_)), diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 07d5d594b8c1..02864fff04c4 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1117,9 +1117,7 @@ TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresDropped) { default: RELEASE_ASSERT(false, fmt::format("Unknown downstream protocol {}", downstream_protocol_)); }; - if (downstream_protocol_ != Http::CodecClient::Type::HTTP3) { - EXPECT_EQ(1L, TestUtility::findCounter(stats, stat_name)->value()); - } + EXPECT_EQ(1L, TestUtility::findCounter(stats, stat_name)->value()); } // Verify that by default headers with underscores in their names remain in both requests and diff --git a/test/mocks/upstream/cluster_info.cc b/test/mocks/upstream/cluster_info.cc index 063dfc76f6cd..cd0cd7572ec5 100644 --- a/test/mocks/upstream/cluster_info.cc +++ b/test/mocks/upstream/cluster_info.cc @@ -131,5 +131,9 @@ Http::Http2::CodecStats& MockClusterInfo::http2CodecStats() const { return Http::Http2::CodecStats::atomicGet(http2_codec_stats_, statsScope()); } +Http::Http3::CodecStats& MockClusterInfo::http3CodecStats() const { + return Http::Http3::CodecStats::atomicGet(http3_codec_stats_, statsScope()); +} + } // namespace Upstream } // namespace Envoy diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 31a941bb53ff..640d5bf4f604 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -16,6 +16,7 @@ #include "common/common/thread.h" #include "common/http/http1/codec_stats.h" #include "common/http/http2/codec_stats.h" +#include "common/http/http3/codec_stats.h" #include "common/upstream/upstream_impl.h" #include "test/mocks/runtime/mocks.h" @@ -149,6 +150,7 @@ class MockClusterInfo : public ClusterInfo { Http::Http1::CodecStats& http1CodecStats() const override; Http::Http2::CodecStats& http2CodecStats() const override; + Http::Http3::CodecStats& http3CodecStats() const override; std::string name_{"fake_cluster"}; std::string observability_name_{"observability_name"}; @@ -196,6 +198,7 @@ class MockClusterInfo : public ClusterInfo { absl::optional max_stream_duration_; mutable Http::Http1::CodecStats::AtomicPtr http1_codec_stats_; mutable Http::Http2::CodecStats::AtomicPtr http2_codec_stats_; + mutable Http::Http3::CodecStats::AtomicPtr http3_codec_stats_; }; class MockIdleTimeEnabledClusterInfo : public MockClusterInfo { From 215f63e0661488feacecc62b2ab774a75b722fad Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 29 Mar 2021 23:19:35 -0400 Subject: [PATCH 03/13] enable tests Signed-off-by: Dan Zhang --- .../http_conn_man/response_code_details.rst | 13 ++++++++++++ .../common/quic/envoy_quic_server_stream.cc | 21 +++++++++++++++++++ source/common/quic/envoy_quic_stream.h | 5 ++++- source/common/quic/envoy_quic_utils.cc | 8 ++++--- test/integration/protocol_integration_test.cc | 14 +------------ 5 files changed, 44 insertions(+), 17 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/response_code_details.rst b/docs/root/configuration/http/http_conn_man/response_code_details.rst index 54d6e10eb2cd..3f220a4e6b08 100644 --- a/docs/root/configuration/http/http_conn_man/response_code_details.rst +++ b/docs/root/configuration/http/http_conn_man/response_code_details.rst @@ -99,3 +99,16 @@ All http2 details are rooted at *http2.* http2.unexpected_underscore, Envoy was configured to drop requests with header keys beginning with underscores. http2.unknown.nghttp2.error, An unknown error was encountered by nghttp2 http2.violation.of.messaging.rule, The stream was in violation of a HTTP/2 messaging rule. + +Http3 details +~~~~~~~~~~~~~ + +All http3 details are rooted at *http3.* + +.. csv-table:: + :header: Name, Description + :widths: 1, 2 + + http3.invalid_header_field, One of the HTTP/3 headers was invalid + http3.headers.too.large, The size of headers (or trailers) exceeded the configured limits + http3.unexpected_underscore, Envoy was configured to drop or reject requests with header keys beginning with underscores. diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 1df97d761ccc..43cafcf76513 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -31,6 +31,23 @@ namespace Envoy { namespace Quic { +// Changes or additions to details should be reflected in +// docs/root/configuration/http/http_conn_man/response_code_details_details.rst +class Http3ResponseCodeDetailValues { +public: + // Invalid HTTP header field was received and stream is going to be + // closed. + static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field"; + // The size of headers (or trailers) exceeded the configured limits. + static constexpr absl::string_view headers_too_large = "http3.headers.too.large"; + // Envoy was configured to drop requests with header keys beginning with underscores. + static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore"; + // The peer refused the stream. + static constexpr absl::string_view remote_refused = "http3.remote_refuse"; + // The peer reset the stream. + static constexpr absl::string_view remote_reset = "http3.remote_reset"; +}; + EnvoyQuicServerStream::EnvoyQuicServerStream( quic::QuicStreamId id, quic::QuicSpdySession* session, quic::StreamType type, Http::Http3::CodecStats& stats, @@ -175,6 +192,7 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, quicHeadersToEnvoyHeaders(header_list, *this); if (headers == nullptr) { if (close_connection_upon_invalid_header_) { + details_ = Http3ResponseCodeDetailValues::invalid_http_header; stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); } else { Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); @@ -182,6 +200,7 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, return; } if (!Http::HeaderUtility::authorityIsValid(headers->Host()->value().getStringView())) { + details_ = Http3ResponseCodeDetailValues::invalid_http_header; stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); return; } @@ -257,6 +276,7 @@ void EnvoyQuicServerStream::OnTrailingHeadersComplete(bool fin, size_t frame_len void EnvoyQuicServerStream::OnHeadersTooLarge() { ENVOY_STREAM_LOG(debug, "Headers too large.", *this); + details_ = Http3ResponseCodeDetailValues::headers_too_large; quic::QuicSpdyServerStreamBase::OnHeadersTooLarge(); } @@ -339,6 +359,7 @@ EnvoyQuicServerStream::checkHeaderNameForUnderscores(const std::string& header_n !Http::HeaderUtility::headerNameContainsUnderscore(header_name)) { return HeaderValidationResult::ACCEPT; } + details_ = Http3ResponseCodeDetailValues::invalid_underscore; if (headers_with_underscores_action_ == envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { ENVOY_STREAM_LOG(debug, "Dropping header with invalid characters in its name: {}", *this, diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index 0489ca0a516e..3cfdb4e3cb79 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -5,8 +5,8 @@ #include "envoy/http/codec.h" #include "common/http/codec_helper.h" -#include "common/quic/envoy_quic_utils.h" #include "common/quic/envoy_quic_simulated_watermark_buffer.h" +#include "common/quic/envoy_quic_utils.h" #include "common/quic/quic_filter_manager_connection_impl.h" namespace Envoy { @@ -146,6 +146,8 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, return HeaderValidationResult::ACCEPT; } + absl::string_view responseDetails() override { return details_; } + protected: virtual void switchStreamBlockState(bool should_block) PURE; @@ -165,6 +167,7 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, Http::Http3::CodecStats& stats_; const envoy::config::core::v3::Http3ProtocolOptions& http3_options_; bool close_connection_upon_invalid_header_{false}; + absl::string_view details_; private: // Keeps track of bytes buffered in the stream send buffer in QUICHE and reacts diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 3a1574763b18..7a12774b333d 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -99,10 +99,12 @@ Http::StreamResetReason quicRstErrorToEnvoyRemoteResetReason(quic::QuicRstStream } Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode error) { - if (error == quic::QUIC_NO_ERROR) { - return Http::StreamResetReason::ConnectionTermination; - } else { + switch (error) { + case quic::QUIC_HANDSHAKE_FAILED: + case quic::QUIC_HANDSHAKE_TIMEOUT: return Http::StreamResetReason::ConnectionFailure; + default: + return Http::StreamResetReason::ConnectionTermination; } } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index ec02d9f8c9e0..070b56ed4672 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1098,8 +1098,6 @@ TEST_P(ProtocolIntegrationTest, MaxStreamDurationWithRetryPolicyWhenRetryUpstrea // Verify that headers with underscores in their names are dropped from client requests // but remain in upstream responses. TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresDropped) { - // TODO(danzh) treat underscore in headers according to the config. - EXCLUDE_DOWNSTREAM_HTTP3 config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { @@ -1133,7 +1131,7 @@ TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresDropped) { stat_name = "http2.dropped_headers_with_underscores"; break; case Http::CodecClient::Type::HTTP3: - // TODO(danzh) add stats for H3. + stat_name = "http3.dropped_headers_with_underscores"; break; default: RELEASE_ASSERT(false, fmt::format("Unknown downstream protocol {}", downstream_protocol_)); @@ -1165,8 +1163,6 @@ TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresRemainByDefault) { // Verify that request with headers containing underscores is rejected when configured. TEST_P(DownstreamProtocolIntegrationTest, HeadersWithUnderscoresCauseRequestRejectedByDefault) { - // TODO(danzh) pass headers_with_underscores_action config into QUIC stream. - EXCLUDE_DOWNSTREAM_HTTP3 useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE_DETAILS%"); config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& @@ -1384,8 +1380,6 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingMany) { } TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLength) { - // TODO(danzh) Add content length validation. - EXCLUDE_DOWNSTREAM_HTTP3 initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -1410,8 +1404,6 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLength) { } TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) { - // TODO(danzh) Add override_stream_error_on_invalid_http_message to http3 protocol options. - EXCLUDE_DOWNSTREAM_HTTP3 config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { @@ -1454,8 +1446,6 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) { } TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { - // TODO(danzh) Add content length validation. - EXCLUDE_DOWNSTREAM_HTTP3 initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); auto encoder_decoder = @@ -1478,8 +1468,6 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { // TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc. TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { - // override_stream_error_on_invalid_http_message not supported yet. - EXCLUDE_DOWNSTREAM_HTTP3 config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { From f8b7f03fec48f93dd84a9f22d229451b287064bf Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 30 Mar 2021 13:17:38 -0400 Subject: [PATCH 04/13] fis protodoc Signed-off-by: Dan Zhang --- .../v3/http_connection_manager.proto | 3 +- .../v4alpha/http_connection_manager.proto | 3 +- .../v3/http_connection_manager.proto | 3 +- .../v4alpha/http_connection_manager.proto | 3 +- source/common/http/http3/codec_stats.h | 44 +++++++++++++++++++ 5 files changed, 48 insertions(+), 8 deletions(-) create mode 100644 source/common/http/http3/codec_stats.h 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 1e4574a0b687..a84a7d85d976 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 @@ -326,8 +326,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. - config.core.v3.Http3ProtocolOptions http3_protocol_options = 44 - [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + config.core.v3.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 61f7344ecf2e..23c051ed59cf 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -329,8 +329,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. - config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44 - [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 1891b791e8cb..aaa8108d1778 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -330,8 +330,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. - config.core.v3.Http3ProtocolOptions http3_protocol_options = 44 - [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + config.core.v3.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 61f7344ecf2e..23c051ed59cf 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -329,8 +329,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. - config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44 - [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server // header in responses. If not set, the default is *envoy*. diff --git a/source/common/http/http3/codec_stats.h b/source/common/http/http3/codec_stats.h new file mode 100644 index 000000000000..bb720172907c --- /dev/null +++ b/source/common/http/http3/codec_stats.h @@ -0,0 +1,44 @@ +#pragma once + +#include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" + +#include "common/common/thread.h" + +namespace Envoy { +namespace Http { +namespace Http3 { + +/** + * All stats for the HTTP/3 codec. @see stats_macros.h + * TODO(danzh) populate all of them in codec. + */ +#define ALL_HTTP3_CODEC_STATS(COUNTER, GAUGE) \ + COUNTER(dropped_headers_with_underscores) \ + COUNTER(header_overflow) \ + COUNTER(requests_rejected_with_underscores_in_headers) \ + COUNTER(rx_messaging_error) \ + COUNTER(rx_reset) \ + COUNTER(trailers) \ + COUNTER(tx_reset) \ + GAUGE(streams_active, Accumulate) + +/** + * Wrapper struct for the HTTP/3 codec stats. @see stats_macros.h + */ +struct CodecStats { + using AtomicPtr = Thread::AtomicPtr; + + static CodecStats& atomicGet(AtomicPtr& ptr, Stats::Scope& scope) { + return *ptr.get([&scope]() -> CodecStats* { + return new CodecStats{ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope, "http3."), + POOL_GAUGE_PREFIX(scope, "http3."))}; + }); + } + + ALL_HTTP3_CODEC_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) +}; + +} // namespace Http3 +} // namespace Http +} // namespace Envoy From 720d7927adf829d0ac290e287dde32cfbbb34188 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 30 Mar 2021 22:22:34 -0400 Subject: [PATCH 05/13] move some checks to HeaderUtility Signed-off-by: Dan Zhang --- .../v3/http_connection_manager.proto | 1 + source/common/http/BUILD | 1 + source/common/http/header_utility.cc | 52 +++++++++++++++++++ source/common/http/header_utility.h | 31 +++++++++++ source/common/quic/BUILD | 2 +- .../common/quic/envoy_quic_server_stream.cc | 41 ++++++--------- source/common/quic/envoy_quic_server_stream.h | 6 +-- source/common/quic/envoy_quic_stream.h | 44 +++------------- source/common/quic/envoy_quic_utils.h | 23 ++++---- .../quic_filter_manager_connection_impl.h | 1 - test/common/http/header_utility_test.cc | 44 ++++++++++++++++ test/common/quic/envoy_quic_utils_test.cc | 10 ++-- 12 files changed, 169 insertions(+), 87 deletions(-) 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 a84a7d85d976..51c4e229f137 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 @@ -326,6 +326,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + // [#not-implemented-hide:] config.core.v3.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server diff --git a/source/common/http/BUILD b/source/common/http/BUILD index afcf183e5be8..9d4813bbab96 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -427,6 +427,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/protobuf:utility_lib", "//source/common/runtime:runtime_features_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/route/v3:pkg_cc_proto", "@envoy_api//envoy/type/v3:pkg_cc_proto", ], diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 05482e6ad2c8..5d8dc4f3a899 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -333,5 +333,57 @@ bool HeaderUtility::isModifiableHeader(absl::string_view header) { !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get())); } +HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscores( + const std::string& header_name, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action, + Stats::Counter& dropped_headers_with_underscores, + Stats::Counter& requests_rejected_with_underscores_in_headers) { + if (headers_with_underscores_action == envoy::config::core::v3::HttpProtocolOptions::ALLOW || + !HeaderUtility::headerNameContainsUnderscore(header_name)) { + return HeaderValidationResult::ACCEPT; + } + if (headers_with_underscores_action == + envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { + ENVOY_LOG_MISC(debug, "Dropping header with invalid characters in its name: {}", header_name); + dropped_headers_with_underscores.inc(); + return HeaderValidationResult::DROP; + } + ENVOY_LOG_MISC(debug, "Rejecting request due to header name with underscores: {}", header_name); + requests_rejected_with_underscores_in_headers.inc(); + return HeaderUtility::HeaderValidationResult::REJECT; +} + +HeaderUtility::HeaderValidationResult +HeaderUtility::validateContentLength(absl::string_view header_value, + bool override_stream_error_on_invalid_http_message, + bool& should_close_connection) { + should_close_connection = false; + std::vector values = absl::StrSplit(header_value, ','); + absl::optional content_length; + for (const absl::string_view& value : values) { + uint64_t new_value; + if (!absl::SimpleAtoi(value, &new_value) || + !std::all_of(value.begin(), value.end(), absl::ascii_isdigit)) { + ENVOY_LOG_MISC(debug, "Content length was either unparseable or negative"); + should_close_connection = !override_stream_error_on_invalid_http_message; + return HeaderValidationResult::REJECT; + } + if (!content_length.has_value()) { + content_length = new_value; + continue; + } + if (new_value != content_length.value()) { + ENVOY_LOG_MISC( + debug, + "Parsed content length {} is inconsistent with previously detected content length {}", + new_value, content_length.value()); + should_close_connection = !override_stream_error_on_invalid_http_message; + return HeaderValidationResult::REJECT; + } + } + return HeaderValidationResult::ACCEPT; +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 5fa5b66fc867..c4985aae780c 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -3,6 +3,7 @@ #include #include "envoy/common/regex.h" +#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/config/route/v3/route_components.pb.h" #include "envoy/http/header_map.h" #include "envoy/http/protocol.h" @@ -209,6 +210,36 @@ class HeaderUtility { * may not be modified. */ static bool isModifiableHeader(absl::string_view header); + + enum class HeaderValidationResult { + ACCEPT = 0, + DROP, + REJECT, + }; + + /** + * Check if the given |header_name| has underscore. + * Return HeaderValidationResult and populate the given counters based on + * |headers_with_underscores_action|. + */ + static HeaderValidationResult checkHeaderNameForUnderscores( + const std::string& header_name, + envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction + headers_with_underscores_action, + Stats::Counter& dropped_headers_with_underscores, + Stats::Counter& requests_rejected_with_underscores_in_headers); + + /** + * Check if |header_value| represents valid value for HTTP content-length + * header. + * Return HeaderValidationResult and populate |should_close_connection| + * according to |override_stream_error_on_invalid_http_message|. + */ + static HeaderValidationResult + validateContentLength(absl::string_view header_value, + bool override_stream_error_on_invalid_http_message, + bool& should_close_connection); }; + } // namespace Http } // namespace Envoy diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index 1f8a3a6794bc..dcefc935260e 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -196,7 +196,6 @@ envoy_cc_library( "//source/common/http/http3:codec_stats_lib", "//source/common/network:connection_base_lib", "//source/common/stream_info:stream_info_lib", - "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) @@ -355,6 +354,7 @@ envoy_cc_library( deps = [ "//include/envoy/http:codec_interface", "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/network:address_lib", "//source/common/network:listen_socket_lib", "//source/common/network:socket_option_factory_lib", diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 1630731aa030..d62fb58e2cfe 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -172,8 +172,10 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, std::unique_ptr headers = quicHeadersToEnvoyHeaders(header_list, *this); if (headers == nullptr) { - if (close_connection_upon_invalid_header_) { + if (details_.empty()) { details_ = Http3ResponseCodeDetailValues::invalid_http_header; + } + if (close_connection_upon_invalid_header_) { stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); } else { Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); @@ -334,33 +336,22 @@ QuicFilterManagerConnectionImpl* EnvoyQuicServerStream::filterManagerConnection( return dynamic_cast(session()); } -HeaderValidationResult EnvoyQuicServerStream::validateHeader(const std::string& header_name, - absl::string_view header_value) { - HeaderValidationResult result = EnvoyQuicStream::validateHeader(header_name, header_value); - if (result != HeaderValidationResult::ACCEPT) { +Http::HeaderUtility::HeaderValidationResult +EnvoyQuicServerStream::validateHeader(const std::string& header_name, + absl::string_view header_value) { + Http::HeaderUtility::HeaderValidationResult result = + EnvoyQuicStream::validateHeader(header_name, header_value); + if (result != Http::HeaderUtility::HeaderValidationResult::ACCEPT) { return result; } - return checkHeaderNameForUnderscores(header_name); -} - -HeaderValidationResult -EnvoyQuicServerStream::checkHeaderNameForUnderscores(const std::string& header_name) { - if (headers_with_underscores_action_ == envoy::config::core::v3::HttpProtocolOptions::ALLOW || - !Http::HeaderUtility::headerNameContainsUnderscore(header_name)) { - return HeaderValidationResult::ACCEPT; - } - details_ = Http3ResponseCodeDetailValues::invalid_underscore; - if (headers_with_underscores_action_ == - envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) { - ENVOY_STREAM_LOG(debug, "Dropping header with invalid characters in its name: {}", *this, - header_name); - stats_.dropped_headers_with_underscores_.inc(); - return HeaderValidationResult::DROP; + // Do request specific checks. + result = Http::HeaderUtility::checkHeaderNameForUnderscores( + header_name, headers_with_underscores_action_, stats_.dropped_headers_with_underscores_, + stats_.requests_rejected_with_underscores_in_headers_); + if (result != Http::HeaderUtility::HeaderValidationResult::ACCEPT) { + details_ = Http3ResponseCodeDetailValues::invalid_underscore; } - ENVOY_STREAM_LOG(debug, "Rejecting request due to header name with underscores: {}", *this, - header_name); - stats_.requests_rejected_with_underscores_in_headers_.inc(); - return HeaderValidationResult::INVALID; + return result; } } // namespace Quic diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index b81889f3cd39..9a655288ca5d 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -67,8 +67,8 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, void clearWatermarkBuffer(); // EnvoyQuicStream - HeaderValidationResult validateHeader(const std::string& header_name, - absl::string_view header_value) override; + Http::HeaderUtility::HeaderValidationResult + validateHeader(const std::string& header_name, absl::string_view header_value) override; protected: // EnvoyQuicStream @@ -89,8 +89,6 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, // Deliver awaiting trailers if body has been delivered. void maybeDecodeTrailers(); - HeaderValidationResult checkHeaderNameForUnderscores(const std::string& header_name); - Http::RequestDecoder* request_decoder_{nullptr}; envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index 451b00c6acab..b1e390130b99 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -107,49 +107,17 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, filter_manager_connection_.updateBytesBuffered(old_buffered_bytes, new_buffered_bytes); } - HeaderValidationResult validateHeader(const std::string& header_name, - absl::string_view header_value) override { + Http::HeaderUtility::HeaderValidationResult + validateHeader(const std::string& header_name, absl::string_view header_value) override { bool override_stream_error_on_invalid_http_message = http3_options_.has_override_stream_error_on_invalid_http_message() && http3_options_.override_stream_error_on_invalid_http_message().value(); if (header_name == "content-length") { - return validateContentLength(header_value, override_stream_error_on_invalid_http_message); + return Http::HeaderUtility::validateContentLength( + header_value, override_stream_error_on_invalid_http_message, + close_connection_upon_invalid_header_); } - return HeaderValidationResult::ACCEPT; - } - - HeaderValidationResult validateContentLength(absl::string_view header_value, - bool override_stream_error_on_invalid_http_message) { - std::vector values = absl::StrSplit(header_value, ','); - absl::optional content_length; - for (const absl::string_view& value : values) { - uint64_t new_value; - if (!absl::SimpleAtoi(value, &new_value) || - !std::all_of(value.begin(), value.end(), absl::ascii_isdigit)) { - ENVOY_STREAM_LOG(debug, "Content length was either unparseable or negative", *this); - // TODO(danzh) set value according to override_stream_error_on_invalid_http_message from - // configured http3 options. - if (!override_stream_error_on_invalid_http_message) { - close_connection_upon_invalid_header_ = true; - } - return HeaderValidationResult::INVALID; - } - if (!content_length.has_value()) { - content_length = new_value; - continue; - } - if (new_value != content_length.value()) { - ENVOY_STREAM_LOG( - debug, - "Parsed content length {} is inconsistent with previously detected content length {}", - *this, new_value, content_length.value()); - if (!override_stream_error_on_invalid_http_message) { - close_connection_upon_invalid_header_ = true; - } - return HeaderValidationResult::INVALID; - } - } - return HeaderValidationResult::ACCEPT; + return Http::HeaderUtility::HeaderValidationResult::ACCEPT; } absl::string_view responseDetails() override { return details_; } diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index ffeefcd0f356..a8b2430767b2 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -24,6 +24,7 @@ #include "quiche/quic/core/quic_error_codes.h" #include "quiche/quic/platform/api/quic_ip_address.h" #include "quiche/quic/platform/api/quic_socket_address.h" +#include "common/http/header_utility.h" #include "openssl/ssl.h" @@ -37,17 +38,11 @@ quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address); quic::QuicSocketAddress envoyIpAddressToQuicSocketAddress(const Network::Address::Ip* envoy_ip); -enum class HeaderValidationResult { - ACCEPT = 0, - DROP, - INVALID, -}; - class HeaderValidator { public: virtual ~HeaderValidator() = default; - virtual HeaderValidationResult validateHeader(const std::string& header_name, - absl::string_view header_value) = 0; + virtual Http::HeaderUtility::HeaderValidationResult + validateHeader(const std::string& header_name, absl::string_view header_value) = 0; }; // The returned header map has all keys in lower case. @@ -56,12 +51,14 @@ std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_ HeaderValidator& validator) { auto headers = T::create(); for (const auto& entry : header_list) { - HeaderValidationResult result = validator.validateHeader(entry.first, entry.second); - if (result == HeaderValidationResult::INVALID) { + Http::HeaderUtility::HeaderValidationResult result = + validator.validateHeader(entry.first, entry.second); + switch (result) { + case Http::HeaderUtility::HeaderValidationResult::REJECT: return nullptr; - } - - if (result == HeaderValidationResult::ACCEPT) { + case Http::HeaderUtility::HeaderValidationResult::DROP: + continue; + case Http::HeaderUtility::HeaderValidationResult::ACCEPT: auto key = Http::LowerCaseString(entry.first); if (key != Http::Headers::get().Cookie) { // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. diff --git a/source/common/quic/quic_filter_manager_connection_impl.h b/source/common/quic/quic_filter_manager_connection_impl.h index d80b502b4cf2..19547646ea51 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.h +++ b/source/common/quic/quic_filter_manager_connection_impl.h @@ -2,7 +2,6 @@ #include -#include "envoy/config/core/v3/protocol.pb.h" #include "envoy/event/dispatcher.h" #include "envoy/network/connection.h" diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 15d02fc00343..16bc38719c8d 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -722,5 +722,49 @@ TEST(RequiredHeaders, IsModifiableHeader) { EXPECT_TRUE(HeaderUtility::isModifiableHeader("Content-Type")); } +TEST(ValidateHeaders, HeaderNameWithUnderscores) { + Stats::MockCounter dropped; + Stats::MockCounter rejected; + EXPECT_CALL(dropped, inc()); + EXPECT_CALL(rejected, inc()).Times(0u); + EXPECT_EQ(HeaderUtility::HeaderValidationResult::DROP, + HeaderUtility::checkHeaderNameForUnderscores( + "header_with_underscore", envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER, + dropped, rejected)); + + EXPECT_CALL(dropped, inc()).Times(0u); + EXPECT_CALL(rejected, inc()); + EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::checkHeaderNameForUnderscores( + "header_with_underscore", + envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST, dropped, rejected)); + + EXPECT_EQ(HeaderUtility::HeaderValidationResult::ACCEPT, + HeaderUtility::checkHeaderNameForUnderscores( + "header_with_underscore", envoy::config::core::v3::HttpProtocolOptions::ALLOW, + dropped, rejected)); + + EXPECT_EQ(HeaderUtility::HeaderValidationResult::ACCEPT, + HeaderUtility::checkHeaderNameForUnderscores( + "header", envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST, dropped, + rejected)); +} + +TEST(ValidateHeaders, ContentLength) { + bool should_close_connection; + EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("1, 2", true, should_close_connection)); + EXPECT_FALSE(should_close_connection); + + EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("1, 2", false, should_close_connection)); + EXPECT_TRUE(should_close_connection); +} + +EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("-1", false, should_close_connection)); +EXPECT_TRUE(should_close_connection); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/quic/envoy_quic_utils_test.cc b/test/common/quic/envoy_quic_utils_test.cc index e37e79daf7e9..2ba153ba9a50 100644 --- a/test/common/quic/envoy_quic_utils_test.cc +++ b/test/common/quic/envoy_quic_utils_test.cc @@ -48,7 +48,7 @@ TEST(EnvoyQuicUtilsTest, ConversionBetweenQuicAddressAndEnvoyAddress) { class MockHeaderValidator : public HeaderValidator { public: ~MockHeaderValidator() override = default; - MOCK_METHOD(HeaderValidationResult, validateHeader, + MOCK_METHOD(Http::HeaderUtility::HeaderValidationResult, validateHeader, (const std::string& header_name, absl::string_view header_value)); }; @@ -83,9 +83,9 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { EXPECT_CALL(validator, validateHeader(_, _)) .WillRepeatedly([](const std::string& header_name, absl::string_view) { if (header_name == "key-to-drop") { - return HeaderValidationResult::DROP; + return Http::HeaderUtility::HeaderValidationResult::DROP; } - return HeaderValidationResult::ACCEPT; + return Http::HeaderUtility::HeaderValidationResult::ACCEPT; }); auto envoy_headers2 = quicHeadersToEnvoyHeaders(quic_headers, validator); @@ -101,9 +101,9 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { EXPECT_CALL(validator, validateHeader(_, _)) .WillRepeatedly([](const std::string& header_name, absl::string_view) { if (header_name == "invalid_key") { - return HeaderValidationResult::INVALID; + return Http::HeaderUtility::HeaderValidationResult::REJECT; } - return HeaderValidationResult::ACCEPT; + return Http::HeaderUtility::HeaderValidationResult::ACCEPT; }); EXPECT_EQ(nullptr, quicHeadersToEnvoyHeaders(quic_headers2, validator)); From 85b7003e0677238c567137acd80ef463eaa0ed5d Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 30 Mar 2021 22:33:50 -0400 Subject: [PATCH 06/13] protofmt Signed-off-by: Dan Zhang --- .../v4alpha/http_connection_manager.proto | 1 + .../http_connection_manager/v3/http_connection_manager.proto | 1 + .../v4alpha/http_connection_manager.proto | 1 + 3 files changed, 3 insertions(+) diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 23c051ed59cf..d442451cdae6 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -329,6 +329,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + // [#not-implemented-hide:] config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index aaa8108d1778..4557359d93a7 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -330,6 +330,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + // [#not-implemented-hide:] config.core.v3.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 23c051ed59cf..d442451cdae6 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -329,6 +329,7 @@ message HttpConnectionManager { [(udpa.annotations.security).configure_for_untrusted_downstream = true]; // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec. + // [#not-implemented-hide:] config.core.v4alpha.Http3ProtocolOptions http3_protocol_options = 44; // An optional override that the connection manager will write to the server From e55bd8a3d4b76a4c8187807c2541b4c700f650af Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Wed, 31 Mar 2021 00:58:51 -0400 Subject: [PATCH 07/13] fix test Signed-off-by: Dan Zhang --- test/common/http/header_utility_test.cc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 16bc38719c8d..ca358e2a7a05 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -752,18 +752,21 @@ TEST(ValidateHeaders, HeaderNameWithUnderscores) { TEST(ValidateHeaders, ContentLength) { bool should_close_connection; + EXPECT_EQ(HeaderUtility::HeaderValidationResult::ACCEPT, + HeaderUtility::validateContentLength("1,1", true, should_close_connection)); + EXPECT_FALSE(should_close_connection); + EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, - HeaderUtility::validateContentLength("1, 2", true, should_close_connection)); + HeaderUtility::validateContentLength("1,2", true, should_close_connection)); EXPECT_FALSE(should_close_connection); EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, - HeaderUtility::validateContentLength("1, 2", false, should_close_connection)); + HeaderUtility::validateContentLength("1,2", false, should_close_connection)); EXPECT_TRUE(should_close_connection); -} -EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, - HeaderUtility::validateContentLength("-1", false, should_close_connection)); -EXPECT_TRUE(should_close_connection); + EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("-1", false, should_close_connection)); + EXPECT_TRUE(should_close_connection); } } // namespace Http From 5004e7075317d90f91fa2889ba5316c506d46105 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 1 Apr 2021 01:57:15 -0400 Subject: [PATCH 08/13] remove codec factory Signed-off-by: Dan Zhang --- source/common/http/http3/quic_codec_factory.h | 46 ------------------- 1 file changed, 46 deletions(-) delete mode 100644 source/common/http/http3/quic_codec_factory.h diff --git a/source/common/http/http3/quic_codec_factory.h b/source/common/http/http3/quic_codec_factory.h deleted file mode 100644 index 51daef5b484c..000000000000 --- a/source/common/http/http3/quic_codec_factory.h +++ /dev/null @@ -1,46 +0,0 @@ -#pragma once - -#include - -#include "envoy/config/core/v3/protocol.pb.h" -#include "envoy/config/typed_config.h" -#include "envoy/http/codec.h" -#include "envoy/network/connection.h" - -#include "common/http/http3/codec_stats.h" - -namespace Envoy { -namespace Http { - -// A factory to create Http::ServerConnection instance for QUIC. -class QuicHttpServerConnectionFactory : public Config::UntypedFactory { -public: - ~QuicHttpServerConnectionFactory() override = default; - - virtual std::unique_ptr createQuicServerConnection( - Network::Connection& connection, ConnectionCallbacks& callbacks, - Http::Http3::CodecStats& stats, - const envoy::config::core::v3::Http3ProtocolOptions& http3_options, - const uint32_t max_request_headers_kb, - envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action) PURE; - - std::string category() const override { return "envoy.quic_client_codec"; } -}; - -// A factory to create Http::ClientConnection instance for QUIC. -class QuicHttpClientConnectionFactory : public Config::UntypedFactory { -public: - ~QuicHttpClientConnectionFactory() override = default; - - virtual std::unique_ptr - createQuicClientConnection(Network::Connection& connection, ConnectionCallbacks& callbacks, - Http::Http3::CodecStats& stats, - const envoy::config::core::v3::Http3ProtocolOptions& http3_options, - const uint32_t max_request_headers_kb) PURE; - - std::string category() const override { return "envoy.quic_server_codec"; } -}; - -} // namespace Http -} // namespace Envoy From 511e79a5ef0d632e73119a31c79d460764aae6f2 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 1 Apr 2021 11:48:24 -0400 Subject: [PATCH 09/13] address comment Signed-off-by: Dan Zhang --- source/common/http/header_utility.h | 11 +++++------ source/common/http/utility.cc | 7 ++++++- source/common/quic/envoy_quic_stream.h | 1 - .../common/quic/quic_filter_manager_connection_impl.h | 2 +- test/integration/BUILD | 1 + 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index c4985aae780c..e5d58f39ca8e 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -218,9 +218,9 @@ class HeaderUtility { }; /** - * Check if the given |header_name| has underscore. + * Check if the given header_name has underscore. * Return HeaderValidationResult and populate the given counters based on - * |headers_with_underscores_action|. + * headers_with_underscores_action. */ static HeaderValidationResult checkHeaderNameForUnderscores( const std::string& header_name, @@ -230,10 +230,9 @@ class HeaderUtility { Stats::Counter& requests_rejected_with_underscores_in_headers); /** - * Check if |header_value| represents valid value for HTTP content-length - * header. - * Return HeaderValidationResult and populate |should_close_connection| - * according to |override_stream_error_on_invalid_http_message|. + * Check if header_value represents a valid value for HTTP content-length header. + * Return HeaderValidationResult and populate should_close_connection + * according to override_stream_error_on_invalid_http_message. */ static HeaderValidationResult validateContentLength(absl::string_view header_value, diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index f02199862952..2ee0b2f7fce3 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -207,12 +207,17 @@ envoy::config::core::v3::Http3ProtocolOptions initializeAndValidateOptions(const envoy::config::core::v3::Http3ProtocolOptions& options, bool hcm_stream_error_set, const Protobuf::BoolValue& hcm_stream_error) { + if (options.has_override_stream_error_on_invalid_http_message()) { + return options; + } envoy::config::core::v3::Http3ProtocolOptions options_clone(options); if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.hcm_stream_error_on_invalid_message") && - !options.has_override_stream_error_on_invalid_http_message() && hcm_stream_error_set) { + hcm_stream_error_set) { options_clone.mutable_override_stream_error_on_invalid_http_message()->set_value( hcm_stream_error.value()); + } else { + options_clone.mutable_override_stream_error_on_invalid_http_message()->set_value(false); } return options_clone; } diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index b1e390130b99..9fbc4fb084e0 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -110,7 +110,6 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, Http::HeaderUtility::HeaderValidationResult validateHeader(const std::string& header_name, absl::string_view header_value) override { bool override_stream_error_on_invalid_http_message = - http3_options_.has_override_stream_error_on_invalid_http_message() && http3_options_.override_stream_error_on_invalid_http_message().value(); if (header_name == "content-length") { return Http::HeaderUtility::validateContentLength( diff --git a/source/common/quic/quic_filter_manager_connection_impl.h b/source/common/quic/quic_filter_manager_connection_impl.h index 19547646ea51..e897042b6de7 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.h +++ b/source/common/quic/quic_filter_manager_connection_impl.h @@ -164,7 +164,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, // Keeps the buffer state of the connection, and react upon the changes of how many bytes are // buffered cross all streams' send buffer. The state is evaluated and may be changed upon each // stream write. QUICHE doesn't buffer data in connection, all the data is buffered in stream's - // send buffer.` + // send buffer. EnvoyQuicSimulatedWatermarkBuffer write_buffer_watermark_simulation_; Buffer::OwnedImpl empty_buffer_; }; diff --git a/test/integration/BUILD b/test/integration/BUILD index d0b2981777d8..876f0f8a8e8f 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -656,6 +656,7 @@ envoy_cc_test_library( "//source/common/common:callback_impl_lib", "//source/common/common:linked_object", "//source/common/common:lock_guard_lib", + "//source/common/http/http3:codec_stats_lib", "//source/common/common:thread_lib", "//source/common/config:utility_lib", "//source/common/grpc:codec_lib", From c4e80a1f92e0b1bd0477c0eeb5f10ff074e846f4 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 2 Apr 2021 15:10:33 -0400 Subject: [PATCH 10/13] address comments Signed-off-by: Dan Zhang --- .../http/http_conn_man/response_code_details.rst | 2 +- include/envoy/upstream/upstream.h | 2 +- source/common/http/http1/codec_impl.cc | 1 + source/common/http/http2/codec_impl.cc | 1 + source/common/quic/envoy_quic_server_session.cc | 6 ++++-- 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/response_code_details.rst b/docs/root/configuration/http/http_conn_man/response_code_details.rst index 3f220a4e6b08..ebca3a321f2e 100644 --- a/docs/root/configuration/http/http_conn_man/response_code_details.rst +++ b/docs/root/configuration/http/http_conn_man/response_code_details.rst @@ -110,5 +110,5 @@ All http3 details are rooted at *http3.* :widths: 1, 2 http3.invalid_header_field, One of the HTTP/3 headers was invalid - http3.headers.too.large, The size of headers (or trailers) exceeded the configured limits + http3.headers_too_large, The size of headers (or trailers) exceeded the configured limits http3.unexpected_underscore, Envoy was configured to drop or reject requests with header keys beginning with underscores. diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index f791e1c4911c..dc628e6139db 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -996,7 +996,7 @@ class ClusterInfo { virtual Http::Http2::CodecStats& http2CodecStats() const PURE; /** - * @return the Http2 Codec Stats. + * @return the Http3 Codec Stats. */ virtual Http::Http3::CodecStats& http3CodecStats() const PURE; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index e0505d3265cd..3b245d4f8712 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -474,6 +474,7 @@ Status ConnectionImpl::completeLastHeader() { ENVOY_CONN_LOG(trace, "completed header: key={} value={}", connection_, current_header_field_.getStringView(), current_header_value_.getStringView()); + // TODO(10646): Switch to use HeaderUtility::checkHeaderNameForUnderscores(). RETURN_IF_ERROR(checkHeaderNameForUnderscores()); auto& headers_or_trailers = headersOrTrailers(); if (!current_header_field_.empty()) { diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 125a837ce814..41c083acf2ac 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -1085,6 +1085,7 @@ int ConnectionImpl::saveHeader(const nghttp2_frame* frame, HeaderString&& name, return 0; } + // TODO(10646): Switch to use HeaderUtility::checkHeaderNameForUnderscores(). auto should_return = checkHeaderNameForUnderscores(name.getStringView()); if (should_return) { stream->setDetails(Http2ResponseCodeDetails::get().invalid_underscore); diff --git a/source/common/quic/envoy_quic_server_session.cc b/source/common/quic/envoy_quic_server_session.cc index 9431a0135180..d13da33e4110 100644 --- a/source/common/quic/envoy_quic_server_session.cc +++ b/source/common/quic/envoy_quic_server_session.cc @@ -44,8 +44,10 @@ quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::QuicStr return nullptr; } if (!codec_stats_.has_value() || !http3_options_.has_value()) { - ENVOY_CONN_LOG(error, "Attempt to create stream {} before HCM filter is initialized.", *this, - id); + ENVOY_BUG(false, + fmt::format( + "Quic session {} attempts to create stream {} before HCM filter is initialized.", + this->id(), id)); return nullptr; } auto stream = new EnvoyQuicServerStream(id, this, quic::BIDIRECTIONAL, codec_stats_.value(), From 7f0180538c8c196396d65f3666bf46c8c22a3984 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 2 Apr 2021 18:40:07 -0400 Subject: [PATCH 11/13] enable more protocol test with client stream fix Signed-off-by: Dan Zhang --- .../common/quic/envoy_quic_client_stream.cc | 25 ++++++++-- source/common/quic/envoy_quic_client_stream.h | 4 ++ .../common/quic/envoy_quic_server_stream.cc | 50 ++++++++----------- source/common/quic/envoy_quic_server_stream.h | 4 ++ source/common/quic/envoy_quic_stream.h | 17 +++++++ source/common/quic/envoy_quic_utils.cc | 14 +++++- source/common/quic/envoy_quic_utils.h | 7 ++- test/integration/protocol_integration_test.cc | 18 +++---- 8 files changed, 93 insertions(+), 46 deletions(-) diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 911d423da7ad..1b9dcf6b2505 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -131,7 +131,7 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, } quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list); if (!headers_decompressed() || header_list.empty()) { - Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value()); return; } @@ -141,10 +141,15 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, } std::unique_ptr headers = quicHeadersToEnvoyHeaders(header_list, *this); + if (headers == nullptr) { + onStreamError(close_connection_upon_invalid_header_); + return; + } const absl::optional optional_status = Http::Utility::getResponseStatusNoThrow(*headers); if (!optional_status.has_value()) { - Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + details_ = Http3ResponseCodeDetailValues::invalid_http_header; + onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value()); return; } const uint64_t status = optional_status.value(); @@ -262,7 +267,9 @@ void EnvoyQuicClientStream::Reset(quic::QuicRstStreamErrorCode error) { void EnvoyQuicClientStream::OnConnectionClosed(quic::QuicErrorCode error, quic::ConnectionCloseSource source) { if (!end_stream_decoded_) { - runResetCallbacks(quicErrorCodeToEnvoyResetReason(error)); + runResetCallbacks(source == quic::ConnectionCloseSource::FROM_SELF + ? quicErrorCodeToEnvoyLocalResetReason(error) + : quicErrorCodeToEnvoyRemoteResetReason(error)); } quic::QuicSpdyClientStream::OnConnectionClosed(error, source); } @@ -300,5 +307,17 @@ QuicFilterManagerConnectionImpl* EnvoyQuicClientStream::filterManagerConnection( return dynamic_cast(session()); } +void EnvoyQuicClientStream::onStreamError(bool close_connection_upon_invalid_header) { + if (details_.empty()) { + details_ = Http3ResponseCodeDetailValues::invalid_http_header; + } + + if (close_connection_upon_invalid_header) { + stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); + } else { + Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + } +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index 19e0cd1f67df..486ac9e6565e 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -76,6 +76,10 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, // Deliver awaiting trailers if body has been delivered. void maybeDecodeTrailers(); + // Either reset the stream or close the connection according to + // close_connection_upon_invalid_header. + void onStreamError(bool close_connection_upon_invalid_header); + Http::ResponseDecoder* response_decoder_{nullptr}; bool decoded_100_continue_{false}; diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 4c30b8104cba..bb9b4867ac3e 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -31,23 +31,6 @@ namespace Envoy { namespace Quic { -// Changes or additions to details should be reflected in -// docs/root/configuration/http/http_conn_man/response_code_details_details.rst -class Http3ResponseCodeDetailValues { -public: - // Invalid HTTP header field was received and stream is going to be - // closed. - static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field"; - // The size of headers (or trailers) exceeded the configured limits. - static constexpr absl::string_view headers_too_large = "http3.headers_too_large"; - // Envoy was configured to drop requests with header keys beginning with underscores. - static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore"; - // The peer refused the stream. - static constexpr absl::string_view remote_refused = "http3.remote_refuse"; - // The peer reset the stream. - static constexpr absl::string_view remote_reset = "http3.remote_reset"; -}; - EnvoyQuicServerStream::EnvoyQuicServerStream( quic::QuicStreamId id, quic::QuicSpdySession* session, quic::StreamType type, Http::Http3::CodecStats& stats, @@ -164,7 +147,11 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, return; } quic::QuicSpdyServerStreamBase::OnInitialHeadersComplete(fin, frame_len, header_list); - ASSERT(headers_decompressed() && !header_list.empty()); + if (!headers_decompressed() || header_list.empty()) { + onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value()); + return; + } + ENVOY_STREAM_LOG(debug, "Received headers: {}.", *this, header_list.DebugString()); if (fin) { end_stream_decoded_ = true; @@ -172,19 +159,12 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, std::unique_ptr headers = quicHeadersToEnvoyHeaders(header_list, *this); if (headers == nullptr) { - if (details_.empty()) { - details_ = Http3ResponseCodeDetailValues::invalid_http_header; - } - if (close_connection_upon_invalid_header_) { - stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); - } else { - Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); - } + onStreamError(close_connection_upon_invalid_header_); return; } if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt) { details_ = Http3ResponseCodeDetailValues::invalid_http_header; - stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); + onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value()); return; } request_decoder_->decodeHeaders(std::move(headers), @@ -301,7 +281,9 @@ void EnvoyQuicServerStream::OnConnectionClosed(quic::QuicErrorCode error, // Run reset callback before closing the stream so that the watermark change will not trigger // callbacks. if (!local_end_stream_) { - runResetCallbacks(quicErrorCodeToEnvoyResetReason(error)); + runResetCallbacks(source == quic::ConnectionCloseSource::FROM_SELF + ? quicErrorCodeToEnvoyLocalResetReason(error) + : quicErrorCodeToEnvoyRemoteResetReason(error)); } quic::QuicSpdyServerStreamBase::OnConnectionClosed(error, source); } @@ -354,5 +336,17 @@ EnvoyQuicServerStream::validateHeader(const std::string& header_name, return result; } +void EnvoyQuicServerStream::onStreamError(bool close_connection_upon_invalid_header) { + if (details_.empty()) { + details_ = Http3ResponseCodeDetailValues::invalid_http_header; + } + + if (close_connection_upon_invalid_header) { + stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); + } else { + Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + } +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index 9a655288ca5d..9f2337a8faeb 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -89,6 +89,10 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, // Deliver awaiting trailers if body has been delivered. void maybeDecodeTrailers(); + // Either reset the stream or close the connection according to + // close_connection_upon_invalid_header. + void onStreamError(bool close_connection_upon_invalid_header); + Http::RequestDecoder* request_decoder_{nullptr}; envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index 9fbc4fb084e0..53e2623291b0 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -13,6 +13,23 @@ namespace Envoy { namespace Quic { +// Changes or additions to details should be reflected in +// docs/root/configuration/http/http_conn_man/response_code_details_details.rst +class Http3ResponseCodeDetailValues { +public: + // Invalid HTTP header field was received and stream is going to be + // closed. + static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field"; + // The size of headers (or trailers) exceeded the configured limits. + static constexpr absl::string_view headers_too_large = "http3.headers_too_large"; + // Envoy was configured to drop requests with header keys beginning with underscores. + static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore"; + // The peer refused the stream. + static constexpr absl::string_view remote_refused = "http3.remote_refuse"; + // The peer reset the stream. + static constexpr absl::string_view remote_reset = "http3.remote_reset"; +}; + // Base class for EnvoyQuicServer|ClientStream. class EnvoyQuicStream : public virtual Http::StreamEncoder, public Http::Stream, diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 7f91c0c752d1..3da9c12702ea 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -100,7 +100,19 @@ Http::StreamResetReason quicRstErrorToEnvoyRemoteResetReason(quic::QuicRstStream } } -Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode error) { +Http::StreamResetReason quicErrorCodeToEnvoyLocalResetReason(quic::QuicErrorCode error) { + switch (error) { + case quic::QUIC_HANDSHAKE_FAILED: + case quic::QUIC_HANDSHAKE_TIMEOUT: + return Http::StreamResetReason::ConnectionFailure; + case quic::QUIC_HTTP_FRAME_ERROR: + return Http::StreamResetReason::ProtocolError; + default: + return Http::StreamResetReason::ConnectionTermination; + } +} + +Http::StreamResetReason quicErrorCodeToEnvoyRemoteResetReason(quic::QuicErrorCode error) { switch (error) { case quic::QUIC_HANDSHAKE_FAILED: case quic::QUIC_HANDSHAKE_TIMEOUT: diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index a8b2430767b2..8898bd1e4971 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -99,8 +99,11 @@ Http::StreamResetReason quicRstErrorToEnvoyLocalResetReason(quic::QuicRstStreamE // Called when a QUIC stack reset the stream. Http::StreamResetReason quicRstErrorToEnvoyRemoteResetReason(quic::QuicRstStreamErrorCode rst_err); -// Called when underlying QUIC connection is closed either locally or by peer. -Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode error); +// Called when underlying QUIC connection is closed locally. +Http::StreamResetReason quicErrorCodeToEnvoyLocalResetReason(quic::QuicErrorCode error); + +// Called when underlying QUIC connection is closed by peer. +Http::StreamResetReason quicErrorCodeToEnvoyRemoteResetReason(quic::QuicErrorCode error); // Called when a GOAWAY frame is received. ABSL_MUST_USE_RESULT diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 58af3553ef66..cdfd284bf8f4 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -374,8 +374,7 @@ TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { } TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) { - // TODO(danzh) re-enable after plumbing through http2 option - // "allow_connect". + // TODO(danzh) re-enable after adding http3 option "allow_connect". EXCLUDE_DOWNSTREAM_HTTP3; EXCLUDE_UPSTREAM_HTTP3; // use after free. // Faulty filter that removed host in a CONNECT request. @@ -1274,12 +1273,7 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { default_response_headers_.setStatus( "11111111111111111111111111111111111111111111111111111111111111111"); upstream_request_->encodeHeaders(default_response_headers_, false); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) { - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } else { - // TODO(#14829) QUIC won't disconnect on invalid messaging. - ASSERT_TRUE(upstream_request_->waitForReset()); - } + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); } response->waitForEndStream(); @@ -1315,11 +1309,11 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) { ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status))); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); } else { + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); waitForNextUpstreamRequest(); default_response_headers_.removeStatus(); upstream_request_->encodeHeaders(default_response_headers_, false); - // TODO(#14829) QUIC won't disconnect on invalid messaging. - ASSERT_TRUE(upstream_request_->waitForReset()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); } response->waitForEndStream(); @@ -2265,6 +2259,8 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) { // Make sure that with override_stream_error_on_invalid_http_message true, CONNECT // results in stream teardown not connection teardown. TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) { + // TODO(danzh) add "allow_connect" to http3 options. + EXCLUDE_DOWNSTREAM_HTTP3; if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { return; } @@ -2285,8 +2281,6 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) { {":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}); response->waitForReset(); - // TODO(danzh) plumb through stream_error_on_invalid_http_message. - EXCLUDE_DOWNSTREAM_HTTP3; EXPECT_FALSE(codec_client_->disconnected()); } From 5577f486dcf32f2b619ef94fb11dd20b7b8a3f26 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 5 Apr 2021 14:11:46 -0400 Subject: [PATCH 12/13] onStreamError Signed-off-by: Dan Zhang --- source/common/quic/envoy_quic_client_stream.cc | 10 ++++++++-- source/common/quic/envoy_quic_client_stream.h | 2 +- source/common/quic/envoy_quic_server_stream.cc | 13 ++++++++++--- source/common/quic/envoy_quic_server_stream.h | 2 +- .../quic/envoy_quic_server_session_test.cc | 18 +++++++++++++++++- 5 files changed, 37 insertions(+), 8 deletions(-) diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 1b9dcf6b2505..eae882079a78 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -307,11 +307,17 @@ QuicFilterManagerConnectionImpl* EnvoyQuicClientStream::filterManagerConnection( return dynamic_cast(session()); } -void EnvoyQuicClientStream::onStreamError(bool close_connection_upon_invalid_header) { +void EnvoyQuicClientStream::onStreamError(absl::optional should_close_connection) { if (details_.empty()) { details_ = Http3ResponseCodeDetailValues::invalid_http_header; } - + bool close_connection_upon_invalid_header; + if (should_close_connection != absl::nullopt) { + close_connection_upon_invalid_header = should_close_connection.value(); + } else { + close_connection_upon_invalid_header = + !http3_options_.override_stream_error_on_invalid_http_message().value(); + } if (close_connection_upon_invalid_header) { stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); } else { diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index 486ac9e6565e..6049770a312d 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -78,7 +78,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, // Either reset the stream or close the connection according to // close_connection_upon_invalid_header. - void onStreamError(bool close_connection_upon_invalid_header); + void onStreamError(absl::optional should_close_connection); Http::ResponseDecoder* response_decoder_{nullptr}; diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index bb9b4867ac3e..1f7a668b0e1d 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -148,7 +148,7 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, } quic::QuicSpdyServerStreamBase::OnInitialHeadersComplete(fin, frame_len, header_list); if (!headers_decompressed() || header_list.empty()) { - onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value()); + onStreamError(absl::nullopt); return; } @@ -164,7 +164,7 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, } if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt) { details_ = Http3ResponseCodeDetailValues::invalid_http_header; - onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value()); + onStreamError(absl::nullopt); return; } request_decoder_->decodeHeaders(std::move(headers), @@ -336,11 +336,18 @@ EnvoyQuicServerStream::validateHeader(const std::string& header_name, return result; } -void EnvoyQuicServerStream::onStreamError(bool close_connection_upon_invalid_header) { +void EnvoyQuicServerStream::onStreamError(absl::optional should_close_connection) { if (details_.empty()) { details_ = Http3ResponseCodeDetailValues::invalid_http_header; } + bool close_connection_upon_invalid_header; + if (should_close_connection != absl::nullopt) { + close_connection_upon_invalid_header = should_close_connection.value(); + } else { + close_connection_upon_invalid_header = + !http3_options_.override_stream_error_on_invalid_http_message().value(); + } if (close_connection_upon_invalid_header) { stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers"); } else { diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index 9f2337a8faeb..606da9384b8a 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -91,7 +91,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, // Either reset the stream or close the connection according to // close_connection_upon_invalid_header. - void onStreamError(bool close_connection_upon_invalid_header); + void onStreamError(absl::optional should_close_connection); Http::RequestDecoder* request_decoder_{nullptr}; envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction diff --git a/test/common/quic/envoy_quic_server_session_test.cc b/test/common/quic/envoy_quic_server_session_test.cc index 1538eb8e3371..c2c93111a1dd 100644 --- a/test/common/quic/envoy_quic_server_session_test.cc +++ b/test/common/quic/envoy_quic_server_session_test.cc @@ -1,3 +1,5 @@ +#include + #if defined(__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-parameter" @@ -162,7 +164,6 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam { &compressed_certs_cache_, *dispatcher_, /*send_buffer_limit*/ quic::kDefaultFlowControlSendWindow * 1.5, listener_config_), - read_filter_(new Network::MockReadFilter()), stats_({ALL_HTTP3_CODEC_STATS( POOL_COUNTER_PREFIX(listener_config_.listenerScope(), "http3."), POOL_GAUGE_PREFIX(listener_config_.listenerScope(), "http3."))}) { @@ -213,6 +214,7 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam { bool installReadFilter() { // Setup read filter. + read_filter_ = std::make_shared(), envoy_quic_session_.addReadFilter(read_filter_); EXPECT_EQ(Http::Protocol::Http3, read_filter_->callbacks_->connection().streamInfo().protocol().value()); @@ -290,6 +292,19 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam { INSTANTIATE_TEST_SUITE_P(EnvoyQuicServerSessionTests, EnvoyQuicServerSessionTest, testing::ValuesIn({true, false})); +TEST_P(EnvoyQuicServerSessionTest, NewStreamBeforeInitializingFilter) { + quic::QuicStreamId stream_id = + quic::VersionUsesHttp3(quic_version_[0].transport_version) ? 4u : 5u; + EXPECT_ENVOY_BUG(envoy_quic_session_.GetOrCreateStream(stream_id), + fmt::format("attempts to create stream", envoy_quic_session_.id(), stream_id)); + EXPECT_CALL(*quic_connection_, + SendConnectionClosePacket(quic::QUIC_NO_ERROR, _, "Closed by application")); + EXPECT_CALL(*quic_connection_, SendControlFrame(_)) + .Times(testing::AtMost(1)) + .WillOnce(Invoke([](const quic::QuicFrame&) { return false; })); + envoy_quic_session_.close(Network::ConnectionCloseType::NoFlush); +} + TEST_P(EnvoyQuicServerSessionTest, NewStream) { installReadFilter(); @@ -786,6 +801,7 @@ TEST_P(EnvoyQuicServerSessionTest, GoAway) { } TEST_P(EnvoyQuicServerSessionTest, InitializeFilterChain) { + read_filter_ = std::make_shared(); Network::MockFilterChain filter_chain; crypto_stream_->setProofSourceDetails( std::make_unique(filter_chain)); From 255c01b573be047b5a96dddf635b433e650a1bb0 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 5 Apr 2021 16:37:11 -0400 Subject: [PATCH 13/13] comments Signed-off-by: Dan Zhang --- source/common/quic/envoy_quic_client_stream.h | 2 +- source/common/quic/envoy_quic_server_stream.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index 6049770a312d..73b162feabe8 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -77,7 +77,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, void maybeDecodeTrailers(); // Either reset the stream or close the connection according to - // close_connection_upon_invalid_header. + // should_close_connection and configured http3 options. void onStreamError(absl::optional should_close_connection); Http::ResponseDecoder* response_decoder_{nullptr}; diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index 606da9384b8a..e9059861c5e2 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -90,7 +90,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, void maybeDecodeTrailers(); // Either reset the stream or close the connection according to - // close_connection_upon_invalid_header. + // should_close_connection and configured http3 options. void onStreamError(absl::optional should_close_connection); Http::RequestDecoder* request_decoder_{nullptr};