From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: bgpd: deconfigure md5 key after config was fully reloaded
To: tech@openbsd.org
Date: Wed, 4 Sep 2024 14:42:57 +0200

Right now we call pfkey_remove() in merge_peers() in the parent process
before sending the config over to the session engine. The result is that
the NOTIFICATION sent by the session engine is no longer md5 signed.

Instead delay deallocation of peers in the config until the session engine
sent the IMSG_RECONF_DONE message. By that time old sessions have been
shutdown and any pending notification should have made it out.

See also https://github.com/openbgpd-portable/openbgpd-portable/issues/82
-- 
:wq Claudio

Index: bgpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
diff -u -p -r1.265 bgpd.c
--- bgpd.c	12 Aug 2024 09:04:23 -0000	1.265
+++ bgpd.c	4 Sep 2024 12:19:05 -0000
@@ -575,13 +575,11 @@ reconfigure(char *conffile, struct bgpd_
 
 	merge_config(conf, new_conf);
 
-	if (prepare_listeners(conf) == -1) {
+	if (prepare_listeners(conf) == -1)
 		return (1);
-	}
 
-	if (control_setup(conf) == -1) {
+	if (control_setup(conf) == -1)
 		return (1);
-	}
 
 	return send_config(conf);
 }
@@ -647,6 +645,9 @@ send_config(struct bgpd_config *conf)
 
 	/* send peer list to the SE */
 	RB_FOREACH(p, peer_head, &conf->peers) {
+		if (p->reconf_action == RECONF_DELETE)
+			continue;
+
 		if (imsg_compose(ibuf_se, IMSG_RECONF_PEER, p->conf.id, 0, -1,
 		    &p->conf, sizeof(p->conf)) == -1)
 			return (-1);
@@ -1025,6 +1026,9 @@ dispatch_imsg(struct imsgbuf *imsgbuf, i
 
 				/* redistribute list needs to be reloaded too */
 				kr_reload();
+
+				/* also remove old peers */
+				free_deleted_peers(conf);
 			}
 			reconfpending--;
 			break;
Index: config.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
diff -u -p -r1.110 config.c
--- config.c	14 Aug 2024 19:09:51 -0000	1.110
+++ config.c	4 Sep 2024 12:19:05 -0000
@@ -434,8 +434,7 @@ merge_config(struct bgpd_config *xconf, 
 	 * merge peers:
 	 * - need to know which peers are new, replaced and removed
 	 * - walk over old peers and check if there is a corresponding new
-	 *   peer if so mark it RECONF_KEEP. Remove all old peers.
-	 * - swap lists (old peer list is actually empty).
+	 *   peer if so mark it RECONF_KEEP. Mark all old peers RECONF_DELETE.
 	 */
 	RB_FOREACH_SAFE(p, peer_head, &xconf->peers, nextp) {
 		np = getpeerbyid(conf, p->conf.id);
@@ -443,13 +442,12 @@ merge_config(struct bgpd_config *xconf, 
 			np->reconf_action = RECONF_KEEP;
 			/* copy the auth state since parent uses it */
 			np->auth = p->auth;
+
+			RB_REMOVE(peer_head, &xconf->peers, p);
+			free(p);
 		} else {
-			/* peer no longer exists, clear pfkey state */
-			pfkey_remove(p);
+			p->reconf_action = RECONF_DELETE;
 		}
-
-		RB_REMOVE(peer_head, &xconf->peers, p);
-		free(p);
 	}
 	RB_FOREACH_SAFE(np, peer_head, &conf->peers, nextp) {
 		RB_REMOVE(peer_head, &conf->peers, np);
@@ -459,6 +457,21 @@ merge_config(struct bgpd_config *xconf, 
 
 	/* conf is merged so free it */
 	free_config(conf);
+}
+
+void
+free_deleted_peers(struct bgpd_config *conf)
+{
+	struct peer *p, *nextp;
+
+	RB_FOREACH_SAFE(p, peer_head, &conf->peers, nextp) {
+		if (p->reconf_action == RECONF_DELETE) {
+			/* peer no longer exists, clear pfkey state */
+			pfkey_remove(p);
+			RB_REMOVE(peer_head, &conf->peers, p);
+			free(p);
+		}
+	}
 }
 
 uint32_t
Index: session.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
diff -u -p -r1.172 session.h
--- session.h	20 Aug 2024 11:59:39 -0000	1.172
+++ session.h	4 Sep 2024 12:19:05 -0000
@@ -247,6 +247,7 @@ int	 carp_demote_set(char *, int);
 
 /* config.c */
 void	 merge_config(struct bgpd_config *, struct bgpd_config *);
+void	 free_deleted_peers(struct bgpd_config *);
 int	 prepare_listeners(struct bgpd_config *);
 
 /* control.c */