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);
> }
