Add option to reject early CONNECT data

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Author: yanavlasov

changelogs/current.yaml

@@ -7,6 +7,11 @@ behavior_changes:
     The dynamic module ABI has been updated to support streaming body manipulation. This change also
     fixed potential incorrect behavior when access or modify the request or response body. See
     https://github.com/envoyproxy/envoy/issues/40918 for more details.
+- area: http
+  change: |
+    Added runtime flag ``envoy.reloadable_features.reject_early_connect_data`` to reject ``CONNECT`` requests
+    that receive data before Envoy sent a ``200`` response to the client. While this is not a strictly compliant behavior
+    it is very common as a latency reducing measure. As such the option is disabled by default.
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*

docs/root/configuration/http/http_conn_man/response_code_details.rst

@@ -25,6 +25,7 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen
    downstream_remote_disconnect, The client disconnected unexpectedly.
    duration_timeout, The max connection duration was exceeded.
    direct_response, A direct response was generated by the router filter.
+   early_connect_data, Data was received for a CONNECT request before 200 response headers were sent.
    filter_added_invalid_request_data, A filter added request data at the wrong stage in the filter chain.
    filter_added_invalid_response_data, A filter added response data at the wrong stage in the filter chain.
    filter_chain_not_found, The request was rejected due to no matching filter chain.

envoy/stream_info/stream_info.h

@@ -246,6 +246,8 @@ struct ResponseCodeDetailValues {
   const std::string FilterAddedInvalidRequestData = "filter_added_invalid_request_data";
   // A filter called addDecodedData at the wrong point in the filter chain.
   const std::string FilterAddedInvalidResponseData = "filter_added_invalid_response_data";
+  // Data was received for a CONNECT request before 200 response headers were sent.
+  const std::string EarlyConnectData = "early_connect_data";
   // Changes or additions to details should be reflected in
   // docs/root/configuration/http/http_conn_man/response_code_details.rst
 };

source/common/router/router.cc

@@ -957,7 +957,19 @@ void Filter::sendNoHealthyUpstreamResponse(absl::optional<std::string> optional_
                              absl::nullopt, details);
 }
 
+bool Filter::isEarlyConnectData() {
+  return downstream_headers_ != nullptr && Http::HeaderUtility::isConnect(*downstream_headers_) &&
+         !downstream_response_started_ &&
+         Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reject_early_connect_data");
+}
+
 Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) {
+  ENVOY_STREAM_LOG(debug, "router decoding data: {}", *callbacks_, data.length());
+  if (data.length() > 0 && isEarlyConnectData()) {
+    callbacks_->sendLocalReply(Http::Code::BadRequest, "", nullptr, absl::nullopt,
+                               StreamInfo::ResponseCodeDetails::get().EarlyConnectData);
+    return Http::FilterDataStatus::StopIterationNoBuffer;
+  }
   // upstream_requests_.size() cannot be > 1 because that only happens when a per
   // try timeout occurs with hedge_on_per_try_timeout enabled but the per
   // try timeout timer is not started until onRequestComplete(). It could be zero

source/common/router/router.h

@@ -601,6 +601,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   // Process Orca Load Report if necessary (e.g. cluster has lrsReportMetricNames).
   void maybeProcessOrcaLoadReport(const Envoy::Http::HeaderMap& headers_or_trailers,
                                   UpstreamRequest& upstream_request);
+  bool isEarlyConnectData();
 
   RetryStatePtr retry_state_;
   const FilterConfigSharedPtr config_;

source/common/runtime/runtime_features.cc

@@ -182,6 +182,10 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_disable_client_early_data);
 
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close);
 
+// TODO(yavlasov): Enabling by default will be hugely disruptive to existing traffic.
+// Replace with a config option (default off) post CVE release.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_reject_early_connect_data);
+
 // Block of non-boolean flags. Use of int flags is deprecated. Do not add more.
 ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT
 ABSL_FLAG(uint64_t, re2_max_program_size_warn_level,            // NOLINT

test/integration/fake_upstream.h

@@ -665,6 +665,11 @@ class FakeRawConnection : public FakeConnectionBase {
     data_.clear();
   }
 
+  bool hasData() {
+    absl::MutexLock lock(&lock_);
+    return !data_.empty();
+  }
+
 private:
   struct ReadFilter : public Network::ReadFilterBaseImpl {
     ReadFilter(FakeRawConnection& parent) : parent_(parent) {}

test/integration/tcp_tunneling_integration_test.cc

@@ -447,6 +447,69 @@ TEST_P(ConnectTerminationIntegrationTest, IgnoreH11HostField) {
       sendRawHttpAndWaitForResponse(lookupPort("http"), full_request.c_str(), &response, true););
 }
 
+TEST_P(ConnectTerminationIntegrationTest, EarlyConnectDataRejectedWithOverride) {
+  config_helper_.addRuntimeOverride("envoy.reloadable_features.reject_early_connect_data", "true");
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  // Send CONNECT request and immediately send some data without waiting for 200
+  // response from Envoy.
+  auto encoder_decoder = codec_client_->startRequest(connect_headers_);
+  request_encoder_ = &encoder_decoder.first;
+  codec_client_->sendData(*request_encoder_, "premature data", false);
+  response_ = std::move(encoder_decoder.second);
+
+  // Envoy will try top open upstream connection before the premature CONNECT data is detected.
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_raw_upstream_connection_));
+
+  response_->waitForHeaders();
+  EXPECT_EQ(response_->headers().getStatusValue(), "400");
+  EXPECT_TRUE(response_->waitForEndStream());
+
+  // Because the downstream connection is closed by Envoy without sending any data the
+  // upstream connection will remain in the pool and will not be closed.
+  // However it should not have any data in it.
+  EXPECT_FALSE(fake_raw_upstream_connection_->hasData());
+  cleanupUpstreamAndDownstream();
+}
+
+TEST_P(ConnectTerminationIntegrationTest, EarlyConnectDataAllowedByDefault) {
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  // Send CONNECT request and immediately send some data without waiting for 200
+  // response from Envoy.
+  auto encoder_decoder = codec_client_->startRequest(connect_headers_);
+  request_encoder_ = &encoder_decoder.first;
+  codec_client_->sendData(*request_encoder_, "premature data", false);
+  response_ = std::move(encoder_decoder.second);
+
+  // Wait for the data to arrive upstream.
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_raw_upstream_connection_));
+  ASSERT_TRUE(fake_raw_upstream_connection_->waitForData(
+      FakeRawConnection::waitForInexactMatch("premature data")));
+
+  // Send some data downstream.
+  ASSERT_TRUE(fake_raw_upstream_connection_->write("upstream_send_data"));
+
+  // Wait for the headers and data to arrive downstream.
+  response_->waitForHeaders();
+  response_->waitForBodyData(strlen("upstream_send_data"));
+  EXPECT_EQ("upstream_send_data", response_->body());
+
+  codec_client_->sendData(*request_encoder_, "", true);
+  ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose());
+
+  ASSERT_TRUE(fake_raw_upstream_connection_->close());
+  if (downstream_protocol_ == Http::CodecType::HTTP1) {
+    ASSERT_TRUE(codec_client_->waitForDisconnect());
+  } else {
+    ASSERT_TRUE(response_->waitForEndStream());
+    ASSERT_FALSE(response_->reset());
+  }
+  cleanupUpstreamAndDownstream();
+}
+
 INSTANTIATE_TEST_SUITE_P(HttpAndIpVersions, ConnectTerminationIntegrationTest,
                          testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams(
                              {Http::CodecType::HTTP1, Http::CodecType::HTTP2,