From: claudio Date: Thu, 9 Mar 2023 13:12:19 +0000 (+0000) Subject: Major rework of RFC9234 support. My initial interpretation of the RFC was X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=372bb3aab57965ab60a60f015ffb362db3a6dd69;p=openbsd Major rework of RFC9234 support. My initial interpretation of the RFC was too conservative. Fixes and changes include: - add role output to bgpctl, also adjust the capability output. Note, this changes the JSON output of neighbors a bit. - adjust the config parser to enable the RFC9234 role capability when there is a role set. iBGP and sessions with no role will not announce the role capability. - adjust the role capability announcement to be only on sessions that use either AFI IPv4 or IPv6 and SAFI 1 (AID_INET, AID_INET6). - if there is an OPEN notification indicating that the role capability is bad only disable the capability if it is not enforced. - Adjust capability negotiation, store remote_role on the peer since the neighbors role is no longer needed by the RDE. - inject the OTC attribute on ingress only for AID_INET and AID_INET6. For other AIDs clear the F_ATTR_OTC_LOOP flag. - Adjust the role logic in the RDE and use the peer->role (local role of the system) for all checks. Also remove the check if the role capability was negotiated between peers. - In prefix_eligible() check also if the F_ATTR_OTC_LOOP flag is set. The RFC requires that prefixes must be considered ineligible (and not treat as withdraw as done before) - When generating an UPDATE include the OTC attribute unless the AID is neither AID_INET or AID_INET6. Fixes https://github.com/openbgpd-portable/openbgpd-portable/issues/51 Reported by Pier Carlo Chiodi OK tb@ --- diff --git a/usr.sbin/bgpd/bgpd.h b/usr.sbin/bgpd/bgpd.h index 989f1d36eb2..1cef0670cae 100644 --- a/usr.sbin/bgpd/bgpd.h +++ b/usr.sbin/bgpd/bgpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bgpd.h,v 1.461 2023/02/09 13:43:23 claudio Exp $ */ +/* $OpenBSD: bgpd.h,v 1.462 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -398,13 +398,12 @@ struct capabilities { int8_t flags[AID_MAX]; /* graceful restart per AID flags */ int8_t restart; /* graceful restart, RFC 4724 */ } grestart; - enum role role; /* Open Policy, RFC 9234 */ int8_t mp[AID_MAX]; /* multiprotocol extensions, RFC 4760 */ int8_t add_path[AID_MAX]; /* ADD_PATH, RFC 7911 */ int8_t refresh; /* route refresh, RFC 2918 */ int8_t as4byte; /* 4-byte ASnum, RFC 4893 */ int8_t enhanced_rr; /* enhanced route refresh, RFC 7313 */ - int8_t role_ena; /* 1 for enable, 2 for enforce */ + int8_t policy; /* Open Policy, RFC 9234, 2 = enforce */ }; /* flags for RFC 4724 - graceful restart */ diff --git a/usr.sbin/bgpd/parse.y b/usr.sbin/bgpd/parse.y index 74ce8fd008e..29547342cee 100644 --- a/usr.sbin/bgpd/parse.y +++ b/usr.sbin/bgpd/parse.y @@ -1,4 +1,4 @@ -/* $OpenBSD: parse.y,v 1.441 2023/01/30 16:51:34 claudio Exp $ */ +/* $OpenBSD: parse.y,v 1.442 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2002, 2003, 2004 Henning Brauer @@ -1642,7 +1642,7 @@ peeropts : REMOTEAS as4number { curpeer->conf.eval.maxpaths = $6; } | ANNOUNCE POLICY enforce { - curpeer->conf.capabilities.role_ena = $3; + curpeer->conf.capabilities.policy = $3; } | ROLE STRING { if (strcmp($2, "provider") == 0) { @@ -4284,6 +4284,7 @@ alloc_peer(void) p->conf.capabilities.refresh = 1; p->conf.capabilities.grestart.restart = 1; p->conf.capabilities.as4byte = 1; + p->conf.capabilities.policy = 1; p->conf.local_as = conf->as; p->conf.local_short_as = conf->short_as; p->conf.remote_port = BGP_PORT; @@ -4736,12 +4737,13 @@ neighbor_consistent(struct peer *p) } /* BGP role and RFC 9234 role are only valid for EBGP neighbors */ - if (p->conf.ebgp) { - if (p->conf.role == ROLE_NONE) - p->conf.capabilities.role_ena = 0; - p->conf.capabilities.role = p->conf.role; - } else + if (!p->conf.ebgp) { p->conf.role = ROLE_NONE; + p->conf.capabilities.policy = 0; + } else if (p->conf.role == ROLE_NONE) { + /* no role, no policy capability */ + p->conf.capabilities.policy = 0; + } /* check for duplicate peer definitions */ RB_FOREACH(xp, peer_head, new_peers) diff --git a/usr.sbin/bgpd/printconf.c b/usr.sbin/bgpd/printconf.c index de6b4c3a8fe..3f2e8565f96 100644 --- a/usr.sbin/bgpd/printconf.c +++ b/usr.sbin/bgpd/printconf.c @@ -1,4 +1,4 @@ -/* $OpenBSD: printconf.c,v 1.163 2023/01/24 14:13:12 claudio Exp $ */ +/* $OpenBSD: printconf.c,v 1.164 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -847,9 +847,9 @@ print_announce(struct peer_config *p, const char *c) printf(" max %d", p->eval.maxpaths); printf("\n"); } - if (p->capabilities.role_ena) { + if (p->capabilities.policy) { printf("%s\tannounce policy %s\n", c, - p->capabilities.role_ena == 2 ? "enforce" : "yes"); + p->capabilities.policy == 2 ? "enforce" : "yes"); } } diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c index 9cb68ef9865..dce91d6b274 100644 --- a/usr.sbin/bgpd/rde.c +++ b/usr.sbin/bgpd/rde.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.c,v 1.593 2023/02/13 18:07:53 claudio Exp $ */ +/* $OpenBSD: rde.c,v 1.594 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -1242,7 +1242,7 @@ rde_update_dispatch(struct rde_peer *peer, struct imsg *imsg) uint16_t withdrawn_len; uint16_t attrpath_len; uint16_t nlri_len; - uint8_t aid, prefixlen, safi, subtype, role; + uint8_t aid, prefixlen, safi, subtype; uint32_t fas, pathid; p = imsg->data; @@ -1324,26 +1324,6 @@ rde_update_dispatch(struct rde_peer *peer, struct imsg *imsg) } } - /* inject open policy OTC attribute if needed */ - if (peer_has_open_policy(peer, &role) && - (state.aspath.flags & F_ATTR_OTC) == 0) { - uint32_t tmp; - switch (role) { - case CAPA_ROLE_PROVIDER: - case CAPA_ROLE_RS: - case CAPA_ROLE_PEER: - tmp = htonl(peer->conf.remote_as); - if (attr_optadd(&state.aspath, - ATTR_OPTIONAL|ATTR_TRANSITIVE, ATTR_OTC, - &tmp, sizeof(tmp)) == -1) { - rde_update_err(peer, ERR_UPDATE, - ERR_UPD_ATTRLIST, NULL, 0); - goto done; - } - state.aspath.flags |= F_ATTR_OTC; - } - } - /* aspath needs to be loop free. This is not a hard error. */ if (state.aspath.flags & F_ATTR_ASPATH && peer->conf.ebgp && @@ -1520,6 +1500,28 @@ rde_update_dispatch(struct rde_peer *peer, struct imsg *imsg) NULL, 0); goto done; } + + /* inject open policy OTC attribute if needed */ + if ((state.aspath.flags & F_ATTR_OTC) == 0) { + uint32_t tmp; + switch (peer->role) { + case ROLE_CUSTOMER: + case ROLE_RS_CLIENT: + case ROLE_PEER: + tmp = htonl(peer->conf.remote_as); + if (attr_optadd(&state.aspath, + ATTR_OPTIONAL|ATTR_TRANSITIVE, ATTR_OTC, + &tmp, sizeof(tmp)) == -1) { + rde_update_err(peer, ERR_UPDATE, + ERR_UPD_ATTRLIST, NULL, 0); + goto done; + } + state.aspath.flags |= F_ATTR_OTC; + break; + default: + break; + } + } } while (nlri_len > 0) { if (peer_has_add_path(peer, AID_INET, CAPA_AP_RECV)) { @@ -1580,6 +1582,34 @@ rde_update_dispatch(struct rde_peer *peer, struct imsg *imsg) goto done; } + if (aid == AID_INET6) { + /* inject open policy OTC attribute if needed */ + if ((state.aspath.flags & F_ATTR_OTC) == 0) { + uint32_t tmp; + switch (peer->role) { + case ROLE_CUSTOMER: + case ROLE_RS_CLIENT: + case ROLE_PEER: + tmp = htonl(peer->conf.remote_as); + if (attr_optadd(&state.aspath, + ATTR_OPTIONAL|ATTR_TRANSITIVE, + ATTR_OTC, &tmp, + sizeof(tmp)) == -1) { + rde_update_err(peer, ERR_UPDATE, + ERR_UPD_ATTRLIST, NULL, 0); + goto done; + } + state.aspath.flags |= F_ATTR_OTC; + break; + default: + break; + } + } + } else { + /* Only IPv4 and IPv6 unicast do OTC handling */ + state.aspath.flags &= ~F_ATTR_OTC_LOOP; + } + /* unlock the previously locked nexthop, it is no longer used */ nexthop_unref(state.nexthop); state.nexthop = NULL; @@ -1824,7 +1854,7 @@ rde_attr_parse(u_char *p, uint16_t len, struct rde_peer *peer, int error; uint16_t attr_len, nlen; uint16_t plen = 0; - uint8_t flags, type, role, tmp8; + uint8_t flags, type, tmp8; if (len < 3) { bad_len: @@ -2160,20 +2190,19 @@ bad_flags: if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, ATTR_PARTIAL)) goto bad_flags; - if (peer_has_open_policy(peer, &role)) { - switch (role) { - case CAPA_ROLE_CUSTOMER: - case CAPA_ROLE_RS_CLIENT: - a->flags |= F_ATTR_OTC_LOOP | F_ATTR_PARSE_ERR; - break; - case CAPA_ROLE_PEER: - memcpy(&tmp32, p, sizeof(tmp32)); - tmp32 = ntohl(tmp32); - if (tmp32 != peer->conf.remote_as) - a->flags |= F_ATTR_OTC_LOOP | - F_ATTR_PARSE_ERR; - break; - } + switch (peer->role) { + case ROLE_PROVIDER: + case ROLE_RS: + a->flags |= F_ATTR_OTC_LOOP; + break; + case ROLE_PEER: + memcpy(&tmp32, p, sizeof(tmp32)); + tmp32 = ntohl(tmp32); + if (tmp32 != peer->conf.remote_as) + a->flags |= F_ATTR_OTC_LOOP; + break; + default: + break; } a->flags |= F_ATTR_OTC; goto optattr; diff --git a/usr.sbin/bgpd/rde.h b/usr.sbin/bgpd/rde.h index d9f728ae270..9488f2d5ba4 100644 --- a/usr.sbin/bgpd/rde.h +++ b/usr.sbin/bgpd/rde.h @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.h,v 1.283 2023/02/13 18:07:53 claudio Exp $ */ +/* $OpenBSD: rde.h,v 1.284 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Claudio Jeker and @@ -400,7 +400,6 @@ int rde_match_peer(struct rde_peer *, struct ctl_neighbor *); /* rde_peer.c */ int peer_has_as4byte(struct rde_peer *); int peer_has_add_path(struct rde_peer *, uint8_t, int); -int peer_has_open_policy(struct rde_peer *, uint8_t *); int peer_accept_no_as_set(struct rde_peer *); void peer_init(void); void peer_shutdown(void); diff --git a/usr.sbin/bgpd/rde_decide.c b/usr.sbin/bgpd/rde_decide.c index 9f0646703a1..4313be095c9 100644 --- a/usr.sbin/bgpd/rde_decide.c +++ b/usr.sbin/bgpd/rde_decide.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_decide.c,v 1.99 2023/02/13 18:07:53 claudio Exp $ */ +/* $OpenBSD: rde_decide.c,v 1.100 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Claudio Jeker @@ -487,7 +487,8 @@ prefix_eligible(struct prefix *p) struct rde_aspath *asp = prefix_aspath(p); /* The aspath needs to be loop and error free */ - if (asp == NULL || asp->flags & (F_ATTR_LOOP|F_ATTR_PARSE_ERR)) + if (asp == NULL || + asp->flags & (F_ATTR_LOOP|F_ATTR_OTC_LOOP|F_ATTR_PARSE_ERR)) return 0; /* The nexthop must be valid. */ diff --git a/usr.sbin/bgpd/rde_peer.c b/usr.sbin/bgpd/rde_peer.c index 6d58de17301..181f75ff8d0 100644 --- a/usr.sbin/bgpd/rde_peer.c +++ b/usr.sbin/bgpd/rde_peer.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_peer.c,v 1.29 2023/02/13 18:07:53 claudio Exp $ */ +/* $OpenBSD: rde_peer.c,v 1.30 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2019 Claudio Jeker @@ -61,13 +61,6 @@ peer_has_add_path(struct rde_peer *peer, uint8_t aid, int mode) return (peer->capa.add_path[aid] & mode); } -int -peer_has_open_policy(struct rde_peer *peer, uint8_t *role) -{ - *role = peer->capa.role; - return (peer->capa.role_ena != 0); -} - int peer_accept_no_as_set(struct rde_peer *peer) { diff --git a/usr.sbin/bgpd/rde_update.c b/usr.sbin/bgpd/rde_update.c index 808c5c728ff..5b0eca0760f 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.156 2023/02/13 18:07:53 claudio Exp $ */ +/* $OpenBSD: rde_update.c,v 1.157 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2004 Claudio Jeker @@ -66,7 +66,7 @@ up_test_update(struct rde_peer *peer, struct prefix *p) if (asp == NULL || asp->flags & F_ATTR_PARSE_ERR) fatalx("try to send out a botched path"); - if (asp->flags & F_ATTR_LOOP) + if (asp->flags & (F_ATTR_LOOP | F_ATTR_OTC_LOOP)) fatalx("try to send out a looped path"); if (peer == frompeer) @@ -104,27 +104,28 @@ up_test_update(struct rde_peer *peer, struct prefix *p) /* RFC9234 open policy handling */ static int -up_enforce_open_policy(struct rde_peer *peer, struct filterstate *state) +up_enforce_open_policy(struct rde_peer *peer, struct filterstate *state, + uint8_t aid) { - uint8_t role; - - if (!peer_has_open_policy(peer, &role)) + /* only for IPv4 and IPv6 unicast */ + if (aid != AID_INET && aid != AID_INET6) return 0; /* * do not propagate (consider it filtered) if OTC is present and - * neighbor role is peer, provider or rs. + * local role is peer, customer or rs-client. */ - if (role == CAPA_ROLE_PEER || role == CAPA_ROLE_PROVIDER || - role == CAPA_ROLE_RS) + if (peer->role == ROLE_PEER || peer->role == ROLE_CUSTOMER || + peer->role == ROLE_RS_CLIENT) if (state->aspath.flags & F_ATTR_OTC) - return (1); + return 1; /* - * add OTC attribute if not present for peers, customers and rs-clients. + * add OTC attribute if not present towards peers, customers and + * rs-clients (local roles peer, provider, rs). */ - if (role == CAPA_ROLE_PEER || role == CAPA_ROLE_CUSTOMER || - role == CAPA_ROLE_RS_CLIENT) + if (peer->role == ROLE_PEER || peer->role == ROLE_PROVIDER || + peer->role == ROLE_RS) if ((state->aspath.flags & F_ATTR_OTC) == 0) { uint32_t tmp; @@ -172,7 +173,7 @@ up_process_prefix(struct filter_head *rules, struct rde_peer *peer, } /* Open Policy Check: acts like an output filter */ - if (up_enforce_open_policy(peer, &state)) { + if (up_enforce_open_policy(peer, &state, addr->aid)) { rde_filterstate_clean(&state); return UP_FILTERED; } diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c index 1ef5c6422cc..68e76737d12 100644 --- a/usr.sbin/bgpd/session.c +++ b/usr.sbin/bgpd/session.c @@ -1,4 +1,4 @@ -/* $OpenBSD: session.c,v 1.441 2023/02/14 15:37:45 claudio Exp $ */ +/* $OpenBSD: session.c,v 1.442 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004, 2005 Henning Brauer @@ -1475,10 +1475,12 @@ session_open(struct peer *p) errs += session_capa_add(opb, CAPA_REFRESH, 0); /* BGP open policy, RFC 9234, only for ebgp sessions */ - if (p->conf.ebgp && p->capa.ann.role_ena && - p->capa.ann.role != ROLE_NONE) { + if (p->conf.ebgp && p->capa.ann.policy && + p->conf.role != ROLE_NONE && + (p->capa.ann.mp[AID_INET] || p->capa.ann.mp[AID_INET6] || + mpcapa == 0)) { uint8_t val; - val = role2capa(p->capa.ann.role); + val = role2capa(p->conf.role); errs += session_capa_add(opb, CAPA_ROLE, 1); errs += ibuf_add(opb, &val, 1); } @@ -2515,9 +2517,15 @@ parse_notification(struct peer *peer) "disabling route refresh capability"); break; case CAPA_ROLE: - peer->capa.ann.role_ena = 0; - log_peer_warnx(&peer->conf, - "disabling role capability"); + if (peer->capa.ann.policy == 1) { + peer->capa.ann.policy = 0; + log_peer_warnx(&peer->conf, + "disabling role capability"); + } else { + log_peer_warnx(&peer->conf, + "role capability enforced, " + "not disabling"); + } break; case CAPA_RESTART: peer->capa.ann.grestart.restart = 0; @@ -2660,11 +2668,13 @@ parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) "Bad role capability length: %u", capa_len); break; } - if (!peer->conf.ebgp) + 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 = capa2role(*capa_val); + break; + } + peer->capa.peer.policy = 1; + peer->remote_role = capa2role(*capa_val); break; case CAPA_RESTART: if (capa_len == 2) { @@ -2880,40 +2890,40 @@ capa_neg_calc(struct peer *p, uint8_t *suberr) * See RFC 9234, section 4.2. * These checks should only happen on ebgp sessions. */ - if (p->capa.ann.role_ena != 0 && p->capa.peer.role_ena != 0 && + if (p->capa.ann.policy != 0 && p->capa.peer.policy != 0 && p->conf.ebgp) { - switch (p->capa.ann.role) { + switch (p->conf.role) { case ROLE_PROVIDER: - if (p->capa.peer.role != ROLE_CUSTOMER) + if (p->remote_role != ROLE_CUSTOMER) goto fail; break; case ROLE_RS: - if (p->capa.peer.role != ROLE_RS_CLIENT) + if (p->remote_role != ROLE_RS_CLIENT) goto fail; break; case ROLE_RS_CLIENT: - if (p->capa.peer.role != ROLE_RS) + if (p->remote_role != ROLE_RS) goto fail; break; case ROLE_CUSTOMER: - if (p->capa.peer.role != ROLE_PROVIDER) + if (p->remote_role != ROLE_PROVIDER) goto fail; break; case ROLE_PEER: - if (p->capa.peer.role != ROLE_PEER) + if (p->remote_role != ROLE_PEER) goto fail; break; default: fail: log_peer_warnx(&p->conf, "open policy role mismatch: " - "%s vs %s", log_policy(p->capa.ann.role), - log_policy(p->capa.peer.role)); + "our role %s, their role %s", + log_policy(p->conf.role), + log_policy(p->remote_role)); *suberr = ERR_OPEN_ROLE; return (-1); } - p->capa.neg.role_ena = 1; - p->capa.neg.role = p->capa.peer.role; - } else if (p->capa.ann.role_ena == 2 && p->conf.ebgp) { + p->capa.neg.policy = 1; + } else if (p->capa.ann.policy == 2 && p->conf.ebgp) { /* enforce presence of open policy role capability */ log_peer_warnx(&p->conf, "open policy role enforced but " "not present"); diff --git a/usr.sbin/bgpd/session.h b/usr.sbin/bgpd/session.h index acc2b79bb0f..26fc2503e4e 100644 --- a/usr.sbin/bgpd/session.h +++ b/usr.sbin/bgpd/session.h @@ -1,4 +1,4 @@ -/* $OpenBSD: session.h,v 1.159 2023/02/09 13:43:23 claudio Exp $ */ +/* $OpenBSD: session.h,v 1.160 2023/03/09 13:12:19 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -243,6 +243,7 @@ struct peer { enum session_state state; enum session_state prev_state; enum reconf_action reconf_action; + enum role remote_role; uint16_t short_as; uint16_t holdtime; uint16_t local_port;