From: Claudio Jeker 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 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)