diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 6469d4d52042..a99b85f40574 100644 --- a/changelogs/current.yaml +++ b/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" diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 8b685d8a0f71..ac6d76016105 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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); diff --git a/source/extensions/transport_sockets/http_11_proxy/BUILD b/source/extensions/transport_sockets/http_11_proxy/BUILD index 78df614fb285..c33fe2db6850 100644 --- a/source/extensions/transport_sockets/http_11_proxy/BUILD +++ b/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", diff --git a/source/extensions/transport_sockets/http_11_proxy/connect.cc b/source/extensions/transport_sockets/http_11_proxy/connect.cc index 5e1c10f7d5c7..69bd12ba02e9 100644 --- a/source/extensions/transport_sockets/http_11_proxy/connect.cc +++ b/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; } diff --git a/test/extensions/transport_sockets/http_11_proxy/connect_integration_test.cc b/test/extensions/transport_sockets/http_11_proxy/connect_integration_test.cc index 826f711b63f1..37ecf34ad0c0 100644 --- a/test/extensions/transport_sockets/http_11_proxy/connect_integration_test.cc +++ b/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 diff --git a/test/extensions/transport_sockets/http_11_proxy/connect_test.cc b/test/extensions/transport_sockets/http_11_proxy/connect_test.cc index 816a5db8a8d1..e6b9c3a1aefc 100644 --- a/test/extensions/transport_sockets/http_11_proxy/connect_test.cc +++ b/test/extensions/transport_sockets/http_11_proxy/connect_test.cc @@ -39,10 +39,12 @@ class Http11ConnectTest : public testing::TestWithParam 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>()}; 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 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>(); std::unique_ptr 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();