proxy_filter: Fix the CONNECT implementation when the hostname contai…

…ns a port number (envoyproxy#36072)

Commit Message: Fixes the `CONNECT` implementation when the hostname
contains a port number causing the `CONNECT` request created to be
invalid.
Additional Description: When the port number is not specified, the port
443 will be automatically added just like before.
Risk Level: low
Testing: unit and integration tests
Docs Changes: inline
Release Notes: inline
Platform Specific Features: n/a
Optional Runtime guard: `envoy.reloadable_features.proxy_ssl_port`

---------

Signed-off-by: Fredy Wijaya <[email protected]>

changelogs/current.yaml

@@ -128,6 +128,11 @@ bug_fixes:
     the number of requests per I/O cycle is configured and an HTTP decoder filter that pauses filter chain is present. This behavior
     can be reverted by setting the runtime guard ``envoy.reloadable_features.use_filter_manager_state_for_downstream_end_stream``
     to false.
+- area: proxy_filter
+  change: |
+    Fixed a bug in the ``CONNECT`` implementation that would cause the ``CONNECT`` request created to be invalid when the
+    hostname contains a port number. When the port number is not specified, the port 443 will be automatically added.
+    This behavior can be reverted by setting the runtime guard ``envoy.reloadable_features.proxy_ssl_port`` to ``false``.
 - area: runtime
   change: |
     Fixed an inconsistency in how boolean values are loaded in RTDS, where they were previously converted to "1"/"0"

source/common/runtime/runtime_features.cc

@@ -74,6 +74,7 @@ RUNTIME_GUARD(envoy_reloadable_features_no_timer_based_rate_limit_token_bucket);
 RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout);
 RUNTIME_GUARD(envoy_reloadable_features_prefer_ipv6_dns_on_macos);
 RUNTIME_GUARD(envoy_reloadable_features_proxy_104);
+RUNTIME_GUARD(envoy_reloadable_features_proxy_ssl_port);
 RUNTIME_GUARD(envoy_reloadable_features_proxy_status_mapping_more_core_response_flags);
 RUNTIME_GUARD(envoy_reloadable_features_quic_connect_client_udp_sockets);
 RUNTIME_GUARD(envoy_reloadable_features_quic_receive_ecn);

source/extensions/transport_sockets/http_11_proxy/BUILD

@@ -37,6 +37,7 @@ envoy_cc_library(
         "//source/common/common:scalar_to_byte_vector_lib",
         "//source/common/common:utility_lib",
         "//source/common/config:well_known_names",
+        "//source/common/http:header_utility_lib",
         "//source/common/http/http1:balsa_parser_lib",
         "//source/common/http/http1:legacy_parser_lib",
         "//source/common/network:address_lib",

source/extensions/transport_sockets/http_11_proxy/connect.cc

@@ -8,6 +8,7 @@
 #include "source/common/common/scalar_to_byte_vector.h"
 #include "source/common/common/utility.h"
 #include "source/common/config/well_known_names.h"
+#include "source/common/http/header_utility.h"
 #include "source/common/network/address_impl.h"
 #include "source/common/runtime/runtime_features.h"
 
@@ -37,11 +38,17 @@ UpstreamHttp11ConnectSocket::UpstreamHttp11ConnectSocket(
   // options, we want to maintain the original behavior of this transport socket.
   if (options_ && options_->http11ProxyInfo()) {
     if (transport_socket_->ssl()) {
-      header_buffer_.add(
-          absl::StrCat("CONNECT ", options_->http11ProxyInfo()->hostname, ":443 HTTP/1.1\r\n\r\n"));
+      if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.proxy_ssl_port")) {
+        header_buffer_.add(absl::StrCat(
+            "CONNECT ", options_->http11ProxyInfo()->hostname,
+            Http::HeaderUtility::hostHasPort(options_->http11ProxyInfo()->hostname) ? "" : ":443",
+            " HTTP/1.1\r\n\r\n"));
+      } else {
+        header_buffer_.add(absl::StrCat("CONNECT ", options_->http11ProxyInfo()->hostname,
+                                        ":443 HTTP/1.1\r\n\r\n"));
+      }
       need_to_strip_connect_response_ = true;
     }
-
     return;
   }
 

test/extensions/transport_sockets/http_11_proxy/connect_integration_test.cc

@@ -124,8 +124,9 @@ name: envoy.clusters.dynamic_forward_proxy
     // Strip the CONNECT upgrade.
     std::string prefix_data;
     const std::string hostname(default_request_headers_.getHostValue());
+    const std::string port = Http::HeaderUtility::hostHasPort(hostname) ? "" : ":443";
     ASSERT_TRUE(fake_upstream_connection_->waitForInexactRawData("\r\n\r\n", &prefix_data));
-    EXPECT_EQ(absl::StrCat("CONNECT ", hostname, ":443 HTTP/1.1\r\n\r\n"), prefix_data);
+    EXPECT_EQ(absl::StrCat("CONNECT ", hostname, port, " HTTP/1.1\r\n\r\n"), prefix_data);
 
     absl::string_view content_length = include_content_length ? "Content-Length: 0\r\n" : "";
     // Ship the CONNECT response.
@@ -510,5 +511,33 @@ TEST_P(Http11ConnectHttpIntegrationTest, DfpWithProxyAddressLegacy) {
   EXPECT_EQ("503", response->headers().getStatusValue());
 }
 
+TEST_P(Http11ConnectHttpIntegrationTest, HostWithPort) {
+  initialize();
+
+  absl::string_view second_upstream_address(fake_upstreams_[1]->localAddress()->asStringView());
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  // The connect-proxy header will be stripped by the header-to-proxy-filter and inserted as
+  // metadata.
+  default_request_headers_.setCopy(Envoy::Http::LowerCaseString("connect-proxy"),
+                                   second_upstream_address);
+  default_request_headers_.setHost("sni.lyft.com:443");
+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+
+  ASSERT_TRUE(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+
+  stripConnectUpgradeAndRespond();
+
+  // Enable reading on the new stream, and read the encapsulated request.
+  ASSERT_TRUE(fake_upstream_connection_->readDisable(false));
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+
+  upstream_request_->encodeHeaders(default_response_headers_, true);
+
+  // Wait for the encapsulated response to be received.
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("200", response->headers().getStatusValue());
+}
+
 } // namespace
 } // namespace Envoy

test/extensions/transport_sockets/http_11_proxy/connect_test.cc

@@ -39,10 +39,12 @@ class Http11ConnectTest : public testing::TestWithParam<Network::Address::IpVers
 public:
   Http11ConnectTest() = default;
 
-  void initialize(bool no_proxy_protocol = false) { initializeInternal(no_proxy_protocol, false); }
+  void initialize(bool no_proxy_protocol = false, absl::optional<uint32_t> target_port = {}) {
+    initializeInternal(no_proxy_protocol, false, target_port);
+  }
 
   // Initialize the test with the proxy address provided via endpoint metadata.
-  void initializeWithMetadataProxyAddr() { initializeInternal(false, true); }
+  void initializeWithMetadataProxyAddr() { initializeInternal(false, true, {}); }
 
   void setAddress() {
     std::string address_string =
@@ -86,13 +88,15 @@ class Http11ConnectTest : public testing::TestWithParam<Network::Address::IpVers
       std::make_shared<NiceMock<Envoy::Ssl::MockConnectionInfo>>()};
 
 private:
-  void initializeInternal(bool no_proxy_protocol, bool use_metadata_proxy_addr) {
+  void initializeInternal(bool no_proxy_protocol, bool use_metadata_proxy_addr,
+                          absl::optional<uint32_t> target_port) {
     std::string address_string =
         absl::StrCat(Network::Test::getLoopbackAddressUrlString(GetParam()), ":1234");
     Network::Address::InstanceConstSharedPtr address =
         Network::Utility::parseInternetAddressAndPortNoThrow(address_string);
 
-    const std::string proxy_info_hostname = "www.foo.com";
+    const std::string port = target_port.has_value() ? absl::StrCat(":", *target_port) : "";
+    const std::string proxy_info_hostname = absl::StrCat("www.foo.com", port);
     auto host = std::make_shared<NiceMock<Upstream::MockHostDescription>>();
     std::unique_ptr<Network::TransportSocketOptions::Http11ProxyInfo> info;
 
@@ -144,6 +148,19 @@ TEST_P(Http11ConnectTest, InjectsHeaderOnlyOnceTransportSocketOpts) {
   injectHeaderOnceTest();
 }
 
+TEST_P(Http11ConnectTest, HostWithPort) {
+  initialize(false, 443);
+  injectHeaderOnceTest();
+}
+
+TEST_P(Http11ConnectTest, ProxySslPortRuntimeGuardDisabled) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.reloadable_features.proxy_ssl_port", "false"}});
+
+  initialize();
+  injectHeaderOnceTest();
+}
+
 // Test injects CONNECT only once. Configured via endpoint metadata.
 TEST_P(Http11ConnectTest, InjectsHeaderOnlyOnceEndpointMetadata) {
   initializeWithMetadataProxyAddr();