backport to rel-1.16: proxy_proto - fixing hashing bug #13768 (#13966)

Fix a bug where the transport socket options for the first downstream got reused for subsequent upstream connections.

Risk Level: low
Testing: new integration test
Docs Changes: n/a
Release Notes:
Platform Specific Features:
Fixes #13659

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

docs/root/version_history/current.rst

@@ -13,6 +13,7 @@ Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 * listener: fix crash when disabling or re-enabling listeners due to overload while processing LDS updates.
+* proxy_proto: fixed a bug where the wrong downstream address got sent to upstream connections.
 
 Removed Config or Runtime
 -------------------------

include/envoy/network/proxy_protocol.h

@@ -8,7 +8,11 @@ namespace Network {
 struct ProxyProtocolData {
   const Network::Address::InstanceConstSharedPtr src_addr_;
   const Network::Address::InstanceConstSharedPtr dst_addr_;
+  std::string asStringForHash() const {
+    return std::string(src_addr_ ? src_addr_->asString() : "null") +
+           (dst_addr_ ? dst_addr_->asString() : "null");
+  }
 };
 
 } // namespace Network
-} // namespace Envoy
+} // namespace Envoy

include/envoy/network/transport_socket.h

@@ -14,6 +14,7 @@
 namespace Envoy {
 namespace Network {
 
+class TransportSocketFactory;
 class Connection;
 enum class ConnectionEvent;
 
@@ -198,11 +199,13 @@ class TransportSocketOptions {
   virtual absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const PURE;
 
   /**
-   * @param vector of bytes to which the option should append hash key data that will be used
-   *        to separate connections based on the option. Any data already in the key vector must
-   *        not be modified.
+   * @param key supplies a vector of bytes to which the option should append hash key data that will
+   *        be used to separate connections based on the option. Any data already in the key vector
+   *        must not be modified.
+   * @param factory supplies the factor which will be used for creating the transport socket.
    */
-  virtual void hashKey(std::vector<uint8_t>& key) const PURE;
+  virtual void hashKey(std::vector<uint8_t>& key,
+                       const Network::TransportSocketFactory& factory) const PURE;
 };
 
 // TODO(mattklein123): Rename to TransportSocketOptionsConstSharedPtr in a dedicated follow up.
@@ -226,6 +229,11 @@ class TransportSocketFactory {
    */
   virtual TransportSocketPtr
   createTransportSocket(TransportSocketOptionsSharedPtr options) const PURE;
+
+  /**
+   * @return bool whether the transport socket will use proxy protocol options.
+   */
+  virtual bool usesProxyProtocolOptions() const PURE;
 };
 
 using TransportSocketFactoryPtr = std::unique_ptr<TransportSocketFactory>;

source/common/network/raw_buffer_socket.h

@@ -32,6 +32,7 @@ class RawBufferSocketFactory : public TransportSocketFactory {
   // Network::TransportSocketFactory
   TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  bool usesProxyProtocolOptions() const override { return false; }
 };
 
 } // namespace Network

source/common/network/transport_socket_options_impl.cc

@@ -16,7 +16,8 @@
 namespace Envoy {
 namespace Network {
 namespace {
-void commonHashKey(const TransportSocketOptions& options, std::vector<std::uint8_t>& key) {
+void commonHashKey(const TransportSocketOptions& options, std::vector<std::uint8_t>& key,
+                   const Network::TransportSocketFactory& factory) {
   const auto& server_name_overide = options.serverNameOverride();
   if (server_name_overide.has_value()) {
     pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(server_name_overide.value()), key);
@@ -35,19 +36,30 @@ void commonHashKey(const TransportSocketOptions& options, std::vector<std::uint8
       pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(protocol), key);
     }
   }
+
   const auto& alpn_fallback = options.applicationProtocolFallback();
   if (alpn_fallback.has_value()) {
     pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(*alpn_fallback), key);
   }
+
+  // Proxy protocol options should only be included in the hash if the upstream
+  // socket intends to use them.
+  const auto& proxy_protocol_options = options.proxyProtocolOptions();
+  if (proxy_protocol_options.has_value() && factory.usesProxyProtocolOptions()) {
+    pushScalarToByteVector(
+        StringUtil::CaseInsensitiveHash()(proxy_protocol_options.value().asStringForHash()), key);
+  }
 }
 } // namespace
 
-void AlpnDecoratingTransportSocketOptions::hashKey(std::vector<uint8_t>& key) const {
-  commonHashKey(*this, key);
+void AlpnDecoratingTransportSocketOptions::hashKey(
+    std::vector<uint8_t>& key, const Network::TransportSocketFactory& factory) const {
+  commonHashKey(*this, key, factory);
 }
 
-void TransportSocketOptionsImpl::hashKey(std::vector<uint8_t>& key) const {
-  commonHashKey(*this, key);
+void TransportSocketOptionsImpl::hashKey(std::vector<uint8_t>& key,
+                                         const Network::TransportSocketFactory& factory) const {
+  commonHashKey(*this, key, factory);
 }
 
 TransportSocketOptionsSharedPtr

source/common/network/transport_socket_options_impl.h

@@ -29,7 +29,8 @@ class AlpnDecoratingTransportSocketOptions : public TransportSocketOptions {
   absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const override {
     return inner_options_->proxyProtocolOptions();
   }
-  void hashKey(std::vector<uint8_t>& key) const override;
+  void hashKey(std::vector<uint8_t>& key,
+               const Network::TransportSocketFactory& factory) const override;
 
 private:
   const absl::optional<std::string> alpn_fallback_;
@@ -67,7 +68,8 @@ class TransportSocketOptionsImpl : public TransportSocketOptions {
   absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const override {
     return proxy_protocol_options_;
   }
-  void hashKey(std::vector<uint8_t>& key) const override;
+  void hashKey(std::vector<uint8_t>& key,
+               const Network::TransportSocketFactory& factory) const override;
 
 private:
   const absl::optional<std::string> override_server_name_;

source/common/upstream/cluster_manager_impl.cc

@@ -1380,7 +1380,7 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool(
 
   bool have_transport_socket_options = false;
   if (context && context->upstreamTransportSocketOptions()) {
-    context->upstreamTransportSocketOptions()->hashKey(hash_key);
+    context->upstreamTransportSocketOptions()->hashKey(hash_key, host->transportSocketFactory());
     have_transport_socket_options = true;
   }
 
@@ -1440,7 +1440,7 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPool(
   bool have_transport_socket_options = false;
   if (context != nullptr && context->upstreamTransportSocketOptions() != nullptr) {
     have_transport_socket_options = true;
-    context->upstreamTransportSocketOptions()->hashKey(hash_key);
+    context->upstreamTransportSocketOptions()->hashKey(hash_key, host->transportSocketFactory());
   }
 
   TcpConnPoolsContainer& container = parent_.host_tcp_conn_pool_map_[host];

source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h

@@ -24,6 +24,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory {
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
   bool implementsSecureTransport() const override { return true; }
+  bool usesProxyProtocolOptions() const override { return false; }
 };
 
 // TODO(danzh): when implement ProofSource, examine of it's necessary to

source/extensions/transport_sockets/alts/tsi_socket.h

@@ -98,6 +98,7 @@ class TsiSocketFactory : public Network::TransportSocketFactory {
   TsiSocketFactory(HandshakerFactory handshaker_factory, HandshakeValidator handshake_validator);
 
   bool implementsSecureTransport() const override;
+  bool usesProxyProtocolOptions() const override { return false; }
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
 

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h

@@ -49,6 +49,7 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  bool usesProxyProtocolOptions() const override { return true; }
 
 private:
   Network::TransportSocketFactoryPtr transport_socket_factory_;
@@ -58,4 +59,4 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor
 } // namespace ProxyProtocol
 } // namespace TransportSockets
 } // namespace Extensions
-} // namespace Envoy
+} // namespace Envoy

source/extensions/transport_sockets/tap/tap.cc

@@ -66,6 +66,10 @@ bool TapSocketFactory::implementsSecureTransport() const {
   return transport_socket_factory_->implementsSecureTransport();
 }
 
+bool TapSocketFactory::usesProxyProtocolOptions() const {
+  return transport_socket_factory_->usesProxyProtocolOptions();
+}
+
 } // namespace Tap
 } // namespace TransportSockets
 } // namespace Extensions

source/extensions/transport_sockets/tap/tap.h

@@ -41,6 +41,7 @@ class TapSocketFactory : public Network::TransportSocketFactory,
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  bool usesProxyProtocolOptions() const override;
 
 private:
   Network::TransportSocketFactoryPtr transport_socket_factory_;

source/extensions/transport_sockets/tls/ssl_socket.h

@@ -108,6 +108,7 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  bool usesProxyProtocolOptions() const override { return false; }
 
   // Secret::SecretCallbacks
   void onAddOrUpdateSecret() override;
@@ -132,6 +133,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  bool usesProxyProtocolOptions() const override { return false; }
 
   // Secret::SecretCallbacks
   void onAddOrUpdateSecret() override;

test/common/network/BUILD

@@ -234,6 +234,15 @@ envoy_cc_test(
     ],
 )
 
+envoy_cc_test(
+    name = "raw_buffer_socket_test",
+    srcs = ["raw_buffer_socket_test.cc"],
+    deps = [
+        "//source/common/network:raw_buffer_socket_lib",
+        "//test/test_common:network_utility_lib",
+    ],
+)
+
 envoy_cc_test_library(
     name = "udp_listener_impl_test_base_lib",
     hdrs = ["udp_listener_impl_test_base.h"],

test/common/network/raw_buffer_socket_test.cc

@@ -0,0 +1,16 @@
+#include "common/network/raw_buffer_socket.h"
+
+#include "test/test_common/network_utility.h"
+
+#include "gtest/gtest.h"
+
+namespace Envoy {
+namespace Network {
+
+TEST(RawBufferSocketFactory, RawBufferSocketFactory) {
+  TransportSocketFactoryPtr factory = Envoy::Network::Test::createRawBufferSocketFactory();
+  EXPECT_FALSE(factory->usesProxyProtocolOptions());
+}
+
+} // namespace Network
+} // namespace Envoy

test/common/upstream/transport_socket_matcher_test.cc

@@ -31,6 +31,7 @@ namespace {
 class FakeTransportSocketFactory : public Network::TransportSocketFactory {
 public:
   MOCK_METHOD(bool, implementsSecureTransport, (), (const));
+  MOCK_METHOD(bool, usesProxyProtocolOptions, (), (const));
   MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket,
               (Network::TransportSocketOptionsSharedPtr), (const));
   FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {}
@@ -46,6 +47,7 @@ class FooTransportSocketFactory
       Logger::Loggable<Logger::Id::upstream> {
 public:
   MOCK_METHOD(bool, implementsSecureTransport, (), (const));
+  MOCK_METHOD(bool, usesProxyProtocolOptions, (), (const));
   MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket,
               (Network::TransportSocketOptionsSharedPtr), (const));
 

test/extensions/transport_sockets/alts/tsi_socket_test.cc

@@ -55,7 +55,6 @@ class TsiSocketTest : public testing::Test {
     client_.tsi_socket_ =
         std::make_unique<TsiSocket>(client_.handshaker_factory_, client_validator,
                                     Network::TransportSocketPtr{client_.raw_socket_});
-
     ON_CALL(client_.callbacks_.connection_, dispatcher()).WillByDefault(ReturnRef(dispatcher_));
     ON_CALL(server_.callbacks_.connection_, dispatcher()).WillByDefault(ReturnRef(dispatcher_));
 
@@ -194,6 +193,7 @@ static const std::string ClientToServerData = "hello from client";
 TEST_F(TsiSocketTest, DoesNotHaveSsl) {
   initialize(nullptr, nullptr);
   EXPECT_EQ(nullptr, client_.tsi_socket_->ssl());
+  EXPECT_FALSE(client_.tsi_socket_->canFlushClose());
 
   const auto& socket_ = *client_.tsi_socket_;
   EXPECT_EQ(nullptr, socket_.ssl());
@@ -406,6 +406,10 @@ TEST_F(TsiSocketFactoryTest, ImplementsSecureTransport) {
   EXPECT_TRUE(socket_factory_->implementsSecureTransport());
 }
 
+TEST_F(TsiSocketFactoryTest, UsesProxyProtocolOptions) {
+  EXPECT_FALSE(socket_factory_->usesProxyProtocolOptions());
+}
+
 } // namespace
 } // namespace Alts
 } // namespace TransportSockets

test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc

@@ -104,6 +104,36 @@ TEST_P(ProxyProtocolIntegrationTest, TestV1ProxyProtocol) {
   ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
 }
 
+TEST_P(ProxyProtocolIntegrationTest, TestV1ProxyProtocolMultipleConnections) {
+  if (GetParam() != Network::Address::IpVersion::v4) {
+    return;
+  }
+
+  setup(envoy::config::core::v3::ProxyProtocolConfig::V1, false,
+        "envoy.transport_sockets.raw_buffer");
+  initialize();
+  auto listener_port = lookupPort("listener_0");
+
+  auto loopback2 = Network::Utility::resolveUrl("tcp://127.0.0.2:0");
+  auto tcp_client2 = makeTcpConnection(listener_port, nullptr, loopback2);
+
+  auto tcp_client = makeTcpConnection(listener_port);
+
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_));
+  FakeRawConnectionPtr conn2;
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(conn2));
+
+  std::string data1, data2;
+  ASSERT_TRUE(
+      fake_upstream_connection_->waitForData(FakeRawConnection::waitForAtLeastBytes(32), &data1));
+  ASSERT_TRUE(conn2->waitForData(FakeRawConnection::waitForAtLeastBytes(32), &data2));
+
+  EXPECT_NE(data1, data2);
+
+  tcp_client->close();
+  tcp_client2->close();
+}
+
 // Test header is sent unencrypted using a TLS inner socket
 TEST_P(ProxyProtocolIntegrationTest, TestTLSSocket) {
   setup(envoy::config::core::v3::ProxyProtocolConfig::V1, false, "envoy.transport_sockets.tls");
@@ -201,4 +231,4 @@ TEST_P(ProxyProtocolIntegrationTest, TestV2ProxyProtocol) {
 }
 
 } // namespace
-} // namespace Envoy
+} // namespace Envoy

test/extensions/transport_sockets/tap/tap_config_impl_test.cc

@@ -58,7 +58,11 @@ class PerSocketTapperImplTest : public testing::Test {
     EXPECT_CALL(matcher_, onNewStream(_))
         .WillOnce(Invoke([this](TapCommon::Matcher::MatchStatusVector& statuses) {
           statuses_ = &statuses;
-          statuses[0].matches_ = true;
+          if (fail_match_) {
+            statuses[0].matches_ = false;
+          } else {
+            statuses[0].matches_ = true;
+          }
           statuses[0].might_change_status_ = false;
         }));
     EXPECT_CALL(*config_, streaming()).WillRepeatedly(Return(streaming));
@@ -79,6 +83,7 @@ class PerSocketTapperImplTest : public testing::Test {
   TapCommon::Matcher::MatchStatusVector* statuses_;
   NiceMock<Network::MockConnection> connection_;
   Event::SimulatedTimeSystem time_system_;
+  bool fail_match_{};
 };
 
 // Verify the full streaming flow.
@@ -139,6 +144,15 @@ TEST_F(PerSocketTapperImplTest, StreamingFlow) {
   tapper_->closeSocket(Network::ConnectionEvent::RemoteClose);
 }
 
+TEST_F(PerSocketTapperImplTest, NonMatchingFlow) {
+  fail_match_ = true;
+  setup(true);
+
+  EXPECT_CALL(*sink_manager_, submitTrace_(_)).Times(0);
+  time_system_.setSystemTime(std::chrono::seconds(2));
+  tapper_->closeSocket(Network::ConnectionEvent::RemoteClose);
+}
+
 } // namespace
 } // namespace Tap
 } // namespace TransportSockets

test/integration/base_integration_test.cc

@@ -190,9 +190,10 @@ void BaseIntegrationTest::setUpstreamProtocol(FakeHttpConnection::Type protocol)
 
 IntegrationTcpClientPtr
 BaseIntegrationTest::makeTcpConnection(uint32_t port,
-                                       const Network::ConnectionSocket::OptionsSharedPtr& options) {
+                                       const Network::ConnectionSocket::OptionsSharedPtr& options,
+                                       Network::Address::InstanceConstSharedPtr source_address) {
   return std::make_unique<IntegrationTcpClient>(*dispatcher_, *mock_buffer_factory_, port, version_,
-                                                enable_half_close_, options);
+                                                enable_half_close_, options, source_address);
 }
 
 void BaseIntegrationTest::registerPort(const std::string& key, uint32_t port) {

test/integration/base_integration_test.h

@@ -86,7 +86,9 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
 
   IntegrationTcpClientPtr
   makeTcpConnection(uint32_t port,
-                    const Network::ConnectionSocket::OptionsSharedPtr& options = nullptr);
+                    const Network::ConnectionSocket::OptionsSharedPtr& options = nullptr,
+                    Network::Address::InstanceConstSharedPtr source_address =
+                        Network::Address::InstanceConstSharedPtr());
 
   // Test-wide port map.
   void registerPort(const std::string& key, uint32_t port);