Re: bgpd: filter as path with operators

2016-06-03 Thread Stuart Henderson
On 2016/05/31 09:07, Peter Hessler wrote:
> This feature came about because of a talk from Job at NTT during RIPE72,
> where they will be introducing exactly this ruleset on all of their
> links starting in July.

KPN and GTT will also be doing this.



Re: bgpd: filter as path with operators

2016-06-01 Thread Sebastian Benoit
with feedback from florian, sthen and claudio:

- i removed operators < <= > >=
- i kept != and = for symmetry.
- i thought about just using ! , but then it would be different from the
  prefix operators. Willing to change it if you want that.
- i left the forth argument to aspath_match(), as its easier to undestand
  than other things i could think of
- i removed a debug comment
- the usecase for this is mostly whats shown in the example. I think i can
  also simplyfy some filters setting policy with != in my configs.
- as phessler said, the example filters any AS that should not be seen in
  the wild at the moment. You want those filtered because its bad practice
  not to. Also, if you use private AS inside your network, the example
  config will remind you to make sure not to leak them. I'm reasonably
  convinced that these will not change in the near future.

ok?

diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
index 8ffa8a8..02a31f9 100644
--- etc/examples/bgpd.conf
+++ etc/examples/bgpd.conf
@@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7   
# unique local unicast
 deny from any prefix fe80::/10 prefixlen >= 10 # link local unicast
 deny from any prefix fec0::/10 prefixlen >= 10 # old site local unicast
 deny from any prefix ff00::/8 prefixlen >= 8   # multicast
+
+# filter bogon AS numbers
+# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
+deny from any AS 23456 # AS_TRANS
+deny from any AS 64496 - 64511 # Reserved for use in docs and 
code RFC5398
+deny from any AS 64512 - 65534 # Reserved for Private Use 
RFC6996
+deny from any AS 65535 # Reserved RFC7300
+deny from any AS 65536 - 65551 # Reserved for use in docs and 
code RFC5398 
+deny from any AS 65552 - 131071# Reserved
+deny from any AS 42 - 4294967294   # Reserved for Private Use 
RFC6996
+deny from any AS 4294967295# Reserved RFC7300
diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
index 62569bf..afdcf2c 100644
--- usr.sbin/bgpctl/bgpctl.c
+++ usr.sbin/bgpctl/bgpctl.c
@@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
if (req->flags & F_CTL_DETAIL) {
@@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
bzero(, sizeof(net));
diff --git usr.sbin/bgpd/bgpd.conf.5 usr.sbin/bgpd/bgpd.conf.5
index 84a01ed..126a037 100644
--- usr.sbin/bgpd/bgpd.conf.5
+++ usr.sbin/bgpd/bgpd.conf.5
@@ -1027,7 +1027,10 @@ If a parameter is specified, the rule only applies to 
packets with
 matching attributes.
 .Pp
 .Bl -tag -width Ds -compact
-.It Ar as-type as-number
+.It Xo
+.Ar as-type Op Ar operator
+.Ar as-number
+.Xc
 This rule applies only to
 .Em UPDATES
 where the
@@ -1038,13 +1041,7 @@ The
 is matched against a part of the
 .Em AS path
 specified by the
-.Ar as-type .
-.Ar as-number
-may be set to
-.Ic neighbor-as ,
-which is expanded to the current neighbor remote AS number.
-.Ar as-type
-is one of the following operators:
+.Ar as-type :
 .Pp
 .Bl -tag -width transmit-as -compact
 .It Ic AS
@@ -1057,6 +1054,29 @@ is one of the following operators:
 (all but the rightmost AS number)
 .El
 .Pp
+.Ar as-number
+is an AS number as explained above under
+.Sx GLOBAL CONFIGURATION .
+It may be set to
+.Ic neighbor-as ,
+which is expanded to the current neighbor remote AS number.
+.Pp
+The
+.Ar operator
+can be unspecified (this case is identical to the equality operator), or one 
of the numerical operators
+.Bd -literal -offset indent
+=  (equal)
+!= (unequal)
+-  (range including boundaries)
+>< (except range)
+.Ed
+.Pp
+>< and -
+are binary operators (they take two arguments), with these,
+.Ar as-number
+cannot be set to
+.Ic neighbor-as .
+.Pp
 Multiple
 .Ar as-number
 entries for a given type or
diff --git usr.sbin/bgpd/bgpd.h usr.sbin/bgpd/bgpd.h
index 86dfee9..9c635a8 100644
--- usr.sbin/bgpd/bgpd.h
+++ usr.sbin/bgpd/bgpd.h
@@ -625,6 +625,9 @@ struct filter_as {
u_int32_t   as;
u_int16_t   flags;
enum as_spectype;
+   u_int8_top;
+   u_int32_t   as_min;
+   u_int32_t   as_max;
 };
 
 struct filter_aslen {
@@ -660,7 +663,6 @@ struct filter_extcommunity {
}   data;
 };
 
-
 struct 

Re: bgpd: filter as path with operators

2016-05-31 Thread Sebastian Benoit
Claudio Jeker(cje...@diehard.n-r-g.com) on 2016.05.31 08:10:22 +0200:
> On Mon, May 30, 2016 at 10:43:49PM +0200, Sebastian Benoit wrote:
> > Hi,
> > 
> > this allows to have
> > 
> >   allow from any AS 64512 - 65534 ...
> >   allow from any AS > 100
> > 
> > etc in bgpd.conf.
> > 
> > Ignore the example file for now, i will commit that seperatly anyway.
> > 
> > One obvious improvment would be to be able to use this in bgpctl to restrict
> > the output of "show rib" a bit more. However, that is for another commit i
> > think, the bgpctl bit in this diff is just to make it compile and work.
> > 
> > ok?
> 
> I see no reason to add the unary operators (>, <, ...). Please provide a
> real live example that makes sense. IMO there is no reason to do something
> like AS >  because the selectivity is random.

Yes, i wanted the - and ><, i put the others in because that allows me to
use them in 'bgpctl sh rib', which i thought might be nice there. I planned
to change bgpctl seperatly though.

Opinons? Its easy enough to remove them again.

> I don't mind the - and >< operators and I think they make sense to use.
> I just don't see the point of added complexity for a case that never
> happens.
> 
> more comments inline
>  
> > /Benno
> > 
> > diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
> > index 8ffa8a8..02a31f9 100644
> > --- etc/examples/bgpd.conf
> > +++ etc/examples/bgpd.conf
> > @@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7   
> > # unique local unicast
> >  deny from any prefix fe80::/10 prefixlen >= 10 # link local 
> > unicast
> >  deny from any prefix fec0::/10 prefixlen >= 10 # old site 
> > local unicast
> >  deny from any prefix ff00::/8 prefixlen >= 8   # multicast
> > +
> > +# filter bogon AS numbers
> > +# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
> > +deny from any AS 23456 # AS_TRANS
> > +deny from any AS 64496 - 64511 # Reserved for use in 
> > docs and code RFC5398
> > +deny from any AS 64512 - 65534 # Reserved for Private 
> > Use RFC6996
> > +deny from any AS 65535 # Reserved RFC7300
> > +deny from any AS 65536 - 65551 # Reserved for use in 
> > docs and code RFC5398 
> > +deny from any AS 65552 - 131071# Reserved
> > +deny from any AS 42 - 4294967294   # Reserved for Private Use 
> > RFC6996
> > +deny from any AS 4294967295# Reserved RFC7300
> 
> 
> Did you check how many pathes in a regular feed hit any of those rules?

Yes, AS_TRANS is in the wild constantly, the others appear from time, probably
because spammers try things. As they are not legaly to be used on the public
net there should be a way to block them, if only to spare yourself from
public embarrassment.

> I have seen a few pathes with private or AS_TRANS ASs in them in the wild.
> For a default filterset this may be a bit too restrictive.

Thats why i said i would leave that out of the commit for seperate
discussion.

> > diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
> > index 62569bf..afdcf2c 100644
> > --- usr.sbin/bgpctl/bgpctl.c
> > +++ usr.sbin/bgpctl/bgpctl.c
> > @@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer 
> > *mp, void *arg)
> > /* filter by AS */
> > if (req->as.type != AS_NONE &&
> >!aspath_match(mre->aspath, mre->aspath_len,
> > -  req->as.type, req->as.as))
> > +  >as, req->as.as))
> > continue;
> >  
> > if (req->flags & F_CTL_DETAIL) {
> > @@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer 
> > *mp, void *arg)
> > /* filter by AS */
> > if (req->as.type != AS_NONE &&
> >!aspath_match(mre->aspath, mre->aspath_len,
> > -  req->as.type, req->as.as))
> > +  >as, req->as.as))
> 
> Would be nice if it is not required to pass both the as obj and the
> req->as.as but them matching on neighbor-as needs some work...
> Anyway this is a bikeshed problem.

It is needed in the neighbor-as case. I thought leaving it as it is makes it
obvious that there is a special case.

I will resend the diff when i know if i should throw out =,>=,> etc...

> > continue;
> >  
> > bzero(, sizeof(net));
> > diff --git usr.sbin/bgpd/bgpd.conf.5 usr.sbin/bgpd/bgpd.conf.5
> > index 84a01ed..0017124 100644
> > --- usr.sbin/bgpd/bgpd.conf.5
> > +++ usr.sbin/bgpd/bgpd.conf.5
> > @@ -1027,7 +1027,10 @@ If a parameter is specified, the rule only applies 
> > to packets with
> >  matching attributes.
> >  .Pp
> >  .Bl -tag -width Ds -compact
> > -.It Ar as-type as-number
> > +.It Xo
> > +.Ar as-type Op Ar operator
> > +.Ar as-number
> > +.Xc
> >  This rule applies only to
> >  .Em UPDATES
> >  where the
> > @@ -1038,13 +1041,7 @@ The
> 

Re: bgpd: filter as path with operators

2016-05-31 Thread Peter Hessler
On 2016 May 31 (Tue) at 08:10:22 +0200 (+0200), Claudio Jeker wrote:
:On Mon, May 30, 2016 at 10:43:49PM +0200, Sebastian Benoit wrote:
:> /Benno
:> 
:> diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
:> index 8ffa8a8..02a31f9 100644
:> --- etc/examples/bgpd.conf
:> +++ etc/examples/bgpd.conf
:> @@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7
# unique local unicast
:>  deny from any prefix fe80::/10 prefixlen >= 10  # link local 
unicast
:>  deny from any prefix fec0::/10 prefixlen >= 10  # old site 
local unicast
:>  deny from any prefix ff00::/8 prefixlen >= 8# multicast
:> +
:> +# filter bogon AS numbers
:> +# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
:> +deny from any AS 23456  # AS_TRANS
:> +deny from any AS 64496 - 64511  # Reserved for use in 
docs and code RFC5398
:> +deny from any AS 64512 - 65534  # Reserved for Private 
Use RFC6996
:> +deny from any AS 65535  # Reserved RFC7300
:> +deny from any AS 65536 - 65551  # Reserved for use in 
docs and code RFC5398 
:> +deny from any AS 65552 - 131071 # Reserved
:> +deny from any AS 42 - 4294967294# Reserved for Private Use 
RFC6996
:> +deny from any AS 4294967295 # Reserved RFC7300
:
:
:Did you check how many pathes in a regular feed hit any of those rules?
:I have seen a few pathes with private or AS_TRANS ASs in them in the wild.
:For a default filterset this may be a bit too restrictive.
:

This feature came about because of a talk from Job at NTT during RIPE72,
where they will be introducing exactly this ruleset on all of their
links starting in July.


-- 
This life is a test.  It is only a test.  Had this been an actual life,
you would have received further instructions as to what to do and where
to go.



Re: bgpd: filter as path with operators

2016-05-31 Thread Claudio Jeker
On Mon, May 30, 2016 at 10:43:49PM +0200, Sebastian Benoit wrote:
> Hi,
> 
> this allows to have
> 
>   allow from any AS 64512 - 65534 ...
>   allow from any AS > 100
> 
> etc in bgpd.conf.
> 
> Ignore the example file for now, i will commit that seperatly anyway.
> 
> One obvious improvment would be to be able to use this in bgpctl to restrict
> the output of "show rib" a bit more. However, that is for another commit i
> think, the bgpctl bit in this diff is just to make it compile and work.
> 
> ok?

I see no reason to add the unary operators (>, <, ...). Please provide a
real live example that makes sense. IMO there is no reason to do something
like AS >  because the selectivity is random.
I don't mind the - and >< operators and I think they make sense to use.
I just don't see the point of added complexity for a case that never
happens.

more comments inline
 
> /Benno
> 
> diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
> index 8ffa8a8..02a31f9 100644
> --- etc/examples/bgpd.conf
> +++ etc/examples/bgpd.conf
> @@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7 
> # unique local unicast
>  deny from any prefix fe80::/10 prefixlen >= 10   # link local 
> unicast
>  deny from any prefix fec0::/10 prefixlen >= 10   # old site 
> local unicast
>  deny from any prefix ff00::/8 prefixlen >= 8 # multicast
> +
> +# filter bogon AS numbers
> +# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
> +deny from any AS 23456   # AS_TRANS
> +deny from any AS 64496 - 64511   # Reserved for use in 
> docs and code RFC5398
> +deny from any AS 64512 - 65534   # Reserved for Private 
> Use RFC6996
> +deny from any AS 65535   # Reserved RFC7300
> +deny from any AS 65536 - 65551   # Reserved for use in 
> docs and code RFC5398 
> +deny from any AS 65552 - 131071  # Reserved
> +deny from any AS 42 - 4294967294 # Reserved for Private Use 
> RFC6996
> +deny from any AS 4294967295  # Reserved RFC7300


Did you check how many pathes in a regular feed hit any of those rules?
I have seen a few pathes with private or AS_TRANS ASs in them in the wild.
For a default filterset this may be a bit too restrictive.

> diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
> index 62569bf..afdcf2c 100644
> --- usr.sbin/bgpctl/bgpctl.c
> +++ usr.sbin/bgpctl/bgpctl.c
> @@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
> void *arg)
>   /* filter by AS */
>   if (req->as.type != AS_NONE &&
>  !aspath_match(mre->aspath, mre->aspath_len,
> -req->as.type, req->as.as))
> +>as, req->as.as))
>   continue;
>  
>   if (req->flags & F_CTL_DETAIL) {
> @@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer 
> *mp, void *arg)
>   /* filter by AS */
>   if (req->as.type != AS_NONE &&
>  !aspath_match(mre->aspath, mre->aspath_len,
> -req->as.type, req->as.as))
> +>as, req->as.as))

Would be nice if it is not required to pass both the as obj and the
req->as.as but them matching on neighbor-as needs some work...
Anyway this is a bikeshed problem.

>   continue;
>  
>   bzero(, sizeof(net));
> diff --git usr.sbin/bgpd/bgpd.conf.5 usr.sbin/bgpd/bgpd.conf.5
> index 84a01ed..0017124 100644
> --- usr.sbin/bgpd/bgpd.conf.5
> +++ usr.sbin/bgpd/bgpd.conf.5
> @@ -1027,7 +1027,10 @@ If a parameter is specified, the rule only applies to 
> packets with
>  matching attributes.
>  .Pp
>  .Bl -tag -width Ds -compact
> -.It Ar as-type as-number
> +.It Xo
> +.Ar as-type Op Ar operator
> +.Ar as-number
> +.Xc
>  This rule applies only to
>  .Em UPDATES
>  where the
> @@ -1038,13 +1041,7 @@ The
>  is matched against a part of the
>  .Em AS path
>  specified by the
> -.Ar as-type .
> -.Ar as-number
> -may be set to
> -.Ic neighbor-as ,
> -which is expanded to the current neighbor remote AS number.
> -.Ar as-type
> -is one of the following operators:
> +.Ar as-type :
>  .Pp
>  .Bl -tag -width transmit-as -compact
>  .It Ic AS
> @@ -1057,6 +1054,33 @@ is one of the following operators:
>  (all but the rightmost AS number)
>  .El
>  .Pp
> +.Ar as-number
> +is an AS number as explained above under
> +.Sx GLOBAL CONFIGURATION .
> +It may be set to
> +.Ic neighbor-as ,
> +which is expanded to the current neighbor remote AS number.
> +.Pp
> +The
> +.Ar operator
> +can be unspecified (this case is identical to the equality operator), or one 
> of the numerical operators
> +.Bd -literal -offset indent
> +=(equal)
> +!=   (unequal)
> +<(less than)
> +<=   (less than or equal)
> +>(greater than)
> +>=   (greater than or equal)
> +-(range including boundaries)
> +><   (except 

bgpd: filter as path with operators

2016-05-30 Thread Sebastian Benoit
Hi,

this allows to have

  allow from any AS 64512 - 65534 ...
  allow from any AS > 100

etc in bgpd.conf.

Ignore the example file for now, i will commit that seperatly anyway.

One obvious improvment would be to be able to use this in bgpctl to restrict
the output of "show rib" a bit more. However, that is for another commit i
think, the bgpctl bit in this diff is just to make it compile and work.

ok?

/Benno

diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
index 8ffa8a8..02a31f9 100644
--- etc/examples/bgpd.conf
+++ etc/examples/bgpd.conf
@@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7   
# unique local unicast
 deny from any prefix fe80::/10 prefixlen >= 10 # link local unicast
 deny from any prefix fec0::/10 prefixlen >= 10 # old site local unicast
 deny from any prefix ff00::/8 prefixlen >= 8   # multicast
+
+# filter bogon AS numbers
+# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
+deny from any AS 23456 # AS_TRANS
+deny from any AS 64496 - 64511 # Reserved for use in docs and 
code RFC5398
+deny from any AS 64512 - 65534 # Reserved for Private Use 
RFC6996
+deny from any AS 65535 # Reserved RFC7300
+deny from any AS 65536 - 65551 # Reserved for use in docs and 
code RFC5398 
+deny from any AS 65552 - 131071# Reserved
+deny from any AS 42 - 4294967294   # Reserved for Private Use 
RFC6996
+deny from any AS 4294967295# Reserved RFC7300
diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
index 62569bf..afdcf2c 100644
--- usr.sbin/bgpctl/bgpctl.c
+++ usr.sbin/bgpctl/bgpctl.c
@@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
if (req->flags & F_CTL_DETAIL) {
@@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
bzero(, sizeof(net));
diff --git usr.sbin/bgpd/bgpd.conf.5 usr.sbin/bgpd/bgpd.conf.5
index 84a01ed..0017124 100644
--- usr.sbin/bgpd/bgpd.conf.5
+++ usr.sbin/bgpd/bgpd.conf.5
@@ -1027,7 +1027,10 @@ If a parameter is specified, the rule only applies to 
packets with
 matching attributes.
 .Pp
 .Bl -tag -width Ds -compact
-.It Ar as-type as-number
+.It Xo
+.Ar as-type Op Ar operator
+.Ar as-number
+.Xc
 This rule applies only to
 .Em UPDATES
 where the
@@ -1038,13 +1041,7 @@ The
 is matched against a part of the
 .Em AS path
 specified by the
-.Ar as-type .
-.Ar as-number
-may be set to
-.Ic neighbor-as ,
-which is expanded to the current neighbor remote AS number.
-.Ar as-type
-is one of the following operators:
+.Ar as-type :
 .Pp
 .Bl -tag -width transmit-as -compact
 .It Ic AS
@@ -1057,6 +1054,33 @@ is one of the following operators:
 (all but the rightmost AS number)
 .El
 .Pp
+.Ar as-number
+is an AS number as explained above under
+.Sx GLOBAL CONFIGURATION .
+It may be set to
+.Ic neighbor-as ,
+which is expanded to the current neighbor remote AS number.
+.Pp
+The
+.Ar operator
+can be unspecified (this case is identical to the equality operator), or one 
of the numerical operators
+.Bd -literal -offset indent
+=  (equal)
+!= (unequal)
+<  (less than)
+<= (less than or equal)
+>  (greater than)
+>= (greater than or equal)
+-  (range including boundaries)
+>< (except range)
+.Ed
+.Pp
+>< and -
+are binary operators (they take two arguments), with these,
+.Ar as-number
+cannot be set to
+.Ic neighbor-as .
+.Pp
 Multiple
 .Ar as-number
 entries for a given type or
diff --git usr.sbin/bgpd/bgpd.h usr.sbin/bgpd/bgpd.h
index 86dfee9..9c635a8 100644
--- usr.sbin/bgpd/bgpd.h
+++ usr.sbin/bgpd/bgpd.h
@@ -625,6 +625,9 @@ struct filter_as {
u_int32_t   as;
u_int16_t   flags;
enum as_spectype;
+   u_int8_top;
+   u_int32_t   as_min;
+   u_int32_t   as_max;
 };
 
 struct filter_aslen {
@@ -660,7 +663,6 @@ struct filter_extcommunity {
}   data;
 };
 
-
 struct ctl_show_rib_request {
charrib[PEER_DESCR_LEN];
struct ctl_neighbor neighbor;
@@ -1051,7 +1053,8 @@ const char*log_ext_subtype(u_int8_t);
 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