Refactor up_get_nexthop() to work for all AFI/SAFI cases. Additionally

clean up the possible nexthop overrides to better match the RFC.
- set nexthop self is still overriding all other decisions
- set nexthop no-modify has only relevance for ebgp multihop links
  Instead of using the router locall address the nexthop is passed unmodified
- set nexthop <address> depends on BGP session type
  * for IBGP sessions the address will be used unless it is the same as the
    remote peers address
  * for directly connected EBGP sessions the address is only used if the IP
    is part of the connected network (no matter what other flags are used).
  * for multihop EBGP sessions it depends if no-modify was also set
Adjust manual page to explain this properly.
"probably OK" job@

2 files changed, 109 insertions, 203 deletions

diff --git a/usr.sbin/bgpd/bgpd.conf.5 b/usr.sbin/bgpd/bgpd.conf.5
index 630c3a356e8..69187241105 100644
--- a/usr.sbin/bgpd/bgpd.conf.5
+++ b/usr.sbin/bgpd/bgpd.conf.5
@@ -1,4 +1,4 @@
-.\" $OpenBSD: bgpd.conf.5,v 1.189 2019/05/29 11:13:23 claudio Exp $
+.\" $OpenBSD: bgpd.conf.5,v 1.190 2019/06/07 09:45:48 claudio Exp $
 .\"
 .\" Copyright (c) 2004 Claudio Jeker <claudio@openbsd.org>
 .\" Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -16,7 +16,7 @@
 .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 .\"
-.Dd $Mdocdate: May 29 2019 $
+.Dd $Mdocdate: June 7 2019 $
 .Dt BGPD.CONF 5
 .Os
 .Sh NAME
@@ -1807,15 +1807,28 @@ Set the
 .Em NEXTHOP
 AS path attribute
 to a different nexthop address or use blackhole or reject routes.
-If set to
-.Em no-modify ,
-the nexthop attribute is not modified.
-Unless set to
-.Em self ,
-the nexthop is left unmodified for IBGP
-sessions.
+.Em blackhole
+and
+.Em reject
+only affect the FIB and will not alter the nexthop address.
 .Em self
 forces the nexthop to be set to the local interface address.
+If set to
+.Em no-modify ,
+the nexthop attribute is not modified for EBGP multihop sessions.
+By default EBGP multihop sessions use the local interface address.
+On other IBGP and directly connected EBGP sessions
+.Em no-modify
+is ignored.
+The set
+.Ar address
+is used on IBGP session and on directly connected EBGP session if the
+.Ar address
+is part of the connected network.
+On EBGP multihop session
+.Em no-modify
+has to be set to force the nexthop to
+.Ar address .
 .Bd -literal -offset indent
 set nexthop 192.168.0.1
 set nexthop blackhole
diff --git a/usr.sbin/bgpd/rde_update.c b/usr.sbin/bgpd/rde_update.c
index 6073886a43a..7ded2db049f 100644
--- a/usr.sbin/bgpd/rde_update.c
+++ b/usr.sbin/bgpd/rde_update.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde_update.c,v 1.112 2019/05/31 09:46:31 claudio Exp $ */
+/*	$OpenBSD: rde_update.c,v 1.113 2019/06/07 09:45:48 claudio Exp $ */
 
 /*
  * Copyright (c) 2004 Claudio Jeker <claudio@openbsd.org>
@@ -238,59 +238,84 @@ up_generate_default(struct filter_head *rules, struct rde_peer *peer,
 }
 
 /* only for IPv4 */
-static in_addr_t
-up_get_nexthop(struct rde_peer *peer, struct filterstate *state)
+static struct bgpd_addr *
+up_get_nexthop(struct rde_peer *peer, struct filterstate *state, u_int8_t aid)
 {
-	in_addr_t	mask;
-
-	/* nexthop, already network byte order */
-	if (state->nhflags & NEXTHOP_NOMODIFY) {
-		/* no modify flag set */
-		if (state->nexthop == NULL)
-			return (peer->local_v4_addr.v4.s_addr);
-		else
-			return (state->nexthop->exit_nexthop.v4.s_addr);
-	} else if (state->nhflags & NEXTHOP_SELF)
-		return (peer->local_v4_addr.v4.s_addr);
-	else if (!peer->conf.ebgp) {
+	struct bgpd_addr *peer_local;
+
+	switch (aid) {
+	case AID_INET:
+	case AID_VPN_IPv4:
+		peer_local = &peer->local_v4_addr;
+		break;
+	case AID_INET6:
+	case AID_VPN_IPv6:
+		peer_local = &peer->local_v6_addr;
+		break;
+	default:
+		fatalx("%s, bad AID %s", __func__, aid2str(aid));
+	}
+
+	if (state->nhflags & NEXTHOP_SELF) {
 		/*
-		 * If directly connected use peer->local_v4_addr
-		 * this is only true for announced networks.
+		 * Forcing the nexthop to self is always possible
+		 * and has precedence over other flags.
 		 */
-		if (state->nexthop == NULL)
-			return (peer->local_v4_addr.v4.s_addr);
-		else if (state->nexthop->exit_nexthop.v4.s_addr ==
-		    peer->remote_addr.v4.s_addr)
-			/*
-			 * per RFC: if remote peer address is equal to
-			 * the nexthop set the nexthop to our local address.
-			 * This reduces the risk of routing loops.
-			 */
-			return (peer->local_v4_addr.v4.s_addr);
-		else
-			return (state->nexthop->exit_nexthop.v4.s_addr);
+		return (peer_local);
+	} else if (!peer->conf.ebgp) {
+		/*
+		 * in the ibgp case the nexthop is normally not
+		 * modified unless it points at the peer itself.
+		 */
+		if (state->nexthop == NULL) {
+			/* announced networks without explicit nexthop set */
+			return (peer_local);
+		}
+		/*
+		 * per RFC: if remote peer address is equal to the nexthop set
+		 * the nexthop to our local address. This reduces the risk of
+		 * routing loops. This overrides NEXTHOP_NOMODIFY.
+		 */
+		if (memcmp(&state->nexthop->exit_nexthop,
+		    &peer->remote_addr, sizeof(peer->remote_addr)) == 0) {
+			return (peer_local);
+		}
+		return (&state->nexthop->exit_nexthop);
 	} else if (peer->conf.distance == 1) {
-		/* ebgp directly connected */
+		/*
+		 * In the ebgp directly connected case never send
+		 * out a nexthop that is outside of the connected
+		 * network of the peer. No matter what flags are
+		 * set. This follows section 5.1.3 of RFC 4271.
+		 * So just check if the nexthop is in the same net
+		 * is enough here.
+		 */
 		if (state->nexthop != NULL &&
-		    state->nexthop->flags & NEXTHOP_CONNECTED) {
-			mask = htonl(
-			    prefixlen2mask(state->nexthop->nexthop_netlen));
-			if ((peer->remote_addr.v4.s_addr & mask) ==
-			    (state->nexthop->nexthop_net.v4.s_addr & mask))
-				/* nexthop and peer are in the same net */
-				return (state->nexthop->exit_nexthop.v4.s_addr);
-			else
-				return (peer->local_v4_addr.v4.s_addr);
-		} else
-			return (peer->local_v4_addr.v4.s_addr);
-	} else
-		/* ebgp multihop */
+		    state->nexthop->flags & NEXTHOP_CONNECTED &&
+		    prefix_compare(&peer->remote_addr,
+		    &state->nexthop->nexthop_net,
+		    state->nexthop->nexthop_netlen) == 0) {
+			/* nexthop and peer are in the same net */
+			return (&state->nexthop->exit_nexthop);
+		}
+		return (peer_local);
+	} else {
 		/*
-		 * For ebgp multihop nh->flags should never have
-		 * NEXTHOP_CONNECTED set so it should be possible to unify the
-		 * two ebgp cases. But this is safe and RFC compliant.
+		 * For ebgp multihop make it possible to overrule
+		 * the sent nexthop by setting NEXTHOP_NOMODIFY.
+		 * Similar to the ibgp case there is no same net check
+		 * needed but still ensure that the nexthop is not
+		 * pointing to the peer itself.
 		 */
-		return (peer->local_v4_addr.v4.s_addr);
+		if (state->nhflags & NEXTHOP_NOMODIFY &&
+		    state->nexthop == NULL &&
+		    memcmp(&state->nexthop->exit_nexthop,
+		    &peer->remote_addr, sizeof(peer->remote_addr)) != 0) {
+			/* no modify flag set and nexthop not peer addr */
+			return (&state->nexthop->exit_nexthop);
+		}
+		return (peer_local);
+	}
 }
 
 static int
@@ -350,7 +375,8 @@ up_generate_attr(u_char *buf, int len, struct rde_peer *peer,
 		case ATTR_NEXTHOP:
 			switch (aid) {
 			case AID_INET:
-				nexthop = up_get_nexthop(peer, state);
+				nexthop =
+				    up_get_nexthop(peer, state, aid)->v4.s_addr;
 				if ((r = attr_write(buf + wlen, len,
 				    ATTR_WELL_KNOWN, ATTR_NEXTHOP, &nexthop,
 				    4)) == -1)
@@ -730,10 +756,10 @@ static int
 up_generate_mp_reach(u_char *buf, int len, struct rde_peer *peer,
     struct filterstate *state, u_int8_t aid)
 {
-	u_char		*attrbuf;
-	int		 r;
-	int		 wpos, attrlen;
-	u_int16_t	 tmp;
+	struct bgpd_addr	*nexthop;
+	u_char			*attrbuf;
+	int			 r, wpos, attrlen;
+	u_int16_t		 tmp;
 
 	if (len < 4)
 		return (-1);
@@ -756,54 +782,10 @@ up_generate_mp_reach(u_char *buf, int len, struct rde_peer *peer,
 		attrbuf[3] = sizeof(struct in6_addr);
 		attrbuf[20] = 0; /* Reserved must be 0 */
 
-		/* nexthop dance see also up_get_nexthop() */
+		/* write nexthop */
 		attrbuf += 4;
-		if (state->nhflags & NEXTHOP_NOMODIFY) {
-			/* no modify flag set */
-			if (state->nexthop == NULL)
-				memcpy(attrbuf, &peer->local_v6_addr.v6,
-				    sizeof(struct in6_addr));
-			else
-				memcpy(attrbuf,
-				    &state->nexthop->exit_nexthop.v6,
-				    sizeof(struct in6_addr));
-		} else if (state->nhflags & NEXTHOP_SELF)
-			memcpy(attrbuf, &peer->local_v6_addr.v6,
-			    sizeof(struct in6_addr));
-		else if (!peer->conf.ebgp) {
-			/* ibgp */
-			if (state->nexthop == NULL ||
-			    (state->nexthop->exit_nexthop.aid == AID_INET6 &&
-			    !memcmp(&state->nexthop->exit_nexthop.v6,
-			    &peer->remote_addr.v6, sizeof(struct in6_addr))))
-				memcpy(attrbuf, &peer->local_v6_addr.v6,
-				    sizeof(struct in6_addr));
-			else
-				memcpy(attrbuf,
-				    &state->nexthop->exit_nexthop.v6,
-				    sizeof(struct in6_addr));
-		} else if (peer->conf.distance == 1) {
-			/* ebgp directly connected */
-			if (state->nexthop != NULL &&
-			    state->nexthop->flags & NEXTHOP_CONNECTED)
-				if (prefix_compare(&peer->remote_addr,
-				    &state->nexthop->nexthop_net,
-				    state->nexthop->nexthop_netlen) == 0) {
-					/*
-					 * nexthop and peer are in the same
-					 * subnet
-					 */
-					memcpy(attrbuf,
-					    &state->nexthop->exit_nexthop.v6,
-					    sizeof(struct in6_addr));
-					break;
-				}
-			memcpy(attrbuf, &peer->local_v6_addr.v6,
-			    sizeof(struct in6_addr));
-		} else
-			/* ebgp multihop */
-			memcpy(attrbuf, &peer->local_v6_addr.v6,
-			    sizeof(struct in6_addr));
+		nexthop = up_get_nexthop(peer, state, aid);
+		memcpy(attrbuf, &nexthop->v6, sizeof(struct in6_addr));
 		break;
 	case AID_VPN_IPv4:
 		attrlen = 17; /* AFI + SAFI + NH LEN + NH + Reserved */
@@ -818,55 +800,10 @@ up_generate_mp_reach(u_char *buf, int len, struct rde_peer *peer,
 		bzero(attrbuf + 4, sizeof(u_int64_t));
 		attrbuf[16] = 0; /* Reserved must be 0 */
 
-		/* nexthop dance see also up_get_nexthop() */
+		/* write nexthop */
 		attrbuf += 12;
-		if (state->nhflags & NEXTHOP_NOMODIFY) {
-			/* no modify flag set */
-			if (state->nexthop == NULL)
-				memcpy(attrbuf, &peer->local_v4_addr.v4,
-				    sizeof(struct in_addr));
-			else
-				/* nexthops are stored as IPv4 addrs */
-				memcpy(attrbuf,
-				    &state->nexthop->exit_nexthop.v4,
-				    sizeof(struct in_addr));
-		} else if (state->nhflags & NEXTHOP_SELF) {
-			memcpy(attrbuf, &peer->local_v4_addr.v4,
-			    sizeof(struct in_addr));
-		} else if (!peer->conf.ebgp) {
-			/* ibgp */
-			if (state->nexthop == NULL ||
-			    (state->nexthop->exit_nexthop.aid == AID_INET &&
-			    !memcmp(&state->nexthop->exit_nexthop.v4,
-			    &peer->remote_addr.v4, sizeof(struct in_addr))))
-				memcpy(attrbuf, &peer->local_v4_addr.v4,
-				    sizeof(struct in_addr));
-			else
-				memcpy(attrbuf,
-				    &state->nexthop->exit_nexthop.v4,
-				    sizeof(struct in_addr));
-		} else if (peer->conf.distance == 1) {
-			/* ebgp directly connected */
-			if (state->nexthop != NULL &&
-			    state->nexthop->flags & NEXTHOP_CONNECTED)
-				if (prefix_compare(&peer->remote_addr,
-				    &state->nexthop->nexthop_net,
-				    state->nexthop->nexthop_netlen) == 0) {
-					/*
-					 * nexthop and peer are in the same
-					 * subnet
-					 */
-					memcpy(attrbuf,
-					    &state->nexthop->exit_nexthop.v4,
-					    sizeof(struct in_addr));
-					break;
-				}
-			memcpy(attrbuf, &peer->local_v4_addr.v4,
-			    sizeof(struct in_addr));
-		} else
-			/* ebgp multihop */
-			memcpy(attrbuf, &peer->local_v4_addr.v4,
-			    sizeof(struct in_addr));
+		nexthop = up_get_nexthop(peer, state, aid);
+		memcpy(attrbuf, &nexthop->v4, sizeof(struct in_addr));
 		break;
 	case AID_VPN_IPv6:
 		attrlen = 29; /* AFI + SAFI + NH LEN + NH + Reserved */
@@ -881,54 +818,10 @@ up_generate_mp_reach(u_char *buf, int len, struct rde_peer *peer,
 		bzero(attrbuf + 4, sizeof(u_int64_t));
 		attrbuf[28] = 0; /* Reserved must be 0 */
 
-		/* nexthop dance see also up_get_nexthop() */
+		/* write nexthop */
 		attrbuf += 12;
-		if (state->nhflags & NEXTHOP_NOMODIFY) {
-			/* no modify flag set */
-			if (state->nexthop == NULL)
-				memcpy(attrbuf, &peer->local_v6_addr.v6,
-				    sizeof(struct in6_addr));
-			else
-				memcpy(attrbuf,
-				    &state->nexthop->exit_nexthop.v6,
-				    sizeof(struct in6_addr));
-		} else if (state->nhflags & NEXTHOP_SELF)
-			memcpy(attrbuf, &peer->local_v6_addr.v6,
-			    sizeof(struct in6_addr));
-		else if (!peer->conf.ebgp) {
-			/* ibgp */
-			if (state->nexthop == NULL ||
-			    (state->nexthop->exit_nexthop.aid == AID_INET6 &&
-			    !memcmp(&state->nexthop->exit_nexthop.v6,
-			    &peer->remote_addr.v6, sizeof(struct in6_addr))))
-				memcpy(attrbuf, &peer->local_v6_addr.v6,
-				    sizeof(struct in6_addr));
-			else
-				memcpy(attrbuf,
-				    &state->nexthop->exit_nexthop.v6,
-				    sizeof(struct in6_addr));
-		} else if (peer->conf.distance == 1) {
-			/* ebgp directly connected */
-			if (state->nexthop != NULL &&
-			    state->nexthop->flags & NEXTHOP_CONNECTED)
-				if (prefix_compare(&peer->remote_addr,
-				    &state->nexthop->nexthop_net,
-				    state->nexthop->nexthop_netlen) == 0) {
-					/*
-					* nexthop and peer are in the same
-					* subnet
-					*/
-					memcpy(attrbuf,
-					    &state->nexthop->exit_nexthop.v6,
-					    sizeof(struct in6_addr));
-					break;
-				}
-			memcpy(attrbuf, &peer->local_v6_addr.v6,
-			    sizeof(struct in6_addr));
-		} else
-			/* ebgp multihop */
-			memcpy(attrbuf, &peer->local_v6_addr.v6,
-			    sizeof(struct in6_addr));
+		nexthop = up_get_nexthop(peer, state, aid);
+		memcpy(attrbuf, &nexthop->v6, sizeof(struct in6_addr));
 		break;
 	default:
 		fatalx("up_generate_mp_reach: unknown AID");