On Tue, Dec 27, 2022 at 05:44:39PM +0100, Claudio Jeker wrote:
> The role capability only works on ebgp sessions. It makes no sense on
> ibgp sessions and the RFC 9234 does not define any behaviour for that.
> I decided to:
> - Exclude the role capability for ibgp sessions when sending an OPEN
> - Warn when a role capability is received on an iBGP session
> - Make sure the capability negotiation is skipped for ibgp sessions,
> this disables the role capability on the session.
>
> I kept the peer capability intact so that it shows in bgpctl show nei
> output.
>
> This diff also includes the necessary bit in the notification handling for
> capabilities (I forgot to add CAPA_ROLE there in the initial diff).
ok (two tiny nits below)
> --
> :wq Claudio
>
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.436
> diff -u -p -r1.436 session.c
> --- session.c 18 Oct 2022 12:24:51 -0000 1.436
> +++ session.c 27 Dec 2022 16:41:29 -0000
> @@ -1441,8 +1441,8 @@ session_open(struct peer *p)
> if (p->capa.ann.refresh) /* no data */
> errs += session_capa_add(opb, CAPA_REFRESH, 0);
>
> - /* BGP open policy, RFC 9234 */
> - if (p->capa.ann.role_ena) {
> + /* BGP open policy, RFC 9234, only for ebgp sessions */
> + if (p->capa.ann.role_ena && p->conf.ebgp) {
> errs += session_capa_add(opb, CAPA_ROLE, 1);
> errs += ibuf_add(opb, &p->capa.ann.role, 1);
> }
> @@ -2478,6 +2478,11 @@ parse_notification(struct peer *peer)
> log_peer_warnx(&peer->conf,
> "disabling route refresh capability");
> break;
> + case CAPA_ROLE:
> + peer->capa.ann.role_ena = 0;
> + log_peer_warnx(&peer->conf,
> + "disabling role capability");
> + break;
> case CAPA_RESTART:
> peer->capa.ann.grestart.restart = 0;
> log_peer_warnx(&peer->conf,
> @@ -2616,10 +2621,13 @@ parse_capabilities(struct peer *peer, u_
> case CAPA_ROLE:
> if (capa_len != 1) {
> log_peer_warnx(&peer->conf,
> - "Bad open policy capability length: "
> + "Bad role capability length: "
> "%u", capa_len);
you could merge the "%u" into the previous line
> break;
> }
> + if (!peer->conf.ebgp)
> + log_peer_warnx(&peer->conf,
> + "Received role capability on iBGP session");
> peer->capa.peer.role_ena = 1;
> peer->capa.peer.role = *capa_val;
> break;
> @@ -2835,8 +2843,10 @@ capa_neg_calc(struct peer *p, uint8_t *s
> * Make sure that the roles match and set the negotiated capability
> * to the role of the peer. So the RDE can inject the OTC attribute.
> * See RFC 9234, section 4.2.
> + * These check should only happen on ebgp sessions.
checks
> */
> - if (p->capa.ann.role_ena != 0 && p->capa.peer.role_ena != 0) {
> + if (p->capa.ann.role_ena != 0 && p->capa.peer.role_ena != 0 &&
> + p->conf.ebgp) {
> switch (p->capa.ann.role) {
> case CAPA_ROLE_PROVIDER:
> if (p->capa.peer.role != CAPA_ROLE_CUSTOMER)
> @@ -2868,7 +2878,7 @@ capa_neg_calc(struct peer *p, uint8_t *s
> }
> p->capa.neg.role_ena = 1;
> p->capa.neg.role = p->capa.peer.role;
> - } else if (p->capa.ann.role_ena == 2) {
> + } else if (p->capa.ann.role_ena == 2 && p->conf.ebgp) {
> /* enforce presence of open policy role capability */
> log_peer_warnx(&p->conf, "open policy role enforced but "
> "not present");
>
