From 59945aab04ee8f60a8141da619abf54077eafd4a Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 20 Nov 2020 18:38:53 -0800 Subject: [PATCH] proxy protocol: set downstreamRemoteAddress on StreamInfo (#14131) This fixes a regression which resulted in the downstreamRemoteAddress on the StreamInfo for a connection not having the address supplied by the proxy protocol filter, but instead having the address of the directly connected peer. This issue does not affect HTTP filters. Fixes #14087 Signed-off-by: Greg Greenway --- docs/root/version_history/current.rst | 1 + source/server/connection_handler_impl.cc | 5 +- test/integration/BUILD | 4 + .../proxy_proto_integration_test.cc | 75 +++++++++++++++++++ .../proxy_proto_integration_test.h | 22 ++---- 5 files changed, 91 insertions(+), 16 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 4bfc93a81324..a1393af57896 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -37,6 +37,7 @@ Bug Fixes * http: reject requests with missing required headers after filter chain processing. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. * proxy_proto: fixed a bug where the wrong downstream address got sent to upstream connections. +* proxy_proto: fixed a bug where network filters would not have the correct downstreamRemoteAddress() when accessed from the StreamInfo. This could result in incorrect enforcement of RBAC rules in the RBAC network filter (but not in the RBAC HTTP filter), or incorrect access log addresses from tcp_proxy. * tls: fix detection of the upstream connection close event. * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. * udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index beba66b72f0b..91b92f763ffc 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -460,9 +460,10 @@ void ConnectionHandlerImpl::ActiveTcpListener::resumeListening() { void ConnectionHandlerImpl::ActiveTcpListener::newConnection( Network::ConnectionSocketPtr&& socket, std::unique_ptr stream_info) { - // Refresh local address in case it was restored by a listener filter like the original_dst - // filter. + // Refresh addresses in case they are modified by listener filters, such as proxy protocol or + // original_dst. stream_info->setDownstreamLocalAddress(socket->localAddress()); + stream_info->setDownstreamRemoteAddress(socket->remoteAddress()); // Find matching filter chain. const auto filter_chain = config_->filterChainManager().findFilterChain(*socket); diff --git a/test/integration/BUILD b/test/integration/BUILD index 43dfa373074d..15731c7e8aef 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1069,10 +1069,14 @@ envoy_cc_test( ":http_integration_lib", "//source/common/buffer:buffer_lib", "//source/common/http:codec_client_lib", + "//source/extensions/access_loggers/file:config", "//source/extensions/filters/listener/proxy_protocol:config", + "//source/extensions/filters/network/tcp_proxy:config", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/access_loggers/file/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto", ], ) diff --git a/test/integration/proxy_proto_integration_test.cc b/test/integration/proxy_proto_integration_test.cc index 064d884899ec..9cea358aa4d1 100644 --- a/test/integration/proxy_proto_integration_test.cc +++ b/test/integration/proxy_proto_integration_test.cc @@ -2,6 +2,8 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/cluster/v3/cluster.pb.h" +#include "envoy/extensions/access_loggers/file/v3/file.pb.h" +#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" #include "common/buffer/buffer_impl.h" @@ -14,6 +16,24 @@ namespace Envoy { +static void +insertProxyProtocolFilterConfigModifier(envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + ::envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proxy_protocol; + auto rule = proxy_protocol.add_rules(); + rule->set_tlv_type(0x02); + rule->mutable_on_tlv_present()->set_key("PP2TypeAuthority"); + + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto* ppv_filter = listener->add_listener_filters(); + ppv_filter->set_name("envoy.listener.proxy_protocol"); + ppv_filter->mutable_typed_config()->PackFrom(proxy_protocol); +} + +ProxyProtoIntegrationTest::ProxyProtoIntegrationTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) { + config_helper_.addConfigModifier(insertProxyProtocolFilterConfigModifier); +} + INSTANTIATE_TEST_SUITE_P(IpVersions, ProxyProtoIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -201,4 +221,59 @@ TEST_P(ProxyProtoIntegrationTest, ClusterProvided) { testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } +ProxyProtoTcpIntegrationTest::ProxyProtoTcpIntegrationTest() + : BaseIntegrationTest(GetParam(), ConfigHelper::tcpProxyConfig()) { + config_helper_.addConfigModifier(insertProxyProtocolFilterConfigModifier); + config_helper_.renameListener("tcp_proxy"); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, ProxyProtoTcpIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +// This tests that the StreamInfo contains the correct addresses. +TEST_P(ProxyProtoTcpIntegrationTest, AccessLog) { + std::string access_log_path = TestEnvironment::temporaryPath( + fmt::format("access_log{}.txt", version_ == Network::Address::IpVersion::v4 ? "v4" : "v6")); + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto* filter_chain = listener->mutable_filter_chains(0); + auto* config_blob = filter_chain->mutable_filters(0)->mutable_typed_config(); + + ASSERT_TRUE( + config_blob + ->Is()); + auto tcp_proxy_config = MessageUtil::anyConvert(*config_blob); + + auto* access_log = tcp_proxy_config.add_access_log(); + access_log->set_name("accesslog"); + envoy::extensions::access_loggers::file::v3::FileAccessLog access_log_config; + access_log_config.set_path(access_log_path); + access_log_config.mutable_log_format()->set_text_format( + "remote=%DOWNSTREAM_REMOTE_ADDRESS% local=%DOWNSTREAM_LOCAL_ADDRESS%"); + access_log->mutable_typed_config()->PackFrom(access_log_config); + config_blob->PackFrom(tcp_proxy_config); + }); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + ASSERT_TRUE(tcp_client->write("PROXY TCP4 1.2.3.4 254.254.254.254 12345 1234\r\nhello", false)); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(fake_upstream_connection->waitForData(5)); + ASSERT_TRUE(fake_upstream_connection->close()); + tcp_client->close(); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + + std::string log_result; + // Access logs only get flushed to disk periodically, so poll until the log is non-empty + do { + log_result = api_->fileSystem().fileReadToEnd(access_log_path); + } while (log_result.empty()); + + EXPECT_EQ(log_result, "remote=1.2.3.4:12345 local=254.254.254.254:1234"); +} + } // namespace Envoy diff --git a/test/integration/proxy_proto_integration_test.h b/test/integration/proxy_proto_integration_test.h index 140d5b63d3f7..3a719ad44aaf 100644 --- a/test/integration/proxy_proto_integration_test.h +++ b/test/integration/proxy_proto_integration_test.h @@ -16,19 +16,13 @@ namespace Envoy { class ProxyProtoIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: - ProxyProtoIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) { - config_helper_.addConfigModifier( - [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { - ::envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proxy_protocol; - auto rule = proxy_protocol.add_rules(); - rule->set_tlv_type(0x02); - rule->mutable_on_tlv_present()->set_key("PP2TypeAuthority"); - - auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); - auto* ppv_filter = listener->add_listener_filters(); - ppv_filter->set_name("envoy.listener.proxy_protocol"); - ppv_filter->mutable_typed_config()->PackFrom(proxy_protocol); - }); - } + ProxyProtoIntegrationTest(); }; + +class ProxyProtoTcpIntegrationTest : public testing::TestWithParam, + public BaseIntegrationTest { +public: + ProxyProtoTcpIntegrationTest(); +}; + } // namespace Envoy