Download raw body.
bgpd: enforce presence of capabilities
On Wed, Apr 03, 2024 at 04:20:55PM +0200, Claudio Jeker wrote:
> This diff allows an operatore to enforce that certain capabilities need to
> be present on the session to be established.
> Main usage is 'announce as4byte enforce' but it also works with e.g
> 'announce inet vpn enforce'.
>
> For simple capabilities that are just yes/no options enforce is just used
> instead of yes. For complex capabilities (multi-protocol and add-path) the
> enforce is an extra optional keyword.
>
> Now the ERR_OPEN_CAPA notification is a mess since it requires to include
> data that we don't have (the encoding of the capability the neighbor
> forgot to send). We do a best effort here (again this matters for
> multi-protocol and add-path but also graceful restart).
> The code needed for that in capa_neg_calc() is not pretty maybe someone
> has a better idea.
I'm ok with this diff.
The end of capa_neg_calc() could be made a bit more compact by factoring
the 'goto fail:' path into a helper function
static int
capa_enforce_fail(struct peer *p, uint8_t code, uint8_t len, uint8_t aid)
Then you could
if (p->capa.ann.refresh == 2 && p->capa.neg.refresh == 0)
return capa_enforce_fail(p, CAPA_REFRESH, 0, 0);
etc. The downside is that you then have two magic numbers that don't
explain themselves, while your current approach documents that nicely.
Two small nits inline.
>
> Regress test for this is ready but not included.
> --
> :wq Claudio
>
> Index: bgpd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
> diff -u -p -r1.238 bgpd.conf.5
> --- bgpd.conf.5 18 Mar 2024 10:16:50 -0000 1.238
> +++ bgpd.conf.5 26 Mar 2024 13:25:44 -0000
> @@ -956,6 +956,7 @@ The neighbor properties are as follows:
> .Ic announce
> .Pq Ic IPv4 Ns | Ns Ic IPv6
> .Pq Ic none Ns | Ns Ic unicast Ns | Ns Ic vpn Ns | Ns Ic flowspec
> +.Op Ic enforce
> .Xc
> For the given address family, control which
> .Em subsequent address families
> @@ -979,7 +980,7 @@ for the same address family of the sessi
> .Pp
> .It Xo
> .Ic announce add-path recv
> -.Pq Ic yes Ns | Ns Ic no
> +.Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce
> .Xc
> If set to
> .Ic yes ,
> @@ -991,12 +992,14 @@ The default is
> .It Xo
> .Ic announce add-path send
> .Pq Ic no Ns | Ns Ic all
> +.Op Ic enforce
> .Xc
> .It Xo
> .Ic announce add-path send
> -.Pq Ic best Ns | Ns Ic ecmp | Ns Ic as-wide-best
> +.Pq Ic best Ns | Ns Ic ecmp Ns | Ns Ic as-wide-best
> .Op Ic plus Ar num
> .Op Ic max Ar num
> +.Op Ic enforce
> .Xc
> If set to
> .Ic all ,
> @@ -1047,12 +1050,16 @@ is ignored.
> .Pp
> .It Xo
> .Ic announce as-4byte
> -.Pq Ic yes Ns | Ns Ic no
> +.Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce
> .Xc
> If set to
> .Ic no ,
> the 4-byte AS capability is not announced and so native 4-byte AS support is
> disabled.
> +If
> +.Ic enforce
> +is set the session will only establish if the neighbor also announces
This needs a comma after "set" (there's a few more of these below and the
manual). I'd probably write this as follows:
If
.Ic enforce
is set, the session will only be established if the neighbor also announces
the capability.
While I'm sure about the comma after the conditional clause, I'm less sure
whether the active voice of 'establish' is incorrect. It reads odd to me.
> +the capability.
> The default is
> .Ic yes .
> .Pp
> @@ -1069,11 +1076,15 @@ The default is
> .Pp
> .It Xo
> .Ic announce enhanced refresh
> -.Pq Ic yes Ns | Ns Ic no
> +.Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce
> .Xc
> If set to
> .Ic yes ,
> the enhanced route refresh capability is announced.
> +If
> +.Ic enforce
> +is set the session will only establish if the neighbor also announces
> +the capability.
> The default is
> .Ic no .
> .Pp
> @@ -1089,23 +1100,27 @@ the session will be closed.
> If
> .Ic enforce
> is set the session will only establish if the neighbor also announces
> -the open policy capability.
> +the capability.
> The default is
> .Ic no .
> .Pp
> .It Xo
> .Ic announce refresh
> -.Pq Ic yes Ns | Ns Ic no
> +.Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce
> .Xc
> If set to
> .Ic no ,
> the route refresh capability is not announced.
> +If
> +.Ic enforce
> +is set the session will only establish if the neighbor also announces
> +the capability.
> The default is
> .Ic yes .
> .Pp
> .It Xo
> .Ic announce restart
> -.Pq Ic yes Ns | Ns Ic no
> +.Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce
> .Xc
> If set to
> .Ic no ,
> @@ -1113,6 +1128,10 @@ the graceful restart capability is not a
> Currently only the End-of-RIB marker is supported and announced by the
> .Ic restart
> capability.
> +If
> +.Ic enforce
> +is set the session will only establish if the neighbor also announces
> +the capability.
> The default is
> .Ic yes .
> .Pp
> Index: bgpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> diff -u -p -r1.489 bgpd.h
> --- bgpd.h 22 Mar 2024 15:41:34 -0000 1.489
> +++ bgpd.h 23 Mar 2024 07:26:26 -0000
> @@ -432,6 +432,9 @@ enum capa_codes {
> #define CAPA_AP_RECV 0x01
> #define CAPA_AP_SEND 0x02
> #define CAPA_AP_BIDIR 0x03
> +#define CAPA_AP_MASK 0x0f
> +#define CAPA_AP_RECV_ENFORCE 0x10 /* internal only */
> +#define CAPA_AP_SEND_ENFORCE 0x20 /* internal only */
>
> /* values for RFC 9234 - BGP Open Policy */
> #define CAPA_ROLE_PROVIDER 0x00
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> diff -u -p -r1.458 parse.y
> --- parse.y 3 Apr 2024 08:57:26 -0000 1.458
> +++ parse.y 3 Apr 2024 13:34:10 -0000
> @@ -273,6 +273,7 @@ typedef struct {
> %type <v.number> asnumber as4number as4number_any optnumber
> %type <v.number> espah af safi restart origincode nettype
> %type <v.number> yesno inout restricted expires enforce
> +%type <v.number> yesnoenforce enforce
> %type <v.number> validity aspa_validity
> %type <v.number> addpathextra addpathmax
> %type <v.number> port proto_item tos length flag icmptype
> @@ -1889,7 +1890,7 @@ peeropts : REMOTEAS as4number {
> }
> curpeer->conf.min_holdtime = $3;
> }
> - | ANNOUNCE af safi {
> + | ANNOUNCE af safi enforce {
> uint8_t aid, safi;
> uint16_t afi;
>
> @@ -1905,42 +1906,48 @@ peeropts : REMOTEAS as4number {
> yyerror("unknown AFI/SAFI pair");
> YYERROR;
> }
> - curpeer->conf.capabilities.mp[aid] = 1;
> + if ($4)
> + curpeer->conf.capabilities.mp[aid] = 2;
> + else
> + curpeer->conf.capabilities.mp[aid] = 1;
> }
> }
> | ANNOUNCE CAPABILITIES yesno {
> curpeer->conf.announce_capa = $3;
> }
> - | ANNOUNCE REFRESH yesno {
> + | ANNOUNCE REFRESH yesnoenforce {
> curpeer->conf.capabilities.refresh = $3;
> }
> - | ANNOUNCE ENHANCED REFRESH yesno {
> + | ANNOUNCE ENHANCED REFRESH yesnoenforce {
> curpeer->conf.capabilities.enhanced_rr = $4;
> }
> - | ANNOUNCE RESTART yesno {
> + | ANNOUNCE RESTART yesnoenforce {
> curpeer->conf.capabilities.grestart.restart = $3;
> }
> - | ANNOUNCE AS4BYTE yesno {
> + | ANNOUNCE AS4BYTE yesnoenforce {
> curpeer->conf.capabilities.as4byte = $3;
> }
> - | ANNOUNCE ADDPATH RECV yesno {
> + | ANNOUNCE ADDPATH RECV yesnoenforce {
> int8_t *ap = curpeer->conf.capabilities.add_path;
> uint8_t i;
>
> - for (i = AID_MIN; i < AID_MAX; i++)
> - if ($4)
> + for (i = AID_MIN; i < AID_MAX; i++) {
> + if ($4) {
> + if ($4 == 2)
> + ap[i] |= CAPA_AP_RECV_ENFORCE;
> ap[i] |= CAPA_AP_RECV;
> - else
> + } else
> ap[i] &= ~CAPA_AP_RECV;
> + }
> }
> - | ANNOUNCE ADDPATH SEND STRING addpathextra addpathmax {
> + | ANNOUNCE ADDPATH SEND STRING addpathextra addpathmax enforce {
> int8_t *ap = curpeer->conf.capabilities.add_path;
> enum addpath_mode mode;
> u_int8_t i;
>
> if (!strcmp($4, "no")) {
> free($4);
> - if ($5 != 0 || $6 != 0) {
> + if ($5 != 0 || $6 != 0 || $7 != 0) {
> yyerror("no additional option allowed "
> "for 'add-path send no'");
> YYERROR;
> @@ -1970,16 +1977,18 @@ peeropts : REMOTEAS as4number {
> YYERROR;
> }
> for (i = AID_MIN; i < AID_MAX; i++) {
> - if (mode != ADDPATH_EVAL_NONE)
> + if (mode != ADDPATH_EVAL_NONE) {
> + if ($7)
> + ap[i] |= CAPA_AP_SEND_ENFORCE;
> ap[i] |= CAPA_AP_SEND;
> - else
> + } else
> ap[i] &= ~CAPA_AP_SEND;
> }
> curpeer->conf.eval.mode = mode;
> curpeer->conf.eval.extrapaths = $5;
> curpeer->conf.eval.maxpaths = $6;
> }
> - | ANNOUNCE POLICY enforce {
> + | ANNOUNCE POLICY yesnoenforce {
> curpeer->conf.capabilities.policy = $3;
> }
> | ROLE STRING {
> @@ -3083,7 +3092,11 @@ delete : /* empty */ { $$ = 0; }
> | DELETE { $$ = 1; }
> ;
>
> -enforce : yesno { $$ = $1; }
> +enforce : /* empty */ { $$ = 0; }
> + | ENFORCE { $$ = 2; }
> + ;
> +
> +yesnoenforce : yesno { $$ = $1; }
> | ENFORCE { $$ = 2; }
> ;
>
> Index: printconf.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v
> diff -u -p -r1.170 printconf.c
> --- printconf.c 20 Mar 2024 09:35:46 -0000 1.170
> +++ printconf.c 26 Mar 2024 13:35:43 -0000
> @@ -916,37 +916,67 @@ void
> print_announce(struct peer_config *p, const char *c)
> {
> 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])
> + if (p->capabilities.mp[aid] == 2) {
> + printf("%s\tannounce %s enforce\n", c, aid2str(aid));
> + match = 1;
> + } else if (p->capabilities.mp[aid]) {
> printf("%s\tannounce %s\n", c, aid2str(aid));
> + match = 1;
> + }
> + if (!match) {
> + printf("%s\tannounce IPv4 none\n", c);
> + printf("%s\tannounce IPv6 none\n", c);
> + }
>
> - if (p->capabilities.refresh == 0)
> + if (p->capabilities.refresh == 2)
> + printf("%s\tannounce refresh enforce\n", c);
> + else if (p->capabilities.refresh == 0)
> printf("%s\tannounce refresh no\n", c);
> - if (p->capabilities.enhanced_rr == 1)
> +
> + if (p->capabilities.enhanced_rr == 2)
> + printf("%s\tannounce enhanced refresh enforce\n", c);
> + else if (p->capabilities.enhanced_rr == 1)
> printf("%s\tannounce enhanced refresh yes\n", c);
> - if (p->capabilities.grestart.restart == 0)
> +
> + if (p->capabilities.grestart.restart == 2)
> + printf("%s\tannounce restart enforce\n", c);
> + else if (p->capabilities.grestart.restart == 0)
> printf("%s\tannounce restart no\n", c);
> - if (p->capabilities.as4byte == 0)
> +
> + if (p->capabilities.as4byte == 2)
> + printf("%s\tannounce as4byte enforce\n", c);
> + else if (p->capabilities.as4byte == 0)
> printf("%s\tannounce as4byte no\n", c);
> - if (p->capabilities.add_path[0] & CAPA_AP_RECV)
> +
> + if (p->capabilities.add_path[AID_MIN] & CAPA_AP_RECV_ENFORCE)
> + printf("%s\tannounce add-path recv enforce\n", c);
> + else if (p->capabilities.add_path[AID_MIN] & CAPA_AP_RECV)
> printf("%s\tannounce add-path recv yes\n", c);
> - if (p->capabilities.add_path[0] & CAPA_AP_SEND) {
> +
> + if (p->capabilities.add_path[AID_MIN] & CAPA_AP_SEND) {
> printf("%s\tannounce add-path send %s", c,
> print_addpath_mode(p->eval.mode));
> if (p->eval.extrapaths != 0)
> printf(" plus %d", p->eval.extrapaths);
> if (p->eval.maxpaths != 0)
> printf(" max %d", p->eval.maxpaths);
> + if (p->capabilities.add_path[AID_MIN] & CAPA_AP_SEND_ENFORCE)
> + printf(" enforce");
> printf("\n");
> }
> - if (p->capabilities.policy) {
> - printf("%s\tannounce policy %s\n", c,
> - p->capabilities.policy == 2 ? "enforce" : "yes");
> - }
> +
> + if (p->capabilities.policy == 2)
> + printf("%s\tannounce policy enforce\n", c);
> + else if (p->capabilities.policy == 1)
> + printf("%s\tannounce policy yes\n", c);
> + else
> + printf("%s\tannounce policy no\n", c);
> }
>
> void
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> diff -u -p -r1.467 session.c
> --- session.c 26 Mar 2024 12:45:29 -0000 1.467
> +++ session.c 3 Apr 2024 14:09:41 -0000
> @@ -89,7 +89,7 @@ int parse_update(struct peer *);
> int parse_rrefresh(struct peer *);
> int parse_notification(struct peer *);
> int parse_capabilities(struct peer *, u_char *, uint16_t, uint32_t *);
> -int capa_neg_calc(struct peer *, uint8_t *);
> +int capa_neg_calc(struct peer *);
> void session_dispatch_imsg(struct imsgbuf *, int, u_int *);
> void session_up(struct peer *);
> void session_down(struct peer *);
> @@ -1562,12 +1562,13 @@ session_open(struct peer *p)
> for (i = AID_MIN; i < AID_MAX; i++) {
> if (p->capa.ann.mp[i]) {
> errs += session_capa_add_afi(opb,
> - i, p->capa.ann.add_path[i]);
> + i, p->capa.ann.add_path[i] &
> + CAPA_AP_MASK);
> }
> }
> } else { /* AID_INET */
> errs += session_capa_add_afi(opb, AID_INET,
> - p->capa.ann.add_path[AID_INET]);
> + p->capa.ann.add_path[AID_INET] & CAPA_AP_MASK);
> }
> }
>
> @@ -2167,7 +2168,7 @@ parse_open(struct peer *peer)
> uint16_t holdtime, oholdtime, myholdtime;
> uint32_t as, bgpid;
> uint16_t optparamlen, extlen, plen, op_len;
> - uint8_t op_type, suberr = 0;
> + uint8_t op_type;
>
> p = peer->rbuf->rptr;
> p += MSGSIZE_HEADER_MARKER;
> @@ -2350,8 +2351,7 @@ bad_len:
> return (-1);
> }
>
> - if (capa_neg_calc(peer, &suberr) == -1) {
> - session_notification(peer, ERR_OPEN, suberr, NULL);
> + if (capa_neg_calc(peer) == -1) {
> change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
> return (-1);
> }
> @@ -2735,9 +2735,10 @@ parse_capabilities(struct peer *peer, u_
> }
>
> int
> -capa_neg_calc(struct peer *p, uint8_t *suberr)
> +capa_neg_calc(struct peer *p)
> {
> - uint8_t i, hasmp = 0;
> + struct ibuf *ebuf;
> + uint8_t i, hasmp = 0, capa_code, capa_len, capa_aid = 0;
>
> /* a capability is accepted only if both sides announced it */
>
> @@ -2811,6 +2812,8 @@ capa_neg_calc(struct peer *p, uint8_t *s
> */
> memset(p->capa.neg.add_path, 0, sizeof(p->capa.neg.add_path));
> for (i = AID_MIN; i < AID_MAX; i++) {
> + if (p->capa.neg.mp[i] == 0)
> + continue;
> if ((p->capa.ann.add_path[i] & CAPA_AP_RECV) &&
> (p->capa.peer.add_path[i] & CAPA_AP_SEND)) {
> p->capa.neg.add_path[i] |= CAPA_AP_RECV;
> @@ -2836,43 +2839,113 @@ capa_neg_calc(struct peer *p, uint8_t *s
> switch (p->conf.role) {
> case ROLE_PROVIDER:
> if (p->remote_role != ROLE_CUSTOMER)
> - goto fail;
> + goto policyfail;
> break;
> case ROLE_RS:
> if (p->remote_role != ROLE_RS_CLIENT)
> - goto fail;
> + goto policyfail;
> break;
> case ROLE_RS_CLIENT:
> if (p->remote_role != ROLE_RS)
> - goto fail;
> + goto policyfail;
> break;
> case ROLE_CUSTOMER:
> if (p->remote_role != ROLE_PROVIDER)
> - goto fail;
> + goto policyfail;
> break;
> case ROLE_PEER:
> if (p->remote_role != ROLE_PEER)
> - goto fail;
> + goto policyfail;
break;
> default:
> - fail:
> + policyfail:
> log_peer_warnx(&p->conf, "open policy role mismatch: "
> "our role %s, their role %s",
> log_policy(p->conf.role),
> log_policy(p->remote_role));
> - *suberr = ERR_OPEN_ROLE;
> + session_notification(p, ERR_OPEN, ERR_OPEN_ROLE,
> + NULL);
could unwrap the previous line
> return (-1);
> }
> p->capa.neg.policy = 1;
> - } else if (p->capa.ann.policy == 2 && p->conf.ebgp) {
> - /* enforce presence of open policy role capability */
> + }
> +
> + /* enforce presence of open policy role capability */
> + if (p->capa.ann.policy == 2 && p->capa.peer.policy == 0 &&
> + p->conf.ebgp) {
> log_peer_warnx(&p->conf, "open policy role enforced but "
> "not present");
> - *suberr = ERR_OPEN_ROLE;
> + session_notification(p, ERR_OPEN, ERR_OPEN_ROLE, NULL);
> return (-1);
> }
>
> + /* enforce presence of other capabilities */
> + if (p->capa.ann.refresh == 2 && p->capa.neg.refresh == 0) {
> + capa_code = CAPA_REFRESH;
> + capa_len = 0;
> + goto fail;
> + }
> + if (p->capa.ann.enhanced_rr == 2 && p->capa.neg.enhanced_rr == 0) {
> + capa_code = CAPA_ENHANCED_RR;
> + capa_len = 0;
> + goto fail;
> + }
> + if (p->capa.ann.as4byte == 2 && p->capa.neg.as4byte == 0) {
> + capa_code = CAPA_AS4BYTE;
> + capa_len = 4;
> + goto fail;
> + }
> + if (p->capa.ann.grestart.restart == 2 &&
> + p->capa.neg.grestart.restart == 0) {
> + capa_code = CAPA_RESTART;
> + capa_len = 2;
> + goto fail;
> + }
> + for (i = AID_MIN; i < AID_MAX; i++) {
> + if (p->capa.ann.mp[i] == 2 && p->capa.neg.mp[i] == 0) {
> + capa_code = CAPA_MP;
> + capa_len = 4;
> + capa_aid = i;
> + goto fail;
> + }
> + }
> +
> + for (i = AID_MIN; i < AID_MAX; i++) {
> + if (p->capa.neg.mp[i] == 0)
> + continue;
> + if ((p->capa.ann.add_path[i] & CAPA_AP_RECV_ENFORCE) &&
> + (p->capa.neg.add_path[i] & CAPA_AP_RECV) == 0) {
> + capa_code = CAPA_ADD_PATH;
> + capa_len = 4;
> + capa_aid = i;
> + goto fail;
> + }
> + if ((p->capa.ann.add_path[i] & CAPA_AP_SEND_ENFORCE) &&
> + (p->capa.neg.add_path[i] & CAPA_AP_SEND) == 0) {
> + capa_code = CAPA_ADD_PATH;
> + capa_len = 4;
> + capa_aid = i;
> + goto fail;
> + }
> + }
> +
> return (0);
> +
> + fail:
> + if ((ebuf = ibuf_dynamic(2, 256)) == NULL)
> + return (-1);
> + /* best effort, no problem if it fails */
> + session_capa_add(ebuf, capa_code, capa_len);
> + if (capa_code == CAPA_MP)
> + session_capa_add_mp(ebuf, capa_aid);
> + else if (capa_code == CAPA_ADD_PATH)
> + session_capa_add_afi(ebuf, capa_aid, 0);
> + else if (capa_len > 0)
> + ibuf_add_zero(ebuf, capa_len);
> +
> + session_notification(p, ERR_OPEN, ERR_OPEN_CAPA, ebuf);
> + ibuf_free(ebuf);
> + return (-1);
> }
>
> void
>
bgpd: enforce presence of capabilities