proxy protocol: set downstreamRemoteAddress on StreamInfo (envoyproxy…

…#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 envoyproxy#14087 Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Greg Greenway <ggreenway@users.noreply.github.com> Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
cpakulski · Nov 24, 2020 · e0c33d8 · e0c33d8
1 parent b1540ba
commit e0c33d8
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 14 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -4,3 +4,4 @@
 Changes
 -------
 * listener: fix crash when disabling or re-enabling listeners due to overload while processing LDS updates.
+* 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.
diff --git a/test/integration/BUILD b/test/integration/BUILD
@@ -837,10 +837,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/config/filter/network/tcp_proxy/v2:pkg_cc_proto",
+        "@envoy_api//envoy/extensions/access_loggers/file/v3:pkg_cc_proto",
     ],
 )
 

diff --git 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/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.h"
+#include "envoy/extensions/access_loggers/file/v3/file.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,58 @@ 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<API_NO_BOOST(envoy::config::filter::network::tcp_proxy::v2::TcpProxy)>());
+    auto tcp_proxy_config = MessageUtil::anyConvert<API_NO_BOOST(
+        envoy::config::filter::network::tcp_proxy::v2::TcpProxy)>(*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
@@ -16,19 +16,13 @@ namespace Envoy {
 class ProxyProtoIntegrationTest : public testing::TestWithParam<Network::Address::IpVersion>,
                                   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<Network::Address::IpVersion>,
+                                     public BaseIntegrationTest {
+public:
+  ProxyProtoTcpIntegrationTest();
+};
+
 } // namespace Envoy