bgpd refactor aspath_match a bit

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

bgpd refactor aspath_match a bit

Claudio Jeker
For origin validation I chacked the source_as in struct rde_aspath this
is not really the right place. It should be in struct aspath since that
holds all the ASPATH related stuff. Change this, move aspath_match out of
util.c back into rde_attr.c and adjust code to use the cached value also
in match from any source-as XYZ rules.
This last bit causes a minor behavioural change since the old code
extracted the last non AS_SET asnumber. The new code follows the ROA RFC
and returns the rightmost AS for AS_SEQUENCE, the local AS for empty paths
and AS_NONE (which is 0) for everything else.
So now 'match from any source-as 0' will return all paths that do not have
a final AS_SEQUENCE segment.

The reason for this change is that I don't want to have two different
behaviours for what we call source-as (the one in roa-set and the one on a
filter).

Diff includes bgpctl change and is based from /usr/src
--
:wq Claudio

Index: usr.sbin/bgpctl/bgpctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.223
diff -u -p -r1.223 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c 1 Nov 2018 10:09:52 -0000 1.223
+++ usr.sbin/bgpctl/bgpctl.c 23 Nov 2018 14:41:53 -0000
@@ -97,6 +97,7 @@ void mrt_to_bgpd_addr(union mrt_addr *
 const char *msg_type(u_int8_t);
 void network_bulk(struct parse_result *);
 const char *print_auth_method(enum auth_method);
+int match_aspath(void *, u_int16_t, struct filter_as *);
 
 struct imsgbuf *ibuf;
 struct mrt_parser show_mrt = { show_mrt_dump, show_mrt_state, show_mrt_msg };
@@ -2080,8 +2081,7 @@ show_mrt_dump(struct mrt_rib *mr, struct
  }
  /* filter by AS */
  if (req->as.type != AS_UNDEF &&
-   !aspath_match(mre->aspath, mre->aspath_len,
-   &req->as, 0))
+   !match_aspath(mre->aspath, mre->aspath_len, &req->as))
  continue;
 
  if (req->flags & F_CTL_DETAIL) {
@@ -2146,8 +2146,7 @@ network_mrt_dump(struct mrt_rib *mr, str
  }
  /* filter by AS */
  if (req->as.type != AS_UNDEF &&
-   !aspath_match(mre->aspath, mre->aspath_len,
-   &req->as, 0))
+   !match_aspath(mre->aspath, mre->aspath_len, &req->as))
  continue;
 
  bzero(&net, sizeof(net));
@@ -2685,7 +2684,67 @@ msg_type(u_int8_t type)
 }
 
 int
-as_set_match(const struct as_set *a, u_int32_t asnum)
+match_aspath(void *data, u_int16_t len, struct filter_as *f)
 {
+ u_int8_t *seg;
+ int final;
+ u_int16_t seg_size;
+ u_int8_t i, seg_len;
+ u_int32_t as = 0;
+
+ if (f->type == AS_EMPTY) {
+ if (len == 0)
+ return (1);
+ else
+ return (0);
+ }
+
+ seg = data;
+
+ /* just check the leftmost AS */
+ if (f->type == AS_PEER && len >= 6) {
+ as = aspath_extract(seg, 0);
+ if (f->as_min == as)
+ return (1);
+ else
+ return (0);
+ }
+
+ for (; len >= 6; len -= seg_size, seg += seg_size) {
+ seg_len = seg[1];
+ seg_size = 2 + sizeof(u_int32_t) * seg_len;
+
+ final = (len == seg_size);
+
+ if (f->type == AS_SOURCE) {
+ /*
+ * Just extract the rightmost AS
+ * but if that segment is an AS_SET then the rightmost
+ * AS of a previous AS_SEQUENCE segment should be used.
+ * Because of that just look at AS_SEQUENCE segments.
+ */
+ if (seg[0] == AS_SEQUENCE)
+ as = aspath_extract(seg, seg_len - 1);
+ /* not yet in the final segment */
+ if (!final)
+ continue;
+ if (f->as_min == as)
+ return (1);
+ else
+ return (0);
+ }
+ /* AS_TRANSIT or AS_ALL */
+ for (i = 0; i < seg_len; i++) {
+ /*
+ * the source (rightmost) AS is excluded from
+ * AS_TRANSIT matches.
+ */
+ if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
+ return (0);
+ as = aspath_extract(seg, i);
+ if (f->as_min == as)
+ return (1);
+ }
+ }
  return (0);
 }
Index: usr.sbin/bgpd/bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.354
diff -u -p -r1.354 bgpd.h
--- usr.sbin/bgpd/bgpd.h 14 Nov 2018 14:03:36 -0000 1.354
+++ usr.sbin/bgpd/bgpd.h 23 Nov 2018 14:41:47 -0000
@@ -1262,7 +1262,6 @@ const char *log_shutcomm(const char *);
 int aspath_snprint(char *, size_t, void *, u_int16_t);
 int aspath_asprint(char **, void *, u_int16_t);
 size_t aspath_strlen(void *, u_int16_t);
-int aspath_match(void *, u_int16_t, struct filter_as *, u_int32_t);
 u_int32_t aspath_extract(const void *, int);
 int aspath_verify(void *, u_int16_t, int);
 #define AS_ERR_LEN -1
Index: usr.sbin/bgpd/rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.449
diff -u -p -r1.449 rde.c
--- usr.sbin/bgpd/rde.c 10 Nov 2018 11:19:01 -0000 1.449
+++ usr.sbin/bgpd/rde.c 23 Nov 2018 14:41:47 -0000
@@ -482,7 +482,6 @@ rde_dispatch_imsg_session(struct imsgbuf
  asp->origin = csr.origin;
  asp->flags |= F_PREFIX_ANNOUNCED | F_ANN_DYNAMIC;
  asp->aspath = aspath_get(asdata, csr.aspath_len);
- asp->source_as = aspath_origin(asp->aspath);
  netconf_s.asp = asp;
  break;
  case IMSG_NETWORK_ATTR:
@@ -1125,10 +1124,6 @@ rde_update_dispatch(struct imsg *imsg)
  }
  }
 
- if (state.aspath.flags & F_ATTR_ASPATH)
- state.aspath.source_as =
-    aspath_origin(state.aspath.aspath);
-
  rde_reflector(peer, &state.aspath);
  }
 
@@ -1393,7 +1388,7 @@ rde_update_update(struct rde_peer *peer,
 
  peer->prefix_rcvd_update++;
  vstate = rde_roa_validity(&conf->rde_roa, prefix, prefixlen,
-    in->aspath.source_as);
+    aspath_origin(in->aspath.aspath));
 
  /* add original path to the Adj-RIB-In */
  if (path_update(&ribs[RIB_ADJ_IN].rib, peer, in, prefix, prefixlen,
@@ -2225,8 +2220,7 @@ rde_dump_filter(struct prefix *p, struct
     (asp->flags & F_ATTR_PARSE_ERR) == 0)
  return;
  if (req->type == IMSG_CTL_SHOW_RIB_AS &&
-    !aspath_match(asp->aspath->data, asp->aspath->len,
-    &req->as, 0))
+    !aspath_match(asp->aspath, &req->as, 0))
  return;
  if (req->type == IMSG_CTL_SHOW_RIB_COMMUNITY &&
     !community_match(asp, req->community.as,
@@ -3090,7 +3084,7 @@ rde_softreconfig_in(struct rib_entry *re
  if (conf->rde_roa.dirty) {
  /* ROA validation state update */
  vstate = rde_roa_validity(&conf->rde_roa,
-    &prefix, pt->prefixlen, asp->source_as);
+    &prefix, pt->prefixlen, aspath_origin(asp->aspath));
  if (vstate != p->validation_state) {
  force_eval = 1;
  p->validation_state = vstate;
@@ -3644,7 +3638,6 @@ network_add(struct network_config *nc, i
  asp = path_get();
  asp->aspath = aspath_get(NULL, 0);
  asp->origin = ORIGIN_IGP;
- asp->source_as = aspath_origin(asp->aspath);
  asp->flags = F_ATTR_ORIGIN | F_ATTR_ASPATH |
     F_ATTR_LOCALPREF | F_PREFIX_ANNOUNCED;
  /* the nexthop is unset unless a default set overrides it */
@@ -3658,7 +3651,7 @@ network_add(struct network_config *nc, i
     peerself);
 
  vstate = rde_roa_validity(&conf->rde_roa, &nc->prefix,
-    nc->prefixlen, asp->source_as);
+    nc->prefixlen, aspath_origin(asp->aspath));
  if (path_update(&ribs[RIB_ADJ_IN].rib, peerself, &state, &nc->prefix,
     nc->prefixlen, vstate) == 1)
  peerself->prefix_cnt++;
Index: usr.sbin/bgpd/rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.202
diff -u -p -r1.202 rde.h
--- usr.sbin/bgpd/rde.h 4 Nov 2018 12:34:54 -0000 1.202
+++ usr.sbin/bgpd/rde.h 23 Nov 2018 14:41:47 -0000
@@ -125,6 +125,7 @@ struct rde_peer {
 
 struct aspath {
  LIST_ENTRY(aspath) entry;
+ u_int32_t source_as; /* cached source_as */
  int refcnt; /* reference count */
  u_int16_t len; /* total length of aspath in octets */
  u_int16_t ascnt; /* number of AS hops in data */
@@ -208,7 +209,6 @@ struct rde_aspath {
  struct aspath *aspath;
  u_int64_t hash;
  u_int32_t flags; /* internally used */
- u_int32_t source_as; /* cached source_as */
  u_int32_t med; /* multi exit disc */
  u_int32_t lpref; /* local pref */
  u_int32_t weight; /* low prio lpref */
@@ -354,11 +354,11 @@ u_char *aspath_deflate(u_char *, u_int1
 void aspath_merge(struct rde_aspath *, struct attr *);
 u_char *aspath_dump(struct aspath *);
 u_int16_t aspath_length(struct aspath *);
-u_int16_t aspath_count(const void *, u_int16_t);
 u_int32_t aspath_neighbor(struct aspath *);
 u_int32_t aspath_origin(struct aspath *);
 int aspath_loopfree(struct aspath *, u_int32_t);
 int aspath_compare(struct aspath *, struct aspath *);
+int aspath_match(struct aspath *, struct filter_as *, u_int32_t);
 u_char *aspath_prepend(struct aspath *, u_int32_t, int, u_int16_t *);
 int aspath_lenmatch(struct aspath *, enum aslen_spec, u_int);
 int community_match(struct rde_aspath *, int, int);
Index: usr.sbin/bgpd/rde_attr.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
retrieving revision 1.112
diff -u -p -r1.112 rde_attr.c
--- usr.sbin/bgpd/rde_attr.c 10 Oct 2018 06:21:47 -0000 1.112
+++ usr.sbin/bgpd/rde_attr.c 23 Nov 2018 14:41:47 -0000
@@ -441,6 +441,8 @@ attr_put(struct attr *a)
 
 /* aspath specific functions */
 
+static u_int16_t aspath_count(const void *, u_int16_t);
+static u_int32_t aspath_extract_origin(const void *, u_int16_t);
 static u_int16_t aspath_countlength(struct aspath *, u_int16_t, int);
 static void aspath_countcopy(struct aspath *, u_int16_t, u_int8_t *,
      u_int16_t, int);
@@ -530,6 +532,7 @@ aspath_get(void *data, u_int16_t len)
  aspath->refcnt = 0;
  aspath->len = len;
  aspath->ascnt = aspath_count(data, len);
+ aspath->source_as = aspath_extract_origin(data, len);
  memcpy(aspath->data, data, len);
 
  /* link */
@@ -667,7 +670,22 @@ aspath_length(struct aspath *aspath)
  return (aspath->len);
 }
 
-u_int16_t
+u_int32_t
+aspath_neighbor(struct aspath *aspath)
+{
+ /* Empty aspath is OK -- internal AS route. */
+ if (aspath->len == 0)
+ return (rde_local_as());
+ return (aspath_extract(aspath->data, 0));
+}
+
+u_int32_t
+aspath_origin(struct aspath *aspath)
+{
+ return aspath->source_as;
+}
+
+static u_int16_t
 aspath_count(const void *data, u_int16_t len)
 {
  const u_int8_t *seg;
@@ -692,6 +710,41 @@ aspath_count(const void *data, u_int16_t
  return (cnt);
 }
 
+/*
+ * The origin AS number derived from a Route as follows:
+ * o  the rightmost AS in the final segment of the AS_PATH attribute
+ *    in the Route if that segment is of type AS_SEQUENCE, or
+ * o  the BGP speaker's own AS number if that segment is of type
+ *    AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
+ * o  the distinguished value "NONE" if the final segment of the
+ *    AS_PATH attribute is of any other type.
+ */
+static u_int32_t
+aspath_extract_origin(const void *data, u_int16_t len)
+{
+ const u_int8_t *seg;
+ u_int32_t as = AS_NONE;
+ u_int16_t seg_size;
+ u_int8_t seg_len;
+
+ /* AS_PATH is empty */
+ if (len == 0)
+ return (rde_local_as());
+
+ seg = data;
+ for (; len > 0; len -= seg_size, seg += seg_size) {
+ seg_len = seg[1];
+ seg_size = 2 + sizeof(u_int32_t) * seg_len;
+
+ if (len == seg_size && seg[0] == AS_SEQUENCE) {
+ as = aspath_extract(seg, seg_len - 1);
+ }
+ if (seg_size > len)
+ fatalx("%s: would overflow", __func__);
+ }
+ return (as);
+}
+
 static u_int16_t
 aspath_countlength(struct aspath *aspath, u_int16_t cnt, int headcnt)
 {
@@ -771,50 +824,6 @@ aspath_countcopy(struct aspath *aspath,
  }
 }
 
-u_int32_t
-aspath_neighbor(struct aspath *aspath)
-{
- /* Empty aspath is OK -- internal AS route. */
- if (aspath->len == 0)
- return (rde_local_as());
- return (aspath_extract(aspath->data, 0));
-}
-
-/*
- * The origin AS number derived from a Route as follows:
- * o  the rightmost AS in the final segment of the AS_PATH attribute
- *    in the Route if that segment is of type AS_SEQUENCE, or
- * o  the BGP speaker's own AS number if that segment is of type
- *    AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
- * o  the distinguished value "NONE" if the final segment of the
- *   AS_PATH attribute is of any other type.
- */
-u_int32_t
-aspath_origin(struct aspath *aspath)
-{
- u_int8_t *seg;
- u_int32_t as = AS_NONE;
- u_int16_t len, seg_size;
- u_int8_t seg_len;
-
- /* AS_PATH is empty */
- if (aspath->len == 0)
- return (rde_local_as());
-
- seg = aspath->data;
- for (len = aspath->len; len > 0; len -= seg_size, seg += seg_size) {
- seg_len = seg[1];
- seg_size = 2 + sizeof(u_int32_t) * seg_len;
-
- if (len == seg_size && seg[0] == AS_SEQUENCE) {
- as = aspath_extract(seg, seg_len - 1);
- }
- if (seg_size > len)
- fatalx("%s: would overflow", __func__);
- }
- return (as);
-}
-
 int
 aspath_loopfree(struct aspath *aspath, u_int32_t myAS)
 {
@@ -872,6 +881,102 @@ aspath_lookup(const void *data, u_int16_
  return (NULL);
 }
 
+
+static int
+as_compare(struct filter_as *f, u_int32_t as, u_int32_t neighas)
+{
+ u_int32_t match;
+
+ if (f->flags & AS_FLAG_AS_SET_NAME) /* should not happen */
+ return (0);
+ if (f->flags & AS_FLAG_AS_SET)
+ return (as_set_match(f->aset, as));
+
+ if (f->flags & AS_FLAG_NEIGHBORAS)
+ match = neighas;
+ else
+ match = f->as_min;
+
+ switch (f->op) {
+ case OP_NONE:
+ case OP_EQ:
+ if (as == match)
+ return (1);
+ break;
+ case OP_NE:
+ if (as != match)
+ return (1);
+ break;
+ case OP_RANGE:
+ if (as >= f->as_min && as <= f->as_max)
+ return (1);
+ break;
+ case OP_XRANGE:
+ if (as < f->as_min || as > f->as_max)
+ return (1);
+ break;
+ }
+ return (0);
+}
+
+/* we need to be able to search more than one as */
+int
+aspath_match(struct aspath *aspath, struct filter_as *f, u_int32_t neighas)
+{
+ const u_int8_t *seg;
+ int final;
+ u_int16_t len, seg_size;
+ u_int8_t i, seg_len;
+ u_int32_t as = 0;
+
+ if (f->type == AS_EMPTY) {
+ if (aspath_length(aspath) == 0)
+ return (1);
+ else
+ return (0);
+ }
+
+ /* just check the leftmost AS */
+ if (f->type == AS_PEER) {
+ as = aspath_neighbor(aspath);
+ if (as_compare(f, as, neighas))
+ return (1);
+ else
+ return (0);
+ }
+
+ /* use the cached source_as as origin */
+ if (f->type == AS_SOURCE) {
+ as = aspath_origin(aspath);
+ if (as_compare(f, as, neighas))
+ return (1);
+ else
+ return (0);
+ }
+
+ seg = aspath->data;
+ len = aspath->len;
+ for (; len >= 6; len -= seg_size, seg += seg_size) {
+ seg_len = seg[1];
+ seg_size = 2 + sizeof(u_int32_t) * seg_len;
+
+ final = (len == seg_size);
+
+ /* AS_TRANSIT or AS_ALL */
+ for (i = 0; i < seg_len; i++) {
+ /*
+ * the source (rightmost) AS is excluded from
+ * AS_TRANSIT matches.
+ */
+ if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
+ return (0);
+ as = aspath_extract(seg, i);
+ if (as_compare(f, as, neighas))
+ return (1);
+ }
+ }
+ return (0);
+}
 
 /*
  * Returns a new prepended aspath. Old needs to be freed by caller.
Index: usr.sbin/bgpd/rde_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
retrieving revision 1.113
diff -u -p -r1.113 rde_filter.c
--- usr.sbin/bgpd/rde_filter.c 14 Nov 2018 14:03:36 -0000 1.113
+++ usr.sbin/bgpd/rde_filter.c 23 Nov 2018 14:41:47 -0000
@@ -366,8 +366,8 @@ rde_filter_match(struct filter_rule *f,
  }
 
  if (asp != NULL && f->match.as.type != AS_UNDEF) {
- if (aspath_match(asp->aspath->data, asp->aspath->len,
-    &f->match.as, peer->conf.remote_as) == 0)
+ if (aspath_match(asp->aspath, &f->match.as,
+    peer->conf.remote_as) == 0)
  return (0);
  }
 
@@ -497,7 +497,7 @@ rde_filter_match(struct filter_rule *f,
  pt_getaddr(p->re->prefix, prefix);
  plen = p->re->prefix->prefixlen;
  if (trie_roa_check(&f->match.originset.ps->th, prefix, plen,
-    asp->source_as) != ROA_VALID)
+    aspath_origin(asp->aspath)) != ROA_VALID)
  return (0);
  }
 
Index: usr.sbin/bgpd/rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.186
diff -u -p -r1.186 rde_rib.c
--- usr.sbin/bgpd/rde_rib.c 14 Nov 2018 12:14:41 -0000 1.186
+++ usr.sbin/bgpd/rde_rib.c 23 Nov 2018 14:41:47 -0000
@@ -657,10 +657,6 @@ path_compare(struct rde_aspath *a, struc
  return (1);
  if (a->pftableid < b->pftableid)
  return (-1);
- if (a->source_as > b->source_as)
- return (1);
- if (a->source_as < b->source_as)
- return (-1);
 
  r = aspath_compare(a->aspath, b->aspath);
  if (r > 0)
@@ -761,7 +757,6 @@ path_copy(struct rde_aspath *dst, const
  dst->lpref = src->lpref;
  dst->weight = src->weight;
  dst->origin = src->origin;
- dst->source_as = src->source_as;
  dst->rtlabelid = rtlabel_ref(src->rtlabelid);
  dst->pftableid = pftable_ref(src->pftableid);
 
Index: usr.sbin/bgpd/util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
retrieving revision 1.40
diff -u -p -r1.40 util.c
--- usr.sbin/bgpd/util.c 26 Sep 2018 14:38:19 -0000 1.40
+++ usr.sbin/bgpd/util.c 23 Nov 2018 14:41:47 -0000
@@ -312,110 +312,6 @@ aspath_strlen(void *data, u_int16_t len)
  return (total_size);
 }
 
-static int
-as_compare(struct filter_as *f, u_int32_t as, u_int32_t neighas)
-{
- u_int32_t match;
-
- if (f->flags & AS_FLAG_AS_SET_NAME) /* should not happen */
- return (0);
- if (f->flags & AS_FLAG_AS_SET)
- return (as_set_match(f->aset, as));
-
- if (f->flags & AS_FLAG_NEIGHBORAS)
- match = neighas;
- else
- match = f->as_min;
-
- switch (f->op) {
- case OP_NONE:
- case OP_EQ:
- if (as == match)
- return (1);
- break;
- case OP_NE:
- if (as != match)
- return (1);
- break;
- case OP_RANGE:
- if (as >= f->as_min && as <= f->as_max)
- return (1);
- break;
- case OP_XRANGE:
- if (as < f->as_min || as > f->as_max)
- return (1);
- break;
- }
- return (0);
-}
-
-/* we need to be able to search more than one as */
-int
-aspath_match(void *data, u_int16_t len, struct filter_as *f, u_int32_t neighas)
-{
- u_int8_t *seg;
- int final;
- u_int16_t seg_size;
- u_int8_t i, seg_len;
- u_int32_t as = 0;
-
- if (f->type == AS_EMPTY) {
- if (len == 0)
- return (1);
- else
- return (0);
- }
-
- seg = data;
-
- /* just check the leftmost AS */
- if (f->type == AS_PEER && len >= 6) {
- as = aspath_extract(seg, 0);
- if (as_compare(f, as, neighas))
- return (1);
- else
- return (0);
- }
-
- for (; len >= 6; len -= seg_size, seg += seg_size) {
- seg_len = seg[1];
- seg_size = 2 + sizeof(u_int32_t) * seg_len;
-
- final = (len == seg_size);
-
- if (f->type == AS_SOURCE) {
- /*
- * Just extract the rightmost AS
- * but if that segment is an AS_SET then the rightmost
- * AS of a previous AS_SEQUENCE segment should be used.
- * Because of that just look at AS_SEQUENCE segments.
- */
- if (seg[0] == AS_SEQUENCE)
- as = aspath_extract(seg, seg_len - 1);
- /* not yet in the final segment */
- if (!final)
- continue;
- if (as_compare(f, as, neighas))
- return (1);
- else
- return (0);
- }
- /* AS_TRANSIT or AS_ALL */
- for (i = 0; i < seg_len; i++) {
- /*
- * the source (rightmost) AS is excluded from
- * AS_TRANSIT matches.
- */
- if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
- return (0);
- as = aspath_extract(seg, i);
- if (as_compare(f, as, neighas))
- return (1);
- }
- }
- return (0);
-}
-
 /*
  * Extract the asnum out of the as segment at the specified position.
  * Direct access is not possible because of non-aligned reads.

Reply | Threaded
Open this post in threaded view
|

Re: bgpd refactor aspath_match a bit

Job Snijders
Hi Claudio,

On Fri, Nov 23, 2018 at 03:55:18PM +0100, Claudio Jeker wrote:

> For origin validation I chacked the source_as in struct rde_aspath
> this is not really the right place. It should be in struct aspath
> since that holds all the ASPATH related stuff. Change this, move
> aspath_match out of util.c back into rde_attr.c and adjust code to use
> the cached value also in match from any source-as XYZ rules.
> This last bit causes a minor behavioural change since the old code
> extracted the last non AS_SET asnumber. The new code follows the ROA
> RFC and returns the rightmost AS for AS_SEQUENCE, the local AS for
> empty paths and AS_NONE (which is 0) for everything else.
> So now 'match from any source-as 0' will return all paths that do not
> have a final AS_SEQUENCE segment.
>
> The reason for this change is that I don't want to have two different
> behaviours for what we call source-as (the one in roa-set and the one on a
> filter).

Something is off, it seems 'source-as 0' is matching anything that has
an AS_SET attribute set:

    $ bgpctl show rib source-as 0 | head
    flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
           S = Stale, E = Error
    origin validation state: N = not-found, V = valid, ! = invalid
    origin: i = IGP, e = EGP, ? = Incomplete

    flags ovs destination          gateway          lpref   med aspath origin
    I*>     N 5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?
    I*>     N 5.101.110.0/24       192.147.168.1      100     0 2914 14061 { 46652 } i
    I*>     N 5.175.0.0/19         192.147.168.1      100     0 2914 1299 20773 { 8972 } i
    I*>     N 8.41.202.0/24        192.147.168.1      100     0 2914 13789 30372 { 40179 } i

Similarly, this should return at least 5.39.176.0/21:

    $ bgpctl show rib source-as 8530
    flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
           S = Stale, E = Error
    origin validation state: N = not-found, V = valid, ! = invalid
    origin: i = IGP, e = EGP, ? = Incomplete

    flags ovs destination          gateway          lpref   med aspath origin
    I*>     N 80.87.16.0/20        192.147.168.1      100     0 2914 8530 ?
    I*>     N 87.236.128.0/21      192.147.168.1      100     0 2914 8530 ?
    I*>     N 88.151.152.0/21      192.147.168.1      100     0 2914 8530 ?
    I*>     N 89.38.120.0/21       192.147.168.1      100     0 2914 8530 i
    I*>     N 93.115.176.0/20      192.147.168.1      100     0 2914 8530 i
    I*>     N 185.52.144.0/22      192.147.168.1      100     0 2914 8530 ?

Kind regards,

Job

Reply | Threaded
Open this post in threaded view
|

Re: bgpd refactor aspath_match a bit

Claudio Jeker
On Tue, Nov 27, 2018 at 04:21:53PM +0100, Job Snijders wrote:

> Hi Claudio,
>
> On Fri, Nov 23, 2018 at 03:55:18PM +0100, Claudio Jeker wrote:
> > For origin validation I chacked the source_as in struct rde_aspath
> > this is not really the right place. It should be in struct aspath
> > since that holds all the ASPATH related stuff. Change this, move
> > aspath_match out of util.c back into rde_attr.c and adjust code to use
> > the cached value also in match from any source-as XYZ rules.
> > This last bit causes a minor behavioural change since the old code
> > extracted the last non AS_SET asnumber. The new code follows the ROA
> > RFC and returns the rightmost AS for AS_SEQUENCE, the local AS for
> > empty paths and AS_NONE (which is 0) for everything else.
> > So now 'match from any source-as 0' will return all paths that do not
> > have a final AS_SEQUENCE segment.
> >
> > The reason for this change is that I don't want to have two different
> > behaviours for what we call source-as (the one in roa-set and the one on a
> > filter).
>
> Something is off, it seems 'source-as 0' is matching anything that has
> an AS_SET attribute set:
>
>     $ bgpctl show rib source-as 0 | head
>     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
>            S = Stale, E = Error
>     origin validation state: N = not-found, V = valid, ! = invalid
>     origin: i = IGP, e = EGP, ? = Incomplete
>
>     flags ovs destination          gateway          lpref   med aspath origin
>     I*>     N 5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?
>     I*>     N 5.101.110.0/24       192.147.168.1      100     0 2914 14061 { 46652 } i
>     I*>     N 5.175.0.0/19         192.147.168.1      100     0 2914 1299 20773 { 8972 } i
>     I*>     N 8.41.202.0/24        192.147.168.1      100     0 2914 13789 30372 { 40179 } i
>
> Similarly, this should return at least 5.39.176.0/21:
>
>     $ bgpctl show rib source-as 8530
>     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
>            S = Stale, E = Error
>     origin validation state: N = not-found, V = valid, ! = invalid
>     origin: i = IGP, e = EGP, ? = Incomplete
>
>     flags ovs destination          gateway          lpref   med aspath origin
>     I*>     N 80.87.16.0/20        192.147.168.1      100     0 2914 8530 ?
>     I*>     N 87.236.128.0/21      192.147.168.1      100     0 2914 8530 ?
>     I*>     N 88.151.152.0/21      192.147.168.1      100     0 2914 8530 ?
>     I*>     N 89.38.120.0/21       192.147.168.1      100     0 2914 8530 i
>     I*>     N 93.115.176.0/20      192.147.168.1      100     0 2914 8530 i
>     I*>     N 185.52.144.0/22      192.147.168.1      100     0 2914 8530 ?
>

I implemented source-as the way ROA is defining it. So anything which ends
with a AS_SET will return AS_NONE (which is 0). OpenBGPD has no way to
have an AS_PATH that has a real 0 in the AS_PATH (those UPDATES are
treated as withdraw). Because of this also the 5.39.176.0/21 is no longer
matching in 'bgpctl show rib source-as 8530'.

I'm a bit on the edge here about where to go and currently prefer to
follow a RFC (which in this case is RFC6811).

 o  Route Origin ASN: The origin AS number derived from a Route as
    follows:

    *  the rightmost AS in the final segment of the AS_PATH attribute
         in the Route if that segment is of type AS_SEQUENCE, or

    *  the BGP speaker's own AS number if that segment is of type
       AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
       or

    *  the distinguished value "NONE" if the final segment of the
       AS_PATH attribute is of any other type.

As mentioned above I found it strange when behaviour is different because
of where it is used.

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: bgpd refactor aspath_match a bit

Job Snijders
On Tue, Nov 27, 2018 at 06:23:53PM +0100, Claudio Jeker wrote:

> On Tue, Nov 27, 2018 at 04:21:53PM +0100, Job Snijders wrote:
> > On Fri, Nov 23, 2018 at 03:55:18PM +0100, Claudio Jeker wrote:
> > > For origin validation I chacked the source_as in struct rde_aspath
> > > this is not really the right place. It should be in struct aspath
> > > since that holds all the ASPATH related stuff. Change this, move
> > > aspath_match out of util.c back into rde_attr.c and adjust code to use
> > > the cached value also in match from any source-as XYZ rules.
> > > This last bit causes a minor behavioural change since the old code
> > > extracted the last non AS_SET asnumber. The new code follows the ROA
> > > RFC and returns the rightmost AS for AS_SEQUENCE, the local AS for
> > > empty paths and AS_NONE (which is 0) for everything else.
> > > So now 'match from any source-as 0' will return all paths that do not
> > > have a final AS_SEQUENCE segment.
> > >
> > > The reason for this change is that I don't want to have two different
> > > behaviours for what we call source-as (the one in roa-set and the one on a
> > > filter).
> >
> > Something is off, it seems 'source-as 0' is matching anything that has
> > an AS_SET attribute set:
> >
> >     $ bgpctl show rib source-as 0 | head
> >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> >            S = Stale, E = Error
> >     origin validation state: N = not-found, V = valid, ! = invalid
> >     origin: i = IGP, e = EGP, ? = Incomplete
> >
> >     flags ovs destination          gateway          lpref   med aspath origin
> >     I*>     N 5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?
> >     I*>     N 5.101.110.0/24       192.147.168.1      100     0 2914 14061 { 46652 } i
> >     I*>     N 5.175.0.0/19         192.147.168.1      100     0 2914 1299 20773 { 8972 } i
> >     I*>     N 8.41.202.0/24        192.147.168.1      100     0 2914 13789 30372 { 40179 } i
> >
> > Similarly, this should return at least 5.39.176.0/21:
> >
> >     $ bgpctl show rib source-as 8530
> >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> >            S = Stale, E = Error
> >     origin validation state: N = not-found, V = valid, ! = invalid
> >     origin: i = IGP, e = EGP, ? = Incomplete
> >
> >     flags ovs destination          gateway          lpref   med aspath origin
> >     I*>     N 80.87.16.0/20        192.147.168.1      100     0 2914 8530 ?
> >     I*>     N 87.236.128.0/21      192.147.168.1      100     0 2914 8530 ?
> >     I*>     N 88.151.152.0/21      192.147.168.1      100     0 2914 8530 ?
> >     I*>     N 89.38.120.0/21       192.147.168.1      100     0 2914 8530 i
> >     I*>     N 93.115.176.0/20      192.147.168.1      100     0 2914 8530 i
> >     I*>     N 185.52.144.0/22      192.147.168.1      100     0 2914 8530 ?
> >
>
> I implemented source-as the way ROA is defining it. So anything which ends
> with a AS_SET will return AS_NONE (which is 0). OpenBGPD has no way to
> have an AS_PATH that has a real 0 in the AS_PATH (those UPDATES are
> treated as withdraw). Because of this also the 5.39.176.0/21 is no longer
> matching in 'bgpctl show rib source-as 8530'.

I'm not sure it should behave that way.

'bgpctl show rib source-as 8530' really ought to return prefixes like
80.87.16.0/20 but also 5.39.176.0/21.

> I'm a bit on the edge here about where to go and currently prefer to
> follow a RFC (which in this case is RFC6811).
>
>  o  Route Origin ASN: The origin AS number derived from a Route as
>     follows:
>
>     *  the rightmost AS in the final segment of the AS_PATH attribute
>          in the Route if that segment is of type AS_SEQUENCE, or
>
>     *  the BGP speaker's own AS number if that segment is of type
>        AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
>        or
>
>     *  the distinguished value "NONE" if the final segment of the
>        AS_PATH attribute is of any other type.
>
> As mentioned above I found it strange when behaviour is different because
> of where it is used.

RFC 6811 describes how BGP Origin Validation should be performed, but is
not a guideline how to treat AS_SETs in general or how CLIs should
function. Perhaps 'source-as 0' should just throw an error in all
contexts (both bgpd.conf and bgpctl), since 0 can never be in an AS_SET
or AS_SEQUENCE. Perhaps we shouldn't overload the number.

1/ Maybe 'source-as self' or 'source-as none' can be used to match all
routes originated by the AS in which this bgpd instance runs.

2/ Similarly, the program could be made so that 'AS 15562' (lets use
15562 as example) from the Global Configuration passed on to bgpctl and
used in the filter ruleset means both "what is originated by 15562" and
also what is originated in the own AS (and doesn't have AS_PATH yet, but
we know the router runs in 15562 because of the global config parameter).

Kind regards,

Job

Reply | Threaded
Open this post in threaded view
|

Re: bgpd refactor aspath_match a bit

Claudio Jeker
On Tue, Nov 27, 2018 at 06:55:51PM +0100, Job Snijders wrote:

> On Tue, Nov 27, 2018 at 06:23:53PM +0100, Claudio Jeker wrote:
> > On Tue, Nov 27, 2018 at 04:21:53PM +0100, Job Snijders wrote:
> > > On Fri, Nov 23, 2018 at 03:55:18PM +0100, Claudio Jeker wrote:
> > > > For origin validation I chacked the source_as in struct rde_aspath
> > > > this is not really the right place. It should be in struct aspath
> > > > since that holds all the ASPATH related stuff. Change this, move
> > > > aspath_match out of util.c back into rde_attr.c and adjust code to use
> > > > the cached value also in match from any source-as XYZ rules.
> > > > This last bit causes a minor behavioural change since the old code
> > > > extracted the last non AS_SET asnumber. The new code follows the ROA
> > > > RFC and returns the rightmost AS for AS_SEQUENCE, the local AS for
> > > > empty paths and AS_NONE (which is 0) for everything else.
> > > > So now 'match from any source-as 0' will return all paths that do not
> > > > have a final AS_SEQUENCE segment.
> > > >
> > > > The reason for this change is that I don't want to have two different
> > > > behaviours for what we call source-as (the one in roa-set and the one on a
> > > > filter).
> > >
> > > Something is off, it seems 'source-as 0' is matching anything that has
> > > an AS_SET attribute set:
> > >
> > >     $ bgpctl show rib source-as 0 | head
> > >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> > >            S = Stale, E = Error
> > >     origin validation state: N = not-found, V = valid, ! = invalid
> > >     origin: i = IGP, e = EGP, ? = Incomplete
> > >
> > >     flags ovs destination          gateway          lpref   med aspath origin
> > >     I*>     N 5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?
> > >     I*>     N 5.101.110.0/24       192.147.168.1      100     0 2914 14061 { 46652 } i
> > >     I*>     N 5.175.0.0/19         192.147.168.1      100     0 2914 1299 20773 { 8972 } i
> > >     I*>     N 8.41.202.0/24        192.147.168.1      100     0 2914 13789 30372 { 40179 } i
> > >
> > > Similarly, this should return at least 5.39.176.0/21:
> > >
> > >     $ bgpctl show rib source-as 8530
> > >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> > >            S = Stale, E = Error
> > >     origin validation state: N = not-found, V = valid, ! = invalid
> > >     origin: i = IGP, e = EGP, ? = Incomplete
> > >
> > >     flags ovs destination          gateway          lpref   med aspath origin
> > >     I*>     N 80.87.16.0/20        192.147.168.1      100     0 2914 8530 ?
> > >     I*>     N 87.236.128.0/21      192.147.168.1      100     0 2914 8530 ?
> > >     I*>     N 88.151.152.0/21      192.147.168.1      100     0 2914 8530 ?
> > >     I*>     N 89.38.120.0/21       192.147.168.1      100     0 2914 8530 i
> > >     I*>     N 93.115.176.0/20      192.147.168.1      100     0 2914 8530 i
> > >     I*>     N 185.52.144.0/22      192.147.168.1      100     0 2914 8530 ?
> > >
> >
> > I implemented source-as the way ROA is defining it. So anything which ends
> > with a AS_SET will return AS_NONE (which is 0). OpenBGPD has no way to
> > have an AS_PATH that has a real 0 in the AS_PATH (those UPDATES are
> > treated as withdraw). Because of this also the 5.39.176.0/21 is no longer
> > matching in 'bgpctl show rib source-as 8530'.
>
> I'm not sure it should behave that way.
>
> 'bgpctl show rib source-as 8530' really ought to return prefixes like
> 80.87.16.0/20 but also 5.39.176.0/21.

But isn't this different from other implementations? At least I would
expect that the AS-path regex '8530$' would not match on the AS_SET path
either. My issue is that we have 'source-as' in roa-set, origin-set and on
filters in bgpd.conf plus the source-as used by bgpctl. Depending on
context they behave differently. So if AS 8530 is in the roa-set
and I do bgpctl show rib source-as 8530 the result will be different to
what would match in the roa-set.
We already had a lot of confusion about announce and that is why I decided
to make them behave the same.
 

> > I'm a bit on the edge here about where to go and currently prefer to
> > follow a RFC (which in this case is RFC6811).
> >
> >  o  Route Origin ASN: The origin AS number derived from a Route as
> >     follows:
> >
> >     *  the rightmost AS in the final segment of the AS_PATH attribute
> >          in the Route if that segment is of type AS_SEQUENCE, or
> >
> >     *  the BGP speaker's own AS number if that segment is of type
> >        AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
> >        or
> >
> >     *  the distinguished value "NONE" if the final segment of the
> >        AS_PATH attribute is of any other type.
> >
> > As mentioned above I found it strange when behaviour is different because
> > of where it is used.
>
> RFC 6811 describes how BGP Origin Validation should be performed, but is
> not a guideline how to treat AS_SETs in general or how CLIs should
> function. Perhaps 'source-as 0' should just throw an error in all
> contexts (both bgpd.conf and bgpctl), since 0 can never be in an AS_SET
> or AS_SEQUENCE. Perhaps we shouldn't overload the number.

The overloading is done to simplify some internals. It is indeed possible
to disallow direct use of 0 in bgpctl and replace it with 'none'. On the
other hand it seems people are used to write belt&suspenders like configs
where filter rules (e.g. deny form any AS 0) are added which are not needed.
 
> 1/ Maybe 'source-as self' or 'source-as none' can be used to match all
> routes originated by the AS in which this bgpd instance runs.

'source-as self' and 'source-as none' are two different things in my
opinion. The empty AS path case is convered by 'empty-as' in the filters
and introducing 'source-as none' with a different meaning then the one
used by RFC6811 will not imporve anything.
 
> 2/ Similarly, the program could be made so that 'AS 15562' (lets use
> 15562 as example) from the Global Configuration passed on to bgpctl and
> used in the filter ruleset means both "what is originated by 15562" and
> also what is originated in the own AS (and doesn't have AS_PATH yet, but
> we know the router runs in 15562 because of the global config parameter).

This is the behaviour with this diff:

bgpctl show rib source-as 196618
flags ovs destination          gateway          lpref   med aspath origin
AI*>    N 10.12.12.0/24        0.0.0.0            100     0 i
AI*>    N 10.12.13.0/24        0.0.0.0            100     0 i

196618 is the AS of this instance.


I see two different discussion points here. Or two distinct behaviour
changes that get introduced by this diff:
1/ source-as behaviour for empty AS_PATH. With this diff 'source-as $myAS'
will match for empty AS_PATH. In my opinion this makes sense but the
interaction with empty-as should be reconsidered. (e.g. are people
currently using both forms to achieve different behaviour in their own
AS).

2/ source-as handling of AS_PATH that end with an AS_SET (bgpd currently
does not support any other segment type). Here there is two things:
 a) should 'source-as 8530' match for the path with the AS_SET e.g:
       2914 8530 { 198753 }
    Should we really consider AS 8530 being the source of that prefix?
    What about 198753?

 b) handling of 'source-as 0' ideally this should be disallowed and
    instead 'source-as none' introduced. Which would then collect all
    the AS_SET paths from above. b) depends on the outcome of a)

As mentioned above the idea of making source-as behave like the source-as
in roa-set for consitency is my reason but if people think the current
behaviour has to be preserved (with the behaviour difference between
source-as usage) then that is fine and I will adjust the diff.

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: bgpd refactor aspath_match a bit

Claudio Jeker
On Wed, Nov 28, 2018 at 10:35:37AM +0100, Claudio Jeker wrote:

> On Tue, Nov 27, 2018 at 06:55:51PM +0100, Job Snijders wrote:
> > On Tue, Nov 27, 2018 at 06:23:53PM +0100, Claudio Jeker wrote:
> > > On Tue, Nov 27, 2018 at 04:21:53PM +0100, Job Snijders wrote:
> > > > On Fri, Nov 23, 2018 at 03:55:18PM +0100, Claudio Jeker wrote:
> > > > > For origin validation I chacked the source_as in struct rde_aspath
> > > > > this is not really the right place. It should be in struct aspath
> > > > > since that holds all the ASPATH related stuff. Change this, move
> > > > > aspath_match out of util.c back into rde_attr.c and adjust code to use
> > > > > the cached value also in match from any source-as XYZ rules.
> > > > > This last bit causes a minor behavioural change since the old code
> > > > > extracted the last non AS_SET asnumber. The new code follows the ROA
> > > > > RFC and returns the rightmost AS for AS_SEQUENCE, the local AS for
> > > > > empty paths and AS_NONE (which is 0) for everything else.
> > > > > So now 'match from any source-as 0' will return all paths that do not
> > > > > have a final AS_SEQUENCE segment.
> > > > >
> > > > > The reason for this change is that I don't want to have two different
> > > > > behaviours for what we call source-as (the one in roa-set and the one on a
> > > > > filter).
> > > >
> > > > Something is off, it seems 'source-as 0' is matching anything that has
> > > > an AS_SET attribute set:
> > > >
> > > >     $ bgpctl show rib source-as 0 | head
> > > >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> > > >            S = Stale, E = Error
> > > >     origin validation state: N = not-found, V = valid, ! = invalid
> > > >     origin: i = IGP, e = EGP, ? = Incomplete
> > > >
> > > >     flags ovs destination          gateway          lpref   med aspath origin
> > > >     I*>     N 5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?
> > > >     I*>     N 5.101.110.0/24       192.147.168.1      100     0 2914 14061 { 46652 } i
> > > >     I*>     N 5.175.0.0/19         192.147.168.1      100     0 2914 1299 20773 { 8972 } i
> > > >     I*>     N 8.41.202.0/24        192.147.168.1      100     0 2914 13789 30372 { 40179 } i
> > > >
> > > > Similarly, this should return at least 5.39.176.0/21:
> > > >
> > > >     $ bgpctl show rib source-as 8530
> > > >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> > > >            S = Stale, E = Error
> > > >     origin validation state: N = not-found, V = valid, ! = invalid
> > > >     origin: i = IGP, e = EGP, ? = Incomplete
> > > >
> > > >     flags ovs destination          gateway          lpref   med aspath origin
> > > >     I*>     N 80.87.16.0/20        192.147.168.1      100     0 2914 8530 ?
> > > >     I*>     N 87.236.128.0/21      192.147.168.1      100     0 2914 8530 ?
> > > >     I*>     N 88.151.152.0/21      192.147.168.1      100     0 2914 8530 ?
> > > >     I*>     N 89.38.120.0/21       192.147.168.1      100     0 2914 8530 i
> > > >     I*>     N 93.115.176.0/20      192.147.168.1      100     0 2914 8530 i
> > > >     I*>     N 185.52.144.0/22      192.147.168.1      100     0 2914 8530 ?
> > > >
> > >
> > > I implemented source-as the way ROA is defining it. So anything which ends
> > > with a AS_SET will return AS_NONE (which is 0). OpenBGPD has no way to
> > > have an AS_PATH that has a real 0 in the AS_PATH (those UPDATES are
> > > treated as withdraw). Because of this also the 5.39.176.0/21 is no longer
> > > matching in 'bgpctl show rib source-as 8530'.
> >
> > I'm not sure it should behave that way.
> >
> > 'bgpctl show rib source-as 8530' really ought to return prefixes like
> > 80.87.16.0/20 but also 5.39.176.0/21.
>
> But isn't this different from other implementations? At least I would
> expect that the AS-path regex '8530$' would not match on the AS_SET path
> either. My issue is that we have 'source-as' in roa-set, origin-set and on
> filters in bgpd.conf plus the source-as used by bgpctl. Depending on
> context they behave differently. So if AS 8530 is in the roa-set
> and I do bgpctl show rib source-as 8530 the result will be different to
> what would match in the roa-set.
> We already had a lot of confusion about announce and that is why I decided
> to make them behave the same.
>  
> > > I'm a bit on the edge here about where to go and currently prefer to
> > > follow a RFC (which in this case is RFC6811).
> > >
> > >  o  Route Origin ASN: The origin AS number derived from a Route as
> > >     follows:
> > >
> > >     *  the rightmost AS in the final segment of the AS_PATH attribute
> > >          in the Route if that segment is of type AS_SEQUENCE, or
> > >
> > >     *  the BGP speaker's own AS number if that segment is of type
> > >        AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
> > >        or
> > >
> > >     *  the distinguished value "NONE" if the final segment of the
> > >        AS_PATH attribute is of any other type.
> > >
> > > As mentioned above I found it strange when behaviour is different because
> > > of where it is used.
> >
> > RFC 6811 describes how BGP Origin Validation should be performed, but is
> > not a guideline how to treat AS_SETs in general or how CLIs should
> > function. Perhaps 'source-as 0' should just throw an error in all
> > contexts (both bgpd.conf and bgpctl), since 0 can never be in an AS_SET
> > or AS_SEQUENCE. Perhaps we shouldn't overload the number.
>
> The overloading is done to simplify some internals. It is indeed possible
> to disallow direct use of 0 in bgpctl and replace it with 'none'. On the
> other hand it seems people are used to write belt&suspenders like configs
> where filter rules (e.g. deny form any AS 0) are added which are not needed.
>  
> > 1/ Maybe 'source-as self' or 'source-as none' can be used to match all
> > routes originated by the AS in which this bgpd instance runs.
>
> 'source-as self' and 'source-as none' are two different things in my
> opinion. The empty AS path case is convered by 'empty-as' in the filters
> and introducing 'source-as none' with a different meaning then the one
> used by RFC6811 will not imporve anything.
>  
> > 2/ Similarly, the program could be made so that 'AS 15562' (lets use
> > 15562 as example) from the Global Configuration passed on to bgpctl and
> > used in the filter ruleset means both "what is originated by 15562" and
> > also what is originated in the own AS (and doesn't have AS_PATH yet, but
> > we know the router runs in 15562 because of the global config parameter).
>
> This is the behaviour with this diff:
>
> bgpctl show rib source-as 196618
> flags ovs destination          gateway          lpref   med aspath origin
> AI*>    N 10.12.12.0/24        0.0.0.0            100     0 i
> AI*>    N 10.12.13.0/24        0.0.0.0            100     0 i
>
> 196618 is the AS of this instance.
>
>
> I see two different discussion points here. Or two distinct behaviour
> changes that get introduced by this diff:
> 1/ source-as behaviour for empty AS_PATH. With this diff 'source-as $myAS'
> will match for empty AS_PATH. In my opinion this makes sense but the
> interaction with empty-as should be reconsidered. (e.g. are people
> currently using both forms to achieve different behaviour in their own
> AS).
>
> 2/ source-as handling of AS_PATH that end with an AS_SET (bgpd currently
> does not support any other segment type). Here there is two things:
>  a) should 'source-as 8530' match for the path with the AS_SET e.g:
>        2914 8530 { 198753 }
>     Should we really consider AS 8530 being the source of that prefix?
>     What about 198753?
>
>  b) handling of 'source-as 0' ideally this should be disallowed and
>     instead 'source-as none' introduced. Which would then collect all
>     the AS_SET paths from above. b) depends on the outcome of a)
>
> As mentioned above the idea of making source-as behave like the source-as
> in roa-set for consitency is my reason but if people think the current
> behaviour has to be preserved (with the behaviour difference between
> source-as usage) then that is fine and I will adjust the diff.
>

This is the same diff as before minus the behaviour change of source-as.
Lets get this in and then we can figure out if we want to change
something.

--
:wq Claudio

Index: usr.sbin/bgpctl/bgpctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.225
diff -u -p -r1.225 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c 5 Dec 2018 06:53:52 -0000 1.225
+++ usr.sbin/bgpctl/bgpctl.c 6 Dec 2018 09:14:17 -0000
@@ -97,6 +97,7 @@ void mrt_to_bgpd_addr(union mrt_addr *
 const char *msg_type(u_int8_t);
 void network_bulk(struct parse_result *);
 const char *print_auth_method(enum auth_method);
+int match_aspath(void *, u_int16_t, struct filter_as *);
 
 struct imsgbuf *ibuf;
 struct mrt_parser show_mrt = { show_mrt_dump, show_mrt_state, show_mrt_msg };
@@ -2056,8 +2057,7 @@ show_mrt_dump(struct mrt_rib *mr, struct
  }
  /* filter by AS */
  if (req->as.type != AS_UNDEF &&
-   !aspath_match(mre->aspath, mre->aspath_len,
-   &req->as, 0))
+   !match_aspath(mre->aspath, mre->aspath_len, &req->as))
  continue;
 
  if (req->flags & F_CTL_DETAIL) {
@@ -2122,8 +2122,7 @@ network_mrt_dump(struct mrt_rib *mr, str
  }
  /* filter by AS */
  if (req->as.type != AS_UNDEF &&
-   !aspath_match(mre->aspath, mre->aspath_len,
-   &req->as, 0))
+   !match_aspath(mre->aspath, mre->aspath_len, &req->as))
  continue;
 
  bzero(&net, sizeof(net));
@@ -2661,7 +2660,67 @@ msg_type(u_int8_t type)
 }
 
 int
-as_set_match(const struct as_set *a, u_int32_t asnum)
+match_aspath(void *data, u_int16_t len, struct filter_as *f)
 {
+ u_int8_t *seg;
+ int final;
+ u_int16_t seg_size;
+ u_int8_t i, seg_len;
+ u_int32_t as = 0;
+
+ if (f->type == AS_EMPTY) {
+ if (len == 0)
+ return (1);
+ else
+ return (0);
+ }
+
+ seg = data;
+
+ /* just check the leftmost AS */
+ if (f->type == AS_PEER && len >= 6) {
+ as = aspath_extract(seg, 0);
+ if (f->as_min == as)
+ return (1);
+ else
+ return (0);
+ }
+
+ for (; len >= 6; len -= seg_size, seg += seg_size) {
+ seg_len = seg[1];
+ seg_size = 2 + sizeof(u_int32_t) * seg_len;
+
+ final = (len == seg_size);
+
+ if (f->type == AS_SOURCE) {
+ /*
+ * Just extract the rightmost AS
+ * but if that segment is an AS_SET then the rightmost
+ * AS of a previous AS_SEQUENCE segment should be used.
+ * Because of that just look at AS_SEQUENCE segments.
+ */
+ if (seg[0] == AS_SEQUENCE)
+ as = aspath_extract(seg, seg_len - 1);
+ /* not yet in the final segment */
+ if (!final)
+ continue;
+ if (f->as_min == as)
+ return (1);
+ else
+ return (0);
+ }
+ /* AS_TRANSIT or AS_ALL */
+ for (i = 0; i < seg_len; i++) {
+ /*
+ * the source (rightmost) AS is excluded from
+ * AS_TRANSIT matches.
+ */
+ if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
+ return (0);
+ as = aspath_extract(seg, i);
+ if (f->as_min == as)
+ return (1);
+ }
+ }
  return (0);
 }
Index: usr.sbin/bgpd/bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.355
diff -u -p -r1.355 bgpd.h
--- usr.sbin/bgpd/bgpd.h 28 Nov 2018 08:32:26 -0000 1.355
+++ usr.sbin/bgpd/bgpd.h 28 Nov 2018 09:10:01 -0000
@@ -1258,7 +1258,6 @@ const char *log_shutcomm(const char *);
 int aspath_snprint(char *, size_t, void *, u_int16_t);
 int aspath_asprint(char **, void *, u_int16_t);
 size_t aspath_strlen(void *, u_int16_t);
-int aspath_match(void *, u_int16_t, struct filter_as *, u_int32_t);
 u_int32_t aspath_extract(const void *, int);
 int aspath_verify(void *, u_int16_t, int);
 #define AS_ERR_LEN -1
Index: usr.sbin/bgpd/rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.451
diff -u -p -r1.451 rde.c
--- usr.sbin/bgpd/rde.c 29 Nov 2018 15:11:27 -0000 1.451
+++ usr.sbin/bgpd/rde.c 3 Dec 2018 15:09:46 -0000
@@ -482,7 +482,6 @@ rde_dispatch_imsg_session(struct imsgbuf
  asp->origin = csr.origin;
  asp->flags |= F_PREFIX_ANNOUNCED | F_ANN_DYNAMIC;
  asp->aspath = aspath_get(asdata, csr.aspath_len);
- asp->source_as = aspath_origin(asp->aspath);
  netconf_s.asp = asp;
  break;
  case IMSG_NETWORK_ATTR:
@@ -1121,10 +1120,6 @@ rde_update_dispatch(struct imsg *imsg)
  }
  }
 
- if (state.aspath.flags & F_ATTR_ASPATH)
- state.aspath.source_as =
-    aspath_origin(state.aspath.aspath);
-
  rde_reflector(peer, &state.aspath);
  }
 
@@ -1389,7 +1384,7 @@ rde_update_update(struct rde_peer *peer,
 
  peer->prefix_rcvd_update++;
  vstate = rde_roa_validity(&conf->rde_roa, prefix, prefixlen,
-    in->aspath.source_as);
+    aspath_origin(in->aspath.aspath));
 
  /* add original path to the Adj-RIB-In */
  if (path_update(&ribs[RIB_ADJ_IN].rib, peer, in, prefix, prefixlen,
@@ -2220,8 +2215,8 @@ rde_dump_filter(struct prefix *p, struct
  if ((req->flags & F_CTL_INVALID) &&
     (asp->flags & F_ATTR_PARSE_ERR) == 0)
  return;
- if (req->as.type != AS_UNDEF && !aspath_match(asp->aspath->data,
-    asp->aspath->len, &req->as, 0))
+ if (req->as.type != AS_UNDEF &&
+    !aspath_match(asp->aspath, &req->as, 0))
  return;
  switch (req->community.type) {
  case COMMUNITY_TYPE_NONE:
@@ -3085,7 +3080,7 @@ rde_softreconfig_in(struct rib_entry *re
  if (conf->rde_roa.dirty) {
  /* ROA validation state update */
  vstate = rde_roa_validity(&conf->rde_roa,
-    &prefix, pt->prefixlen, asp->source_as);
+    &prefix, pt->prefixlen, aspath_origin(asp->aspath));
  if (vstate != p->validation_state) {
  force_eval = 1;
  p->validation_state = vstate;
@@ -3660,7 +3655,6 @@ network_add(struct network_config *nc, i
  asp = path_get();
  asp->aspath = aspath_get(NULL, 0);
  asp->origin = ORIGIN_IGP;
- asp->source_as = aspath_origin(asp->aspath);
  asp->flags = F_ATTR_ORIGIN | F_ATTR_ASPATH |
     F_ATTR_LOCALPREF | F_PREFIX_ANNOUNCED;
  /* the nexthop is unset unless a default set overrides it */
@@ -3674,7 +3668,7 @@ network_add(struct network_config *nc, i
     peerself);
 
  vstate = rde_roa_validity(&conf->rde_roa, &nc->prefix,
-    nc->prefixlen, asp->source_as);
+    nc->prefixlen, aspath_origin(asp->aspath));
  if (path_update(&ribs[RIB_ADJ_IN].rib, peerself, &state, &nc->prefix,
     nc->prefixlen, vstate) == 1)
  peerself->prefix_cnt++;
Index: usr.sbin/bgpd/rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.203
diff -u -p -r1.203 rde.h
--- usr.sbin/bgpd/rde.h 28 Nov 2018 08:32:27 -0000 1.203
+++ usr.sbin/bgpd/rde.h 28 Nov 2018 09:10:02 -0000
@@ -125,6 +125,7 @@ struct rde_peer {
 
 struct aspath {
  LIST_ENTRY(aspath) entry;
+ u_int32_t source_as; /* cached source_as */
  int refcnt; /* reference count */
  u_int16_t len; /* total length of aspath in octets */
  u_int16_t ascnt; /* number of AS hops in data */
@@ -208,7 +209,6 @@ struct rde_aspath {
  struct aspath *aspath;
  u_int64_t hash;
  u_int32_t flags; /* internally used */
- u_int32_t source_as; /* cached source_as */
  u_int32_t med; /* multi exit disc */
  u_int32_t lpref; /* local pref */
  u_int32_t weight; /* low prio lpref */
@@ -354,11 +354,11 @@ u_char *aspath_deflate(u_char *, u_int1
 void aspath_merge(struct rde_aspath *, struct attr *);
 u_char *aspath_dump(struct aspath *);
 u_int16_t aspath_length(struct aspath *);
-u_int16_t aspath_count(const void *, u_int16_t);
 u_int32_t aspath_neighbor(struct aspath *);
 u_int32_t aspath_origin(struct aspath *);
 int aspath_loopfree(struct aspath *, u_int32_t);
 int aspath_compare(struct aspath *, struct aspath *);
+int aspath_match(struct aspath *, struct filter_as *, u_int32_t);
 u_char *aspath_prepend(struct aspath *, u_int32_t, int, u_int16_t *);
 int aspath_lenmatch(struct aspath *, enum aslen_spec, u_int);
 
Index: usr.sbin/bgpd/rde_attr.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
retrieving revision 1.113
diff -u -p -r1.113 rde_attr.c
--- usr.sbin/bgpd/rde_attr.c 28 Nov 2018 08:32:27 -0000 1.113
+++ usr.sbin/bgpd/rde_attr.c 6 Dec 2018 11:13:57 -0000
@@ -441,6 +441,8 @@ attr_put(struct attr *a)
 
 /* aspath specific functions */
 
+static u_int16_t aspath_count(const void *, u_int16_t);
+static u_int32_t aspath_extract_origin(const void *, u_int16_t);
 static u_int16_t aspath_countlength(struct aspath *, u_int16_t, int);
 static void aspath_countcopy(struct aspath *, u_int16_t, u_int8_t *,
      u_int16_t, int);
@@ -530,6 +532,7 @@ aspath_get(void *data, u_int16_t len)
  aspath->refcnt = 0;
  aspath->len = len;
  aspath->ascnt = aspath_count(data, len);
+ aspath->source_as = aspath_extract_origin(data, len);
  memcpy(aspath->data, data, len);
 
  /* link */
@@ -667,7 +670,22 @@ aspath_length(struct aspath *aspath)
  return (aspath->len);
 }
 
-u_int16_t
+u_int32_t
+aspath_neighbor(struct aspath *aspath)
+{
+ /* Empty aspath is OK -- internal AS route. */
+ if (aspath->len == 0)
+ return (rde_local_as());
+ return (aspath_extract(aspath->data, 0));
+}
+
+u_int32_t
+aspath_origin(struct aspath *aspath)
+{
+ return aspath->source_as;
+}
+
+static u_int16_t
 aspath_count(const void *data, u_int16_t len)
 {
  const u_int8_t *seg;
@@ -692,6 +710,41 @@ aspath_count(const void *data, u_int16_t
  return (cnt);
 }
 
+/*
+ * The origin AS number derived from a Route as follows:
+ * o  the rightmost AS in the final segment of the AS_PATH attribute
+ *    in the Route if that segment is of type AS_SEQUENCE, or
+ * o  the BGP speaker's own AS number if that segment is of type
+ *    AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
+ * o  the distinguished value "NONE" if the final segment of the
+ *    AS_PATH attribute is of any other type.
+ */
+static u_int32_t
+aspath_extract_origin(const void *data, u_int16_t len)
+{
+ const u_int8_t *seg;
+ u_int32_t as = AS_NONE;
+ u_int16_t seg_size;
+ u_int8_t seg_len;
+
+ /* AS_PATH is empty */
+ if (len == 0)
+ return (rde_local_as());
+
+ seg = data;
+ for (; len > 0; len -= seg_size, seg += seg_size) {
+ seg_len = seg[1];
+ seg_size = 2 + sizeof(u_int32_t) * seg_len;
+
+ if (len == seg_size && seg[0] == AS_SEQUENCE) {
+ as = aspath_extract(seg, seg_len - 1);
+ }
+ if (seg_size > len)
+ fatalx("%s: would overflow", __func__);
+ }
+ return (as);
+}
+
 static u_int16_t
 aspath_countlength(struct aspath *aspath, u_int16_t cnt, int headcnt)
 {
@@ -771,50 +824,6 @@ aspath_countcopy(struct aspath *aspath,
  }
 }
 
-u_int32_t
-aspath_neighbor(struct aspath *aspath)
-{
- /* Empty aspath is OK -- internal AS route. */
- if (aspath->len == 0)
- return (rde_local_as());
- return (aspath_extract(aspath->data, 0));
-}
-
-/*
- * The origin AS number derived from a Route as follows:
- * o  the rightmost AS in the final segment of the AS_PATH attribute
- *    in the Route if that segment is of type AS_SEQUENCE, or
- * o  the BGP speaker's own AS number if that segment is of type
- *    AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
- * o  the distinguished value "NONE" if the final segment of the
- *   AS_PATH attribute is of any other type.
- */
-u_int32_t
-aspath_origin(struct aspath *aspath)
-{
- u_int8_t *seg;
- u_int32_t as = AS_NONE;
- u_int16_t len, seg_size;
- u_int8_t seg_len;
-
- /* AS_PATH is empty */
- if (aspath->len == 0)
- return (rde_local_as());
-
- seg = aspath->data;
- for (len = aspath->len; len > 0; len -= seg_size, seg += seg_size) {
- seg_len = seg[1];
- seg_size = 2 + sizeof(u_int32_t) * seg_len;
-
- if (len == seg_size && seg[0] == AS_SEQUENCE) {
- as = aspath_extract(seg, seg_len - 1);
- }
- if (seg_size > len)
- fatalx("%s: would overflow", __func__);
- }
- return (as);
-}
-
 int
 aspath_loopfree(struct aspath *aspath, u_int32_t myAS)
 {
@@ -872,6 +881,110 @@ aspath_lookup(const void *data, u_int16_
  return (NULL);
 }
 
+
+static int
+as_compare(struct filter_as *f, u_int32_t as, u_int32_t neighas)
+{
+ u_int32_t match;
+
+ if (f->flags & AS_FLAG_AS_SET_NAME) /* should not happen */
+ return (0);
+ if (f->flags & AS_FLAG_AS_SET)
+ return (as_set_match(f->aset, as));
+
+ if (f->flags & AS_FLAG_NEIGHBORAS)
+ match = neighas;
+ else
+ match = f->as_min;
+
+ switch (f->op) {
+ case OP_NONE:
+ case OP_EQ:
+ if (as == match)
+ return (1);
+ break;
+ case OP_NE:
+ if (as != match)
+ return (1);
+ break;
+ case OP_RANGE:
+ if (as >= f->as_min && as <= f->as_max)
+ return (1);
+ break;
+ case OP_XRANGE:
+ if (as < f->as_min || as > f->as_max)
+ return (1);
+ break;
+ }
+ return (0);
+}
+
+/* we need to be able to search more than one as */
+int
+aspath_match(struct aspath *aspath, struct filter_as *f, u_int32_t neighas)
+{
+ const u_int8_t *seg;
+ int final;
+ u_int16_t len, seg_size;
+ u_int8_t i, seg_len;
+ u_int32_t as = AS_NONE;
+
+ if (f->type == AS_EMPTY) {
+ if (aspath_length(aspath) == 0)
+ return (1);
+ else
+ return (0);
+ }
+
+ /* just check the leftmost AS */
+ if (f->type == AS_PEER) {
+ as = aspath_neighbor(aspath);
+ if (as_compare(f, as, neighas))
+ return (1);
+ else
+ return (0);
+ }
+
+ seg = aspath->data;
+ len = aspath->len;
+ for (; len >= 6; len -= seg_size, seg += seg_size) {
+ seg_len = seg[1];
+ seg_size = 2 + sizeof(u_int32_t) * seg_len;
+
+ final = (len == seg_size);
+
+ if (f->type == AS_SOURCE) {
+ /*
+ * Just extract the rightmost AS
+ * but if that segment is an AS_SET then the rightmost
+ * AS of a previous AS_SEQUENCE segment should be used.
+ * Because of that just look at AS_SEQUENCE segments.
+ */
+ if (seg[0] == AS_SEQUENCE)
+ as = aspath_extract(seg, seg_len - 1);
+ /* not yet in the final segment */
+ if (!final)
+ continue;
+ if (as_compare(f, as, neighas))
+ return (1);
+ else
+ return (0);
+ }
+ /* AS_TRANSIT or AS_ALL */
+ for (i = 0; i < seg_len; i++) {
+ /*
+ * the source (rightmost) AS is excluded from
+ * AS_TRANSIT matches.
+ */
+ if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
+ return (0);
+ as = aspath_extract(seg, i);
+ if (as_compare(f, as, neighas))
+ return (1);
+ }
+ }
+ return (0);
+}
 
 /*
  * Returns a new prepended aspath. Old needs to be freed by caller.
Index: usr.sbin/bgpd/rde_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
retrieving revision 1.114
diff -u -p -r1.114 rde_filter.c
--- usr.sbin/bgpd/rde_filter.c 28 Nov 2018 08:32:27 -0000 1.114
+++ usr.sbin/bgpd/rde_filter.c 28 Nov 2018 09:10:02 -0000
@@ -219,8 +219,8 @@ rde_filter_match(struct filter_rule *f,
  }
 
  if (asp != NULL && f->match.as.type != AS_UNDEF) {
- if (aspath_match(asp->aspath->data, asp->aspath->len,
-    &f->match.as, peer->conf.remote_as) == 0)
+ if (aspath_match(asp->aspath, &f->match.as,
+    peer->conf.remote_as) == 0)
  return (0);
  }
 
@@ -289,7 +289,7 @@ rde_filter_match(struct filter_rule *f,
  pt_getaddr(p->re->prefix, prefix);
  plen = p->re->prefix->prefixlen;
  if (trie_roa_check(&f->match.originset.ps->th, prefix, plen,
-    asp->source_as) != ROA_VALID)
+    aspath_origin(asp->aspath)) != ROA_VALID)
  return (0);
  }
 
Index: usr.sbin/bgpd/rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.186
diff -u -p -r1.186 rde_rib.c
--- usr.sbin/bgpd/rde_rib.c 14 Nov 2018 12:14:41 -0000 1.186
+++ usr.sbin/bgpd/rde_rib.c 23 Nov 2018 14:41:47 -0000
@@ -657,10 +657,6 @@ path_compare(struct rde_aspath *a, struc
  return (1);
  if (a->pftableid < b->pftableid)
  return (-1);
- if (a->source_as > b->source_as)
- return (1);
- if (a->source_as < b->source_as)
- return (-1);
 
  r = aspath_compare(a->aspath, b->aspath);
  if (r > 0)
@@ -761,7 +757,6 @@ path_copy(struct rde_aspath *dst, const
  dst->lpref = src->lpref;
  dst->weight = src->weight;
  dst->origin = src->origin;
- dst->source_as = src->source_as;
  dst->rtlabelid = rtlabel_ref(src->rtlabelid);
  dst->pftableid = pftable_ref(src->pftableid);
 
Index: usr.sbin/bgpd/util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
retrieving revision 1.40
diff -u -p -r1.40 util.c
--- usr.sbin/bgpd/util.c 26 Sep 2018 14:38:19 -0000 1.40
+++ usr.sbin/bgpd/util.c 23 Nov 2018 14:41:47 -0000
@@ -312,110 +312,6 @@ aspath_strlen(void *data, u_int16_t len)
  return (total_size);
 }
 
-static int
-as_compare(struct filter_as *f, u_int32_t as, u_int32_t neighas)
-{
- u_int32_t match;
-
- if (f->flags & AS_FLAG_AS_SET_NAME) /* should not happen */
- return (0);
- if (f->flags & AS_FLAG_AS_SET)
- return (as_set_match(f->aset, as));
-
- if (f->flags & AS_FLAG_NEIGHBORAS)
- match = neighas;
- else
- match = f->as_min;
-
- switch (f->op) {
- case OP_NONE:
- case OP_EQ:
- if (as == match)
- return (1);
- break;
- case OP_NE:
- if (as != match)
- return (1);
- break;
- case OP_RANGE:
- if (as >= f->as_min && as <= f->as_max)
- return (1);
- break;
- case OP_XRANGE:
- if (as < f->as_min || as > f->as_max)
- return (1);
- break;
- }
- return (0);
-}
-
-/* we need to be able to search more than one as */
-int
-aspath_match(void *data, u_int16_t len, struct filter_as *f, u_int32_t neighas)
-{
- u_int8_t *seg;
- int final;
- u_int16_t seg_size;
- u_int8_t i, seg_len;
- u_int32_t as = 0;
-
- if (f->type == AS_EMPTY) {
- if (len == 0)
- return (1);
- else
- return (0);
- }
-
- seg = data;
-
- /* just check the leftmost AS */
- if (f->type == AS_PEER && len >= 6) {
- as = aspath_extract(seg, 0);
- if (as_compare(f, as, neighas))
- return (1);
- else
- return (0);
- }
-
- for (; len >= 6; len -= seg_size, seg += seg_size) {
- seg_len = seg[1];
- seg_size = 2 + sizeof(u_int32_t) * seg_len;
-
- final = (len == seg_size);
-
- if (f->type == AS_SOURCE) {
- /*
- * Just extract the rightmost AS
- * but if that segment is an AS_SET then the rightmost
- * AS of a previous AS_SEQUENCE segment should be used.
- * Because of that just look at AS_SEQUENCE segments.
- */
- if (seg[0] == AS_SEQUENCE)
- as = aspath_extract(seg, seg_len - 1);
- /* not yet in the final segment */
- if (!final)
- continue;
- if (as_compare(f, as, neighas))
- return (1);
- else
- return (0);
- }
- /* AS_TRANSIT or AS_ALL */
- for (i = 0; i < seg_len; i++) {
- /*
- * the source (rightmost) AS is excluded from
- * AS_TRANSIT matches.
- */
- if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
- return (0);
- as = aspath_extract(seg, i);
- if (as_compare(f, as, neighas))
- return (1);
- }
- }
- return (0);
-}
-
 /*
  * Extract the asnum out of the as segment at the specified position.
  * Direct access is not possible because of non-aligned reads.

Reply | Threaded
Open this post in threaded view
|

Re: bgpd refactor aspath_match a bit

Denis Fondras-3
On Thu, Dec 06, 2018 at 12:21:19PM +0100, Claudio Jeker wrote:

> On Wed, Nov 28, 2018 at 10:35:37AM +0100, Claudio Jeker wrote:
> > On Tue, Nov 27, 2018 at 06:55:51PM +0100, Job Snijders wrote:
> > > On Tue, Nov 27, 2018 at 06:23:53PM +0100, Claudio Jeker wrote:
> > > > On Tue, Nov 27, 2018 at 04:21:53PM +0100, Job Snijders wrote:
> > > > > On Fri, Nov 23, 2018 at 03:55:18PM +0100, Claudio Jeker wrote:
> > > > > > For origin validation I chacked the source_as in struct rde_aspath
> > > > > > this is not really the right place. It should be in struct aspath
> > > > > > since that holds all the ASPATH related stuff. Change this, move
> > > > > > aspath_match out of util.c back into rde_attr.c and adjust code to use
> > > > > > the cached value also in match from any source-as XYZ rules.
> > > > > > This last bit causes a minor behavioural change since the old code
> > > > > > extracted the last non AS_SET asnumber. The new code follows the ROA
> > > > > > RFC and returns the rightmost AS for AS_SEQUENCE, the local AS for
> > > > > > empty paths and AS_NONE (which is 0) for everything else.
> > > > > > So now 'match from any source-as 0' will return all paths that do not
> > > > > > have a final AS_SEQUENCE segment.
> > > > > >
> > > > > > The reason for this change is that I don't want to have two different
> > > > > > behaviours for what we call source-as (the one in roa-set and the one on a
> > > > > > filter).
> > > > >
> > > > > Something is off, it seems 'source-as 0' is matching anything that has
> > > > > an AS_SET attribute set:
> > > > >
> > > > >     $ bgpctl show rib source-as 0 | head
> > > > >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> > > > >            S = Stale, E = Error
> > > > >     origin validation state: N = not-found, V = valid, ! = invalid
> > > > >     origin: i = IGP, e = EGP, ? = Incomplete
> > > > >
> > > > >     flags ovs destination          gateway          lpref   med aspath origin
> > > > >     I*>     N 5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?
> > > > >     I*>     N 5.101.110.0/24       192.147.168.1      100     0 2914 14061 { 46652 } i
> > > > >     I*>     N 5.175.0.0/19         192.147.168.1      100     0 2914 1299 20773 { 8972 } i
> > > > >     I*>     N 8.41.202.0/24        192.147.168.1      100     0 2914 13789 30372 { 40179 } i
> > > > >
> > > > > Similarly, this should return at least 5.39.176.0/21:
> > > > >
> > > > >     $ bgpctl show rib source-as 8530
> > > > >     flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> > > > >            S = Stale, E = Error
> > > > >     origin validation state: N = not-found, V = valid, ! = invalid
> > > > >     origin: i = IGP, e = EGP, ? = Incomplete
> > > > >
> > > > >     flags ovs destination          gateway          lpref   med aspath origin
> > > > >     I*>     N 80.87.16.0/20        192.147.168.1      100     0 2914 8530 ?
> > > > >     I*>     N 87.236.128.0/21      192.147.168.1      100     0 2914 8530 ?
> > > > >     I*>     N 88.151.152.0/21      192.147.168.1      100     0 2914 8530 ?
> > > > >     I*>     N 89.38.120.0/21       192.147.168.1      100     0 2914 8530 i
> > > > >     I*>     N 93.115.176.0/20      192.147.168.1      100     0 2914 8530 i
> > > > >     I*>     N 185.52.144.0/22      192.147.168.1      100     0 2914 8530 ?
> > > > >
> > > >
> > > > I implemented source-as the way ROA is defining it. So anything which ends
> > > > with a AS_SET will return AS_NONE (which is 0). OpenBGPD has no way to
> > > > have an AS_PATH that has a real 0 in the AS_PATH (those UPDATES are
> > > > treated as withdraw). Because of this also the 5.39.176.0/21 is no longer
> > > > matching in 'bgpctl show rib source-as 8530'.
> > >
> > > I'm not sure it should behave that way.
> > >
> > > 'bgpctl show rib source-as 8530' really ought to return prefixes like
> > > 80.87.16.0/20 but also 5.39.176.0/21.
> >
> > But isn't this different from other implementations? At least I would
> > expect that the AS-path regex '8530$' would not match on the AS_SET path
> > either. My issue is that we have 'source-as' in roa-set, origin-set and on
> > filters in bgpd.conf plus the source-as used by bgpctl. Depending on
> > context they behave differently. So if AS 8530 is in the roa-set
> > and I do bgpctl show rib source-as 8530 the result will be different to
> > what would match in the roa-set.
> > We already had a lot of confusion about announce and that is why I decided
> > to make them behave the same.
> >  
> > > > I'm a bit on the edge here about where to go and currently prefer to
> > > > follow a RFC (which in this case is RFC6811).
> > > >
> > > >  o  Route Origin ASN: The origin AS number derived from a Route as
> > > >     follows:
> > > >
> > > >     *  the rightmost AS in the final segment of the AS_PATH attribute
> > > >          in the Route if that segment is of type AS_SEQUENCE, or
> > > >
> > > >     *  the BGP speaker's own AS number if that segment is of type
> > > >        AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
> > > >        or
> > > >
> > > >     *  the distinguished value "NONE" if the final segment of the
> > > >        AS_PATH attribute is of any other type.
> > > >
> > > > As mentioned above I found it strange when behaviour is different because
> > > > of where it is used.
> > >
> > > RFC 6811 describes how BGP Origin Validation should be performed, but is
> > > not a guideline how to treat AS_SETs in general or how CLIs should
> > > function. Perhaps 'source-as 0' should just throw an error in all
> > > contexts (both bgpd.conf and bgpctl), since 0 can never be in an AS_SET
> > > or AS_SEQUENCE. Perhaps we shouldn't overload the number.
> >
> > The overloading is done to simplify some internals. It is indeed possible
> > to disallow direct use of 0 in bgpctl and replace it with 'none'. On the
> > other hand it seems people are used to write belt&suspenders like configs
> > where filter rules (e.g. deny form any AS 0) are added which are not needed.
> >  
> > > 1/ Maybe 'source-as self' or 'source-as none' can be used to match all
> > > routes originated by the AS in which this bgpd instance runs.
> >
> > 'source-as self' and 'source-as none' are two different things in my
> > opinion. The empty AS path case is convered by 'empty-as' in the filters
> > and introducing 'source-as none' with a different meaning then the one
> > used by RFC6811 will not imporve anything.
> >  
> > > 2/ Similarly, the program could be made so that 'AS 15562' (lets use
> > > 15562 as example) from the Global Configuration passed on to bgpctl and
> > > used in the filter ruleset means both "what is originated by 15562" and
> > > also what is originated in the own AS (and doesn't have AS_PATH yet, but
> > > we know the router runs in 15562 because of the global config parameter).
> >
> > This is the behaviour with this diff:
> >
> > bgpctl show rib source-as 196618
> > flags ovs destination          gateway          lpref   med aspath origin
> > AI*>    N 10.12.12.0/24        0.0.0.0            100     0 i
> > AI*>    N 10.12.13.0/24        0.0.0.0            100     0 i
> >
> > 196618 is the AS of this instance.
> >
> >
> > I see two different discussion points here. Or two distinct behaviour
> > changes that get introduced by this diff:
> > 1/ source-as behaviour for empty AS_PATH. With this diff 'source-as $myAS'
> > will match for empty AS_PATH. In my opinion this makes sense but the
> > interaction with empty-as should be reconsidered. (e.g. are people
> > currently using both forms to achieve different behaviour in their own
> > AS).
> >
> > 2/ source-as handling of AS_PATH that end with an AS_SET (bgpd currently
> > does not support any other segment type). Here there is two things:
> >  a) should 'source-as 8530' match for the path with the AS_SET e.g:
> >        2914 8530 { 198753 }
> >     Should we really consider AS 8530 being the source of that prefix?
> >     What about 198753?
> >
> >  b) handling of 'source-as 0' ideally this should be disallowed and
> >     instead 'source-as none' introduced. Which would then collect all
> >     the AS_SET paths from above. b) depends on the outcome of a)
> >
> > As mentioned above the idea of making source-as behave like the source-as
> > in roa-set for consitency is my reason but if people think the current
> > behaviour has to be preserved (with the behaviour difference between
> > source-as usage) then that is fine and I will adjust the diff.
> >
>
> This is the same diff as before minus the behaviour change of source-as.
> Lets get this in and then we can figure out if we want to change
> something.
>

let's get this in. OK denis@

> --
> :wq Claudio
>
> Index: usr.sbin/bgpctl/bgpctl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.225
> diff -u -p -r1.225 bgpctl.c
> --- usr.sbin/bgpctl/bgpctl.c 5 Dec 2018 06:53:52 -0000 1.225
> +++ usr.sbin/bgpctl/bgpctl.c 6 Dec 2018 09:14:17 -0000
> @@ -97,6 +97,7 @@ void mrt_to_bgpd_addr(union mrt_addr *
>  const char *msg_type(u_int8_t);
>  void network_bulk(struct parse_result *);
>  const char *print_auth_method(enum auth_method);
> +int match_aspath(void *, u_int16_t, struct filter_as *);
>  
>  struct imsgbuf *ibuf;
>  struct mrt_parser show_mrt = { show_mrt_dump, show_mrt_state, show_mrt_msg };
> @@ -2056,8 +2057,7 @@ show_mrt_dump(struct mrt_rib *mr, struct
>   }
>   /* filter by AS */
>   if (req->as.type != AS_UNDEF &&
> -   !aspath_match(mre->aspath, mre->aspath_len,
> -   &req->as, 0))
> +   !match_aspath(mre->aspath, mre->aspath_len, &req->as))
>   continue;
>  
>   if (req->flags & F_CTL_DETAIL) {
> @@ -2122,8 +2122,7 @@ network_mrt_dump(struct mrt_rib *mr, str
>   }
>   /* filter by AS */
>   if (req->as.type != AS_UNDEF &&
> -   !aspath_match(mre->aspath, mre->aspath_len,
> -   &req->as, 0))
> +   !match_aspath(mre->aspath, mre->aspath_len, &req->as))
>   continue;
>  
>   bzero(&net, sizeof(net));
> @@ -2661,7 +2660,67 @@ msg_type(u_int8_t type)
>  }
>  
>  int
> -as_set_match(const struct as_set *a, u_int32_t asnum)
> +match_aspath(void *data, u_int16_t len, struct filter_as *f)
>  {
> + u_int8_t *seg;
> + int final;
> + u_int16_t seg_size;
> + u_int8_t i, seg_len;
> + u_int32_t as = 0;
> +
> + if (f->type == AS_EMPTY) {
> + if (len == 0)
> + return (1);
> + else
> + return (0);
> + }
> +
> + seg = data;
> +
> + /* just check the leftmost AS */
> + if (f->type == AS_PEER && len >= 6) {
> + as = aspath_extract(seg, 0);
> + if (f->as_min == as)
> + return (1);
> + else
> + return (0);
> + }
> +
> + for (; len >= 6; len -= seg_size, seg += seg_size) {
> + seg_len = seg[1];
> + seg_size = 2 + sizeof(u_int32_t) * seg_len;
> +
> + final = (len == seg_size);
> +
> + if (f->type == AS_SOURCE) {
> + /*
> + * Just extract the rightmost AS
> + * but if that segment is an AS_SET then the rightmost
> + * AS of a previous AS_SEQUENCE segment should be used.
> + * Because of that just look at AS_SEQUENCE segments.
> + */
> + if (seg[0] == AS_SEQUENCE)
> + as = aspath_extract(seg, seg_len - 1);
> + /* not yet in the final segment */
> + if (!final)
> + continue;
> + if (f->as_min == as)
> + return (1);
> + else
> + return (0);
> + }
> + /* AS_TRANSIT or AS_ALL */
> + for (i = 0; i < seg_len; i++) {
> + /*
> + * the source (rightmost) AS is excluded from
> + * AS_TRANSIT matches.
> + */
> + if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
> + return (0);
> + as = aspath_extract(seg, i);
> + if (f->as_min == as)
> + return (1);
> + }
> + }
>   return (0);
>  }
> Index: usr.sbin/bgpd/bgpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.355
> diff -u -p -r1.355 bgpd.h
> --- usr.sbin/bgpd/bgpd.h 28 Nov 2018 08:32:26 -0000 1.355
> +++ usr.sbin/bgpd/bgpd.h 28 Nov 2018 09:10:01 -0000
> @@ -1258,7 +1258,6 @@ const char *log_shutcomm(const char *);
>  int aspath_snprint(char *, size_t, void *, u_int16_t);
>  int aspath_asprint(char **, void *, u_int16_t);
>  size_t aspath_strlen(void *, u_int16_t);
> -int aspath_match(void *, u_int16_t, struct filter_as *, u_int32_t);
>  u_int32_t aspath_extract(const void *, int);
>  int aspath_verify(void *, u_int16_t, int);
>  #define AS_ERR_LEN -1
> Index: usr.sbin/bgpd/rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.451
> diff -u -p -r1.451 rde.c
> --- usr.sbin/bgpd/rde.c 29 Nov 2018 15:11:27 -0000 1.451
> +++ usr.sbin/bgpd/rde.c 3 Dec 2018 15:09:46 -0000
> @@ -482,7 +482,6 @@ rde_dispatch_imsg_session(struct imsgbuf
>   asp->origin = csr.origin;
>   asp->flags |= F_PREFIX_ANNOUNCED | F_ANN_DYNAMIC;
>   asp->aspath = aspath_get(asdata, csr.aspath_len);
> - asp->source_as = aspath_origin(asp->aspath);
>   netconf_s.asp = asp;
>   break;
>   case IMSG_NETWORK_ATTR:
> @@ -1121,10 +1120,6 @@ rde_update_dispatch(struct imsg *imsg)
>   }
>   }
>  
> - if (state.aspath.flags & F_ATTR_ASPATH)
> - state.aspath.source_as =
> -    aspath_origin(state.aspath.aspath);
> -
>   rde_reflector(peer, &state.aspath);
>   }
>  
> @@ -1389,7 +1384,7 @@ rde_update_update(struct rde_peer *peer,
>  
>   peer->prefix_rcvd_update++;
>   vstate = rde_roa_validity(&conf->rde_roa, prefix, prefixlen,
> -    in->aspath.source_as);
> +    aspath_origin(in->aspath.aspath));
>  
>   /* add original path to the Adj-RIB-In */
>   if (path_update(&ribs[RIB_ADJ_IN].rib, peer, in, prefix, prefixlen,
> @@ -2220,8 +2215,8 @@ rde_dump_filter(struct prefix *p, struct
>   if ((req->flags & F_CTL_INVALID) &&
>      (asp->flags & F_ATTR_PARSE_ERR) == 0)
>   return;
> - if (req->as.type != AS_UNDEF && !aspath_match(asp->aspath->data,
> -    asp->aspath->len, &req->as, 0))
> + if (req->as.type != AS_UNDEF &&
> +    !aspath_match(asp->aspath, &req->as, 0))
>   return;
>   switch (req->community.type) {
>   case COMMUNITY_TYPE_NONE:
> @@ -3085,7 +3080,7 @@ rde_softreconfig_in(struct rib_entry *re
>   if (conf->rde_roa.dirty) {
>   /* ROA validation state update */
>   vstate = rde_roa_validity(&conf->rde_roa,
> -    &prefix, pt->prefixlen, asp->source_as);
> +    &prefix, pt->prefixlen, aspath_origin(asp->aspath));
>   if (vstate != p->validation_state) {
>   force_eval = 1;
>   p->validation_state = vstate;
> @@ -3660,7 +3655,6 @@ network_add(struct network_config *nc, i
>   asp = path_get();
>   asp->aspath = aspath_get(NULL, 0);
>   asp->origin = ORIGIN_IGP;
> - asp->source_as = aspath_origin(asp->aspath);
>   asp->flags = F_ATTR_ORIGIN | F_ATTR_ASPATH |
>      F_ATTR_LOCALPREF | F_PREFIX_ANNOUNCED;
>   /* the nexthop is unset unless a default set overrides it */
> @@ -3674,7 +3668,7 @@ network_add(struct network_config *nc, i
>      peerself);
>  
>   vstate = rde_roa_validity(&conf->rde_roa, &nc->prefix,
> -    nc->prefixlen, asp->source_as);
> +    nc->prefixlen, aspath_origin(asp->aspath));
>   if (path_update(&ribs[RIB_ADJ_IN].rib, peerself, &state, &nc->prefix,
>      nc->prefixlen, vstate) == 1)
>   peerself->prefix_cnt++;
> Index: usr.sbin/bgpd/rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.203
> diff -u -p -r1.203 rde.h
> --- usr.sbin/bgpd/rde.h 28 Nov 2018 08:32:27 -0000 1.203
> +++ usr.sbin/bgpd/rde.h 28 Nov 2018 09:10:02 -0000
> @@ -125,6 +125,7 @@ struct rde_peer {
>  
>  struct aspath {
>   LIST_ENTRY(aspath) entry;
> + u_int32_t source_as; /* cached source_as */
>   int refcnt; /* reference count */
>   u_int16_t len; /* total length of aspath in octets */
>   u_int16_t ascnt; /* number of AS hops in data */
> @@ -208,7 +209,6 @@ struct rde_aspath {
>   struct aspath *aspath;
>   u_int64_t hash;
>   u_int32_t flags; /* internally used */
> - u_int32_t source_as; /* cached source_as */
>   u_int32_t med; /* multi exit disc */
>   u_int32_t lpref; /* local pref */
>   u_int32_t weight; /* low prio lpref */
> @@ -354,11 +354,11 @@ u_char *aspath_deflate(u_char *, u_int1
>  void aspath_merge(struct rde_aspath *, struct attr *);
>  u_char *aspath_dump(struct aspath *);
>  u_int16_t aspath_length(struct aspath *);
> -u_int16_t aspath_count(const void *, u_int16_t);
>  u_int32_t aspath_neighbor(struct aspath *);
>  u_int32_t aspath_origin(struct aspath *);
>  int aspath_loopfree(struct aspath *, u_int32_t);
>  int aspath_compare(struct aspath *, struct aspath *);
> +int aspath_match(struct aspath *, struct filter_as *, u_int32_t);
>  u_char *aspath_prepend(struct aspath *, u_int32_t, int, u_int16_t *);
>  int aspath_lenmatch(struct aspath *, enum aslen_spec, u_int);
>  
> Index: usr.sbin/bgpd/rde_attr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
> retrieving revision 1.113
> diff -u -p -r1.113 rde_attr.c
> --- usr.sbin/bgpd/rde_attr.c 28 Nov 2018 08:32:27 -0000 1.113
> +++ usr.sbin/bgpd/rde_attr.c 6 Dec 2018 11:13:57 -0000
> @@ -441,6 +441,8 @@ attr_put(struct attr *a)
>  
>  /* aspath specific functions */
>  
> +static u_int16_t aspath_count(const void *, u_int16_t);
> +static u_int32_t aspath_extract_origin(const void *, u_int16_t);
>  static u_int16_t aspath_countlength(struct aspath *, u_int16_t, int);
>  static void aspath_countcopy(struct aspath *, u_int16_t, u_int8_t *,
>       u_int16_t, int);
> @@ -530,6 +532,7 @@ aspath_get(void *data, u_int16_t len)
>   aspath->refcnt = 0;
>   aspath->len = len;
>   aspath->ascnt = aspath_count(data, len);
> + aspath->source_as = aspath_extract_origin(data, len);
>   memcpy(aspath->data, data, len);
>  
>   /* link */
> @@ -667,7 +670,22 @@ aspath_length(struct aspath *aspath)
>   return (aspath->len);
>  }
>  
> -u_int16_t
> +u_int32_t
> +aspath_neighbor(struct aspath *aspath)
> +{
> + /* Empty aspath is OK -- internal AS route. */
> + if (aspath->len == 0)
> + return (rde_local_as());
> + return (aspath_extract(aspath->data, 0));
> +}
> +
> +u_int32_t
> +aspath_origin(struct aspath *aspath)
> +{
> + return aspath->source_as;
> +}
> +
> +static u_int16_t
>  aspath_count(const void *data, u_int16_t len)
>  {
>   const u_int8_t *seg;
> @@ -692,6 +710,41 @@ aspath_count(const void *data, u_int16_t
>   return (cnt);
>  }
>  
> +/*
> + * The origin AS number derived from a Route as follows:
> + * o  the rightmost AS in the final segment of the AS_PATH attribute
> + *    in the Route if that segment is of type AS_SEQUENCE, or
> + * o  the BGP speaker's own AS number if that segment is of type
> + *    AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
> + * o  the distinguished value "NONE" if the final segment of the
> + *    AS_PATH attribute is of any other type.
> + */
> +static u_int32_t
> +aspath_extract_origin(const void *data, u_int16_t len)
> +{
> + const u_int8_t *seg;
> + u_int32_t as = AS_NONE;
> + u_int16_t seg_size;
> + u_int8_t seg_len;
> +
> + /* AS_PATH is empty */
> + if (len == 0)
> + return (rde_local_as());
> +
> + seg = data;
> + for (; len > 0; len -= seg_size, seg += seg_size) {
> + seg_len = seg[1];
> + seg_size = 2 + sizeof(u_int32_t) * seg_len;
> +
> + if (len == seg_size && seg[0] == AS_SEQUENCE) {
> + as = aspath_extract(seg, seg_len - 1);
> + }
> + if (seg_size > len)
> + fatalx("%s: would overflow", __func__);
> + }
> + return (as);
> +}
> +
>  static u_int16_t
>  aspath_countlength(struct aspath *aspath, u_int16_t cnt, int headcnt)
>  {
> @@ -771,50 +824,6 @@ aspath_countcopy(struct aspath *aspath,
>   }
>  }
>  
> -u_int32_t
> -aspath_neighbor(struct aspath *aspath)
> -{
> - /* Empty aspath is OK -- internal AS route. */
> - if (aspath->len == 0)
> - return (rde_local_as());
> - return (aspath_extract(aspath->data, 0));
> -}
> -
> -/*
> - * The origin AS number derived from a Route as follows:
> - * o  the rightmost AS in the final segment of the AS_PATH attribute
> - *    in the Route if that segment is of type AS_SEQUENCE, or
> - * o  the BGP speaker's own AS number if that segment is of type
> - *    AS_CONFED_SEQUENCE or AS_CONFED_SET or if the AS_PATH is empty,
> - * o  the distinguished value "NONE" if the final segment of the
> - *   AS_PATH attribute is of any other type.
> - */
> -u_int32_t
> -aspath_origin(struct aspath *aspath)
> -{
> - u_int8_t *seg;
> - u_int32_t as = AS_NONE;
> - u_int16_t len, seg_size;
> - u_int8_t seg_len;
> -
> - /* AS_PATH is empty */
> - if (aspath->len == 0)
> - return (rde_local_as());
> -
> - seg = aspath->data;
> - for (len = aspath->len; len > 0; len -= seg_size, seg += seg_size) {
> - seg_len = seg[1];
> - seg_size = 2 + sizeof(u_int32_t) * seg_len;
> -
> - if (len == seg_size && seg[0] == AS_SEQUENCE) {
> - as = aspath_extract(seg, seg_len - 1);
> - }
> - if (seg_size > len)
> - fatalx("%s: would overflow", __func__);
> - }
> - return (as);
> -}
> -
>  int
>  aspath_loopfree(struct aspath *aspath, u_int32_t myAS)
>  {
> @@ -872,6 +881,110 @@ aspath_lookup(const void *data, u_int16_
>   return (NULL);
>  }
>  
> +
> +static int
> +as_compare(struct filter_as *f, u_int32_t as, u_int32_t neighas)
> +{
> + u_int32_t match;
> +
> + if (f->flags & AS_FLAG_AS_SET_NAME) /* should not happen */
> + return (0);
> + if (f->flags & AS_FLAG_AS_SET)
> + return (as_set_match(f->aset, as));
> +
> + if (f->flags & AS_FLAG_NEIGHBORAS)
> + match = neighas;
> + else
> + match = f->as_min;
> +
> + switch (f->op) {
> + case OP_NONE:
> + case OP_EQ:
> + if (as == match)
> + return (1);
> + break;
> + case OP_NE:
> + if (as != match)
> + return (1);
> + break;
> + case OP_RANGE:
> + if (as >= f->as_min && as <= f->as_max)
> + return (1);
> + break;
> + case OP_XRANGE:
> + if (as < f->as_min || as > f->as_max)
> + return (1);
> + break;
> + }
> + return (0);
> +}
> +
> +/* we need to be able to search more than one as */
> +int
> +aspath_match(struct aspath *aspath, struct filter_as *f, u_int32_t neighas)
> +{
> + const u_int8_t *seg;
> + int final;
> + u_int16_t len, seg_size;
> + u_int8_t i, seg_len;
> + u_int32_t as = AS_NONE;
> +
> + if (f->type == AS_EMPTY) {
> + if (aspath_length(aspath) == 0)
> + return (1);
> + else
> + return (0);
> + }
> +
> + /* just check the leftmost AS */
> + if (f->type == AS_PEER) {
> + as = aspath_neighbor(aspath);
> + if (as_compare(f, as, neighas))
> + return (1);
> + else
> + return (0);
> + }
> +
> + seg = aspath->data;
> + len = aspath->len;
> + for (; len >= 6; len -= seg_size, seg += seg_size) {
> + seg_len = seg[1];
> + seg_size = 2 + sizeof(u_int32_t) * seg_len;
> +
> + final = (len == seg_size);
> +
> + if (f->type == AS_SOURCE) {
> + /*
> + * Just extract the rightmost AS
> + * but if that segment is an AS_SET then the rightmost
> + * AS of a previous AS_SEQUENCE segment should be used.
> + * Because of that just look at AS_SEQUENCE segments.
> + */
> + if (seg[0] == AS_SEQUENCE)
> + as = aspath_extract(seg, seg_len - 1);
> + /* not yet in the final segment */
> + if (!final)
> + continue;
> + if (as_compare(f, as, neighas))
> + return (1);
> + else
> + return (0);
> + }
> + /* AS_TRANSIT or AS_ALL */
> + for (i = 0; i < seg_len; i++) {
> + /*
> + * the source (rightmost) AS is excluded from
> + * AS_TRANSIT matches.
> + */
> + if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
> + return (0);
> + as = aspath_extract(seg, i);
> + if (as_compare(f, as, neighas))
> + return (1);
> + }
> + }
> + return (0);
> +}
>  
>  /*
>   * Returns a new prepended aspath. Old needs to be freed by caller.
> Index: usr.sbin/bgpd/rde_filter.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 rde_filter.c
> --- usr.sbin/bgpd/rde_filter.c 28 Nov 2018 08:32:27 -0000 1.114
> +++ usr.sbin/bgpd/rde_filter.c 28 Nov 2018 09:10:02 -0000
> @@ -219,8 +219,8 @@ rde_filter_match(struct filter_rule *f,
>   }
>  
>   if (asp != NULL && f->match.as.type != AS_UNDEF) {
> - if (aspath_match(asp->aspath->data, asp->aspath->len,
> -    &f->match.as, peer->conf.remote_as) == 0)
> + if (aspath_match(asp->aspath, &f->match.as,
> +    peer->conf.remote_as) == 0)
>   return (0);
>   }
>  
> @@ -289,7 +289,7 @@ rde_filter_match(struct filter_rule *f,
>   pt_getaddr(p->re->prefix, prefix);
>   plen = p->re->prefix->prefixlen;
>   if (trie_roa_check(&f->match.originset.ps->th, prefix, plen,
> -    asp->source_as) != ROA_VALID)
> +    aspath_origin(asp->aspath)) != ROA_VALID)
>   return (0);
>   }
>  
> Index: usr.sbin/bgpd/rde_rib.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> retrieving revision 1.186
> diff -u -p -r1.186 rde_rib.c
> --- usr.sbin/bgpd/rde_rib.c 14 Nov 2018 12:14:41 -0000 1.186
> +++ usr.sbin/bgpd/rde_rib.c 23 Nov 2018 14:41:47 -0000
> @@ -657,10 +657,6 @@ path_compare(struct rde_aspath *a, struc
>   return (1);
>   if (a->pftableid < b->pftableid)
>   return (-1);
> - if (a->source_as > b->source_as)
> - return (1);
> - if (a->source_as < b->source_as)
> - return (-1);
>  
>   r = aspath_compare(a->aspath, b->aspath);
>   if (r > 0)
> @@ -761,7 +757,6 @@ path_copy(struct rde_aspath *dst, const
>   dst->lpref = src->lpref;
>   dst->weight = src->weight;
>   dst->origin = src->origin;
> - dst->source_as = src->source_as;
>   dst->rtlabelid = rtlabel_ref(src->rtlabelid);
>   dst->pftableid = pftable_ref(src->pftableid);
>  
> Index: usr.sbin/bgpd/util.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 util.c
> --- usr.sbin/bgpd/util.c 26 Sep 2018 14:38:19 -0000 1.40
> +++ usr.sbin/bgpd/util.c 23 Nov 2018 14:41:47 -0000
> @@ -312,110 +312,6 @@ aspath_strlen(void *data, u_int16_t len)
>   return (total_size);
>  }
>  
> -static int
> -as_compare(struct filter_as *f, u_int32_t as, u_int32_t neighas)
> -{
> - u_int32_t match;
> -
> - if (f->flags & AS_FLAG_AS_SET_NAME) /* should not happen */
> - return (0);
> - if (f->flags & AS_FLAG_AS_SET)
> - return (as_set_match(f->aset, as));
> -
> - if (f->flags & AS_FLAG_NEIGHBORAS)
> - match = neighas;
> - else
> - match = f->as_min;
> -
> - switch (f->op) {
> - case OP_NONE:
> - case OP_EQ:
> - if (as == match)
> - return (1);
> - break;
> - case OP_NE:
> - if (as != match)
> - return (1);
> - break;
> - case OP_RANGE:
> - if (as >= f->as_min && as <= f->as_max)
> - return (1);
> - break;
> - case OP_XRANGE:
> - if (as < f->as_min || as > f->as_max)
> - return (1);
> - break;
> - }
> - return (0);
> -}
> -
> -/* we need to be able to search more than one as */
> -int
> -aspath_match(void *data, u_int16_t len, struct filter_as *f, u_int32_t neighas)
> -{
> - u_int8_t *seg;
> - int final;
> - u_int16_t seg_size;
> - u_int8_t i, seg_len;
> - u_int32_t as = 0;
> -
> - if (f->type == AS_EMPTY) {
> - if (len == 0)
> - return (1);
> - else
> - return (0);
> - }
> -
> - seg = data;
> -
> - /* just check the leftmost AS */
> - if (f->type == AS_PEER && len >= 6) {
> - as = aspath_extract(seg, 0);
> - if (as_compare(f, as, neighas))
> - return (1);
> - else
> - return (0);
> - }
> -
> - for (; len >= 6; len -= seg_size, seg += seg_size) {
> - seg_len = seg[1];
> - seg_size = 2 + sizeof(u_int32_t) * seg_len;
> -
> - final = (len == seg_size);
> -
> - if (f->type == AS_SOURCE) {
> - /*
> - * Just extract the rightmost AS
> - * but if that segment is an AS_SET then the rightmost
> - * AS of a previous AS_SEQUENCE segment should be used.
> - * Because of that just look at AS_SEQUENCE segments.
> - */
> - if (seg[0] == AS_SEQUENCE)
> - as = aspath_extract(seg, seg_len - 1);
> - /* not yet in the final segment */
> - if (!final)
> - continue;
> - if (as_compare(f, as, neighas))
> - return (1);
> - else
> - return (0);
> - }
> - /* AS_TRANSIT or AS_ALL */
> - for (i = 0; i < seg_len; i++) {
> - /*
> - * the source (rightmost) AS is excluded from
> - * AS_TRANSIT matches.
> - */
> - if (final && i == seg_len - 1 && f->type == AS_TRANSIT)
> - return (0);
> - as = aspath_extract(seg, i);
> - if (as_compare(f, as, neighas))
> - return (1);
> - }
> - }
> - return (0);
> -}
> -
>  /*
>   * Extract the asnum out of the as segment at the specified position.
>   * Direct access is not possible because of non-aligned reads.
>