Merge pull request #18006 from FRRouting/mergify/bp/dev/10.3/pr-17959

bgpd: Do not start BGP session if BGP identifier is not set (backport #17959)

9 files changed, 65 insertions, 4 deletions

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index 1a30cb37f4..c7b7f9e284 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -607,6 +607,7 @@ const char *const peer_down_str[] = {
 	"Admin. shutdown (RTT)",
 	"Suppress Fib Turned On or Off",
 	"Password config change",
+	"Router ID is missing",
 };
 
 static void bgp_graceful_restart_timer_off(struct peer_connection *connection,
diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c
index af5d815d30..3df4aa286e 100644
--- a/bgpd/bgp_network.c
+++ b/bgpd/bgp_network.c
@@ -571,7 +571,7 @@ static void bgp_accept(struct event *thread)
 
 	/* Do not try to reconnect if the peer reached maximum
 	 * prefixes, restart timer is still running or the peer
-	 * is shutdown.
+	 * is shutdown, or BGP identifier is not set (0.0.0.0).
 	 */
 	if (BGP_PEER_START_SUPPRESSED(peer1)) {
 		if (bgp_debug_neighbor_events(peer1)) {
@@ -588,6 +588,14 @@ static void bgp_accept(struct event *thread)
 		return;
 	}
 
+	if (peer1->bgp->router_id.s_addr == INADDR_ANY) {
+		zlog_warn("[Event] Incoming BGP connection rejected from %s due missing BGP identifier, set it with `bgp router-id`",
+			  peer1->host);
+		peer1->last_reset = PEER_DOWN_ROUTER_ID_ZERO;
+		close(bgp_sock);
+		return;
+	}
+
 	if (bgp_debug_neighbor_events(peer1))
 		zlog_debug("[Event] connection from %s fd %d, active peer status %d fd %d",
 			   inet_sutop(&su, buf), bgp_sock, connection1->status,
@@ -776,6 +784,13 @@ enum connect_result bgp_connect(struct peer_connection *connection)
 	assert(!CHECK_FLAG(connection->thread_flags, PEER_THREAD_READS_ON));
 	ifindex_t ifindex = 0;
 
+	if (peer->bgp->router_id.s_addr == INADDR_ANY) {
+		peer->last_reset = PEER_DOWN_ROUTER_ID_ZERO;
+		zlog_warn("%s: BGP identifier is missing for peer %s, set it with `bgp router-id`",
+			  __func__, peer->host);
+		return connect_error;
+	}
+
 	if (peer->conf_if && BGP_CONNECTION_SU_UNSPEC(connection)) {
 		if (bgp_debug_neighbor_events(peer))
 			zlog_debug("Peer address not learnt: Returning from connect");
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 96a78e6662..ee904391c1 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -1863,6 +1863,7 @@ struct peer {
 #define PEER_DOWN_RTT_SHUTDOWN          35U /* Automatically shutdown due to RTT */
 #define PEER_DOWN_SUPPRESS_FIB_PENDING	 36U /* Suppress fib pending changed */
 #define PEER_DOWN_PASSWORD_CHANGE	 37U /* neighbor password command */
+#define PEER_DOWN_ROUTER_ID_ZERO	 38U /* router-id is 0.0.0.0 */
 	/*
 	 * Remember to update peer_down_str in bgp_fsm.c when you add
 	 * a new value to the last_reset reason
diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst
index 1493c2fb98..5add30b6f4 100644
--- a/doc/user/bgp.rst
+++ b/doc/user/bgp.rst
@@ -282,7 +282,9 @@ internal or external.
    interface and address information. In that case default router ID value is
    selected as the largest IP Address of the interfaces. When `router zebra` is
    not enabled *bgpd* can't get interface information so `router-id` is set to
-   0.0.0.0. So please set router-id by hand.
+   0.0.0.0, which is invalid and BGP session can't be established.
+
+   So please set router-id by manually.
 
 
 .. _bgp-multiple-autonomous-systems:
@@ -2938,7 +2940,7 @@ BGP Extended Communities in Route Map
 
 ``CO:COLOR``
    This is a format to define colors value. ``CO`` part is always 00 (default),
-   it can be used to support the requirements of Color-Only steering when using 
+   it can be used to support the requirements of Color-Only steering when using
    a Null Endpoint in the SR-TE Policy as specified in Section 8.8 of [RFC9256].
    The below shows in detail what the different combinations of ``CO`` bits can
    match on to for the purpose of determining what type of SR-TE Policy Tunnel
diff --git a/tests/topotests/bgp_ipv6_ll_peering/r1/bgpd.conf b/tests/topotests/bgp_ipv6_ll_peering/r1/bgpd.conf
index 724cbf84ab..a26efb4c4f 100644
--- a/tests/topotests/bgp_ipv6_ll_peering/r1/bgpd.conf
+++ b/tests/topotests/bgp_ipv6_ll_peering/r1/bgpd.conf
@@ -4,3 +4,6 @@ router bgp 65001
  neighbor fe80:1::2 remote-as external
  neighbor fe80:1::2 timers 3 10
  neighbor fe80:1::2 interface r1-eth0
+ neighbor fe80:1::3 remote-as external
+ neighbor fe80:1::3 timers 3 10
+ neighbor fe80:1::3 interface r1-eth1
diff --git a/tests/topotests/bgp_ipv6_ll_peering/r1/zebra.conf b/tests/topotests/bgp_ipv6_ll_peering/r1/zebra.conf
index 4e93d4f4e5..f1bbff2e44 100644
--- a/tests/topotests/bgp_ipv6_ll_peering/r1/zebra.conf
+++ b/tests/topotests/bgp_ipv6_ll_peering/r1/zebra.conf
@@ -2,3 +2,6 @@
 interface r1-eth0
  ipv6 address fe80:1::1/64
 !
+interface r1-eth1
+ ipv6 address fe80:1::1/64
+!
diff --git a/tests/topotests/bgp_ipv6_ll_peering/r3/bgpd.conf b/tests/topotests/bgp_ipv6_ll_peering/r3/bgpd.conf
new file mode 100644
index 0000000000..f1684880b3
--- /dev/null
+++ b/tests/topotests/bgp_ipv6_ll_peering/r3/bgpd.conf
@@ -0,0 +1,5 @@
+router bgp 65003
+ no bgp ebgp-requires-policy
+ neighbor fe80:1::1 remote-as external
+ neighbor fe80:1::1 timers 3 10
+ neighbor fe80:1::1 interface r3-eth0
diff --git a/tests/topotests/bgp_ipv6_ll_peering/r3/zebra.conf b/tests/topotests/bgp_ipv6_ll_peering/r3/zebra.conf
new file mode 100644
index 0000000000..71053cd2c3
--- /dev/null
+++ b/tests/topotests/bgp_ipv6_ll_peering/r3/zebra.conf
@@ -0,0 +1,4 @@
+!
+interface r3-eth0
+ ipv6 address fe80:1::3/64
+!
diff --git a/tests/topotests/bgp_ipv6_ll_peering/test_bgp_ipv6_ll_peering.py b/tests/topotests/bgp_ipv6_ll_peering/test_bgp_ipv6_ll_peering.py
index aaa68ea340..fbd4097605 100644
--- a/tests/topotests/bgp_ipv6_ll_peering/test_bgp_ipv6_ll_peering.py
+++ b/tests/topotests/bgp_ipv6_ll_peering/test_bgp_ipv6_ll_peering.py
@@ -27,13 +27,17 @@ pytestmark = [pytest.mark.bgpd]
 
 
 def build_topo(tgen):
-    for routern in range(1, 3):
+    for routern in range(1, 4):
         tgen.add_router("r{}".format(routern))
 
     switch = tgen.add_switch("s1")
     switch.add_link(tgen.gears["r1"])
     switch.add_link(tgen.gears["r2"])
 
+    switch = tgen.add_switch("s2")
+    switch.add_link(tgen.gears["r1"])
+    switch.add_link(tgen.gears["r3"])
+
 
 def setup_module(mod):
     tgen = Topogen(build_topo, mod.__name__)
@@ -64,6 +68,7 @@ def test_bgp_ipv6_link_local_peering():
         pytest.skip(tgen.errors)
 
     r1 = tgen.gears["r1"]
+    r3 = tgen.gears["r3"]
 
     def _bgp_converge():
         output = json.loads(r1.vtysh_cmd("show bgp summary json"))
@@ -82,6 +87,28 @@ def test_bgp_ipv6_link_local_peering():
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
     assert result is None, "Failed to see BGP convergence on R2"
 
+    def _bgp_router_id_missing():
+        output = json.loads(r3.vtysh_cmd("show bgp summary failed json"))
+        expected = {
+            "ipv4Unicast": {
+                "routerId": "0.0.0.0",
+                "as": 65003,
+                "peers": {
+                    "fe80:1::1": {
+                        "connectionsEstablished": 0,
+                        "connectionsDropped": 0,
+                        "peerUptime": "never",
+                        "lastResetDueTo": "Router ID is missing",
+                    }
+                },
+            }
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(_bgp_router_id_missing)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, "r3 should stay down due to missing router ID"
+
 
 if __name__ == "__main__":
     args = ["-s"] + sys.argv[1:]