http: explicitly blocking CONNECT for HTTP/1.1 (#9637)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

docs/root/intro/version_history.rst

@@ -16,6 +16,7 @@ Version history
 * decompressor: remove decompressor hard assert failure and replace with an error flag.
 * ext_authz: added :ref:`configurable ability<envoy_api_field_config.filter.http.ext_authz.v2.ExtAuthz.include_peer_certificate>` to send the :ref:`certificate<envoy_api_field_service.auth.v2.AttributeContext.Peer.certificate>` to the `ext_authz` service.
 * health check: gRPC health checker sets the gRPC deadline to the configured timeout duration.
+* http: added strict validation that CONNECT is refused as it is not yet implemented. This can be reversed temporarily by setting the runtime feature `envoy.reloadable_features.strict_method_validation` to false.
 * http: added support for http1 trailers. To enable use :ref:`enable_trailers <envoy_api_field_core.Http1ProtocolOptions.enable_trailers>`.
 * http: added the ability to sanitize headers nominated by the Connection header. This new behavior is guarded by envoy.reloadable_features.connection_header_sanitization which defaults to true.
 * http: blocks unsupported transfer-encodings. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.reject_unsupported_transfer_encodings` to false.

source/common/http/header_utility.cc

@@ -16,6 +16,7 @@ namespace Http {
 
 struct SharedResponseCodeDetailsValues {
   const absl::string_view InvalidAuthority = "http.invalid_authority";
+  const absl::string_view ConnectUnsupported = "http.connect_not_supported";
 };
 
 using SharedResponseCodeDetails = ConstSingleton<SharedResponseCodeDetailsValues>;
@@ -178,6 +179,11 @@ HeaderUtility::requestHeadersValid(const HeaderMap& headers) {
       headers.Host() && !HeaderUtility::authorityIsValid(headers.Host()->value().getStringView())) {
     return SharedResponseCodeDetails::get().InvalidAuthority;
   }
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_method_validation") &&
+      headers.Method() &&
+      Http::Headers::get().MethodValues.Connect == headers.Method()->value().getStringView()) {
+    return SharedResponseCodeDetails::get().ConnectUnsupported;
+  }
   return absl::nullopt;
 }
 

source/common/runtime/runtime_features.cc

@@ -29,9 +29,9 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.buffer_filter_populate_content_length",
     "envoy.reloadable_features.outlier_detection_support_for_grpc_status",
     "envoy.reloadable_features.connection_header_sanitization",
-    "envoy.reloadable_features.strict_header_validation",
     "envoy.reloadable_features.strict_authority_validation",
     "envoy.reloadable_features.reject_unsupported_transfer_encodings",
+    "envoy.reloadable_features.strict_method_validation",
 };
 
 // This is a section for officially sanctioned runtime features which are too

test/common/http/conn_manager_impl_test.cc

@@ -1568,7 +1568,7 @@ TEST_F(HttpConnectionManagerImplTest, NoPath) {
   NiceMock<MockStreamEncoder> encoder;
   EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void {
     decoder = &conn_manager_->newStream(encoder);
-    HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":method", "CONNECT"}}};
+    HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":method", "NOT_CONNECT"}}};
     decoder->decodeHeaders(std::move(headers), true);
     data.drain(4);
   }));

test/integration/BUILD

@@ -340,7 +340,7 @@ envoy_cc_test(
     ],
     # As this test has many H1/H2/v4/v6 tests it takes a while to run.
     # Shard it enough to bring the run time in line with other integration tests.
-    shard_count = 3,
+    shard_count = 5,
     deps = [
         ":http_protocol_integration_lib",
         "//source/common/http:header_map_lib",

test/integration/protocol_integration_test.cc

@@ -1499,6 +1499,42 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidAuthority) {
   }
 }
 
+TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) {
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response = codec_client_->makeHeaderOnlyRequest(
+      Http::TestHeaderMapImpl{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}});
+
+  if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) {
+    response->waitForEndStream();
+    EXPECT_EQ("400", response->headers().Status()->value().getStringView());
+    EXPECT_TRUE(response->complete());
+  } else {
+    response->waitForReset();
+    codec_client_->waitForDisconnect();
+  }
+}
+
+// Make sure that with stream_error_on_invalid_http_messaging true, CONNECT
+// results in stream teardown not connection teardown.
+TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {
+  if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) {
+    return;
+  }
+  config_helper_.addConfigModifier([](envoy::extensions::filters::network::http_connection_manager::
+                                          v3alpha::HttpConnectionManager& hcm) -> void {
+    hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true);
+  });
+
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response = codec_client_->makeHeaderOnlyRequest(
+      Http::TestHeaderMapImpl{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}});
+
+  response->waitForReset();
+  EXPECT_FALSE(codec_client_->disconnected());
+}
+
 // For tests which focus on downstream-to-Envoy behavior, and don't need to be
 // run with both HTTP/1 and HTTP/2 upstreams.
 INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest,