From c51be677bf9e3d23a870c990316a9fa474f2f18c Mon Sep 17 00:00:00 2001
From: Matt Klein <mklein@lyft.com>
Date: Tue, 30 Nov 2021 23:37:37 +0000
Subject: [PATCH] listener: disallow duplicate port specification

This bug existed previously, but with the recent reuse port
as the default change is now more obvious. Previously, we would
allow multiple listeners to listen on the same port, which is
obviously very wrong. This change blocks that at config load time.

Fixes https://github.com/envoyproxy/envoy/issues/19099

Signed-off-by: Matt Klein <mklein@lyft.com>
---
 source/server/listener_manager_impl.cc    | 14 ++++++------
 test/server/listener_manager_impl_test.cc | 28 +++++++++++++++++++++++
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc
index 545645f01cc2..2ae4f35f1bfc 100644
--- a/source/server/listener_manager_impl.cc
+++ b/source/server/listener_manager_impl.cc
@@ -961,13 +961,13 @@ Network::DrainableFilterChainSharedPtr ListenerFilterChainFactoryBuilder::buildF
 void ListenerManagerImpl::setNewOrDrainingSocketFactory(
     const std::string& name, const envoy::config::core::v3::Address& proto_address,
     ListenerImpl& listener) {
-  // Typically we catch address issues when we try to bind to the same address multiple times.
-  // However, for listeners that do not bind we must check to make sure we are not duplicating. This
-  // is an edge case and nothing will explicitly break, but there is no possibility that two
-  // listeners that do not bind will ever be used. Only the first one will be used when searched for
-  // by address. Thus we block it.
-  if (!listener.bindToPort() && (hasListenerWithCompatibleAddress(warming_listeners_, listener) ||
-                                 hasListenerWithCompatibleAddress(active_listeners_, listener))) {
+  // For listeners that do not bind or listeners that do not bind to port 0 we must check to make
+  // sure we are not duplicating the address. This avoids ambiguity about about which non-binding
+  // listener is used or even worse for the binding to port != 0 and reuse port case multiple
+  // different listeners receiving connections.
+  if ((!listener.bindToPort() || listener.config().address().socket_address().port_value() != 0) &&
+      (hasListenerWithCompatibleAddress(warming_listeners_, listener) ||
+       hasListenerWithCompatibleAddress(active_listeners_, listener))) {
     const std::string message =
         fmt::format("error adding listener: '{}' has duplicate address '{}' as existing listener",
                     name, listener.address()->asString());
diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc
index 8002094c4646..69f7166ef129 100644
--- a/test/server/listener_manager_impl_test.cc
+++ b/test/server/listener_manager_impl_test.cc
@@ -206,6 +206,34 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, DefaultListenerPerConnectionBuffe
   EXPECT_EQ(1024 * 1024U, manager_->listeners().back().get().perConnectionBufferLimitBytes());
 }
 
+TEST_F(ListenerManagerImplWithRealFiltersTest, DuplicatePortNotAllowed) {
+  const std::string yaml1 = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1234
+filter_chains:
+- filters: []
+  )EOF";
+
+  const std::string yaml2 = R"EOF(
+name: bar
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1234
+filter_chains:
+- filters: []
+  )EOF";
+
+  EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, default_bind_type, _, 0));
+  manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml1), "", true);
+  EXPECT_THROW_WITH_MESSAGE(
+      manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml2), "", true), EnvoyException,
+      "error adding listener: 'bar' has duplicate address '127.0.0.1:1234' as existing listener");
+}
+
 TEST_F(ListenerManagerImplWithRealFiltersTest, SetListenerPerConnectionBufferLimit) {
   const std::string yaml = R"EOF(
 address: