From 3859e4efc1597368df647323c5a3cc1771cb64ca Mon Sep 17 00:00:00 2001
From: Ondrej Zajicek <santiago@crfreenet.org>
Date: Fri, 9 Dec 2022 05:51:30 +0100
Subject: [PATCH] BGP: Improve handling of hold and keepalive timers

The effective keepalive time now scales relative to the negotiated
hold time, to maintain proportion between the keepalive time and the
hold time. This avoids issues when both keepalive and hold times
were configured, the hold time was negotiated to a smaller value,
but the keepalive time stayed the same.

Add new options 'min hold time' and 'min keepalive time', which reject
session attempts with too small hold time.

Improve validation of config options an their documentation.

Thanks to Alexander Zubkov and Sergei Goriunov for suggestions.
---
 doc/bird.sgml       | 20 +++++++++++++++++---
 proto/bgp/bgp.c     | 15 +++++++++++++++
 proto/bgp/bgp.h     |  5 ++++-
 proto/bgp/config.Y  |  6 ++++--
 proto/bgp/packets.c | 20 ++++++++++++++++++--
 5 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/doc/bird.sgml b/doc/bird.sgml
index 47848f82..8925c2cc 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -2750,9 +2750,16 @@ using the following configuration parameters:
 
 	<tag><label id="bgp-hold-time">hold time <m/number/</tag>
 	Time in seconds to wait for a Keepalive message from the other side
-	before considering the connection stale. Default: depends on agreement
-	with the neighboring router, we prefer 240 seconds if the other side is
-	willing to accept it.
+	before considering the connection stale. The effective value is
+	negotiated during session establishment and it is a minimum of this
+	configured value and the value proposed by the peer. The zero value has
+	a special meaning, signifying that no keepalives are used. Default: 240
+	seconds.
+
+	<tag><label id="bgp-min-hold-time">min hold time <m/number/</tag>
+	Minimum value of the hold time that is accepted during session negotiation.
+	If the peer proposes a lower value, the session is rejected with error.
+	Default: none.
 
 	<tag><label id="bgp-startup-hold-time">startup hold time <m/number/</tag>
 	Value of the hold timer used before the routers have a chance to exchange
@@ -2760,8 +2767,15 @@ using the following configuration parameters:
 
 	<tag><label id="bgp-keepalive-time">keepalive time <m/number/</tag>
 	Delay in seconds between sending of two consecutive Keepalive messages.
+	The effective value depends on the negotiated hold time, as it is scaled
+	to maintain proportion between the keepalive time and the hold time.
 	Default: One third of the hold time.
 
+	<tag><label id="bgp-min-keepalive-time">min keepalive time <m/number/</tag>
+	Minimum value of the keepalive time that is accepted during session
+	negotiation. If the proposed hold time would lead to a lower value of
+	the keepalive time, the session is rejected with error. Default: none.
+
 	<tag><label id="bgp-connect-delay-time">connect delay time <m/number/</tag>
 	Delay in seconds between protocol startup and the first attempt to
 	connect. Default: 5 seconds.
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index ad78d5e5..f4098e04 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -2007,6 +2007,21 @@ bgp_postconfig(struct proto_config *CF)
   if (internal && cf->enforce_first_as)
     cf_error("Enforce first AS check is requires EBGP sessions");
 
+  if (cf->keepalive_time > cf->hold_time)
+    cf_error("Keepalive time must be at most hold time");
+
+  if (cf->keepalive_time > (cf->hold_time / 2))
+    log(L_WARN "Keepalive time should be at most 1/2 of hold time");
+
+  if (cf->min_hold_time > cf->hold_time)
+    cf_error("Min hold time (%u) exceeds hold time (%u)",
+	     cf->min_hold_time, cf->hold_time);
+
+  uint keepalive_time = cf->keepalive_time ?: cf->hold_time / 3;
+  if (cf->min_keepalive_time > keepalive_time)
+    cf_error("Min keepalive time (%u) exceeds keepalive time (%u)",
+	     cf->min_keepalive_time, keepalive_time);
+
 
   struct bgp_channel_config *cc;
   BGP_CF_WALK_CHANNELS(cf, cc)
diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index eda5d229..ec802fde 100644
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -120,8 +120,11 @@ struct bgp_config {
   unsigned llgr_time;			/* Long-lived graceful restart stale time */
   unsigned connect_delay_time;		/* Minimum delay between connect attempts */
   unsigned connect_retry_time;		/* Timeout for connect attempts */
-  unsigned hold_time, initial_hold_time;
+  unsigned hold_time;
+  unsigned min_hold_time;		/* Minimum accepted hold time */
+  unsigned initial_hold_time;
   unsigned keepalive_time;
+  unsigned min_keepalive_time;		/* Minimum accepted keepalive time */
   unsigned error_amnesia_time;		/* Errors are forgotten after */
   unsigned error_delay_time_min;	/* Time to wait after an error is detected */
   unsigned error_delay_time_max;
diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index 2294119e..4e83147e 100644
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -153,7 +153,8 @@ bgp_proto:
  | bgp_proto RS CLIENT bool ';' { BGP_CFG->rs_client = $4; }
  | bgp_proto CONFEDERATION expr ';' { BGP_CFG->confederation = $3; }
  | bgp_proto CONFEDERATION MEMBER bool ';' { BGP_CFG->confederation_member = $4; }
- | bgp_proto HOLD TIME expr ';' { BGP_CFG->hold_time = $4; }
+ | bgp_proto HOLD TIME expr ';' { BGP_CFG->hold_time = $4; if (($4 && $4<3) || ($4>65535)) cf_error("Hold time must be in range 3-65535 or zero"); }
+ | bgp_proto MIN HOLD TIME expr ';' { BGP_CFG->min_hold_time = $5; }
  | bgp_proto STARTUP HOLD TIME expr ';' { BGP_CFG->initial_hold_time = $5; }
  | bgp_proto DIRECT ';' { BGP_CFG->multihop = 0; }
  | bgp_proto MULTIHOP ';' { BGP_CFG->multihop = 64; }
@@ -177,7 +178,8 @@ bgp_proto:
  | bgp_proto START DELAY TIME expr ';' { BGP_CFG->connect_delay_time = $5; log(L_WARN "%s: Start delay time option is deprecated, use connect delay time", this_proto->name); }
  | bgp_proto CONNECT DELAY TIME expr ';' { BGP_CFG->connect_delay_time = $5; }
  | bgp_proto CONNECT RETRY TIME expr ';' { BGP_CFG->connect_retry_time = $5; }
- | bgp_proto KEEPALIVE TIME expr ';' { BGP_CFG->keepalive_time = $4; }
+ | bgp_proto KEEPALIVE TIME expr ';' { BGP_CFG->keepalive_time = $4; if (($4<1) || ($4>65535)) cf_error("Keepalive time must be in range 1-65535"); }
+ | bgp_proto MIN KEEPALIVE TIME expr ';' { BGP_CFG->min_keepalive_time = $5; }
  | bgp_proto ERROR FORGET TIME expr ';' { BGP_CFG->error_amnesia_time = $5; }
  | bgp_proto ERROR WAIT TIME expr ',' expr ';' { BGP_CFG->error_delay_time_min = $5; BGP_CFG->error_delay_time_max = $7; }
  | bgp_proto DISABLE AFTER ERROR bool ';' { BGP_CFG->disable_after_error = $5; }
diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c
index c464e9c7..afcacffa 100644
--- a/proto/bgp/packets.c
+++ b/proto/bgp/packets.c
@@ -847,9 +847,25 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
   if (bgp_read_options(conn, pkt+29, pkt[28], len-29) < 0)
     return;
 
+  /* RFC 4271 4.2 - hold time must be either 0 or at least 3 */
   if (hold > 0 && hold < 3)
   { bgp_error(conn, 2, 6, pkt+22, 2); return; }
 
+  /* Compute effective hold and keepalive times */
+  uint hold_time = MIN(hold, p->cf->hold_time);
+  uint keepalive_time = p->cf->keepalive_time ?
+    (p->cf->keepalive_time * hold_time / p->cf->hold_time) :
+    hold_time / 3;
+
+  /* Keepalive time might be rounded down to zero */
+  if (hold_time && !keepalive_time)
+    keepalive_time = 1;
+
+  /* Check effective values against configured minimums */
+  if ((hold_time < p->cf->min_hold_time) ||
+      (keepalive_time < p->cf->min_keepalive_time))
+  { bgp_error(conn, 2, 6, pkt+22, 2); return; }
+
   /* RFC 6286 2.2 - router ID is nonzero and AS-wide unique */
   if (!id || (p->is_internal && id == p->local_id))
   { bgp_error(conn, 2, 3, pkt+24, -4); return; }
@@ -947,8 +963,8 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
   }
 
   /* Update our local variables */
-  conn->hold_time = MIN(hold, p->cf->hold_time);
-  conn->keepalive_time = p->cf->keepalive_time ? : conn->hold_time / 3;
+  conn->hold_time = hold_time;
+  conn->keepalive_time = keepalive_time;
   conn->as4_session = conn->local_caps->as4_support && caps->as4_support;
   conn->ext_messages = conn->local_caps->ext_messages && caps->ext_messages;
   p->remote_id = id;