From b9bc4f19acc21d5d7beba124a1ff504bc683aa87 Mon Sep 17 00:00:00 2001 From: Thomas Mangin Date: Tue, 24 Sep 2019 01:20:32 +0100 Subject: [PATCH] large change required to perform better route validation for annoucement only --- CHANGELOG | 3 ++ lib/exabgp/configuration/announce/__init__.py | 12 ++++++++ lib/exabgp/configuration/announce/ip.py | 26 ++++++++--------- lib/exabgp/configuration/announce/label.py | 29 ++++++++++--------- lib/exabgp/configuration/announce/path.py | 23 +++++++-------- lib/exabgp/configuration/announce/vpls.py | 12 ++------ lib/exabgp/configuration/announce/vpn.py | 25 +++++++++------- lib/exabgp/configuration/configuration.py | 3 +- lib/exabgp/configuration/core/tokeniser.py | 6 ++++ lib/exabgp/configuration/l2vpn/vpls.py | 4 +-- lib/exabgp/configuration/static/__init__.py | 29 ++++++++++++++----- lib/exabgp/configuration/static/parser.py | 4 +++ lib/exabgp/reactor/api/__init__.py | 2 +- 13 files changed, 106 insertions(+), 72 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 64f4f7c91..feb1f1a87 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -6,6 +6,9 @@ Version explained: Version 4.1.3: * Feature: "rate-limit" (per neighbor) limit the number of BGP message(s) handled per second * Fix: use local IP for router-id when the peer is auto-deteted (and not the remote IP) + * Fix: potential python3/python2 bytes vs string issues when generating updates + * Fix: label is mandatory when using RD, force it, and perform better checks on the configuration + * CHANGE: large change to the configuration code (should not have any effect but the devil is in the details) Version 4.1.2 * Feature: exabgpcli autocomplete diff --git a/lib/exabgp/configuration/announce/__init__.py b/lib/exabgp/configuration/announce/__init__.py index 7e90d3829..26e8b5de9 100644 --- a/lib/exabgp/configuration/announce/__init__.py +++ b/lib/exabgp/configuration/announce/__init__.py @@ -28,6 +28,9 @@ def pack_int (afi, integer): class ParseAnnounce (Section): + syntax = '' + afi = None + def post (self): self._split() routes = self.scope.pop(self.name) @@ -87,6 +90,15 @@ def _split (self): for splat in self.split(route): self.scope.append_route(splat) + def _check(self): + if not self.check(self.scope.get(self.name), self.afi): + return self.error.set(self.syntax) + return True + + @staticmethod + def check (change,afi): + raise RuntimeError('need to be implemented by subclasses') + class SectionAnnounce (ParseAnnounce): name = 'announce' diff --git a/lib/exabgp/configuration/announce/ip.py b/lib/exabgp/configuration/announce/ip.py index c52bea149..635cfec8a 100644 --- a/lib/exabgp/configuration/announce/ip.py +++ b/lib/exabgp/configuration/announce/ip.py @@ -119,7 +119,6 @@ class AnnounceIP (ParseAnnounce): } name = 'ip' - afi = None def __init__ (self, tokeniser, scope, error, logger): ParseAnnounce.__init__(self,tokeniser,scope,error,logger) @@ -131,27 +130,24 @@ def pre (self): return True def post (self): - return True - - def _check (self): - if not self.check(self.scope.get(self.name),self.afi): - return self.error.set(self.syntax) - return True + return self._check() @staticmethod def check (change,afi): - if change.nlri.nexthop is NoNextHop \ - and change.nlri.action == OUT.ANNOUNCE \ + if change.nlri.action == OUT.ANNOUNCE \ + and change.nlri.nexthop is NoNextHop \ and change.nlri.afi == afi \ and change.nlri.safi in (SAFI.unicast,SAFI.multicast): return False + return True def ip (tokeniser,afi,safi): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(tokeniser) - nlri = INET(afi,safi,OUT.ANNOUNCE) + nlri = INET(afi, safi, action) nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) change = Change( @@ -176,15 +172,19 @@ def ip (tokeniser,afi,safi): change.nlri.nexthop = nexthop change.attributes.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) + + if not AnnounceIP.check(change,afi): + raise ValueError('invalid announcement (missing next-hop ?)') return [change] def ip_multicast (tokeniser,afi,safi): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(tokeniser) - nlri = INET(afi,safi,OUT.ANNOUNCE) + nlri = INET(afi,safi,action) nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) change = Change( @@ -209,7 +209,7 @@ def ip_multicast (tokeniser,afi,safi): change.nlri.nexthop = nexthop change.attributes.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) return [change] diff --git a/lib/exabgp/configuration/announce/label.py b/lib/exabgp/configuration/announce/label.py index 1243bf0df..cb92c810d 100644 --- a/lib/exabgp/configuration/announce/label.py +++ b/lib/exabgp/configuration/announce/label.py @@ -17,6 +17,7 @@ from exabgp.protocol.family import SAFI from exabgp.bgp.message.update.nlri.label import Label +from exabgp.bgp.message.update.nlri.qualifier import Labels from exabgp.bgp.message.update.nlri.cidr import CIDR from exabgp.bgp.message.update.attribute import Attributes @@ -27,7 +28,7 @@ from exabgp.configuration.static.mpls import label -class AnnounceLabel (ParseAnnounce): +class AnnounceLabel (AnnouncePath): # put next-hop first as it is a requirement atm definition = [ 'label <15 bits number>', @@ -53,30 +54,29 @@ class AnnounceLabel (ParseAnnounce): afi = None def __init__ (self, tokeniser, scope, error, logger): - ParseAnnounce.__init__(self,tokeniser,scope,error,logger) + AnnouncePath.__init__(self,tokeniser,scope,error,logger) def clear (self): return True - def _check (self): - if not self.check(self.scope.get(self.name),self.afi): - return self.error.set(self.syntax) - return True - @staticmethod def check (change,afi): - if change.nlri.nexthop is NoNextHop \ - and change.nlri.action == OUT.ANNOUNCE \ - and change.nlri.afi == afi \ - and change.nlri.safi in (SAFI.unicast,SAFI.multicast): + if not AnnouncePath.check(change,afi): + return False + + if change.nlri.action == OUT.ANNOUNCE \ + and change.nlri.has_label() \ + and change.nlri.labels is Labels.NOLABEL: return False + return True def ip_label (tokeniser,afi,safi): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(tokeniser) - nlri = Label(afi,safi,OUT.ANNOUNCE) + nlri = Label(afi, safi, action) nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) change = Change( @@ -101,7 +101,10 @@ def ip_label (tokeniser,afi,safi): change.nlri.nexthop = nexthop change.attributes.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) + + if not AnnounceLabel.check(change,afi): + raise ValueError('invalid announcement (missing next-hop or label ?)') return [change] diff --git a/lib/exabgp/configuration/announce/path.py b/lib/exabgp/configuration/announce/path.py index 81fab172a..b93e78d13 100644 --- a/lib/exabgp/configuration/announce/path.py +++ b/lib/exabgp/configuration/announce/path.py @@ -27,7 +27,7 @@ from exabgp.configuration.static.parser import path_information -class AnnouncePath (ParseAnnounce): +class AnnouncePath (AnnounceIP): # put next-hop first as it is a requirement atm definition = [ 'label <15 bits number>', @@ -53,30 +53,24 @@ class AnnouncePath (ParseAnnounce): afi = None def __init__ (self, tokeniser, scope, error, logger): - ParseAnnounce.__init__(self,tokeniser,scope,error,logger) + AnnounceIP.__init__(self,tokeniser,scope,error,logger) def clear (self): return True - def _check (self): - if not self.check(self.scope.get(self.name),self.afi): - return self.error.set(self.syntax) - return True - @staticmethod def check (change,afi): - if change.nlri.nexthop is NoNextHop \ - and change.nlri.action == OUT.ANNOUNCE \ - and change.nlri.afi == afi \ - and change.nlri.safi in (SAFI.unicast,SAFI.multicast): + if not AnnounceIP.check(change,afi): return False + return True def ip_unicast (tokeniser,afi,safi): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(tokeniser) - nlri = INET(afi,safi,OUT.ANNOUNCE) + nlri = INET(afi, safi, action) nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) change = Change( @@ -101,7 +95,10 @@ def ip_unicast (tokeniser,afi,safi): change.nlri.nexthop = nexthop change.attributes.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) + + if not AnnouncePath.check(change,afi): + raise ValueError('invalid announcement (missing next-hop ?)') return [change] diff --git a/lib/exabgp/configuration/announce/vpls.py b/lib/exabgp/configuration/announce/vpls.py index ec6643770..eda69dee4 100644 --- a/lib/exabgp/configuration/announce/vpls.py +++ b/lib/exabgp/configuration/announce/vpls.py @@ -142,18 +142,12 @@ def __init__ (self, tokeniser, scope, error, logger): def clear (self): return True - def _check (self): - if not self.check(self.scope.get(self.name),self.afi): - return self.error.set(self.syntax) - return True + def post (self): + return self._check() @staticmethod def check (change,afi): - # if change.nlri.nexthop is NoNextHop \ - # and change.nlri.action == OUT.ANNOUNCE \ - # and change.nlri.afi == afi \ - # and change.nlri.safi in (SAFI.unicast,SAFI.multicast): - # return False + # No check performed :-( return True diff --git a/lib/exabgp/configuration/announce/vpn.py b/lib/exabgp/configuration/announce/vpn.py index 408444b32..bdf8201b1 100644 --- a/lib/exabgp/configuration/announce/vpn.py +++ b/lib/exabgp/configuration/announce/vpn.py @@ -18,6 +18,7 @@ from exabgp.bgp.message.update.nlri import IPVPN from exabgp.bgp.message.update.nlri.cidr import CIDR +from exabgp.bgp.message.update.nlri.qualifier import RouteDistinguisher from exabgp.bgp.message.update.attribute import Attributes from exabgp.configuration.announce import ParseAnnounce @@ -58,25 +59,24 @@ def __init__ (self, tokeniser, scope, error, logger): def clear (self): return True - def _check (self): - if not self.check(self.scope.get(self.name),self.afi): - return self.error.set(self.syntax) - return True - @staticmethod def check (change,afi): - if change.nlri.nexthop is NoNextHop \ - and change.nlri.action == OUT.ANNOUNCE \ - and change.nlri.afi == afi \ - and change.nlri.safi in (SAFI.unicast,SAFI.multicast): + if not AnnounceLabel.check(change,afi): + return False + + if change.nlri.action == OUT.ANNOUNCE \ + and change.nlri.has_rd() \ + and change.nlri.rd is RouteDistinguisher.NORD: return False + return True def ip_vpn (tokeniser,afi,safi): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(tokeniser) - nlri = IPVPN(afi,safi,OUT.ANNOUNCE) + nlri = IPVPN(afi, safi, action) nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) change = Change( @@ -101,7 +101,10 @@ def ip_vpn (tokeniser,afi,safi): change.nlri.nexthop = nexthop change.attributes.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) + + if not AnnounceVPN.check(change,afi): + raise ValueError('invalid announcement (missing next-hop, label or rd ?)') return [change] diff --git a/lib/exabgp/configuration/configuration.py b/lib/exabgp/configuration/configuration.py index 4b2292f5c..3069cd44f 100644 --- a/lib/exabgp/configuration/configuration.py +++ b/lib/exabgp/configuration/configuration.py @@ -482,10 +482,11 @@ def _link (self): if api[key]: self.processes[process].setdefault(key,[]).append(neighbor.router_id) - def partial (self, section, text): + def partial (self, section, text, action='announce'): self._cleanup() # this perform a big cleanup (may be able to be smarter) self._clear() self.tokeniser.set_api(text if text.endswith(';') or text.endswith('}') else text + ' ;') + self.tokeniser.set_action(action) if self.parseSection(section) is not True: self._rollback_reload() diff --git a/lib/exabgp/configuration/core/tokeniser.py b/lib/exabgp/configuration/core/tokeniser.py index c8d6f4f18..7217160d1 100644 --- a/lib/exabgp/configuration/core/tokeniser.py +++ b/lib/exabgp/configuration/core/tokeniser.py @@ -22,6 +22,7 @@ def __init__ (self): self.next = deque() self.tokens = [] self.generator = iter([]) + self.announce = True def replenish (self, content): self.next.clear() @@ -31,6 +32,7 @@ def replenish (self, content): def clear (self): self.replenish([]) + self.announce = True def __call__ (self): if self.next: @@ -154,6 +156,10 @@ def _source (data): def set_api (self, line): return self._set(self._tokenise(iter([line]))) + def set_action(self, command): + if command != 'announce': + self.iterate.announce = False + def __call__ (self): self.number += 1 try: diff --git a/lib/exabgp/configuration/l2vpn/vpls.py b/lib/exabgp/configuration/l2vpn/vpls.py index 94343ee65..142581104 100644 --- a/lib/exabgp/configuration/l2vpn/vpls.py +++ b/lib/exabgp/configuration/l2vpn/vpls.py @@ -136,9 +136,7 @@ def pre (self): return True def post (self): - if not self._check(): - return False - return True + return self._check() def _check (self): feedback = self.scope.get_route().feedback() diff --git a/lib/exabgp/configuration/static/__init__.py b/lib/exabgp/configuration/static/__init__.py index 977ebeea7..b3f58538c 100644 --- a/lib/exabgp/configuration/static/__init__.py +++ b/lib/exabgp/configuration/static/__init__.py @@ -10,6 +10,10 @@ from exabgp.configuration.static.route import ParseStaticRoute from exabgp.configuration.static.parser import prefix +from exabgp.configuration.announce.path import AnnouncePath +from exabgp.configuration.announce.label import AnnounceLabel +from exabgp.configuration.announce.vpn import AnnounceVPN + from exabgp.protocol.ip import IP from exabgp.protocol.family import SAFI @@ -49,14 +53,19 @@ def post (self): @ParseStatic.register('route','append-route') def route (tokeniser): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(tokeniser) + check = lambda change,afi: True if 'rd' in tokeniser.tokens or 'route-distinguisher' in tokeniser.tokens: - nlri = IPVPN(IP.toafi(ipmask.top()),SAFI.mpls_vpn,OUT.ANNOUNCE) + nlri = IPVPN(IP.toafi(ipmask.top()),SAFI.mpls_vpn,action) + check = AnnounceVPN.check elif 'label' in tokeniser.tokens: - nlri = Label(IP.toafi(ipmask.top()),SAFI.nlri_mpls,OUT.ANNOUNCE) + nlri = Label(IP.toafi(ipmask.top()),SAFI.nlri_mpls,action) + check = AnnounceLabel.check else: - nlri = INET(IP.toafi(ipmask.top()),IP.tosafi(ipmask.top()),OUT.ANNOUNCE) + nlri = INET(IP.toafi(ipmask.top()), IP.tosafi(ipmask.top()), action) + check = AnnouncePath.check nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) @@ -90,21 +99,25 @@ def route (tokeniser): change.nlri.nexthop = nexthop change.attributes.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) + + if not check(change,nlri.afi): + raise ValueError('invalid route (missing next-hop, label or rd ?)') return list(ParseStatic.split(change)) @ParseStatic.register('attributes','append-route') def attributes (tokeniser): + action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW ipmask = prefix(lambda: tokeniser.tokens[-1]) if 'rd' in tokeniser.tokens or 'route-distinguisher' in tokeniser.tokens: - nlri = IPVPN(IP.toafi(ipmask.top()),SAFI.mpls_vpn,OUT.ANNOUNCE) + nlri = IPVPN(IP.toafi(ipmask.top()), SAFI.mpls_vpn, action) elif 'label' in tokeniser.tokens: - nlri = Label(IP.toafi(ipmask.top()),SAFI.nlri_mpls,OUT.ANNOUNCE) + nlri = Label(IP.toafi(ipmask.top()), SAFI.nlri_mpls, action) else: - nlri = INET(IP.toafi(ipmask.top()),IP.tosafi(ipmask.top()),OUT.ANNOUNCE) + nlri = INET(IP.toafi(ipmask.top()), IP.tosafi(ipmask.top()), action) nlri.cidr = CIDR(ipmask.pack(),ipmask.mask) attr = Attributes() @@ -140,7 +153,7 @@ def attributes (tokeniser): nlri.nexthop = nexthop attr.add(attribute) else: - raise ValueError('route: unknown command "%s"' % command) + raise ValueError('unknown command "%s"' % command) changes = [] while True: diff --git a/lib/exabgp/configuration/static/parser.py b/lib/exabgp/configuration/static/parser.py index ed7769d2c..59b920038 100644 --- a/lib/exabgp/configuration/static/parser.py +++ b/lib/exabgp/configuration/static/parser.py @@ -94,6 +94,8 @@ def next_hop (tokeniser): ip = IP.create(value) return ip,NextHop(ip.top()) +# XXX: using OUT.UNSET should we use the following ? +# action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW def inet (tokeniser): ipmask = prefix(tokeniser) @@ -109,6 +111,8 @@ def inet (tokeniser): Attributes() ) +# XXX: using OUT.ANNOUNCE should we use the following ? +# action = OUT.ANNOUNCE if tokeniser.announce else OUT.WITHDRAW def mpls (tokeniser): ipmask = prefix(tokeniser) diff --git a/lib/exabgp/reactor/api/__init__.py b/lib/exabgp/reactor/api/__init__.py index 7dca14af1..fa12cf83c 100644 --- a/lib/exabgp/reactor/api/__init__.py +++ b/lib/exabgp/reactor/api/__init__.py @@ -50,7 +50,7 @@ def api_route (self, command): action, line = command.split(' ',1) self.configuration.static.clear() - if not self.configuration.partial('static',line): + if not self.configuration.partial('static',line,action): return [] if self.configuration.scope.location():