From a4de7e4c8dca2dc70da6bd8a8f4ecebca23340d3 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 15 Nov 2024 09:53:38 +0200 Subject: [PATCH 1/2] tests: Check if IPv6 prefix has a valid nexthop if multiple NLRIs exist Signed-off-by: Donatas Abraitis (cherry picked from commit 229466ebd11596c99902f3f63b8b03c17ab0e124) --- .../topotests/bgp_invalid_nexthop/__init__.py | 0 .../topotests/bgp_invalid_nexthop/exabgp.env | 53 +++++++++++ .../bgp_invalid_nexthop/peer1/exabgp.cfg | 19 ++++ .../topotests/bgp_invalid_nexthop/r1/frr.conf | 14 +++ .../test_bgp_invalid_nexthop.py | 90 +++++++++++++++++++ 5 files changed, 176 insertions(+) create mode 100644 tests/topotests/bgp_invalid_nexthop/__init__.py create mode 100644 tests/topotests/bgp_invalid_nexthop/exabgp.env create mode 100644 tests/topotests/bgp_invalid_nexthop/peer1/exabgp.cfg create mode 100644 tests/topotests/bgp_invalid_nexthop/r1/frr.conf create mode 100644 tests/topotests/bgp_invalid_nexthop/test_bgp_invalid_nexthop.py diff --git a/tests/topotests/bgp_invalid_nexthop/__init__.py b/tests/topotests/bgp_invalid_nexthop/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/topotests/bgp_invalid_nexthop/exabgp.env b/tests/topotests/bgp_invalid_nexthop/exabgp.env new file mode 100644 index 000000000000..28e642360a39 --- /dev/null +++ b/tests/topotests/bgp_invalid_nexthop/exabgp.env @@ -0,0 +1,53 @@ +[exabgp.api] +encoder = text +highres = false +respawn = false +socket = '' + +[exabgp.bgp] +openwait = 60 + +[exabgp.cache] +attributes = true +nexthops = true + +[exabgp.daemon] +daemonize = true +pid = '/var/run/exabgp/exabgp.pid' +user = 'exabgp' +##daemonize = false + +[exabgp.log] +all = false +configuration = true +daemon = true +destination = '/var/log/exabgp.log' +enable = true +level = INFO +message = false +network = true +packets = false +parser = false +processes = true +reactor = true +rib = false +routes = false +short = false +timers = false + +[exabgp.pdb] +enable = false + +[exabgp.profile] +enable = false +file = '' + +[exabgp.reactor] +speed = 1.0 + +[exabgp.tcp] +acl = false +bind = '' +delay = 0 +once = false +port = 179 diff --git a/tests/topotests/bgp_invalid_nexthop/peer1/exabgp.cfg b/tests/topotests/bgp_invalid_nexthop/peer1/exabgp.cfg new file mode 100644 index 000000000000..45b0f20d77a7 --- /dev/null +++ b/tests/topotests/bgp_invalid_nexthop/peer1/exabgp.cfg @@ -0,0 +1,19 @@ +neighbor fc00::1 { + router-id 10.0.0.2; + local-address fc00::2; + local-as 65002; + peer-as 65001; + group-updates false; + + family { + ipv4 unicast; + ipv6 unicast; + } + + static { + route 2001:db8:100::/64 { + next-hop 0.0.0.0; + next-hop fc00::2; + } + } +} diff --git a/tests/topotests/bgp_invalid_nexthop/r1/frr.conf b/tests/topotests/bgp_invalid_nexthop/r1/frr.conf new file mode 100644 index 000000000000..05e1a6c8259e --- /dev/null +++ b/tests/topotests/bgp_invalid_nexthop/r1/frr.conf @@ -0,0 +1,14 @@ +! +interface r1-eth0 + ip address fc00::1/64 +! +router bgp 65001 + bgp router-id 10.0.0.1 + no bgp default ipv4-unicast + no bgp ebgp-requires-policy + neighbor fc00::2 remote-as external + neighbor fc00::2 timers 3 10 + address-family ipv6 + neighbor fc00::2 activate + exit-address-family +! diff --git a/tests/topotests/bgp_invalid_nexthop/test_bgp_invalid_nexthop.py b/tests/topotests/bgp_invalid_nexthop/test_bgp_invalid_nexthop.py new file mode 100644 index 000000000000..ae482aa9c6a5 --- /dev/null +++ b/tests/topotests/bgp_invalid_nexthop/test_bgp_invalid_nexthop.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# +# Copyright (c) 2024 by +# Donatas Abraitis +# + +""" +""" + +import os +import sys +import json +import pytest +import functools + +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +from lib import topotest +from lib.topogen import Topogen, get_topogen + +pytestmark = [pytest.mark.bgpd] + + +def build_topo(tgen): + r1 = tgen.add_router("r1") + peer1 = tgen.add_exabgp_peer("peer1", ip="fc00::2/64", defaultRoute="via fc00::1") + + switch = tgen.add_switch("s1") + switch.add_link(r1) + switch.add_link(peer1) + + +def setup_module(mod): + tgen = Topogen(build_topo, mod.__name__) + tgen.start_topology() + + for _, (rname, router) in enumerate(tgen.routers().items(), 1): + router.load_frr_config(os.path.join(CWD, "{}/frr.conf".format(rname))) + + tgen.start_router() + + peer = tgen.gears["peer1"] + peer.start(os.path.join(CWD, "peer1"), os.path.join(CWD, "exabgp.env")) + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_invalid_nexthop(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r1 = tgen.gears["r1"] + + def _bgp_converge(): + output = json.loads(r1.vtysh_cmd("show bgp ipv6 unicast json")) + expected = { + "routes": { + "2001:db8:100::/64": [ + {"valid": True, "nexthops": [{"ip": "fc00::2", "afi": "ipv6"}]} + ] + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial(_bgp_converge) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "2001:db8:100::/64 does not have a valid nexthop" + + +def test_memory_leak(): + "Run the memory leak test and report results." + tgen = get_topogen() + if not tgen.is_memleak_enabled(): + pytest.skip("Memory leak test/report is disabled") + + tgen.report_memory_leaks() + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From 65a43b57efd60c4fdf80c935750046ba861ec79f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 15 Nov 2024 09:54:07 +0200 Subject: [PATCH 2/2] bgpd: Validate both nexthop information (NEXTHOP and NLRI) If we receive an IPv6 prefix e.g.: 2001:db8:100::/64 with nextop: 0.0.0.0, and mp_nexthop: fc00::2, we should not treat this with an invalid nexthop because of 0.0.0.0. We MUST check for MP_REACH attribute also and decide later if we have at least one a valid nexthop. Signed-off-by: Donatas Abraitis (cherry picked from commit a0d2734e879f78fbef5f1815429de331b9940c73) # Conflicts: # bgpd/bgp_route.c --- bgpd/bgp_route.c | 46 +++++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 619252b0c401..db309b7b7a7d 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -4406,7 +4406,7 @@ bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, uint8_t type, uint8_t stype, struct attr *attr, struct bgp_dest *dest) { - bool ret = false; + bool nh_invalid = false; bool is_bgp_static_route = (type == ZEBRA_ROUTE_BGP && stype == BGP_ROUTE_STATIC) ? true : false; @@ -4428,6 +4428,7 @@ bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, (safi != SAFI_UNICAST && safi != SAFI_MULTICAST && safi != SAFI_EVPN)) return false; +<<<<<<< HEAD /* If NEXT_HOP is present, validate it. */ if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) { if (attr->nexthop.s_addr == INADDR_ANY || @@ -4435,6 +4436,17 @@ bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, bgp_nexthop_self(bgp, afi, type, stype, attr, dest)) return true; } +======= + /* If NEXT_HOP is present, validate it: + * The route can have both nexthop + mp_nexthop encoded as multiple NLRIs, + * and we MUST check if at least one of them is valid. + * E.g.: IPv6 prefix can be with nexthop: 0.0.0.0, and mp_nexthop: fc00::1. + */ + if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP))) + nh_invalid = (attr->nexthop.s_addr == INADDR_ANY || + !ipv4_unicast_valid(&attr->nexthop) || + bgp_nexthop_self(bgp, afi, type, stype, attr, dest)); +>>>>>>> a0d2734e87 (bgpd: Validate both nexthop information (NEXTHOP and NLRI)) /* If MP_NEXTHOP is present, validate it. */ /* Note: For IPv6 nexthops, we only validate the global (1st) nexthop; @@ -4449,39 +4461,31 @@ bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, switch (attr->mp_nexthop_len) { case BGP_ATTR_NHLEN_IPV4: case BGP_ATTR_NHLEN_VPNV4: - ret = (attr->mp_nexthop_global_in.s_addr == - INADDR_ANY || - !ipv4_unicast_valid( - &attr->mp_nexthop_global_in) || - bgp_nexthop_self(bgp, afi, type, stype, attr, - dest)); + nh_invalid = (attr->mp_nexthop_global_in.s_addr == INADDR_ANY || + !ipv4_unicast_valid(&attr->mp_nexthop_global_in) || + bgp_nexthop_self(bgp, afi, type, stype, attr, dest)); break; case BGP_ATTR_NHLEN_IPV6_GLOBAL: case BGP_ATTR_NHLEN_VPNV6_GLOBAL: - ret = (IN6_IS_ADDR_UNSPECIFIED( - &attr->mp_nexthop_global) - || IN6_IS_ADDR_LOOPBACK(&attr->mp_nexthop_global) - || IN6_IS_ADDR_MULTICAST( - &attr->mp_nexthop_global) - || bgp_nexthop_self(bgp, afi, type, stype, attr, - dest)); + nh_invalid = (IN6_IS_ADDR_UNSPECIFIED(&attr->mp_nexthop_global) || + IN6_IS_ADDR_LOOPBACK(&attr->mp_nexthop_global) || + IN6_IS_ADDR_MULTICAST(&attr->mp_nexthop_global) || + bgp_nexthop_self(bgp, afi, type, stype, attr, dest)); break; case BGP_ATTR_NHLEN_IPV6_GLOBAL_AND_LL: - ret = (IN6_IS_ADDR_LOOPBACK(&attr->mp_nexthop_global) - || IN6_IS_ADDR_MULTICAST( - &attr->mp_nexthop_global) - || bgp_nexthop_self(bgp, afi, type, stype, attr, - dest)); + nh_invalid = (IN6_IS_ADDR_LOOPBACK(&attr->mp_nexthop_global) || + IN6_IS_ADDR_MULTICAST(&attr->mp_nexthop_global) || + bgp_nexthop_self(bgp, afi, type, stype, attr, dest)); break; default: - ret = true; + nh_invalid = true; break; } } - return ret; + return nh_invalid; } static void bgp_attr_add_no_export_community(struct attr *attr)