H/2: discard Host header when :authority is present (envoyproxy#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 <[email protected]>
Winbobob · Dec 8, 2023 · a71c76d · a71c76d
1 parent 75b338b
commit a71c76d
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 5 deletions.
diff --git 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
@@ -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<absl::string_view>(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
@@ -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
@@ -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
@@ -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,14 +2216,61 @@ 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());
   EXPECT_EQ(Http2Frame::ResponseStatus::Ok, frame.responseStatus());
   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;