On Thu, Jun 07, 2018 at 12:14:07PM +0200, Claudio Jeker wrote:
> On Thu, Jun 07, 2018 at 10:20:17AM +0100, Stuart Henderson wrote:
> > On 2018/06/07 10:26, Claudio Jeker wrote:
> > > On Wed, Jun 06, 2018 at 11:04:56PM +0200, Claudio Jeker wrote:
> > > > So the announce keyword in bgpd is massivly overloaded.
> > > > It is one of the most common things new bgpd users are unsure about.
> > > > These are all possible announce options:
> > > > 
> > > > announce (IPv4|IPv6) (none|unicast|vpn)
> > > > announce as-4byte (yes|no)
> > > > announce capabilities (yes|no)
> > > > announce refresh (yes|no)
> > > > announce restart (yes|no)
> > > > and
> > > > announce (all|none|self|default-route)
> > > > 
> > > > Especially the last one is a bit confusing because it influences the way
> > > > the filter work. This was good in the beginning of bgpd but now I think
> > > > the time is right to move on. Filtering in general got more important 
> > > > and
> > > > using large-communities as a method to tag prefixes so that they can be
> > > > matched against those later is important.
> > > > People are easily confused by `announce IPv4 unicast` vs `announce all`
> > > > and so on.
> > > > 
> > > > The following diff does a few things.
> > > > a) it removes the `announce (all|none|self|default-route)` version
> > > > b) `announce none` is now `export none`
> > > > c) `announce default-route` is now `export default-route`
> > > > d) the examples file is adjusted accordingly and also the network
> > > >    statements are no setting a large-community which is used by the
> > > >    filters.
> > > > e) the filters are split in input and output and to a lesser extent
> > > >    ibgp vs ebgp.
> > > > f) since anounce self is gone the filters will now default deny (which 
> > > > is
> > > >    not a big thing and has the benefit that at start all routes show up 
> > > > in
> > > >    Adj-RIB-In first).
> > > > 
> > > > In general people using `announce all` can just remove the line. When
> > > > using `annouce self` more care is needed since the filters may need 
> > > > extra
> > > > adjustment. `announce none` and `announce default-route` are simple
> > > > renames, every thing else remains the same.
> > > > 
> > > 
> > > To be more precise what needs to be changed in your config.
> > > 
> > > - neighbors using 'announce none' just rename it to 'export none'
> > > - neighbors using 'announce default-route' just rename it to 'export
> > >   default-route'
> > > 
> > > For all other cases 'announce self' and 'announce all' needs to be removed
> > > and filters need to be put in place instead. A good example is in
> > > /etc/example/bgpd.conf. In short you want to permit ibgp in and out and
> > > for ebgp you want to only allow reasonable things in and only your
> > > announcements out. How is this done best is by tagging announcements with
> > > a community. I use large-community here because they are more expressive.
> > > 
> > > # tag announcements with a community
> > > network 10.0.1.0/24 set large-community $ASN:1:1
> > > 
> > > ... neighbor configs ...
> > > 
> > > # first inbound
> > > allow from ibgp
> > > # the example filters out a lot of bad prefixes here which is good
> > > # In general you don't trust the outside world so this is where you should
> > > # filter the most.
> > > allow from ebgp inet prefixlen 8 - 24
> > > allow from ebgp inet6 prefixlen 16 - 48
> > > # scrub our community range from ebgp
> > > match from ebgp set large-community delete $ASN:*:*
> > > 
> > > # now outbound filters
> > > # again ibgp is generally allowed
> > > allow to ibgp
> > > # for ebgp make sure to not leak any unintended networks so best is to
> > > # just allow the networks we tagged.
> > > allow to ebgp large-community $ASN:1:1
> > > # another option is to go eplicit by network
> > > allow to ebgp prefix 10.0.1.0/24
> > > 
> > > For a pure ibgp router the basic rules can be simplified to:
> > >    allow from ibgp
> > >    allow to ibgp
> > > 
> > > While most configs probably have inbound filters they may lack outbound
> > > filters. It is possible to check the effect of this change by adding
> > >    deny from any
> > >    deny to any
> > > at the start of your filters. `bgpctl show rib out nei myPeerName` is a
> > > great way to see what is announced before and after.
> > > 
> > > See this as an oportunity to bring your bgpd filter up to current
> > > standards.
> > > -- 
> > > :wq Claudio
> > > 
> > 
> > It would be helpful during upgrades if it's possible to write some
> > configurations that work the same on both the old and new versions.
> > That way the configuration can be changed to a version which will
> > still work before you get to the point where you can't simply revert
> > to the old config file). I think if the new version would accept
> > but ignore "announce all", that would be enough.
> 
> Yes, I can add a dummy "announce *" handler. In general people should be
> already able to write filters that work before and after the change.
> Happy to add a bit of code to ease the change but it also comes with a
> price that users may not adjust their configs because they don't fail to
> load.
>  
> > Remember that "announce self" is implicit for ebgp neighbours unless
> > there's another "announce" keyword, so people might be using it without
> > realising. It would not be very good if those people (who are probably
> > not dealing with BGP often) started redistributing all their transit
> > routes to peers ;)
> 
> I know, this is why there is also the change to default deny.
> Now if somebody uses implicit announce self with a ruleset that has
>   allow to any
> in it then this will blow up badly. Should not be the common case but
> I guess there is always one or two specialists out there. I spent some
> time thinking on ways to catch that but I did not find a solution that
> has no other major drawbacks.
> 
> The only way forward is that people start fixing their configs now and
> then when they upgrade there is no surprises.
> 

Here a version that just logs a warning for 'announce all' but fails for
all other usages.

-- 
:wq Claudio

Index: etc/examples/bgpd.conf
===================================================================
RCS file: /cvs/src/etc/examples/bgpd.conf,v
retrieving revision 1.8
diff -u -p -r1.8 bgpd.conf
--- etc/examples/bgpd.conf      29 Sep 2017 11:00:39 -0000      1.8
+++ etc/examples/bgpd.conf      6 Jun 2018 15:43:24 -0000
@@ -3,11 +3,12 @@
 # see bgpd.conf(5)
 
 #macros
+ASN="65001"
 peer1="10.1.0.2"
 peer2="10.1.0.3"
 
 # global configuration
-AS 65001
+AS $ASN
 router-id 10.0.0.1
 # holdtime 180
 # holdtime min 3
@@ -16,7 +17,10 @@ router-id 10.0.0.1
 # fib-update no
 # route-collector no
 # log updates
-# network 10.0.1.0/24
+
+# Announce networks, tag them with a large community to ease filtering 
+# network 10.0.1.0/24 set large-community $ASN:1:1
+# network static set large-community $ASN:1:2
 
 # restricted socket for bgplg(8)
 # socket "/var/www/run/bgpd.rsock" restricted
@@ -26,12 +30,10 @@ group "peering AS65002" {
        remote-as 65002
        neighbor $peer1 {
                descr   "AS 65001 peer 1"
-               announce self
                tcp md5sig password mekmitasdigoat
        }
        neighbor $peer2 {
                descr "AS 65001 peer 2"
-               announce all
                local-address 10.0.0.8
                ipsec esp ike
        }
@@ -79,14 +81,12 @@ neighbor 10.2.1.1 {
            aes 4e0f2f1b5c4e3c0d0e2f2d3b8c5c8f0b
 }
 
-# do not send or use routes from EBGP neighbors without
-# further explicit configuration
-deny from ebgp
-deny to ebgp
+##
+## inbound rules: default is deny
+##
 
-# allow updates to and from IBGP neighbors
+# IBGP: allow all updates from our neighbors
 allow from ibgp
-allow to ibgp
 
 # filter out prefixes longer than 24 or shorter than 8 bits for IPv4
 # and longer than 48 or shorter than 16 bits for IPv6.
@@ -144,3 +144,15 @@ deny from any AS 65536 - 65551                     # Reser
 deny from any AS 65552 - 131071                        # Reserved
 deny from any AS 4200000000 - 4294967294       # Reserved for Private Use 
RFC6996
 deny from any AS 4294967295                    # Reserved RFC7300
+
+##
+## outbound rules: default is deny
+##
+
+# IBGP: allow all updates to our neighbors
+allow to ibgp
+
+# EBGP: only allow self originated networks to ebgp peers
+# Don't leak any routes from upstream or peering sessions. This is done
+# by checking for routes that are tagged with the large-community $ASN:1:1
+allow to ebgp large-community $ASN:1:1
Index: usr.sbin/bgpd/bgpd.conf.5
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
retrieving revision 1.165
diff -u -p -r1.165 bgpd.conf.5
--- usr.sbin/bgpd/bgpd.conf.5   10 Feb 2018 11:19:09 -0000      1.165
+++ usr.sbin/bgpd/bgpd.conf.5   4 Jun 2018 19:29:55 -0000
@@ -638,41 +638,6 @@ There are several neighbor properties:
 .Bl -tag -width Ds -compact
 .It Xo
 .Ic announce
-.Sm off
-.Pq Ic all | none | self | default-route
-.Sm on
-.Xc
-If set to
-.Ic none ,
-no
-.Em UPDATE
-messages will be sent to the neighbor.
-If set to
-.Ic default-route ,
-only the default route will be announced to the neighbor.
-If set to
-.Ic all ,
-all generated
-.Em UPDATE
-messages will be sent to the neighbor.
-This is usually used for
-.Em transit AS's
-and
-.Em IBGP
-peers.
-The default value
-for
-.Em EBGP
-peers is
-.Ic self ,
-which limits the sent
-.Em UPDATE
-messages to announcements of the local AS.
-The default for IBGP peers is
-.Ic all .
-.Pp
-.It Xo
-.Ic announce
 .Pq Ic IPv4 Ns | Ns Ic IPv6
 .Pq Ic none Ns | Ns Ic unicast Ns | Ns Ic vpn
 .Xc
@@ -849,6 +814,21 @@ The default value for IBGP peers is
 otherwise the default is
 .Ic yes .
 .Pp
+.It Xo
+.Ic export
+.Sm off
+.Pq Ic none | default-route
+.Sm on
+.Xc
+If set to
+.Ic none ,
+no
+.Em UPDATE
+messages will be sent to the neighbor.
+If set to
+.Ic default-route ,
+only the default route will be announced to the neighbor.
+.Pp
 .It Ic holdtime Ar seconds
 Set the holdtime in seconds.
 Inherited from the global configuration if not given.
@@ -1077,6 +1057,7 @@ The last matching
 or
 .Ic deny
 rule decides what action is taken.
+The default action is to deny.
 .Pp
 The following actions can be used in the filter:
 .Bl -tag -width xxxxxxxx
Index: usr.sbin/bgpd/bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.317
diff -u -p -r1.317 bgpd.h
--- usr.sbin/bgpd/bgpd.h        10 Feb 2018 01:24:28 -0000      1.317
+++ usr.sbin/bgpd/bgpd.h        6 Jun 2018 13:22:30 -0000
@@ -239,12 +239,10 @@ struct bgpd_config {
 
 extern int cmd_opts;
 
-enum announce_type {
-       ANNOUNCE_UNDEF,
-       ANNOUNCE_SELF,
-       ANNOUNCE_NONE,
-       ANNOUNCE_DEFAULT_ROUTE,
-       ANNOUNCE_ALL
+enum export_type {
+       EXPORT_UNSET,
+       EXPORT_NONE,
+       EXPORT_DEFAULT_ROUTE
 };
 
 enum enforce_as {
@@ -318,7 +316,7 @@ struct peer_config {
        u_int32_t                remote_as;
        u_int32_t                local_as;
        u_int32_t                max_prefix;
-       enum announce_type       announce_type;
+       enum export_type         export_type;
        enum enforce_as          enforce_as;
        enum enforce_as          enforce_local_as;
        enum reconf_action       reconf_action;
Index: usr.sbin/bgpd/parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.320
diff -u -p -r1.320 parse.y
--- usr.sbin/bgpd/parse.y       26 Apr 2018 14:12:19 -0000      1.320
+++ usr.sbin/bgpd/parse.y       7 Jun 2018 10:44:35 -0000
@@ -189,7 +189,7 @@ typedef struct {
 %}
 
 %token AS ROUTERID HOLDTIME YMIN LISTEN ON FIBUPDATE FIBPRIORITY RTABLE
-%token RDOMAIN RD EXPORTTRGT IMPORTTRGT
+%token RDOMAIN RD EXPORT EXPORTTRGT IMPORTTRGT
 %token RDE RIB EVALUATE IGNORE COMPARE
 %token GROUP NEIGHBOR NETWORK
 %token EBGP IBGP
@@ -1222,21 +1222,26 @@ peeropts        : REMOTEAS as4number    {
                | ANNOUNCE AS4BYTE yesno {
                        curpeer->conf.capabilities.as4byte = $3;
                }
-               | ANNOUNCE SELF {
-                       curpeer->conf.announce_type = ANNOUNCE_SELF;
-               }
                | ANNOUNCE STRING {
-                       if (!strcmp($2, "self"))
-                               curpeer->conf.announce_type = ANNOUNCE_SELF;
-                       else if (!strcmp($2, "none"))
-                               curpeer->conf.announce_type = ANNOUNCE_NONE;
-                       else if (!strcmp($2, "all"))
-                               curpeer->conf.announce_type = ANNOUNCE_ALL;
+                       if (!strcmp($2, "all"))
+                               logit(LOG_ERR, "%s:%d: %s", file->name,
+                                   yylval.lineno,
+                                   "deprecated use of announce all");
+                       else {
+                               yyerror("syntax error");
+                               free($2);
+                               YYERROR;
+                       }
+                       free($2);
+               }
+               | EXPORT STRING {
+                       if (!strcmp($2, "none"))
+                               curpeer->conf.export_type = EXPORT_NONE;
                        else if (!strcmp($2, "default-route"))
-                               curpeer->conf.announce_type =
-                                   ANNOUNCE_DEFAULT_ROUTE;
+                               curpeer->conf.export_type =
+                                   EXPORT_DEFAULT_ROUTE;
                        else {
-                               yyerror("invalid announce type");
+                               yyerror("invalid export type");
                                free($2);
                                YYERROR;
                        }
@@ -2475,6 +2480,7 @@ lookup(char *s)
                { "enforce",            ENFORCE},
                { "esp",                ESP},
                { "evaluate",           EVALUATE},
+               { "export",             EXPORT},
                { "export-target",      EXPORTTRGT},
                { "ext-community",      EXTCOMMUNITY},
                { "fib-priority",       FIBPRIORITY},
@@ -3366,7 +3372,7 @@ alloc_peer(void)
        p->state = STATE_NONE;
        p->next = NULL;
        p->conf.distance = 1;
-       p->conf.announce_type = ANNOUNCE_UNDEF;
+       p->conf.export_type = EXPORT_UNSET;
        p->conf.announce_capa = 1;
        for (i = 0; i < AID_MAX; i++)
                p->conf.capabilities.mp[i] = -1;
@@ -3826,9 +3832,6 @@ neighbor_consistent(struct peer *p)
 
        /* set default values if they where undefined */
        p->conf.ebgp = (p->conf.remote_as != conf->as);
-       if (p->conf.announce_type == ANNOUNCE_UNDEF)
-               p->conf.announce_type = p->conf.ebgp ?
-                   ANNOUNCE_SELF : ANNOUNCE_ALL;
        if (p->conf.enforce_as == ENFORCE_AS_UNDEF)
                p->conf.enforce_as = p->conf.ebgp ?
                    ENFORCE_AS_ON : ENFORCE_AS_OFF;
Index: usr.sbin/bgpd/printconf.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v
retrieving revision 1.107
diff -u -p -r1.107 printconf.c
--- usr.sbin/bgpd/printconf.c   10 Feb 2018 01:24:28 -0000      1.107
+++ usr.sbin/bgpd/printconf.c   4 Jun 2018 19:22:51 -0000
@@ -500,16 +500,10 @@ print_peer(struct peer_config *p, struct
                printf("%s\tannounce restart no\n", c);
        if (p->capabilities.as4byte == 0)
                printf("%s\tannounce as4byte no\n", c);
-       if (p->announce_type == ANNOUNCE_SELF)
-               printf("%s\tannounce self\n", c);
-       else if (p->announce_type == ANNOUNCE_NONE)
-               printf("%s\tannounce none\n", c);
-       else if (p->announce_type == ANNOUNCE_ALL)
-               printf("%s\tannounce all\n", c);
-       else if (p->announce_type == ANNOUNCE_DEFAULT_ROUTE)
-               printf("%s\tannounce default-route\n", c);
-       else
-               printf("%s\tannounce ???\n", c);
+       if (p->export_type == EXPORT_NONE)
+               printf("%s\texport none\n", c);
+       else if (p->export_type == EXPORT_DEFAULT_ROUTE)
+               printf("%s\texport default-route\n", c);
        if (p->enforce_as == ENFORCE_AS_ON)
                printf("%s\tenforce neighbor-as yes\n", c);
        else
Index: usr.sbin/bgpd/rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.379
diff -u -p -r1.379 rde.c
--- usr.sbin/bgpd/rde.c 10 Feb 2018 05:54:31 -0000      1.379
+++ usr.sbin/bgpd/rde.c 4 Jun 2018 19:22:51 -0000
@@ -3538,7 +3538,9 @@ peer_dump(u_int32_t id, u_int8_t aid)
                return;
        }
 
-       if (peer->conf.announce_type == ANNOUNCE_DEFAULT_ROUTE)
+       if (peer->conf.export_type == EXPORT_NONE)
+               /* nothing */;
+       else if (peer->conf.export_type == EXPORT_DEFAULT_ROUTE)
                up_generate_default(out_rules, peer, aid);
        else
                rib_dump(peer->rib, rde_up_dump_upcall, peer, aid);
Index: usr.sbin/bgpd/rde_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
retrieving revision 1.86
diff -u -p -r1.86 rde_filter.c
--- usr.sbin/bgpd/rde_filter.c  10 Feb 2018 04:23:48 -0000      1.86
+++ usr.sbin/bgpd/rde_filter.c  4 Jun 2018 09:57:07 -0000
@@ -948,7 +948,7 @@ rde_filter(struct filter_head *rules, st
     u_int8_t prefixlen, struct rde_peer *from)
 {
        struct filter_rule      *f;
-       enum filter_actions      action = ACTION_ALLOW; /* default allow */
+       enum filter_actions      action = ACTION_DENY; /* default deny */
 
        if (new != NULL)
                *new = NULL;
Index: usr.sbin/bgpd/rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
retrieving revision 1.89
diff -u -p -r1.89 rde_update.c
--- usr.sbin/bgpd/rde_update.c  10 Feb 2018 05:54:31 -0000      1.89
+++ usr.sbin/bgpd/rde_update.c  6 Jun 2018 13:26:15 -0000
@@ -318,26 +318,14 @@ up_test_update(struct rde_peer *peer, st
                        return (0);
        }
 
-       /* announce type handling */
-       switch (peer->conf.announce_type) {
-       case ANNOUNCE_UNDEF:
-       case ANNOUNCE_NONE:
-       case ANNOUNCE_DEFAULT_ROUTE:
+       /* export type handling */
+       if (peer->conf.export_type == EXPORT_NONE ||
+           peer->conf.export_type == EXPORT_DEFAULT_ROUTE) {
                /*
                 * no need to withdraw old prefix as this will be
                 * filtered out as well.
                 */
                return (-1);
-       case ANNOUNCE_ALL:
-               break;
-       case ANNOUNCE_SELF:
-               /*
-                * pass only prefix that have an aspath count
-                * of zero this is equal to the ^$ regex.
-                */
-               if (asp->aspath->ascnt != 0)
-                       return (0);
-               break;
        }
 
        /* well known communities */

Reply via email to