Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
Re: bgpd: add infrastructure to support rtr sessions with tcp md5 and ipsec
To:
tech@openbsd.org
Date:
Tue, 8 Oct 2024 13:54:58 +0200

Download raw body.

Thread
On Wed, Oct 02, 2024 at 09:28:29AM -0600, Theo de Raadt wrote:
> I've got a weird vibe about this imsg request being IMSG_SOCKET_CLOSE,
> which looks too close to close().
> 
> The way things work is with IMSG_SOCKET_CONN the speaker asks the master
> (who sets up ipsec-or-md5-network configuration, then provides a socket fd,
> but closes this fd in the master itself), then the speaker moves data, and
> eventually the speaker calls [the only] close() on the fd, and tells the
> the master to deconfigure the ipsec-or-md5-network configuration.  This
> operation is being called IMSG_SOCKET_CLOSE.
> 
> Well, isn't actually a close (the imsg receiver does not have a copy of
> the fd).
> 
> I just find the names IMSG_SOCKET_CONN and IMSG_SOCKET_CLOSE a bit weird.

I replace those with IMSG_SOCKET_SETUP and IMSG_SOCKET_TEARDOWN and
similar names in bgpd.c 

I hope this is better.
-- 
:wq Claudio

Index: bgpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
diff -u -p -r1.269 bgpd.c
--- bgpd.c	1 Oct 2024 11:49:24 -0000	1.269
+++ bgpd.c	3 Oct 2024 14:03:35 -0000
@@ -53,8 +53,9 @@ int		control_setup(struct bgpd_config *)
 static void	getsockpair(int [2]);
 int		imsg_send_sockets(struct imsgbuf *, struct imsgbuf *,
 		    struct imsgbuf *);
-void		bgpd_rtr_connect(struct rtr_config *);
+void		bgpd_rtr_conn_setup(struct rtr_config *);
 void		bgpd_rtr_connect_done(int, struct bgpd_config *);
+void		bgpd_rtr_conn_teardown(uint32_t);
 
 int			 cflags;
 volatile sig_atomic_t	 mrtdump;
@@ -71,12 +72,15 @@ char			*rcname;
 
 struct connect_elm {
 	TAILQ_ENTRY(connect_elm)	entry;
+	struct auth_state		auth_state;
 	uint32_t			id;
 	int				fd;
 };
 
 TAILQ_HEAD(, connect_elm)	connect_queue = \
-				    TAILQ_HEAD_INITIALIZER(connect_queue);
+				    TAILQ_HEAD_INITIALIZER(connect_queue),
+				socket_queue = \
+				    TAILQ_HEAD_INITIALIZER(socket_queue);
 u_int				connect_cnt;
 #define MAX_CONNECT_CNT		32
 
@@ -657,7 +661,7 @@ send_config(struct bgpd_config *conf)
 		if (p->reconf_action == RECONF_REINIT)
 			if (pfkey_establish(&p->auth_state, &p->auth_conf,
 			    session_localaddr(p), &p->conf.remote_addr) == -1)
-				log_peer_warnx(&p->conf, "pfkey setup failed");
+				log_peer_warnx(&p->conf, "auth setup failed");
 	}
 
 	/* networks go via kroute to the RDE */
@@ -1053,19 +1057,27 @@ dispatch_imsg(struct imsgbuf *imsgbuf, i
 				reconfpending = 3; /* expecting 2 DONE msg */
 			}
 			break;
-		case IMSG_SOCKET_CONN:
+		case IMSG_SOCKET_SETUP:
 			if (idx != PFD_PIPE_RTR) {
 				log_warnx("connect request not from RTR");
 			} else {
+				uint32_t rtrid = imsg_get_id(&imsg);
 				SIMPLEQ_FOREACH(r, &conf->rtrs, entry) {
-					if (imsg_get_id(&imsg) == r->id)
+					if (rtrid == r->id)
 						break;
 				}
 				if (r == NULL)
-					log_warnx("unknown rtr id %d",
-					    imsg_get_id(&imsg));
+					log_warnx("unknown rtr id %d", rtrid);
 				else
-					bgpd_rtr_connect(r);
+					bgpd_rtr_conn_setup(r);
+			}
+			break;
+		case IMSG_SOCKET_TEARDOWN:
+			if (idx != PFD_PIPE_RTR) {
+				log_warnx("connect request not from RTR");
+			} else {
+				uint32_t rtrid = imsg_get_id(&imsg);
+				bgpd_rtr_conn_teardown(rtrid);
 			}
 			break;
 		case IMSG_CTL_SHOW_RTR:
@@ -1358,7 +1370,7 @@ imsg_send_sockets(struct imsgbuf *se, st
 }
 
 void
-bgpd_rtr_connect(struct rtr_config *r)
+bgpd_rtr_conn_setup(struct rtr_config *r)
 {
 	struct connect_elm *ce;
 	struct sockaddr *sa;
@@ -1377,13 +1389,16 @@ bgpd_rtr_connect(struct rtr_config *r)
 		return;
 	}
 
+	if (pfkey_establish(&ce->auth_state, &r->auth,
+	    &r->local_addr, &r->remote_addr) == -1)
+		log_warnx("rtr %s: pfkey setup failed", r->descr);
+
 	ce->id = r->id;
 	ce->fd = socket(aid2af(r->remote_addr.aid),
 	    SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, IPPROTO_TCP);
 	if (ce->fd == -1) {
 		log_warn("rtr %s", r->descr);
-		free(ce);
-		return;
+		goto fail;
 	}
 
 	switch (r->remote_addr.aid) {
@@ -1409,13 +1424,14 @@ bgpd_rtr_connect(struct rtr_config *r)
 		return;
 	}
 
+	if (tcp_md5_set(ce->fd, &r->auth, &r->remote_addr) == -1)
+		log_warn("rtr %s: setting md5sig", r->descr);
+
 	if ((sa = addr2sa(&r->local_addr, 0, &len)) != NULL) {
 		if (bind(ce->fd, sa, len) == -1) {
 			log_warn("rtr %s: bind to %s", r->descr,
 			    log_addr(&r->local_addr));
-			close(ce->fd);
-			free(ce);
-			return;
+			goto fail;
 		}
 	}
 
@@ -1424,16 +1440,20 @@ bgpd_rtr_connect(struct rtr_config *r)
 		if (errno != EINPROGRESS) {
 			log_warn("rtr %s: connect to %s:%u", r->descr,
 			    log_addr(&r->remote_addr), r->remote_port);
-			close(ce->fd);
-			free(ce);
-			return;
+			goto fail;
 		}
 		TAILQ_INSERT_TAIL(&connect_queue, ce, entry);
 		connect_cnt++;
 		return;
 	}
 
-	imsg_compose(ibuf_rtr, IMSG_SOCKET_CONN, ce->id, 0, ce->fd, NULL, 0);
+	imsg_compose(ibuf_rtr, IMSG_SOCKET_SETUP, ce->id, 0, ce->fd, NULL, 0);
+	TAILQ_INSERT_TAIL(&socket_queue, ce, entry);
+	return;
+
+ fail:
+	if (ce->fd != -1)
+		close(ce->fd);
 	free(ce);
 }
 
@@ -1477,11 +1497,26 @@ bgpd_rtr_connect_done(int fd, struct bgp
 		goto fail;
 	}
 
-	imsg_compose(ibuf_rtr, IMSG_SOCKET_CONN, ce->id, 0, ce->fd, NULL, 0);
-	free(ce);
+	imsg_compose(ibuf_rtr, IMSG_SOCKET_SETUP, ce->id, 0, ce->fd, NULL, 0);
+	TAILQ_INSERT_TAIL(&socket_queue, ce, entry);
 	return;
 
 fail:
 	close(fd);
 	free(ce);
+}
+
+void
+bgpd_rtr_conn_teardown(uint32_t id)
+{
+	struct connect_elm *ce;
+
+	TAILQ_FOREACH(ce, &socket_queue, entry) {
+		if (ce->id == id) {
+			pfkey_remove(&ce->auth_state);
+			TAILQ_REMOVE(&socket_queue, ce, entry);
+			free(ce);
+			return;
+		}
+	}
 }
Index: bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
diff -u -p -r1.497 bgpd.h
--- bgpd.h	1 Oct 2024 11:49:24 -0000	1.497
+++ bgpd.h	3 Oct 2024 14:03:06 -0000
@@ -569,6 +569,7 @@ enum rtr_error {
 struct rtr_config {
 	SIMPLEQ_ENTRY(rtr_config)	entry;
 	char				descr[PEER_DESCR_LEN];
+	struct auth_config		auth;
 	struct bgpd_addr		remote_addr;
 	struct bgpd_addr		local_addr;
 	uint32_t			id;
@@ -645,6 +646,8 @@ enum imsg_type {
 	IMSG_SOCKET_CONN,
 	IMSG_SOCKET_CONN_CTL,
 	IMSG_SOCKET_CONN_RTR,
+	IMSG_SOCKET_SETUP,
+	IMSG_SOCKET_TEARDOWN,
 	IMSG_RECONF_CONF,
 	IMSG_RECONF_RIB,
 	IMSG_RECONF_PEER,
Index: rtr.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rtr.c,v
diff -u -p -r1.23 rtr.c
--- rtr.c	10 Sep 2024 08:37:52 -0000	1.23
+++ rtr.c	8 Oct 2024 08:29:50 -0000
@@ -338,15 +338,15 @@ rtr_dispatch_imsg_parent(struct imsgbuf 
 				fatal(NULL);
 			imsg_init(ibuf_rde, fd);
 			break;
-		case IMSG_SOCKET_CONN:
+		case IMSG_SOCKET_SETUP:
 			if ((fd = imsg_get_fd(&imsg)) == -1) {
 				log_warnx("expected to receive imsg fd "
 				    "but didn't receive any");
 				break;
 			}
 			if ((rs = rtr_get(rtrid)) == NULL) {
-				log_warnx("IMSG_SOCKET_CONN: unknown rtr id %d",
-				    rtrid);
+				log_warnx("IMSG_SOCKET_SETUP: "
+				    "unknown rtr id %d", rtrid);
 				close(fd);
 				break;
 			}
Index: rtr_proto.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
diff -u -p -r1.40 rtr_proto.c
--- rtr_proto.c	10 Sep 2024 08:41:13 -0000	1.40
+++ rtr_proto.c	3 Oct 2024 13:59:26 -0000
@@ -1130,6 +1130,8 @@ rtr_fsm(struct rtr_session *rs, enum rtr
 			rs->r.wpos = 0;
 			close(rs->fd);
 			rs->fd = -1;
+			rtr_imsg_compose(IMSG_SOCKET_TEARDOWN, rs->id, 0,
+			    NULL, 0);
 		}
 		/* try to reopen session */
 		if (!rs->errored)
@@ -1158,7 +1160,7 @@ rtr_fsm(struct rtr_session *rs, enum rtr
 		case RTR_STATE_CLOSED:
 		case RTR_STATE_NEGOTIATION:
 			timer_set(&rs->timers, Timer_Rtr_Retry, rs->retry);
-			rtr_imsg_compose(IMSG_SOCKET_CONN, rs->id, 0, NULL, 0);
+			rtr_imsg_compose(IMSG_SOCKET_SETUP, rs->id, 0, NULL, 0);
 			break;
 		case RTR_STATE_ESTABLISHED:
 			if (rs->session_id == -1)