Download raw body.
bgpd: remove announce capabilities option
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);
>
bgpd: remove announce capabilities option