On Mon, Apr 03, 2023 at 10:41:15AM +0200, Claudio Jeker wrote: > Flowspec RFC 8955 and 8956 allows to propegate traffic filtering rules > to other routers. The main use case is to drop DDoS traffic further > upstream and by that reducing the impact of such denial of service > attacks. > > This diff only adds the needed plumbing to announce the MP capability for > flowspec. Any flowspec UPDATE received will be silently ignored and there > is no way to send anything. Still this is a first small step and more > changes will follow to send flowspec rules. > > The diff includes all the defines needed for flowspec. > On top of this there is a small fix in rde_update_dispatch() to > properly jump over unknown MP attributes (2 hunks). > In rde_get_mp_nexthop() I moved the AID_VPN_IPv6 case around and used the > same error message for lenght check in all cases. This is why the diff is > a bit larger. > > I successfully tested this against exabgp.
ok Some minor comments: > Index: bgpd.h [...] > @@ -1063,10 +1073,47 @@ struct ext_comm_pairs { > { EXT_COMMUNITY_TRANS_EVPN, 0x01, "esi-lab" }, \ > { EXT_COMMUNITY_TRANS_EVPN, 0x02, "esi-rt" }, \ > \ > + { EXT_COMMUNITY_GEN_TWO_AS, 0x06, "flow-rate" }, \ > + { EXT_COMMUNITY_GEN_TWO_AS, 0x0c, "flow-pps" }, \ > + { EXT_COMMUNITY_GEN_TWO_AS, 0x07, "flow-action" }, \ > + { EXT_COMMUNITY_GEN_TWO_AS, 0x08, "flow-rt-redir" }, \ > + { EXT_COMMUNITY_GEN_IPV4, 0x08, "flow-rt-redir" }, \ > + { EXT_COMMUNITY_GEN_FOUR_AS, 0x08, "flow-rt-redir" }, \ > + { EXT_COMMUNITY_GEN_TWO_AS, 0x09, "flow-dscp" }, \ Perhaps keep the empty line before { 0 } here? > { 0 } \ > } > > extern const struct ext_comm_pairs iana_ext_comms[]; > + > +/* BGP flowspec defines RFC 8955 */ I'd also mention RFC 8956 since FLOWSPEC_TYPE_FLOW isn't in RFC 8955 (or add a comment to that define). > +#define FLOWSPEC_LEN_LIMIT 0xf0 > +#define FLOWSPEC_OP_EOL 0x80 > +#define FLOWSPEC_OP_AND 0x40 > +#define FLOWSPEC_OP_LEN_MASK 0x30 > +#define FLOWSPEC_OP_LEN_SHIFT 4 > +#define FLOWSPEC_OP_LEN(op) \ > + (1 << (((op) & FLOWSPEC_OP_LEN_MASK) >> FLOWSPEC_OP_LEN_SHIFT)) > +#define FLOWSPEC_OP_NUM_LT 0x04 > +#define FLOWSPEC_OP_NUM_GT 0x02 > +#define FLOWSPEC_OP_NUM_EQ 0x01 > +#define FLOWSPEC_OP_BIT_NOT 0x02 > +#define FLOWSPEC_OP_BIT_MATCH 0x01 > + > +#define FLOWSPEC_TYPE_MIN 1 > +#define FLOWSPEC_TYPE_DEST 1 > +#define FLOWSPEC_TYPE_SOURCE 2 > +#define FLOWSPEC_TYPE_PROTO 3 > +#define FLOWSPEC_TYPE_PORT 4 > +#define FLOWSPEC_TYPE_DST_PORT 5 > +#define FLOWSPEC_TYPE_SRC_PORT 6 > +#define FLOWSPEC_TYPE_ICMP_TYPE 7 > +#define FLOWSPEC_TYPE_ICMP_CODE 8 > +#define FLOWSPEC_TYPE_TCP_FLAGS 9 > +#define FLOWSPEC_TYPE_PKT_LEN 10 > +#define FLOWSPEC_TYPE_DSCP 11 > +#define FLOWSPEC_TYPE_FRAG 12 > +#define FLOWSPEC_TYPE_FLOW 13 > +#define FLOWSPEC_TYPE_MAX 13 > > struct filter_prefix { > struct bgpd_addr addr; [...] > Index: rde.c [...] > + case AID_FLOWSPECv4: > + case AID_FLOWSPECv6: > + /* nexthop must be 0 and ignored for flowspec */ > + if (nhlen != 0) { > + log_warnx("bad %s nexthop, bad size %d", aid2str(aid), > + nhlen); > + return (-1); > + } > + totlen += nhlen + 1; > + return (totlen); It's fine as it is, but since nhlen == 0 I was a bit taken aback. Perhaps the previous two lines could be replaced with: return (totlen + 1); > default: > log_warnx("bad multiprotocol nexthop, bad AID"); > return (-1); > } > > - nexthop_unref(state->nexthop); /* just to be sure */ > state->nexthop = nexthop_get(&nexthop); > > /* ignore reserved (old SNPA) field as per RFC4760 */ > totlen += nhlen + 1; > - data += nhlen + 1; > > return (totlen); > }