From: Theo Buehler Subject: Re: bgpd: remove announce capabilities option To: tech@openbsd.org Date: Fri, 12 Apr 2024 13:20:13 +0200 On Thu, Apr 11, 2024 at 02:57:37PM +0200, Claudio Jeker wrote: > Running BGP without capabilities is so 90is that I think it is time to not > allow it anymore. > > When we get an optional parameter in OPEN error then stop the session. > There is no way a session will establish like this. Normally bgpd just > retries over and over again so maybe we should do the same here as well > and just handle this case like any other notification. > > Adjust session_stop to handle the shutdown reason as argument to simplify > some other code paths. Remove the no punish case in parse_open since this > actually causes more issues than it solves. > > Idle sessions need to clear a possible Timer_IdleHold when an EVNT_STOP > happens. This should prevent an idle session from reactivating after > 'bgpctl nei XYZ down'. > > Last but not least, parse_notification() is now a void function and does > the state change to IDLE in the function itself. This all makes sense to me and reads fine. While it is surely a rare config, it's still a breaking change, so I think it would be nice to make sure you mention this in the next release notes. Not sure if a current.html entry is needed. Your call. ok tb > -- > :wq Claudio > > Index: bgpctl/output_json.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v > diff -u -p -r1.42 output_json.c > --- bgpctl/output_json.c 31 Jan 2024 11:23:20 -0000 1.42 > +++ bgpctl/output_json.c 9 Apr 2024 09:46:55 -0000 > @@ -250,7 +250,6 @@ json_neighbor_full(struct peer *p) > json_do_string("role", log_policy(p->conf.role)); > > /* capabilities */ > - json_do_bool("announce_capabilities", p->conf.announce_capa); > json_neighbor_capabilities(&p->conf.capabilities); > > json_do_end(); > Index: bgpd/bgpd.conf.5 > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v > diff -u -p -r1.239 bgpd.conf.5 > --- bgpd/bgpd.conf.5 9 Apr 2024 09:03:18 -0000 1.239 > +++ bgpd/bgpd.conf.5 9 Apr 2024 12:44:25 -0000 > @@ -1064,17 +1064,6 @@ The default is > .Ic yes . > .Pp > .It Xo > -.Ic announce capabilities > -.Pq Ic yes Ns | Ns Ic no > -.Xc > -If set to > -.Ic no , > -capability negotiation is disabled during the establishment of the session. > -This can be helpful to connect to old or broken BGP implementations. > -The default is > -.Ic yes . > -.Pp > -.It Xo > .Ic announce enhanced refresh > .Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce > .Xc > Index: bgpd/bgpd.h > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v > diff -u -p -r1.491 bgpd.h > --- bgpd/bgpd.h 9 Apr 2024 12:09:19 -0000 1.491 > +++ bgpd/bgpd.h 10 Apr 2024 08:20:06 -0000 > @@ -479,7 +479,6 @@ struct peer_config { > uint8_t distance; /* 1 = direct, >1 = multihop */ > uint8_t passive; > uint8_t down; > - uint8_t announce_capa; > uint8_t reflector_client; > uint8_t ttlsec; /* TTL security hack */ > uint8_t flags; > Index: bgpd/control.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/control.c,v > diff -u -p -r1.116 control.c > --- bgpd/control.c 11 Jan 2024 15:46:25 -0000 1.116 > +++ bgpd/control.c 9 Apr 2024 10:32:01 -0000 > @@ -393,31 +393,28 @@ control_dispatch_msg(struct pollfd *pfd, > case IMSG_CTL_NEIGHBOR_DOWN: > neighbor.reason[ > sizeof(neighbor.reason) - 1] = '\0'; > - strlcpy(p->conf.reason, > - neighbor.reason, > - sizeof(p->conf.reason)); > p->conf.down = 1; > - session_stop(p, ERR_CEASE_ADMIN_DOWN); > + session_stop(p, ERR_CEASE_ADMIN_DOWN, > + neighbor.reason); > control_result(c, CTL_RES_OK); > break; > case IMSG_CTL_NEIGHBOR_CLEAR: > neighbor.reason[ > sizeof(neighbor.reason) - 1] = '\0'; > - strlcpy(p->conf.reason, > - neighbor.reason, > - sizeof(p->conf.reason)); > p->IdleHoldTime = > INTERVAL_IDLE_HOLD_INITIAL; > p->errcnt = 0; > if (!p->conf.down) { > session_stop(p, > - ERR_CEASE_ADMIN_RESET); > + ERR_CEASE_ADMIN_RESET, > + neighbor.reason); > timer_set(&p->timers, > Timer_IdleHold, > SESSION_CLEAR_DELAY); > } else { > session_stop(p, > - ERR_CEASE_ADMIN_DOWN); > + ERR_CEASE_ADMIN_DOWN, > + neighbor.reason); > } > control_result(c, CTL_RES_OK); > break; > Index: bgpd/parse.y > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v > diff -u -p -r1.460 parse.y > --- bgpd/parse.y 9 Apr 2024 12:40:01 -0000 1.460 > +++ bgpd/parse.y 10 Apr 2024 08:20:06 -0000 > @@ -246,7 +246,7 @@ typedef struct { > %token EBGP IBGP > %token FLOWSPEC PROTO FLAGS FRAGMENT TOS LENGTH ICMPTYPE CODE > %token LOCALAS REMOTEAS DESCR LOCALADDR MULTIHOP PASSIVE MAXPREFIX RESTART > -%token ANNOUNCE CAPABILITIES REFRESH AS4BYTE CONNECTRETRY ENHANCED ADDPATH > +%token ANNOUNCE REFRESH AS4BYTE CONNECTRETRY ENHANCED ADDPATH > %token SEND RECV PLUS POLICY ROLE > %token DEMOTE ENFORCE NEIGHBORAS ASOVERRIDE REFLECTOR DEPEND DOWN > %token DUMP IN OUT SOCKET RESTRICTED > @@ -1912,9 +1912,6 @@ peeropts : REMOTEAS as4number { > curpeer->conf.capabilities.mp[aid] = 1; > } > } > - | ANNOUNCE CAPABILITIES yesno { > - curpeer->conf.announce_capa = $3; > - } > | ANNOUNCE REFRESH yesnoenforce { > curpeer->conf.capabilities.refresh = $3; > } > @@ -3522,7 +3519,6 @@ lookup(char *s) > { "aspa-set", ASPASET}, > { "avs", AVS}, > { "blackhole", BLACKHOLE}, > - { "capabilities", CAPABILITIES}, > { "community", COMMUNITY}, > { "compare", COMPARE}, > { "connect-retry", CONNECTRETRY}, > @@ -4635,7 +4631,6 @@ alloc_peer(void) > p->reconf_action = RECONF_REINIT; > p->conf.distance = 1; > p->conf.export_type = EXPORT_UNSET; > - p->conf.announce_capa = 1; > p->conf.capabilities.refresh = 1; > p->conf.capabilities.grestart.restart = 1; > p->conf.capabilities.as4byte = 1; > Index: bgpd/printconf.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v > diff -u -p -r1.171 printconf.c > --- bgpd/printconf.c 9 Apr 2024 09:03:18 -0000 1.171 > +++ bgpd/printconf.c 9 Apr 2024 10:01:51 -0000 > @@ -918,9 +918,6 @@ print_announce(struct peer_config *p, co > uint8_t aid; > int match = 0; > > - if (p->announce_capa == 0) > - printf("%s\tannounce capabilities no\n", c); > - > for (aid = AID_MIN; aid < AID_MAX; aid++) > if (p->capabilities.mp[aid] == 2) { > printf("%s\tannounce %s enforce\n", c, aid2str(aid)); > Index: bgpd/session.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v > diff -u -p -r1.469 session.c > --- bgpd/session.c 10 Apr 2024 09:05:32 -0000 1.469 > +++ bgpd/session.c 11 Apr 2024 08:25:44 -0000 > @@ -66,7 +66,6 @@ int session_setup_socket(struct peer *); > void session_accept(int); > int session_connect(struct peer *); > void session_tcp_established(struct peer *); > -void session_capa_ann_none(struct peer *); > int session_capa_add(struct ibuf *, uint8_t, uint8_t); > int session_capa_add_mp(struct ibuf *, uint8_t); > int session_capa_add_afi(struct ibuf *, uint8_t, uint8_t); > @@ -87,7 +86,7 @@ int parse_header(struct peer *, u_char * > int parse_open(struct peer *); > int parse_update(struct peer *); > int parse_rrefresh(struct peer *); > -int parse_notification(struct peer *); > +void parse_notification(struct peer *); > int parse_capabilities(struct peer *, u_char *, uint16_t, uint32_t *); > int capa_neg_calc(struct peer *); > void session_dispatch_imsg(struct imsgbuf *, int, u_int *); > @@ -266,11 +265,12 @@ session_main(int debug, int verbose) > if (p->demoted) > session_demote(p, -1); > p->conf.demote_group[0] = 0; > - session_stop(p, ERR_CEASE_PEER_UNCONF); > + session_stop(p, ERR_CEASE_PEER_UNCONF, > + NULL); > timer_remove_all(&p->timers); > tcp_md5_del_listener(conf, p); > - log_peer_warnx(&p->conf, "removed"); > RB_REMOVE(peer_head, &conf->peers, p); > + log_peer_warnx(&p->conf, "removed"); > free(p); > peer_cnt--; > continue; > @@ -513,12 +513,10 @@ session_main(int debug, int verbose) > } > > RB_FOREACH_SAFE(p, peer_head, &conf->peers, next) { > - RB_REMOVE(peer_head, &conf->peers, p); > - strlcpy(p->conf.reason, > - "bgpd shutting down", > - sizeof(p->conf.reason)); > - session_stop(p, ERR_CEASE_ADMIN_DOWN); > + session_stop(p, ERR_CEASE_ADMIN_DOWN, "bgpd shutting down"); > timer_remove_all(&p->timers); > + tcp_md5_del_listener(conf, p); > + RB_REMOVE(peer_head, &conf->peers, p); > free(p); > } > > @@ -624,6 +622,9 @@ bgp_fsm(struct peer *peer, enum session_ > } > peer->passive = 0; > break; > + case EVNT_STOP: > + timer_stop(&peer->timers, Timer_IdleHold); > + break; > default: > /* ignore */ > break; > @@ -723,13 +724,7 @@ bgp_fsm(struct peer *peer, enum session_ > change_state(peer, STATE_OPENCONFIRM, event); > break; > case EVNT_RCVD_NOTIFICATION: > - if (parse_notification(peer)) { > - change_state(peer, STATE_IDLE, event); > - /* don't punish, capa negotiation */ > - timer_set(&peer->timers, Timer_IdleHold, 0); > - peer->IdleHoldTime /= 2; > - } else > - change_state(peer, STATE_IDLE, event); > + parse_notification(peer); > break; > default: > session_notification(peer, > @@ -769,7 +764,6 @@ bgp_fsm(struct peer *peer, enum session_ > break; > case EVNT_RCVD_NOTIFICATION: > parse_notification(peer); > - change_state(peer, STATE_IDLE, event); > break; > default: > session_notification(peer, > @@ -815,7 +809,6 @@ bgp_fsm(struct peer *peer, enum session_ > break; > case EVNT_RCVD_NOTIFICATION: > parse_notification(peer); > - change_state(peer, STATE_IDLE, event); > break; > default: > session_notification(peer, > @@ -937,8 +930,6 @@ change_state(struct peer *peer, enum ses > /* initialize capability negotiation structures */ > memcpy(&peer->capa.ann, &peer->conf.capabilities, > sizeof(peer->capa.ann)); > - if (!peer->conf.announce_capa) > - session_capa_ann_none(peer); > } > break; > case STATE_CONNECT: > @@ -1336,12 +1327,6 @@ session_tcp_established(struct peer *pee > &peer->if_scope); > } > > -void > -session_capa_ann_none(struct peer *peer) > -{ > - memset(&peer->capa.ann, 0, sizeof(peer->capa.ann)); > -} > - > int > session_capa_add(struct ibuf *opb, uint8_t capa_code, uint8_t capa_len) > { > @@ -2326,9 +2311,6 @@ bad_len: > session_notification(peer, ERR_OPEN, ERR_OPEN_OPT, > NULL); > change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN); > - /* no punish */ > - timer_set(&peer->timers, Timer_IdleHold, 0); > - peer->IdleHoldTime /= 2; > return (-1); > } > } > @@ -2493,7 +2475,7 @@ parse_rrefresh(struct peer *peer) > return (0); > } > > -int > +void > parse_notification(struct peer *peer) > { > struct ibuf ibuf; > @@ -2518,7 +2500,7 @@ parse_notification(struct peer *peer) > if (ibuf_get_n8(&ibuf, &errcode) == -1 || > ibuf_get_n8(&ibuf, &subcode) == -1) { > log_peer_warnx(&peer->conf, "received bad notification"); > - return (-1); > + goto done; > } > > peer->errcnt++; > @@ -2541,12 +2523,15 @@ parse_notification(struct peer *peer) > } > } > > + /* other side does not support capabilities, give up */ > if (errcode == ERR_OPEN && subcode == ERR_OPEN_OPT) { > - session_capa_ann_none(peer); > - return (1); > + change_state(peer, STATE_IDLE, EVNT_RCVD_NOTIFICATION); > + session_stop(peer, ERR_CEASE_CONN_REJECT, NULL); > + return; > } > > - return (0); > +done: > + change_state(peer, STATE_IDLE, EVNT_RCVD_NOTIFICATION); > } > > int > @@ -3165,7 +3150,8 @@ session_dispatch_imsg(struct imsgbuf *im > } else if (!depend_ok && p->depend_ok) { > p->depend_ok = depend_ok; > session_stop(p, > - ERR_CEASE_OTHER_CHANGE); > + ERR_CEASE_OTHER_CHANGE, > + NULL); > } > } > break; > @@ -3631,21 +3617,21 @@ session_demote(struct peer *p, int level > } > > void > -session_stop(struct peer *peer, uint8_t subcode) > +session_stop(struct peer *peer, uint8_t subcode, const char *reason) > { > struct ibuf *ibuf; > - char *communication; > > - communication = peer->conf.reason; > + if (reason != NULL) > + strlcpy(peer->conf.reason, reason, sizeof(peer->conf.reason)); > > ibuf = ibuf_dynamic(0, REASON_LEN); > > if ((subcode == ERR_CEASE_ADMIN_DOWN || > subcode == ERR_CEASE_ADMIN_RESET) && > - communication != NULL && *communication != '\0' && > + reason != NULL && *reason != '\0' && > ibuf != NULL) { > - if (ibuf_add_n8(ibuf, strlen(communication)) == -1 || > - ibuf_add(ibuf, communication, strlen(communication))) { > + if (ibuf_add_n8(ibuf, strlen(reason)) == -1 || > + ibuf_add(ibuf, reason, strlen(reason))) { > log_peer_warnx(&peer->conf, > "trying to send overly long shutdown reason"); > ibuf_free(ibuf); > @@ -3660,6 +3646,13 @@ session_stop(struct peer *peer, uint8_t > break; > default: > /* session not open, no need to send notification */ > + if (subcode >= sizeof(suberr_cease_names) / sizeof(char *) || > + suberr_cease_names[subcode] == NULL) > + log_peer_warnx(&peer->conf, "session stop: %s, " > + "unknown subcode %u", errnames[ERR_CEASE], subcode); > + else > + log_peer_warnx(&peer->conf, "session stop: %s, %s", > + errnames[ERR_CEASE], suberr_cease_names[subcode]); > break; > } > ibuf_free(ibuf); > Index: bgpd/session.h > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session.h,v > diff -u -p -r1.168 session.h > --- bgpd/session.h 22 Mar 2024 07:19:28 -0000 1.168 > +++ bgpd/session.h 9 Apr 2024 09:48:06 -0000 > @@ -331,7 +331,7 @@ int peer_matched(struct peer *, struct > int imsg_ctl_parent(struct imsg *); > int imsg_ctl_rde(struct imsg *); > int imsg_ctl_rde_msg(int, uint32_t, pid_t); > -void session_stop(struct peer *, uint8_t); > +void session_stop(struct peer *, uint8_t, const char *); > > /* timer.c */ > struct timer *timer_get(struct timer_head *, enum Timer); >