diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index ca94d04e5f4a1..7c6e444711358 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -541,7 +541,7 @@ message KeepaliveSettings { [(validate.rules).duration = {gte {nanos: 1000000}}]; } -// [#next-free-field: 20] +// [#next-free-field: 21] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; @@ -774,6 +774,10 @@ message Http2ProtocolOptions { // request failures. google.protobuf.UInt32Value max_header_field_size_kb = 19 [(validate.rules).uint32 = {lte: 256 gte: 64}]; + + // Whether to disallow obsolete text in header field values. + // If not set, it defaults to false. + bool disallow_obs_text = 20; } // [#not-implemented-hide:] @@ -785,7 +789,7 @@ message GrpcProtocolOptions { } // A message which allows using HTTP/3. -// [#next-free-field: 9] +// [#next-free-field: 10] message Http3ProtocolOptions { QuicProtocolOptions quic_protocol_options = 1; @@ -827,6 +831,10 @@ message Http3ProtocolOptions { // Disables connection level flow control for HTTP/3 streams. This is useful in situations where the streams share the same connection // but originate from different end-clients, so that each stream can make progress independently at non-front-line proxies. bool disable_connection_flow_control_for_streams = 8; + + // Whether to disallow obsolete text in header field values. + // If not set, it defaults to false for alignment with HTTP/2. + bool disallow_obs_text = 9; } // A message to control transformations to the :scheme header diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 3752eaf92f4d0..ad89c6ca1da8a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -2082,6 +2082,7 @@ ConnectionImpl::Http2Options::Http2Options( og_options_.max_header_field_size = max_headers_kb * 1024; og_options_.allow_extended_connect = http2_options.allow_connect(); og_options_.allow_different_host_and_authority = true; + og_options_.allow_obs_text = !http2_options.disallow_obs_text(); if (!PROTOBUF_GET_WRAPPED_OR_DEFAULT(http2_options, enable_huffman_encoding, true)) { if (http2_options.has_hpack_table_size() && http2_options.hpack_table_size().value() == 0) { og_options_.compression_option = http2::adapter::OgHttp2Session::Options::DISABLE_COMPRESSION; diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index a9e48b495bc88..a427ae965c29f 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -9,6 +9,7 @@ #include "envoy/http/codec.h" #include "source/common/http/codec_helper.h" +#include "source/common/protobuf/utility.h" #include "source/common/quic/envoy_quic_simulated_watermark_buffer.h" #include "source/common/quic/envoy_quic_utils.h" @@ -50,6 +51,9 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, if (http3_options_.disable_connection_flow_control_for_streams()) { quic_stream_.DisableConnectionFlowControlForThisStream(); } + if (!http3_options_.disallow_obs_text()) { + header_validator_.SetObsTextOption(http2::adapter::ObsTextOption::kAllow); + } } ~EnvoyQuicStream() override = default; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 5b139938a6feb..466c5d15ddce1 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -135,6 +135,12 @@ class Http2CodecImplTestFixture { return stream_ids; } + static std::unique_ptr + createHttp2Options(const envoy::config::core::v3::Http2ProtocolOptions& http2_options, + uint32_t max_headers_kb) { + return std::make_unique(http2_options, max_headers_kb); + } + static Http2SettingsTuple smallWindowHttp2Settings() { return std::make_tuple(CommonUtility::OptionsLimits::DEFAULT_HPACK_TABLE_SIZE, CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS, @@ -211,11 +217,13 @@ class Http2CodecImplTestFixture { // Ensure that tests driveToCompletion(). Some tests set `expect_buffered_data_on_teardown_` to // indicate that they purposefully leave buffered data. - if (expect_buffered_data_on_teardown_) { - EXPECT_TRUE(client_wrapper_->buffer_.length() > 0 || server_wrapper_->buffer_.length() > 0); - } else { - EXPECT_EQ(client_wrapper_->buffer_.length(), 0); - EXPECT_EQ(server_wrapper_->buffer_.length(), 0); + if (client_wrapper_ != nullptr && server_wrapper_ != nullptr) { + if (expect_buffered_data_on_teardown_) { + EXPECT_TRUE(client_wrapper_->buffer_.length() > 0 || server_wrapper_->buffer_.length() > 0); + } else { + EXPECT_EQ(client_wrapper_->buffer_.length(), 0); + EXPECT_EQ(server_wrapper_->buffer_.length(), 0); + } } } @@ -570,6 +578,59 @@ class Http2CodecImplTest : public ::testing::TestWithParamencodeHeaders(request_headers, true).ok()); + driveToCompletion(); + EXPECT_FALSE(server_wrapper_->status_.ok()); + // Drain the buffer as we expect a connection error and some data might be left. + server_wrapper_->buffer_.drain(server_wrapper_->buffer_.length()); +} + +TEST_P(Http2CodecImplTest, DisallowObsTextBehaviorAllow) { + if (skipForUhv() || http2_implementation_ != Http2Impl::Oghttp2) { + return; + } + + // With disallow_obs_text = false, the request is accepted. + server_http2_options_.set_disallow_obs_text(false); + initialize(); + + InSequence s; + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + + HeaderString header_name; + header_name.setCopy("custom-header"); + HeaderString header_value; + setHeaderStringUnvalidated(header_value, "foo\x80"); + request_headers.addViaMove(std::move(header_name), std::move(header_value)); + + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); + driveToCompletion(); + EXPECT_TRUE(server_wrapper_->status_.ok()); +} + TEST_P(Http2CodecImplTest, SimpleRequestResponse) { initialize(); diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index 7ed208aca3d67..9ec06778a1ad4 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -965,5 +965,30 @@ TEST_F(EnvoyQuicServerStreamTest, DuplicatedPathHeader) { quic_stream_->OnStreamHeaderList(true, 0, header_list); } +TEST_F(EnvoyQuicServerStreamTest, DisallowObsTextBehavior) { + EXPECT_CALL(stream_callbacks_, onResetStream(_, _)).Times(testing::AnyNumber()); + { + envoy::config::core::v3::Http3ProtocolOptions options; + options.set_disallow_obs_text(true); + auto stream = std::make_unique( + stream_id_ + 16, &quic_session_, quic::BIDIRECTIONAL, stats_, options, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); + // \x80 is obsolete text + EXPECT_EQ(Http::HeaderUtility::HeaderValidationResult::REJECT, + stream->validateHeader("custom-header", "foo\x80")); + EXPECT_EQ(Http::HeaderUtility::HeaderValidationResult::ACCEPT, + stream->validateHeader("custom-header", "foo")); + } + { + envoy::config::core::v3::Http3ProtocolOptions options; + options.set_disallow_obs_text(false); + auto stream = std::make_unique( + stream_id_ + 20, &quic_session_, quic::BIDIRECTIONAL, stats_, options, + envoy::config::core::v3::HttpProtocolOptions::ALLOW); + EXPECT_EQ(Http::HeaderUtility::HeaderValidationResult::ACCEPT, + stream->validateHeader("custom-header", "foo\x80")); + } +} + } // namespace Quic } // namespace Envoy diff --git a/test/integration/default_header_validator_integration_test.cc b/test/integration/default_header_validator_integration_test.cc index 5e93fddbc53df..79d900ea3fd42 100644 --- a/test/integration/default_header_validator_integration_test.cc +++ b/test/integration/default_header_validator_integration_test.cc @@ -79,8 +79,10 @@ class DownstreamUhvIntegrationTest : public HttpProtocolIntegrationTest { std::string additionallyAllowedCharactersInUrlPath() { // All codecs allow the following characters that are outside of RFC "<>[]^`{}\| std::string additionally_allowed_characters(R"--("<>[]^`{}\|)--"); - if (downstream_protocol_ == Http::CodecType::HTTP2) { - // Both nghttp2 and oghttp2 allow extended ASCII >= 0x80 in path + if (downstream_protocol_ == Http::CodecType::HTTP2 || + downstream_protocol_ == Http::CodecType::HTTP3) { + // Both nghttp2 and oghttp2 allow extended ASCII >= 0x80 in path. + // HTTP/3 also defaults to allowing it for alignment with HTTP/2. additionally_allowed_characters += generateExtendedAsciiString(); } return additionally_allowed_characters;