From a71c76da8e6e925dd4006c7696911ddf149c0c72 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Fri, 8 Dec 2023 14:35:45 -0500 Subject: [PATCH] H/2: discard Host header when :authority is present (#30005) Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false. --------- Signed-off-by: Yan Avlasov --- changelogs/current.yaml | 5 ++ source/common/http/http2/codec_impl.cc | 12 +++++ source/common/runtime/runtime_features.cc | 1 + test/common/http/http2/http2_frame.h | 6 +-- .../multiplexed_integration_test.cc | 51 ++++++++++++++++++- 5 files changed, 70 insertions(+), 5 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 572490f23df3..fbcd46cad105 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -26,6 +26,11 @@ behavior_changes: change: | Changes the default value of ``envoy.reloadable_features.http2_use_oghttp2`` to true. This changes the codec used for HTTP/2 requests and responses. This behavior can be reverted by setting the feature to false. +- area: http2 + change: | + Discard the ``Host`` header if the ``:authority`` header was received to bring Envoy into compliance with + https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 This behavioral change can be reverted by setting runtime flag + ``envoy.reloadable_features.http2_discard_host_header`` to false. minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 0b0b18c039c2..fb2963d6076a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -2129,6 +2129,18 @@ int ServerConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na // For a server connection, we should never get push promise frames. ASSERT(frame->hd.type == NGHTTP2_HEADERS); ASSERT(frame->headers.cat == NGHTTP2_HCAT_REQUEST || frame->headers.cat == NGHTTP2_HCAT_HEADERS); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_discard_host_header")) { + StreamImpl* stream = getStreamUnchecked(frame->hd.stream_id); + if (stream && name == static_cast(Http::Headers::get().HostLegacy)) { + // Check if there is already the :authority header + const auto result = stream->headers().get(Http::Headers::get().Host); + if (!result.empty()) { + // Discard the host header value + return 0; + } + // Otherwise use host value as :authority + } + } return saveHeader(frame, std::move(name), std::move(value)); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 12429b627ada..b6fecd6b2705 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -52,6 +52,7 @@ RUNTIME_GUARD(envoy_reloadable_features_http1_allow_codec_error_response_after_1 RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirect); RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser); RUNTIME_GUARD(envoy_reloadable_features_http2_decode_metadata_with_quiche); +RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header); RUNTIME_GUARD(envoy_reloadable_features_http2_use_oghttp2); RUNTIME_GUARD(envoy_reloadable_features_http2_validate_authority_with_quiche); RUNTIME_GUARD(envoy_reloadable_features_http_allow_partial_urls_in_referer); diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index e6a2a3372253..2844c6bcebde 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -260,6 +260,9 @@ class Http2Frame { ASSERT(size() >= HeaderSize); setPayloadSize(size() - HeaderSize); } + // Headers are directly encoded + void appendStaticHeader(StaticHeaderIndex index); + void appendHeaderWithoutIndexing(StaticHeaderIndex index, absl::string_view value); private: void buildHeader(Type type, uint32_t payload_size = 0, uint8_t flags = 0, uint32_t stream_id = 0); @@ -277,9 +280,6 @@ class Http2Frame { std::copy(data.begin(), data.end(), data_.begin() + 9); } - // Headers are directly encoded - void appendStaticHeader(StaticHeaderIndex index); - void appendHeaderWithoutIndexing(StaticHeaderIndex index, absl::string_view value); void appendEmptyHeader(); DataContainer data_; diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 4513cc64da5e..71573f90a5e7 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -2199,7 +2199,7 @@ TEST_P(Http2FrameIntegrationTest, HostDifferentFromAuthority) { sendFrame(request); waitForNextUpstreamRequest(); - EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com,two.example.com"); + EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com"); upstream_request_->encodeHeaders(default_response_headers_, true); auto frame = readFrame(); EXPECT_EQ(Http2Frame::Type::Headers, frame.type()); @@ -2216,7 +2216,7 @@ TEST_P(Http2FrameIntegrationTest, HostSameAsAuthority) { sendFrame(request); waitForNextUpstreamRequest(); - EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com,one.example.com"); + EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com"); upstream_request_->encodeHeaders(default_response_headers_, true); auto frame = readFrame(); EXPECT_EQ(Http2Frame::Type::Headers, frame.type()); @@ -2224,6 +2224,53 @@ TEST_P(Http2FrameIntegrationTest, HostSameAsAuthority) { tcp_client_->close(); } +TEST_P(Http2FrameIntegrationTest, HostConcatenatedWithAuthorityWithOverride) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.http2_discard_host_header", "false"); + beginSession(); + + uint32_t request_idx = 0; + auto request = Http2Frame::makeRequest(Http2Frame::makeClientStreamId(request_idx), + "one.example.com", "/path", {{"host", "two.example.com"}}); + sendFrame(request); + + waitForNextUpstreamRequest(); + EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com,two.example.com"); + upstream_request_->encodeHeaders(default_response_headers_, true); + auto frame = readFrame(); + EXPECT_EQ(Http2Frame::Type::Headers, frame.type()); + EXPECT_EQ(Http2Frame::ResponseStatus::Ok, frame.responseStatus()); + tcp_client_->close(); +} + +// All HTTP/2 static headers must be before non-static headers. +// Verify that codecs validate this. +TEST_P(Http2FrameIntegrationTest, HostBeforeAuthorityIsRejected) { +#ifdef ENVOY_ENABLE_UHV + // TODO(yanavlasov): fix this check for oghttp2 in UHV mode. + if (GetParam().http2_implementation == Http2Impl::Oghttp2) { + return; + } +#endif + beginSession(); + + Http2Frame request = Http2Frame::makeEmptyHeadersFrame(Http2Frame::makeClientStreamId(0), + Http2Frame::HeadersFlags::EndHeaders); + request.appendStaticHeader(Http2Frame::StaticHeaderIndex::MethodPost); + request.appendStaticHeader(Http2Frame::StaticHeaderIndex::SchemeHttps); + request.appendHeaderWithoutIndexing(Http2Frame::StaticHeaderIndex::Path, "/path"); + // Add the `host` header before `:authority` + request.appendHeaderWithoutIndexing({"host", "two.example.com"}); + request.appendHeaderWithoutIndexing(Http2Frame::StaticHeaderIndex::Authority, "one.example.com"); + request.adjustPayloadSize(); + + sendFrame(request); + + // By default codec treats stream errors as protocol errors and closes the connection. + tcp_client_->waitForDisconnect(); + tcp_client_->close(); + EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_cx_protocol_error")->value()); +} + TEST_P(Http2FrameIntegrationTest, MultipleHeaderOnlyRequests) { const int kRequestsSentPerIOCycle = 20; autonomous_upstream_ = true;