Re: bgpd: stricter multiprotocol negotiation

2021-04-30 Thread Sebastian Benoit
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.04.28 12:40:46 +0200:
> At the moment bgpd will fall back to IPv4 unicast if there was no match in
> the multiprotocol capabilities between local and remote peer.
> This is not correct, if the router expects a certain AFI/SAFI for the
> session then it should not fall back to IPv4 unicast.
> 
> An example is when bgpd uses IPv6 unicast for a session but the remote
> side sends back IPv4 unicast. In that case the session would unexpectedly
> fall back to IPv4 unicast.
> 
> The right behaviour in that case to estabilsh the connection but no
> UPDATEs are exchanged since there is no matching AFI/SAFI pair.
> 
> This diff implements this. This is based on the fact that p->capa.ann.mp
> (the capabilities we announced) will have something set if a multiprotocol
> capability was inserted in the OPEN message. If nothing is set in
> p->capa.ann.mp then the OPEN message did not include a multiprotocol
> capability and the system needs to fall back to AF_INET since that is the
> default.
> 
> This should help with systems that have silly defaults and annouce only IPv4
> unicast on IPv6 sessions.

reads ok benno@

> -- 
> :wq Claudio
> 
> Index: session.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.412
> diff -u -p -r1.412 session.c
> --- session.c 27 Apr 2021 09:12:23 -  1.412
> +++ session.c 27 Apr 2021 15:13:19 -
> @@ -2557,11 +2557,12 @@ capa_neg_calc(struct peer *p)
>  
>   /* MP: both side must announce capability */
>   for (i = 0; i < AID_MAX; i++) {
> - if (p->capa.ann.mp[i] && p->capa.peer.mp[i]) {
> + if (p->capa.ann.mp[i] && p->capa.peer.mp[i])
>   p->capa.neg.mp[i] = 1;
> - hasmp = 1;
> - } else
> + else
>   p->capa.neg.mp[i] = 0;
> + if (p->capa.ann.mp[i])
> + hasmp = 1;
>   }
>   /* if no MP capability present default to IPv4 unicast mode */
>   if (!hasmp)
> 



Re: bgpd: stricter multiprotocol negotiation

2021-04-28 Thread Theo de Raadt
What I saw is a Ubiquity router v6 session misconfigured to ask for v4
routes.  The current logic decides the MP is inconsistant, and thus AFI.
If no AFI, then provide v4... anyways, that cannot be right.

It was worse because the v4 received routes on the famously high quality
Ubiquity were acted upon in a strange way, but that isn't our bug.

I think this new logic is better.

Claudio Jeker  wrote:

> At the moment bgpd will fall back to IPv4 unicast if there was no match in
> the multiprotocol capabilities between local and remote peer.
> This is not correct, if the router expects a certain AFI/SAFI for the
> session then it should not fall back to IPv4 unicast.
> 
> An example is when bgpd uses IPv6 unicast for a session but the remote
> side sends back IPv4 unicast. In that case the session would unexpectedly
> fall back to IPv4 unicast.
> 
> The right behaviour in that case to estabilsh the connection but no
> UPDATEs are exchanged since there is no matching AFI/SAFI pair.
> 
> This diff implements this. This is based on the fact that p->capa.ann.mp
> (the capabilities we announced) will have something set if a multiprotocol
> capability was inserted in the OPEN message. If nothing is set in
> p->capa.ann.mp then the OPEN message did not include a multiprotocol
> capability and the system needs to fall back to AF_INET since that is the
> default.
> 
> This should help with systems that have silly defaults and annouce only IPv4
> unicast on IPv6 sessions.
> -- 
> :wq Claudio
> 
> Index: session.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.412
> diff -u -p -r1.412 session.c
> --- session.c 27 Apr 2021 09:12:23 -  1.412
> +++ session.c 27 Apr 2021 15:13:19 -
> @@ -2557,11 +2557,12 @@ capa_neg_calc(struct peer *p)
>  
>   /* MP: both side must announce capability */
>   for (i = 0; i < AID_MAX; i++) {
> - if (p->capa.ann.mp[i] && p->capa.peer.mp[i]) {
> + if (p->capa.ann.mp[i] && p->capa.peer.mp[i])
>   p->capa.neg.mp[i] = 1;
> - hasmp = 1;
> - } else
> + else
>   p->capa.neg.mp[i] = 0;
> + if (p->capa.ann.mp[i])
> + hasmp = 1;
>   }
>   /* if no MP capability present default to IPv4 unicast mode */
>   if (!hasmp)
>