On Fri, May 26, 2017 at 07:24:06PM BST, Claudio Jeker wrote:
> On Fri, May 26, 2017 at 08:01:00PM +0200, Peter Hessler wrote:
> > Apropos of "I found it", I implemented support for RFC 7607. It's a
> > super short RFC, but basically it forbids use of AS 0 anywhere.
> >
> > OK?
> >
> >
> > Index: parse.y
> > ===================================================================
> > RCS file: /cvs/openbsd/src/usr.sbin/bgpd/parse.y,v
> > retrieving revision 1.300
> > diff -u -p -u -p -r1.300 parse.y
> > --- parse.y 26 May 2017 14:08:51 -0000 1.300
> > +++ parse.y 26 May 2017 17:55:11 -0000
> > @@ -3661,6 +3661,11 @@ neighbor_consistent(struct peer *p)
> > return (-1);
> > }
> >
> > + if (p->conf.remote_as == 0) {
> > + yyerror("peer AS needs to be not zero");
>
> Needs better wording.
My exact thought... not that I have any say in it :^)
To be more specific: "needs to be not" simply sounds odd.
Regards,
Raf
>
> > + return (-1);
> > + }
> > +
> > /* set default values if they where undefined */
> > p->conf.ebgp = (p->conf.remote_as != conf->as);
> > if (p->conf.announce_type == ANNOUNCE_UNDEF)
> > Index: rde.c
> > ===================================================================
> > RCS file: /cvs/openbsd/src/usr.sbin/bgpd/rde.c,v
> > retrieving revision 1.361
> > diff -u -p -u -p -r1.361 rde.c
> > --- rde.c 25 Jan 2017 03:21:55 -0000 1.361
> > +++ rde.c 26 May 2017 17:43:30 -0000
> > @@ -1102,6 +1102,14 @@ rde_update_dispatch(struct imsg *imsg)
> > /* shift to NLRI information */
> > p += 2 + attrpath_len;
> >
> > + /* aspath must not contain AS 0 */
> > + if (!aspath_loopfree(asp->aspath, 0)) {
> > + log_peer_warnx(&peer->conf, "bad AS 0 in UPDATE");
> > + rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH,
> > + NULL, 0);
>
> This can't be right. We should not cause a NOTIFICATION and session reset
> because of this. Doing that is bad bad bad.
>
> > + goto done;
> > + }
> > +
> > /* aspath needs to be loop free nota bene this is not a hard error */
> > if (peer->conf.ebgp && !aspath_loopfree(asp->aspath, conf->as))
> > asp->flags |= F_ATTR_LOOP;
> > Index: session.c
> > ===================================================================
> > RCS file: /cvs/openbsd/src/usr.sbin/bgpd/session.c,v
> > retrieving revision 1.359
> > diff -u -p -u -p -r1.359 session.c
> > --- session.c 13 Feb 2017 14:48:44 -0000 1.359
> > +++ session.c 5 May 2017 17:26:16 -0000
> > @@ -2017,6 +2017,14 @@ parse_open(struct peer *peer)
> > memcpy(&short_as, p, sizeof(short_as));
> > p += sizeof(short_as);
> > as = peer->short_as = ntohs(short_as);
> > + if (as == 0) {
> > + log_peer_warnx(&peer->conf,
> > + "peer requests unacceptable AS %u", as);
>
> Why not use 0 here instaed of %u?
>
> > + session_notification(peer, ERR_OPEN, ERR_OPEN_AS,
> > + NULL, 0);
> > + change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
> > + return (-1);
> > + }
> >
> > memcpy(&oholdtime, p, sizeof(oholdtime));
> > p += sizeof(oholdtime);
> > @@ -2477,6 +2485,14 @@ parse_capabilities(struct peer *peer, u_
> > }
> > memcpy(&remote_as, capa_val, sizeof(remote_as));
> > *as = ntohl(remote_as);
> > + if (*as == 0) {
> > + log_peer_warnx(&peer->conf,
> > + "peer requests unacceptable AS %u", *as);
>
> Same here.
>
> > + session_notification(peer, ERR_OPEN,
> > ERR_OPEN_AS,
> > + NULL, 0);
> > + change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
> > + return (-1);
> > + }
> > peer->capa.peer.as4byte = 1;
> > break;
> > default:
> >
> >
> > --
> > Taxes, n.:
> > Of life's two certainties, the only one for which you can get
> > an extension.
> >
>
> --
> :wq Claudio
>
