http: pathless CONNECT (#11048)

Removing the synthetic path added to CONNECT requests theoretically completing Envoy CONNECT support.

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

docs/root/version_history/current.rst

@@ -26,6 +26,7 @@ Changes
   Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false.
 * http: remove legacy connection pool code and their runtime features: `envoy.reloadable_features.new_http1_connection_pool_behavior` and
   `envoy.reloadable_features.new_http2_connection_pool_behavior`.
+* http: stopped adding a synthetic path to CONNECT requests, meaning unconfigured CONNECT requests will now return 404 instead of 403. This behavior can be temporarily reverted by setting `envoy.reloadable_features.stop_faking_paths` to false.
 * listener: added in place filter chain update flow for tcp listener update which doesn't close connections if the corresponding network filter chain is equivalent during the listener update.
   Can be disabled by setting runtime feature `envoy.reloadable_features.listener_in_place_filterchain_update` to false.
   Also added additional draining filter chain stat for :ref:`listener manager <config_listener_manager_stats>` to track the number of draining filter chains and the number of in place update attempts.

source/common/http/conn_manager_impl.cc

@@ -38,6 +38,7 @@
 #include "common/http/utility.h"
 #include "common/network/utility.h"
 #include "common/router/config_impl.h"
+#include "common/runtime/runtime_features.h"
 #include "common/runtime/runtime_impl.h"
 #include "common/stats/timespan_impl.h"
 
@@ -774,10 +775,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
                                connection_manager_.read_callbacks_->connection().dispatcher());
   request_headers_ = std::move(headers);
 
-  // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including
-  // auditing of empty path headers. We check for path because HTTP/2 connect requests may have a
-  // path.
-  if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) {
+  if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path() &&
+      !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.stop_faking_paths")) {
     request_headers_->setPath("/");
   }
 
@@ -878,20 +877,23 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
   // Verify header sanity checks which should have been performed by the codec.
   ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false);
 
-  // Currently we only support relative paths at the application layer. We expect the codec to have
-  // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this
-  // when the allow_absolute_url flag is enabled on the HCM.
-  // https://tools.ietf.org/html/rfc7230#section-5.3 We also need to check for the existence of
-  // :path because CONNECT does not have a path, and we don't support that currently.
-  if (!request_headers_->Path() || request_headers_->Path()->value().getStringView().empty() ||
-      request_headers_->Path()->value().getStringView()[0] != '/') {
-    const bool has_path =
-        request_headers_->Path() && !request_headers_->Path()->value().getStringView().empty();
+  // Check for the existence of the :path header for non-CONNECT requests. We expect the codec to
+  // have broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does
+  // this when the allow_absolute_url flag is enabled on the HCM.
+  if ((!HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) ||
+      (request_headers_->Path() && request_headers_->Path()->value().getStringView().empty())) {
+    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr,
+                   state_.is_head_request_, absl::nullopt,
+                   StreamInfo::ResponseCodeDetails::get().MissingPath);
+    return;
+  }
+
+  // Currently we only support relative paths at the application layer.
+  if (request_headers_->Path() && request_headers_->Path()->value().getStringView()[0] != '/') {
     connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc();
     sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr,
                    state_.is_head_request_, absl::nullopt,
-                   has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath
-                            : StreamInfo::ResponseCodeDetails::get().MissingPath);
+                   StreamInfo::ResponseCodeDetails::get().AbsolutePath);
     return;
   }
 

source/common/http/http2/codec_impl.cc

@@ -162,8 +162,17 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea
     Http::Utility::transformUpgradeRequestFromH1toH2(*modified_headers);
     buildHeaders(final_headers, *modified_headers);
   } else if (headers.Method() && headers.Method()->value() == "CONNECT") {
+    // If this is not an upgrade style connect (above branch) it is a bytestream
+    // connect and should have :path and :protocol set accordingly
+    // As HTTP/1.1 does not require a path for CONNECT, we may have to add one
+    // if shifting codecs. For now, default to "/" - this can be made
+    // configurable if necessary.
+    // https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
     modified_headers = createHeaderMap<RequestHeaderMapImpl>(headers);
     modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream);
+    if (!headers.Path()) {
+      modified_headers->setPath("/");
+    }
     buildHeaders(final_headers, *modified_headers);
   } else {
     buildHeaders(final_headers, headers);

source/common/runtime/runtime_features.cc

@@ -63,6 +63,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher",
     "envoy.reloadable_features.fix_upgrade_response",
     "envoy.reloadable_features.listener_in_place_filterchain_update",
+    "envoy.reloadable_features.stop_faking_paths",
 };
 
 // This is a section for officially sanctioned runtime features which are too

test/common/http/BUILD

@@ -230,6 +230,7 @@ envoy_cc_test(
         "//test/mocks/tracing:tracing_mocks",
         "//test/mocks/upstream:upstream_mocks",
         "//test/test_common:logging_lib",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:test_time_lib",
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
         "@envoy_api//envoy/type/tracing/v3:pkg_cc_proto",

test/common/http/conn_manager_impl_test.cc

@@ -46,6 +46,7 @@
 #include "test/mocks/upstream/mocks.h"
 #include "test/test_common/logging.h"
 #include "test/test_common/printers.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/test_time.h"
 
 #include "gmock/gmock.h"
@@ -2859,6 +2860,39 @@ TEST_F(HttpConnectionManagerImplTest, ConnectAsUpgrade) {
   conn_manager_->onData(fake_input, false);
 }
 
+TEST_F(HttpConnectionManagerImplTest, ConnectLegacy) {
+  TestScopedRuntime scoped_runtime;
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.stop_faking_paths", "false"}});
+
+  setup(false, "envoy-custom-server", false);
+
+  NiceMock<MockResponseEncoder> encoder;
+  RequestDecoder* decoder = nullptr;
+
+  EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _))
+      .WillRepeatedly(Return(false));
+
+  EXPECT_CALL(*codec_, dispatch(_))
+      .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status {
+        decoder = &conn_manager_->newStream(encoder);
+        RequestHeaderMapPtr headers{
+            new TestRequestHeaderMapImpl{{":authority", "host"}, {":method", "CONNECT"}}};
+        decoder->decodeHeaders(std::move(headers), false);
+        data.drain(4);
+        return Http::okStatus();
+      }));
+
+  EXPECT_CALL(encoder, encodeHeaders(_, _))
+      .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void {
+        EXPECT_EQ("403", headers.Status()->value().getStringView());
+      }));
+
+  // Kick off the incoming data.
+  Buffer::OwnedImpl fake_input("1234");
+  conn_manager_->onData(fake_input, false);
+}
+
 // Regression test for https://github.com/envoyproxy/envoy/issues/10138
 TEST_F(HttpConnectionManagerImplTest, DrainCloseRaceWithClose) {
   InSequence s;

test/config/utility.cc

@@ -433,6 +433,26 @@ void ConfigHelper::addClusterFilterMetadata(absl::string_view metadata_yaml,
   }
 }
 
+void ConfigHelper::setConnectConfig(
+    envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm,
+    bool terminate_connect) {
+  auto* route_config = hcm.mutable_route_config();
+  ASSERT_EQ(1, route_config->virtual_hosts_size());
+  auto* route = route_config->mutable_virtual_hosts(0)->mutable_routes(0);
+  auto* match = route->mutable_match();
+  match->Clear();
+  match->mutable_connect_matcher();
+
+  if (terminate_connect) {
+    auto* upgrade = route->mutable_route()->add_upgrade_configs();
+    upgrade->set_upgrade_type("CONNECT");
+    upgrade->mutable_connect_config();
+  }
+
+  hcm.add_upgrade_configs()->set_upgrade_type("CONNECT");
+  hcm.mutable_http2_protocol_options()->set_allow_connect(true);
+}
+
 void ConfigHelper::applyConfigModifiers() {
   for (const auto& config_modifier : config_modifiers_) {
     config_modifier(bootstrap_);

test/config/utility.h

@@ -27,6 +27,8 @@ namespace Envoy {
 
 class ConfigHelper {
 public:
+  using HttpConnectionManager =
+      envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager;
   struct ServerSslOptions {
     ServerSslOptions& setRsaCert(bool rsa_cert) {
       rsa_cert_ = rsa_cert;
@@ -67,8 +69,7 @@ class ConfigHelper {
                 envoy::extensions::transport_sockets::tls::v3::CommonTlsContext& common_context);
 
   using ConfigModifierFunction = std::function<void(envoy::config::bootstrap::v3::Bootstrap&)>;
-  using HttpModifierFunction = std::function<void(
-      envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&)>;
+  using HttpModifierFunction = std::function<void(HttpConnectionManager&)>;
 
   // A basic configuration (admin port, cluster_0, one listener) with no network filters.
   static std::string baseConfig();
@@ -196,15 +197,16 @@ class ConfigHelper {
   void addClusterFilterMetadata(absl::string_view metadata_yaml,
                                 absl::string_view cluster_name = "cluster_0");
 
+  // Given an HCM with the default config, set the matcher to be a connect matcher and enable
+  // CONNECT requests.
+  static void setConnectConfig(HttpConnectionManager& hcm, bool terminate_connect);
+
 private:
   // Load the first HCM struct from the first listener into a parsed proto.
-  bool loadHttpConnectionManager(
-      envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm);
+  bool loadHttpConnectionManager(HttpConnectionManager& hcm);
   // Take the contents of the provided HCM proto and stuff them into the first HCM
   // struct of the first listener.
-  void storeHttpConnectionManager(
-      const envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-          hcm);
+  void storeHttpConnectionManager(const HttpConnectionManager& hcm);
 
   // Finds the filter named 'name' from the first filter chain from the first listener.
   envoy::config::listener::v3::Filter* getFilterFromListener(const std::string& name);

test/integration/integration_test.cc

@@ -771,7 +771,7 @@ TEST_P(IntegrationTest, AbsolutePathWithoutPort) {
 
 // Ensure that connect behaves the same with allow_absolute_url enabled and without
 TEST_P(IntegrationTest, Connect) {
-  const std::string& request = "CONNECT www.somewhere.com:80 HTTP/1.1\r\nHost: host\r\n\r\n";
+  const std::string& request = "CONNECT www.somewhere.com:80 HTTP/1.1\r\n\r\n";
   config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
     // Clone the whole listener.
     auto static_resources = bootstrap.mutable_static_resources();
@@ -1301,14 +1301,11 @@ TEST_P(IntegrationTest, TestUpgradeHeaderInResponse) {
 TEST_P(IntegrationTest, ConnectWithNoBody) {
   config_helper_.addConfigModifier(
       [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-              hcm) -> void {
-        hcm.add_upgrade_configs()->set_upgrade_type("CONNECT");
-        hcm.mutable_http2_protocol_options()->set_allow_connect(true);
-      });
+              hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); });
   initialize();
 
   IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http"));
-  tcp_client->write("CONNECT host.com:80 HTTP/1.1\r\nHost: host\r\n\r\n", false);
+  tcp_client->write("CONNECT host.com:80 HTTP/1.1\r\n\r\n", false);
 
   FakeRawConnectionPtr fake_upstream_connection;
   ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
@@ -1338,16 +1335,13 @@ TEST_P(IntegrationTest, ConnectWithNoBody) {
 TEST_P(IntegrationTest, ConnectWithChunkedBody) {
   config_helper_.addConfigModifier(
       [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-              hcm) -> void {
-        hcm.add_upgrade_configs()->set_upgrade_type("CONNECT");
-        hcm.mutable_http2_protocol_options()->set_allow_connect(true);
-      });
+              hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); });
   initialize();
 
   // Send the payload early so we can regression test that body data does not
   // get proxied until after the response headers are sent.
   IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http"));
-  tcp_client->write("CONNECT host.com:80 HTTP/1.1\r\nHost: host\r\n\r\npayload", false);
+  tcp_client->write("CONNECT host.com:80 HTTP/1.1\r\n\r\npayload", false);
 
   FakeRawConnectionPtr fake_upstream_connection;
   ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));

test/integration/protocol_integration_test.cc

@@ -1789,8 +1789,12 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) {
       Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, {":authority", "host.com:80"}});
 
   if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) {
+    // TODO(alyssawilk) either reinstate prior behavior, or include a release
+    // note with this PR.
+    // Because CONNECT requests for HTTP/1 do not include a path, they will fail
+    // to find a route match and return a 404.
     response->waitForEndStream();
-    EXPECT_EQ("403", response->headers().Status()->value().getStringView());
+    EXPECT_EQ("404", response->headers().Status()->value().getStringView());
     EXPECT_TRUE(response->complete());
   } else {
     response->waitForReset();

test/integration/tcp_tunneling_integration_test.cc

@@ -25,19 +25,7 @@ class ConnectTerminationIntegrationTest
     config_helper_.addConfigModifier(
         [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
                 hcm) {
-          auto* route_config = hcm.mutable_route_config();
-          ASSERT_EQ(1, route_config->virtual_hosts_size());
-          auto* route = route_config->mutable_virtual_hosts(0)->mutable_routes(0);
-          auto* match = route->mutable_match();
-          match->Clear();
-          match->mutable_connect_matcher();
-
-          auto* upgrade = route->mutable_route()->add_upgrade_configs();
-          upgrade->set_upgrade_type("CONNECT");
-          upgrade->mutable_connect_config();
-
-          hcm.add_upgrade_configs()->set_upgrade_type("CONNECT");
-          hcm.mutable_http2_protocol_options()->set_allow_connect(true);
+          ConfigHelper::setConnectConfig(hcm, true);
 
           if (enable_timeout_) {
             hcm.mutable_stream_idle_timeout()->set_seconds(0);
@@ -201,10 +189,7 @@ class ProxyingConnectIntegrationTest : public HttpProtocolIntegrationTest {
   void initialize() override {
     config_helper_.addConfigModifier(
         [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-                hcm) -> void {
-          hcm.add_upgrade_configs()->set_upgrade_type("CONNECT");
-          hcm.mutable_http2_protocol_options()->set_allow_connect(true);
-        });
+                hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); });
     HttpProtocolIntegrationTest::initialize();
   }
 

tools/spelling/spelling_dictionary.txt

@@ -423,6 +423,7 @@ builtins
 bulkstrings
 bursty
 bytecode
+bytestream
 cacheability
 callee
 callsite