From 9dbb7eb6ebda016cd14ce8fef403c2b3f7bdd504 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Mon, 24 Jan 2022 03:44:21 +0100 Subject: BGP: Log route updates that were changed to withdraws Typical BGP error handling is treat-as-withdraw, where an invalid route is replaced with a withdraw. Log route network when it happens. --- proto/bgp/attrs.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'proto/bgp/attrs.c') diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index 24ba00ba..0688c7cd 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -1845,6 +1845,10 @@ bgp_rt_notify(struct proto *P, struct channel *C, net *n, rte *new, rte *old) { struct ea_list *attrs = bgp_update_attrs(p, c, new, new->attrs->eattrs, bgp_linpool2); + /* Error during attribute processing */ + if (!attrs) + log(L_ERR "%s: Invalid route %N withdrawn", p->p.name, n->n.addr); + /* If attributes are invalid, we fail back to withdraw */ buck = attrs ? bgp_get_bucket(c, attrs) : bgp_get_withdraw_bucket(c); path = new->attrs->src->global_id; -- cgit v1.2.3 From 963b2c7ce219df6bf9c179fff2dd2386cf26edf9 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Fri, 28 Jan 2022 05:35:22 +0100 Subject: BGP: Use proper class in attribute error messages Most error messages in attribute processing are in rx/decode step and these use L_REMOTE log class. But there are few that are in tx/export step and these should use L_ERR log class. Use tx-specific macro (REJECT()) in tx/export code and rename field err_withdraw to err_reject in struct bgp_export_state to ensure that appropriate error reporting macros are called in proper contexts. --- proto/bgp/attrs.c | 21 ++++++++++++--------- proto/bgp/bgp.h | 2 +- proto/bgp/packets.c | 13 ++++++++----- 3 files changed, 21 insertions(+), 15 deletions(-) (limited to 'proto/bgp/attrs.c') diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index 0688c7cd..968d7d2b 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -45,9 +45,9 @@ * * export - Hook that validates and normalizes attribute during export phase. * Receives eattr, may modify it (e.g., sort community lists for canonical - * representation), UNSET() it (e.g., skip empty lists), or WITHDRAW() it if - * necessary. May assume that eattr has value valid w.r.t. its type, but may be - * invalid w.r.t. BGP constraints. Optional. + * representation), UNSET() it (e.g., skip empty lists), or REJECT() the route + * if necessary. May assume that eattr has value valid w.r.t. its type, but may + * be invalid w.r.t. BGP constraints. Optional. * * encode - Hook that converts internal representation to external one during * packet writing. Receives eattr and puts it in the buffer (including attribute @@ -108,6 +108,9 @@ bgp_set_attr(ea_list **attrs, struct linpool *pool, uint code, uint flags, uintp #define UNSET(a) \ ({ a->type = EAF_TYPE_UNDEF; return; }) +#define REJECT(msg, args...) \ + ({ log(L_ERR "%s: " msg, s->proto->p.name, ## args); s->err_reject = 1; return; }) + #define NEW_BGP "Discarding %s attribute received from AS4-aware neighbor" #define BAD_EBGP "Discarding %s attribute received from EBGP neighbor" #define BAD_LENGTH "Malformed %s attribute - invalid length (%u)" @@ -380,7 +383,7 @@ static void bgp_export_origin(struct bgp_export_state *s, eattr *a) { if (a->u.data > 2) - WITHDRAW(BAD_VALUE, "ORIGIN", a->u.data); + REJECT(BAD_VALUE, "ORIGIN", a->u.data); } static void @@ -902,20 +905,20 @@ bgp_export_mpls_label_stack(struct bgp_export_state *s, eattr *a) /* Perhaps we should just ignore it? */ if (!s->mpls) - WITHDRAW("Unexpected MPLS stack"); + REJECT("Unexpected MPLS stack"); /* Empty MPLS stack is not allowed */ if (!lnum) - WITHDRAW("Malformed MPLS stack - empty"); + REJECT("Malformed MPLS stack - empty"); /* This is ugly, but we must ensure that labels fit into NLRI field */ if ((24*lnum + (net_is_vpn(n) ? 64 : 0) + net_pxlen(n)) > 255) - WITHDRAW("Malformed MPLS stack - too many labels (%u)", lnum); + REJECT("Malformed MPLS stack - too many labels (%u)", lnum); for (uint i = 0; i < lnum; i++) { if (labels[i] > 0xfffff) - WITHDRAW("Malformed MPLS stack - invalid label (%u)", labels[i]); + REJECT("Malformed MPLS stack - invalid label (%u)", labels[i]); /* TODO: Check for special-purpose label values? */ } @@ -1196,7 +1199,7 @@ bgp_export_attrs(struct bgp_export_state *s, ea_list *attrs) for (i = 0; i < count; i++) bgp_export_attr(s, &new->attrs[i], new); - if (s->err_withdraw) + if (s->err_reject) return NULL; return new; diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h index db05b693..08e751e7 100644 --- a/proto/bgp/bgp.h +++ b/proto/bgp/bgp.h @@ -397,7 +397,7 @@ struct bgp_export_state { int mpls; u32 attrs_seen[1]; - uint err_withdraw; + uint err_reject; uint local_next_hop; }; diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index da5a6523..9d21fd34 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -932,6 +932,9 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len) #define WITHDRAW(msg, args...) \ ({ REPORT(msg, ## args); s->err_withdraw = 1; return; }) +#define REJECT(msg, args...) \ + ({ log(L_ERR "%s: " msg, s->proto->p.name, ## args); s->err_reject = 1; return; }) + #define BAD_AFI "Unexpected AF <%u/%u> in UPDATE" #define BAD_NEXT_HOP "Invalid NEXT_HOP attribute" #define NO_NEXT_HOP "Missing NEXT_HOP attribute" @@ -1126,7 +1129,7 @@ bgp_update_next_hop_ip(struct bgp_export_state *s, eattr *a, ea_list **to) /* Check if next hop is valid */ a = bgp_find_attr(*to, BA_NEXT_HOP); if (!a) - WITHDRAW(NO_NEXT_HOP); + REJECT(NO_NEXT_HOP); ip_addr *nh = (void *) a->u.ptr->data; ip_addr peer = s->proto->remote_ip; @@ -1134,20 +1137,20 @@ bgp_update_next_hop_ip(struct bgp_export_state *s, eattr *a, ea_list **to) /* Forbid zero next hop */ if (ipa_zero2(nh[0]) && ((len != 32) || ipa_zero(nh[1]))) - WITHDRAW(BAD_NEXT_HOP " - zero address"); + REJECT(BAD_NEXT_HOP " - zero address"); /* Forbid next hop equal to neighbor IP */ if (ipa_equal(peer, nh[0]) || ((len == 32) && ipa_equal(peer, nh[1]))) - WITHDRAW(BAD_NEXT_HOP " - neighbor address %I", peer); + REJECT(BAD_NEXT_HOP " - neighbor address %I", peer); /* Forbid next hop with non-matching AF */ if ((ipa_is_ip4(nh[0]) != bgp_channel_is_ipv4(s->channel)) && !s->channel->ext_next_hop) - WITHDRAW(BAD_NEXT_HOP MISMATCHED_AF, nh[0], s->channel->desc->name); + REJECT(BAD_NEXT_HOP MISMATCHED_AF, nh[0], s->channel->desc->name); /* Just check if MPLS stack */ if (s->mpls && !bgp_find_attr(*to, BA_MPLS_LABEL_STACK)) - WITHDRAW(NO_LABEL_STACK); + REJECT(NO_LABEL_STACK); } static uint -- cgit v1.2.3 From 4c6ee53f31a7ac667bc597b0fe19b6365abad415 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Fri, 28 Jan 2022 18:13:18 +0100 Subject: BGP: Make routing loops silent One of previous commits added error logging of invalid routes. This also inadvertently caused error logging of route loops, which should be ignored silently. Fix that. --- proto/bgp/attrs.c | 12 ++++++++---- proto/bgp/packets.c | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) (limited to 'proto/bgp/attrs.c') diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index 968d7d2b..15713b63 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -1397,19 +1397,19 @@ bgp_decode_attrs(struct bgp_parse_state *s, byte *data, uint len) /* Reject routes with our ASN in AS_PATH attribute */ if (bgp_as_path_loopy(p, attrs, p->local_as)) - goto withdraw; + goto loop; /* Reject routes with our Confederation ID in AS_PATH attribute; RFC 5065 4.0 */ if ((p->public_as != p->local_as) && bgp_as_path_loopy(p, attrs, p->public_as)) - goto withdraw; + goto loop; /* Reject routes with our Router ID in ORIGINATOR_ID attribute; RFC 4456 8 */ if (p->is_internal && bgp_originator_id_loopy(p, attrs)) - goto withdraw; + goto loop; /* Reject routes with our Cluster ID in CLUSTER_LIST attribute; RFC 4456 8 */ if (p->rr_client && bgp_cluster_list_loopy(p, attrs)) - goto withdraw; + goto loop; /* If there is no local preference, define one */ if (!BIT32_TEST(s->attrs_seen, BA_LOCAL_PREF)) @@ -1430,6 +1430,10 @@ withdraw: s->err_withdraw = 1; return NULL; + +loop: + /* Loops are handled as withdraws, but ignored silently. Do not set err_withdraw. */ + return NULL; } void diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index 9d21fd34..21052186 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -1422,7 +1422,7 @@ bgp_decode_mpls_labels(struct bgp_parse_state *s, byte **pos, uint *len, uint *p /* RFC 8277 2.4 - withdraw does not have variable-size MPLS stack but fixed-size 24-bit Compatibility field, which MUST be ignored */ - if (!a && !s->err_withdraw) + if (!s->reach_nlri_step) return; } while (!(label & BGP_MPLS_BOS)); -- cgit v1.2.3 From 1f2eb2aca8e348fefc1822ec2adcad0cc97768d8 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Mon, 20 Dec 2021 20:25:35 +0100 Subject: BGP: Implement flowspec validation procedure Implement flowspec validation procedure as described in RFC 8955 sec. 6 and RFC 9117. The Validation procedure enforces that only routers in the forwarding path for a network can originate flowspec rules for that network. The patch adds new mechanism for tracking inter-table dependencies, which is necessary as the flowspec validation depends on IP routes, and flowspec rules must be revalidated when best IP routes change. The validation procedure is disabled by default and requires that relevant IP table uses trie, as it uses interval queries for subnets. --- doc/bird.sgml | 28 +++- nest/route.h | 15 +++ nest/rt-table.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++++--- proto/bgp/attrs.c | 4 + proto/bgp/bgp.c | 54 +++++++- proto/bgp/bgp.h | 6 +- proto/bgp/config.Y | 17 ++- proto/bgp/packets.c | 28 ++++ proto/pipe/pipe.c | 3 + 9 files changed, 487 insertions(+), 26 deletions(-) (limited to 'proto/bgp/attrs.c') diff --git a/doc/bird.sgml b/doc/bird.sgml index 39dadaf2..d1d2bdae 100644 --- a/doc/bird.sgml +++ b/doc/bird.sgml @@ -2274,6 +2274,7 @@ avoid routing loops. - BGP Large Communities Attribute - BGP Administrative Shutdown Communication - Default EBGP Route Propagation Behavior without Policies + - Revised Validation Procedure for BGP Flow Specifications Route selection rules @@ -2659,7 +2660,7 @@ using the following configuration parameters: