http: fixing up CONNECT to be RFC compliant (#21440)

Risk Level: Medium (data plane changes)
Testing: updated tests
Docs Changes: n/a
Release Notes: inline
Runtime guard: envoy.reloadable_features.use_rfc_connect
Fixes #20378

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

changelogs/current.yaml

@@ -76,6 +76,11 @@ minor_behavior_changes:
     ``envoy.reloadable_features.lua_respond_with_send_local_reply`` to false.
 
 bug_fixes:
+- area: http
+  change: |
+    Fixed HTTP/2 CONNECT to be RFC compliant, rather than following the abandoned extended connect draft.
+    This behavioral change can be reverted by setting runtime guard ``envoy.reloadable_features.use_rfc_connect`` to false.
+
 - area: runtime
   change: |
     Fixed a bug where was ``envoy.restart_features.no_runtime_singleton`` was inverted.

source/common/http/http2/codec_impl.cc

@@ -300,9 +300,15 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h
     // 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("/");
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {
+      modified_headers->removeScheme();
+      modified_headers->removePath();
+      modified_headers->removeProtocol();
+    } else {
+      modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream);
+      if (!headers.Path()) {
+        modified_headers->setPath("/");
+      }
     }
     encodeHeadersBase(*modified_headers, end_stream);
   } else {

source/common/quic/envoy_quic_client_stream.cc

@@ -44,14 +44,17 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
   auto spdy_headers = envoyHeadersToSpdyHeaderBlock(headers);
   if (headers.Method()) {
     if (headers.Method()->value() == "CONNECT") {
-      // 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
-      spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
-      if (!headers.Path()) {
-        spdy_headers[":path"] = "/";
+      if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {
+        spdy_headers.erase(":scheme");
+        spdy_headers.erase(":path");
+        spdy_headers.erase(":protocol");
+      } else {
+        // Legacy support for abandoned
+        // https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
+        spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
+        if (!headers.Path()) {
+          spdy_headers[":path"] = "/";
+        }
       }
     } else if (headers.Method()->value() == "HEAD") {
       sent_head_request_ = true;

source/common/runtime/runtime_features.cc

@@ -71,6 +71,7 @@ RUNTIME_GUARD(envoy_reloadable_features_top_level_ecds_stats);
 RUNTIME_GUARD(envoy_reloadable_features_udp_listener_updates_filter_chain_in_place);
 RUNTIME_GUARD(envoy_reloadable_features_update_expected_rq_timeout_on_retry);
 RUNTIME_GUARD(envoy_reloadable_features_update_grpc_response_error_tag);
+RUNTIME_GUARD(envoy_reloadable_features_use_rfc_connect);
 RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
 RUNTIME_GUARD(envoy_restart_features_explicit_wildcard_resource);
 RUNTIME_GUARD(envoy_restart_features_remove_runtime_singleton);

source/common/tcp_proxy/upstream.cc

@@ -278,11 +278,14 @@ void Http2Upstream::setRequestEncoder(Http::RequestEncoder& request_encoder, boo
   auto headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
       {Http::Headers::get().Method, config_.usePost() ? "POST" : "CONNECT"},
       {Http::Headers::get().Host, config_.hostname()},
-      {Http::Headers::get().Path, "/"},
-      {Http::Headers::get().Scheme, scheme},
   });
 
-  if (!config_.usePost()) {
+  if (config_.usePost()) {
+    headers->addReference(Http::Headers::get().Path, "/");
+    headers->addReference(Http::Headers::get().Scheme, scheme);
+  } else if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {
+    headers->addReference(Http::Headers::get().Path, "/");
+    headers->addReference(Http::Headers::get().Scheme, scheme);
     headers->addReference(Http::Headers::get().Protocol,
                           Http::Headers::get().ProtocolValues.Bytestream);
   }

test/common/http/http2/codec_impl_test.cc

@@ -3613,7 +3613,8 @@ TEST_P(Http2CodecImplTest, ResetStreamCausesOutboundFlood) {
 }
 
 // CONNECT without upgrade type gets tagged with "bytestream"
-TEST_P(Http2CodecImplTest, ConnectTest) {
+TEST_P(Http2CodecImplTest, ConnectTestOld) {
+  scoped_runtime_.mergeValues({{"envoy.reloadable_features.use_rfc_connect", "false"}});
   client_http2_options_.set_allow_connect(true);
   server_http2_options_.set_allow_connect(true);
   initialize();
@@ -3639,6 +3640,31 @@ TEST_P(Http2CodecImplTest, ConnectTest) {
   driveToCompletion();
 }
 
+TEST_P(Http2CodecImplTest, ConnectTest) {
+  client_http2_options_.set_allow_connect(true);
+  server_http2_options_.set_allow_connect(true);
+  initialize();
+  MockStreamCallbacks callbacks;
+  request_encoder_->getStream().addCallbacks(callbacks);
+
+  TestRequestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  request_headers.setReferenceKey(Headers::get().Method, Http::Headers::get().MethodValues.Connect);
+  request_headers.setReferenceKey(Headers::get().Protocol, "bytestream");
+  TestRequestHeaderMapImpl expected_headers;
+  expected_headers.setReferenceKey(Headers::get().Host, "host");
+  expected_headers.setReferenceKey(Headers::get().Method,
+                                   Http::Headers::get().MethodValues.Connect);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false));
+  EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok());
+  driveToCompletion();
+
+  EXPECT_CALL(callbacks, onResetStream(StreamResetReason::ConnectError, _));
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::ConnectError, _));
+  response_encoder_->getStream().resetStream(StreamResetReason::ConnectError);
+  driveToCompletion();
+}
+
 TEST_P(Http2CodecImplTest, ShouldWaitForDeferredBodyToProcessBeforeProcessingTrailers) {
   // We must initialize before dtor, otherwise we'll touch uninitialized
   // members in dtor.

test/common/tcp_proxy/BUILD

@@ -81,5 +81,6 @@ envoy_cc_test(
         "//test/mocks/http:http_mocks",
         "//test/mocks/tcp:tcp_mocks",
         "//test/mocks/upstream:cluster_manager_mocks",
+        "//test/test_common:test_runtime_lib",
     ],
 )

test/common/tcp_proxy/upstream_test.cc

@@ -9,6 +9,7 @@
 #include "test/mocks/tcp/mocks.h"
 #include "test/test_common/environment.h"
 #include "test/test_common/network_utility.h"
+#include "test/test_common/test_runtime.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -223,7 +224,10 @@ template <typename T> class HttpUpstreamRequestEncoderTest : public testing::Tes
 
 TYPED_TEST_SUITE(HttpUpstreamRequestEncoderTest, Implementations);
 
-TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) {
+TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderOld) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.reloadable_features.use_rfc_connect", "false"}});
+
   this->setupUpstream();
   std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers;
   expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
@@ -243,6 +247,18 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) {
   this->upstream_->setRequestEncoder(this->encoder_, false);
 }
 
+TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) {
+  this->setupUpstream();
+  std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers;
+  expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
+      {Http::Headers::get().Method, "CONNECT"},
+      {Http::Headers::get().Host, this->config_->hostname()},
+  });
+
+  EXPECT_CALL(this->encoder_, encodeHeaders(HeaderMapEqualRef(expected_headers.get()), false));
+  this->upstream_->setRequestEncoder(this->encoder_, false);
+}
+
 TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderUsePost) {
   this->config_message_.set_use_post(true);
   this->setupUpstream();
@@ -287,14 +303,6 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeaders) {
       {Http::Headers::get().Host, this->config_->hostname()},
   });
 
-  if (this->is_http2_) {
-    expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
-    expected_headers->setReferenceKey(Http::Headers::get().Scheme,
-                                      Http::Headers::get().SchemeValues.Http);
-    expected_headers->setReferenceKey(Http::Headers::get().Protocol,
-                                      Http::Headers::get().ProtocolValues.Bytestream);
-  }
-
   expected_headers->setCopy(Http::LowerCaseString("header0"), "value0");
   expected_headers->addCopy(Http::LowerCaseString("header1"), "value1");
   expected_headers->addCopy(Http::LowerCaseString("header1"), "value2");
@@ -323,25 +331,9 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, ConfigReuse) {
       {Http::Headers::get().Host, this->config_->hostname()},
   });
 
-  if (this->is_http2_) {
-    expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
-    expected_headers->setReferenceKey(Http::Headers::get().Scheme,
-                                      Http::Headers::get().SchemeValues.Http);
-    expected_headers->setReferenceKey(Http::Headers::get().Protocol,
-                                      Http::Headers::get().ProtocolValues.Bytestream);
-  }
-
   expected_headers->setCopy(Http::LowerCaseString("key"), "value1");
   expected_headers->addCopy(Http::LowerCaseString("key"), "value2");
 
-  if (this->is_http2_) {
-    expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
-    expected_headers->setReferenceKey(Http::Headers::get().Scheme,
-                                      Http::Headers::get().SchemeValues.Http);
-    expected_headers->setReferenceKey(Http::Headers::get().Protocol,
-                                      Http::Headers::get().ProtocolValues.Bytestream);
-  }
-
   expected_headers->setCopy(Http::LowerCaseString("key"), "value1");
   expected_headers->addCopy(Http::LowerCaseString("key"), "value2");
 
@@ -382,14 +374,6 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn
       {Http::Headers::get().Host, this->config_->hostname()},
   });
 
-  if (this->is_http2_) {
-    expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
-    expected_headers->setReferenceKey(Http::Headers::get().Scheme,
-                                      Http::Headers::get().SchemeValues.Http);
-    expected_headers->setReferenceKey(Http::Headers::get().Protocol,
-                                      Http::Headers::get().ProtocolValues.Bytestream);
-  }
-
   expected_headers->setCopy(Http::LowerCaseString("header0"), "value0");
   expected_headers->addCopy(Http::LowerCaseString("downstream_local_port"), "80");
   auto ip_versions = TestEnvironment::getIpVersionsForTest();

test/extensions/filters/http/compressor/compressor_integration_tests.cc

@@ -268,12 +268,7 @@ TEST_P(CompressorProxyingConnectIntegrationTest, ProxyConnect) {
   RELEASE_ASSERT(result, result.message());
   ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
   EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Method)[0]->value(), "CONNECT");
-  if (upstreamProtocol() == Http::CodecType::HTTP1) {
-    EXPECT_TRUE(upstream_request_->headers().get(Http::Headers::get().Protocol).empty());
-  } else {
-    EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Protocol)[0]->value(),
-              "bytestream");
-  }
+  EXPECT_TRUE(upstream_request_->headers().get(Http::Headers::get().Protocol).empty());
 
   // Send response headers
   upstream_request_->encodeHeaders(default_response_headers_, false);

test/extensions/filters/http/jwt_authn/filter_integration_test.cc

@@ -303,16 +303,11 @@ TEST_P(LocalJwksIntegrationTest, ConnectRequestWithRegExMatch) {
   request_encoder_ = &encoder_decoder.first;
   auto response = std::move(encoder_decoder.second);
 
-  if (downstreamProtocol() == Http::CodecType::HTTP1) {
-    // Because CONNECT requests for HTTP/1 do not include a path, they will fail
-    // to find a route match and return a 404.
-    ASSERT_TRUE(response->waitForEndStream());
-    ASSERT_TRUE(response->complete());
-    EXPECT_EQ("404", response->headers().getStatusValue());
-  } else {
-    ASSERT_TRUE(response->waitForReset());
-    ASSERT_TRUE(codec_client_->waitForDisconnect());
-  }
+  // Because CONNECT requests do not include a path, they will fail
+  // to find a route match and return a 404.
+  ASSERT_TRUE(response->waitForEndStream());
+  ASSERT_TRUE(response->complete());
+  EXPECT_EQ("404", response->headers().getStatusValue());
 }
 
 // The test case with a fake upstream for remote Jwks server.

test/integration/BUILD

@@ -1397,7 +1397,7 @@ envoy_cc_test(
     data = [
         "//test/config/integration/certs",
     ],
-    shard_count = 3,
+    shard_count = 7,
     deps = [
         ":http_integration_lib",
         ":http_protocol_integration_lib",

test/integration/protocol_integration_test.cc

@@ -2733,19 +2733,15 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) {
   request_encoder_ = &encoder_decoder.first;
   auto response = std::move(encoder_decoder.second);
 
-  if (downstreamProtocol() == Http::CodecType::HTTP1) {
-    // Because CONNECT requests for HTTP/1 do not include a path, they will fail
-    // to find a route match and return a 404.
-    ASSERT_TRUE(response->waitForEndStream());
-    EXPECT_EQ("404", response->headers().getStatusValue());
-    EXPECT_TRUE(response->complete());
-  } else {
-    ASSERT_TRUE(response->waitForReset());
-    ASSERT_TRUE(codec_client_->waitForDisconnect());
-  }
+  // Because CONNECT requests do not include a path, they will fail
+  // to find a route match and return a 404.
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("404", response->headers().getStatusValue());
+  EXPECT_TRUE(response->complete());
 }
 
 TEST_P(DownstreamProtocolIntegrationTest, ExtendedConnectIsBlocked) {
+  config_helper_.addRuntimeOverride("envoy.reloadable_features.use_rfc_connect", "false");
   if (downstreamProtocol() == Http::CodecType::HTTP1) {
     return;
   }
@@ -2785,7 +2781,8 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {
   auto response = codec_client_->makeHeaderOnlyRequest(
       Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, {":authority", "sni.lyft.com"}});
 
-  ASSERT_TRUE(response->waitForReset());
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("404", response->headers().getStatusValue());
   EXPECT_FALSE(codec_client_->disconnected());
 }