Merge pull request #17345 from FRRouting/mergify/bp/dev/10.2/pr-17115

nhrpd: fix passphrase handling, add topotest for resolution request (backport #17115)

nhrpd/nhrp_peer.c

@@ -1169,22 +1169,55 @@ static bool nhrp_connection_authorized(struct nhrp_packet_parser *pp)
 	struct nhrp_extension_header *ext;
 	struct zbuf *extensions, pl;
 	int cmp = 1;
+	int pl_pass_length, auth_pass_length;
+	size_t auth_size, pl_size;
 
 	extensions = zbuf_alloc(zbuf_used(&pp->extensions));
 	zbuf_copy_peek(extensions, &pp->extensions, zbuf_used(&pp->extensions));
 	while ((ext = nhrp_ext_pull(extensions, &pl)) != NULL) {
 		switch (htons(ext->type) & ~NHRP_EXTENSION_FLAG_COMPULSORY) {
 		case NHRP_EXTENSION_AUTHENTICATION:
-			cmp = memcmp(auth->buf, pl.buf, zbuf_size(auth));
+			/* Size of authentication extensions
+			 * (varies based on password length)
+			 */
+			auth_size = zbuf_size(auth);
+			pl_size = zbuf_size(&pl);
 			auth_ext = (struct nhrp_cisco_authentication_extension *)
 					   auth->buf;
+
+			if (auth_size == pl_size)
+				cmp = memcmp(auth_ext, pl.buf, auth_size);
+			else
+				cmp = 1;
+
+			if (unlikely(debug_flags & NHRP_DEBUG_COMMON)) {
+				/* 4 bytes in nhrp_cisco_authentication_extension are allocated
+				 * toward the authentication type. The remaining bytes are used for the
+				 * password - so the password length is just the length of the extension - 4
+				 */
+				auth_pass_length = (auth_size - 4);
+				pl_pass_length = (pl_size - 4);
+				/* Because characters are to be printed in HEX, (2* the max pass length) + 1
+				 * is needed for the string representation
+				 */
+				char auth_pass[(2 * NHRP_CISCO_PASS_LEN) + 1] = { 0 },
+					       pl_pass[(2 * NHRP_CISCO_PASS_LEN) + 1] = { 0 };
+				/* Converting bytes in buffer to HEX and saving output as a string -
+				 * Passphrase is converted to HEX in order to avoid printing
+				 * non ACII-compliant characters
+				 */
+				for (int i = 0; i < (auth_pass_length); i++)
+					snprintf(auth_pass + (i * 2), 3, "%02X",
+						 auth_ext->secret[i]);
+				for (int i = 0; i < (pl_pass_length); i++)
+					snprintf(pl_pass + (i * 2), 3, "%02X",
+						 ((struct nhrp_cisco_authentication_extension *)pl.buf)
+							 ->secret[i]);
+
 				debugf(NHRP_DEBUG_COMMON,
 				       "Processing Authentication Extension for (%s:%s|%d)",
-			       auth_ext->secret,
+				       auth_pass, pl_pass, cmp);
-			       ((struct nhrp_cisco_authentication_extension *)
+			}
-					pl.buf)
-				       ->secret,
-			       cmp);
 			break;
 		default:
 			/* Ignoring all received extensions except Authentication*/

nhrpd/nhrp_protocol.h

@@ -73,6 +73,7 @@
 
 /* NHRP Authentication extension types (ala Cisco) */
 #define NHRP_AUTHENTICATION_PLAINTEXT		0x00000001
+#define NHRP_CISCO_PASS_LEN			8
 
 /* NHRP Packet Structures */
 struct nhrp_packet_header {

nhrpd/nhrp_vty.c

@@ -467,7 +467,6 @@ DEFUN(if_no_nhrp_holdtime, if_no_nhrp_holdtime_cmd,
 	return CMD_SUCCESS;
 }
 
-#define NHRP_CISCO_PASS_LEN 8
 DEFPY(if_nhrp_authentication, if_nhrp_authentication_cmd,
       AFI_CMD "nhrp authentication PASSWORD$password",
       AFI_STR

tests/topotests/nhrp_topo/r1/nhrp_shortcut_present.json

@@ -0,0 +1,14 @@
+{
+  "attr":{
+    "entriesCount":1
+  },
+  "table":[
+    {
+      "type":"dynamic",
+      "prefix":"192.168.4.0\/24",
+      "via":"10.255.255.4",
+      "identity":""
+    }
+  ]
+}
+

tests/topotests/nhrp_topo/r1/zebra.conf

@@ -10,3 +10,4 @@ exit
 interface r1-eth1
  ip address 192.168.1.1/24
 !
+ip route 0.0.0.0/0 10.255.255.2

tests/topotests/nhrp_topo/r2/nhrp4_cache.json

@@ -1,8 +1,19 @@
 {
   "attr":{
-    "entriesCount":2
+    "entriesCount":3
   },
   "table":[
+    {
+      "interface":"r2-gre0",
+      "type":"dynamic",
+      "protocol":"10.255.255.4",
+      "nbma":"10.1.1.4",
+      "claimed_nbma":"10.1.1.4",
+      "used":false,
+      "timeout":true,
+      "auth":false,
+      "identity":""
+    },
     {
       "interface":"r2-gre0",
       "type":"local",

tests/topotests/nhrp_topo/r2/nhrp_route4.json

@@ -12,7 +12,31 @@
       "installed":true,
       "internalNextHopNum":1,
       "internalNextHopActiveNum":1,
-      "nexthops":[
+      "nexthops": [
+        {
+          "fib":true,
+          "directlyConnected":true,
+          "interfaceName":"r2-gre0",
+          "active":true
+        }
+      ]
+    }
+  ],
+  "10.255.255.4\/32": [
+    {
+      "prefix":"10.255.255.4\/32",
+      "prefixLen":32,
+      "protocol":"nhrp",
+      "vrfId":0,
+      "vrfName":"default",
+      "selected":true,
+      "destSelected":true,
+      "distance":10,
+      "metric":0,
+      "installed":true,
+      "internalNextHopNum":1,
+      "internalNextHopActiveNum":1,
+      "nexthops": [
         {
           "fib":true,
           "directlyConnected":true,

tests/topotests/nhrp_topo/r2/zebra.conf

@@ -1,3 +1,4 @@
+ip forwarding
 interface r2-eth0
  ip address 10.2.1.2/24
 !
@@ -10,3 +11,5 @@ interface r2-gre0
 interface r2-eth1
  ip address 192.168.2.2/24
 !
+ip route 192.168.4.4/24 10.255.255.4
+ip route 192.168.1.1/24 10.255.255.1

tests/topotests/nhrp_topo/r4/nhrp4_cache.json

@@ -0,0 +1,30 @@
+{
+  "attr":{
+    "entriesCount":2
+  },
+  "table":[
+    {
+      "interface":"r4-gre0",
+      "type":"local",
+      "protocol":"10.255.255.4",
+      "nbma":"10.1.1.4",
+      "claimed_nbma":"10.1.1.4",
+      "used":false,
+      "timeout":false,
+      "auth":false,
+      "identity":"-"
+    },
+    {
+      "interface":"r4-gre0",
+      "type":"nhs",
+      "protocol":"10.255.255.2",
+      "nbma":"10.2.1.2",
+      "claimed_nbma":"10.2.1.2",
+      "used":false,
+      "timeout":true,
+      "auth":false,
+      "identity":""
+    }
+  ]
+}
+

tests/topotests/nhrp_topo/r4/nhrp_route4.json

@@ -0,0 +1,26 @@
+{
+  "10.255.255.2\/32": [
+    {
+      "prefix": "10.255.255.2\/32",
+      "prefixLen": 32,
+      "protocol": "nhrp",
+      "vrfId": 0,
+      "vrfName": "default",
+      "selected": true,
+      "destSelected": true,
+      "distance": 10,
+      "metric": 0,
+      "installed": true,
+      "internalNextHopNum": 1,
+      "internalNextHopActiveNum": 1,
+      "nexthops": [
+        {
+          "fib": true,
+          "directlyConnected": true,
+          "interfaceName": "r4-gre0",
+          "active": true
+        }
+      ]
+    }
+  ]
+}

tests/topotests/nhrp_topo/r4/nhrpd.conf

@@ -0,0 +1,11 @@
+log stdout debugging
+debug nhrp all
+interface r4-gre0
+ ip nhrp authentication secret
+ ip nhrp holdtime 10
+ ip nhrp shortcut
+ ip nhrp network-id 42
+ ip nhrp nhs dynamic nbma 10.2.1.2
+ ip nhrp registration no-unique
+ tunnel source r4-eth0
+exit

tests/topotests/nhrp_topo/r4/zebra.conf

@@ -0,0 +1,13 @@
+interface r4-eth0
+ ip address 10.1.1.4/24
+!
+ip route 10.2.1.0/24 10.1.1.3
+interface r4-gre0
+ ip address 10.255.255.4/32
+ no link-detect
+ ipv6 nd suppress-ra
+exit
+interface r4-eth1
+ ip address 192.168.4.4/24
+!
+ip route 0.0.0.0/0 10.255.255.2

tests/topotests/nhrp_topo/test_nhrp_topo.py

@@ -33,18 +33,52 @@ from lib.common_config import required_linux_kernel_version, retry
 # Required to instantiate the topology builder class.
 
 pytestmark = [pytest.mark.nhrpd]
+TOPOLOGY = """
+                                              192.168.2.0/24
+                                             -----+-----
+                                                  |
+                                                  |
+                                                  |
+                                             +----------+
+                                             |          |
+                                             | R2       |
+                                             | NHS      |
+                                             +----------+
+                                                  | .2
+                                                  |
+                                                  |
+                                                  |
+            GRE P2MP Between                      + 10.2.1.0/24
+            Between Spokes and Hub                |
+                                                  |
+             10.255.255.x/32                 +----+-----+
+                                             |          |
+                                             | R3       |
+                                             |          |
+                                             +----+-----+
+                                                  |.3
+                                                  |
+                                                  |
+                             +----------+         |          +---------+
+               |             |          |         |          |         |       |
+               |             |R1        |         |          | R4      |       |
+192.168.1.0/24 +-------------|NHC       +---------+----------| NHC     | ------+ 192.168.4.0/24
+               |             |          |.1                .4|         |       |
+               |             +----------+      10.1.1.0/24   +---------+       |
+"""
 
 
 def build_topo(tgen):
     "Build function"
 
-    # Create 3 routers.
+    # Create 4 routers.
-    for routern in range(1, 4):
+    for routern in range(1, 5):
         tgen.add_router("r{}".format(routern))
 
     switch = tgen.add_switch("s1")
     switch.add_link(tgen.gears["r1"])
     switch.add_link(tgen.gears["r3"])
+    switch.add_link(tgen.gears["r4"])
     switch = tgen.add_switch("s2")
     switch.add_link(tgen.gears["r2"])
     switch.add_link(tgen.gears["r3"])
@@ -53,6 +87,9 @@ def build_topo(tgen):
     switch = tgen.add_switch("s4")
     switch.add_link(tgen.gears["r1"])
 
+    switch = tgen.add_switch("s5")
+    switch.add_link(tgen.gears["r4"])
+
 
 def _populate_iface():
     tgen = get_topogen()
@@ -62,6 +99,7 @@ def _populate_iface():
         "echo 0 > /proc/sys/net/ipv4/ip_forward_use_pmtu",
         "echo 1 > /proc/sys/net/ipv6/conf/{0}-eth0/disable_ipv6",
         "echo 1 > /proc/sys/net/ipv6/conf/{0}-gre0/disable_ipv6",
+        "iptables -A FORWARD -i {0}-gre0 -o {0}-gre0 -m hashlimit --hashlimit-upto 4/minute --hashlimit-burst 1 --hashlimit-mode srcip,dstip --hashlimit-srcmask 24 --hashlimit-dstmask 24 --hashlimit-name loglimit-0 -j NFLOG --nflog-group 1 --nflog-range 128",
     ]
 
     cmds_tot = [
@@ -84,10 +122,27 @@ def _populate_iface():
         output = tgen.net["r1"].cmd(input)
         logger.info("output: " + output)
 
+        input = cmd.format("r4", "4")
+        logger.info("input: " + input)
+        output = tgen.net["r4"].cmd(input)
+        logger.info("output: " + output)
+
+
+def _verify_iptables():
+    tgen = get_topogen()
+    # Verify iptables is installed
+    # This is needed for creating shortcuts
+    for rname in ("r1", "r4"):
+        rc, _, _ = tgen.net[rname].cmd_status("iptables --version")
+        if rc == 127:
+            return False
+    return True
+
 
 def setup_module(mod):
     "Sets up the pytest environment"
 
+    logger.info("NHRP Topology : \n {}".format(TOPOLOGY))
     result = required_linux_kernel_version("5.0")
     if result is not True:
         pytest.skip("Kernel requirements are not met")
@@ -103,7 +158,7 @@ def setup_module(mod):
             TopoRouter.RD_ZEBRA,
             os.path.join(CWD, "{}/zebra.conf".format(rname)),
         )
-        if rname in ("r1", "r2"):
+        if rname in ("r1", "r2", "r4"):
             router.load_config(
                 TopoRouter.RD_NHRP, os.path.join(CWD, "{}/nhrpd.conf".format(rname))
             )
@@ -226,10 +281,10 @@ def test_nhrp_connection():
 
     # force session to reinitialize
     def relink_session():
-        for r in ["r1", "r2"]:
+        for r in ["r1", "r2", "r4"]:
             tgen.gears[r].vtysh_cmd("clear ip nhrp cache")
-            tgen.net[r].cmd("ip l del {}-gre0".format(r));
+            tgen.net[r].cmd("ip l del {}-gre0".format(r))
-        _populate_iface();
+        _populate_iface()
 
     @retry(retry_timeout=40, initial_wait=5)
     def verify_same_password():
@@ -255,24 +310,29 @@ def test_nhrp_connection():
 
     ### Passwords are different
     logger.info("Modify password and send ping again, should drop")
-    hubrouter.vtysh_cmd("""
+    hubrouter.vtysh_cmd(
+        """
         configure
             interface r2-gre0
                 ip nhrp authentication secret12
-    """)
+    """
+    )
     relink_session()
     verify_mismatched_password()
 
     ### Passwords are the same - again
     logger.info("Recover password and verify conectivity is back")
-    hubrouter.vtysh_cmd("""
+    hubrouter.vtysh_cmd(
+        """
         configure
             interface r2-gre0
                 ip nhrp authentication secret
-    """)
+    """
+    )
     relink_session()
     verify_same_password()
 
+
 def test_route_install():
     "Test use of NHRP routes by other protocols (sharpd here)."
     tgen = get_topogen()
@@ -305,6 +365,134 @@ def test_route_install():
     assert result is None, assertmsg
 
 
+# Initial wait of 30 second because that is
+# what the default purge time is for nhrp -
+# here we are testing that all of the expected
+# retries are sent and logged before a
+# shortcut is purged
+@retry(retry_timeout=10, initial_wait=30)
+def check_retry_debug_info(pingspoke=None):
+    tgen = get_topogen()
+    r1 = tgen.gears["r1"]
+    if pingspoke == None:
+        pingspoke = r1
+    logger.info(f"Check retries are being sent from {pingspoke.name}")
+    output = pingspoke.cmd("grep -c 'Retrying Resolution Request' nhrpd.log")
+    # Making sure that we see all expected retries for a 30 second purge time
+    assertmsg = f"Did not see all expected retries on {pingspoke.name}"
+    assert output.strip() == "6", assertmsg
+    logger.info("Check retries are being sent OK")
+
+
+# Helper function to ping between spokes and
+# check for either complete or incomplete shortcut
+# based on whichever one you are expecting -
+# expect_succesful_shortcut inidcates whether
+# you are expecting to find a complete shortcut
+# (True) or incomplete shortcut (False) as a
+# result of the ping
+@retry(retry_timeout=10, initial_wait=10)
+def create_shortcut(expect_successful_shortcut=True, pingspoke=None, peer_addr=None):
+    tgen = get_topogen()
+    r1 = tgen.gears["r1"]
+    if pingspoke == None:
+        pingspoke = r1
+    if peer_addr == None:
+        peer_addr = "192.168.4.4"
+    # Pinging the other spoke in an attempt to create specified type of shortcut
+    output = pingspoke.cmd(f"ping -c 10 -i .5 {peer_addr}")
+    print(output)
+    output = pingspoke.vtysh_cmd("show ip nhrp shortcut")
+    if expect_successful_shortcut:
+        logger.info(f"Check shortcut creation from {pingspoke.name} to {peer_addr}")
+    else:
+        logger.info(
+            f"Check incomplete shortcut creation from {pingspoke.name} to {peer_addr}"
+        )
+
+    output = pingspoke.vtysh_cmd("show ip nhrp shortcut")
+    print(output)
+    if expect_successful_shortcut:
+        json_file = "{}/{}/nhrp_shortcut_present.json".format(CWD, pingspoke.name)
+        expected = json.loads(open(json_file).read())
+        test_func = partial(
+            topotest.router_json_cmp, pingspoke, "show ip nhrp shortcut json", expected
+        )
+        _, result = topotest.run_and_expect(test_func, None, count=40, wait=0.5)
+
+        if result is not None:
+            assertmsg = (
+                "Shortcut is not being made between spoke {} and peer {}".format(
+                    pingspoke.name, peer_addr
+                )
+            )
+            assert 0, assertmsg
+        else:
+            logger.info("Shortcut creation between spokes OK")
+    else:
+        # Currentlly, 'show ip nhrp shortcut json' does not show incomplete shortcuts
+        # so an explicit check for for the  'incompete' keyword needed here
+        if "incomplete" not in output:
+            assertmsg = (
+                "Incomplete shortcut between spoke {} and peer {} is not seen".format(
+                    pingspoke.name, peer_addr
+                )
+            )
+            assert 0, assertmsg
+        else:
+            logger.info("Incomplete shortcut creation between spokes OK")
+
+
+# This function tests the NHRP resolution request retries by dropping
+# incoming packets (including the NHRP resolution request packets)
+# from a receiving spoke in order to stop the NHRP resolution
+# responses from ever being sent from that receiving spoke  - and in turn
+# resolution responses will not reach the sending spoke.
+# This will trigger the NHRP resolution request retries which
+# can be viewed through log messages.
+def test_nhrp_retry_resolution():
+    """ "
+    Verify resolution requests are retried when resolution responses
+    are not received by a spoke
+    """
+    tgen = get_topogen()
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+    # iptables used to create shortcuts
+    # and subsequent resolution request retries
+    if not _verify_iptables():
+        pytest.skip("iptables is not installed")
+
+    r1 = tgen.gears["r1"]
+    r4 = tgen.gears["r4"]
+
+    logger.info("Testing retrying resolution request functionality")
+    # Make sure that shortcut creation between spokes work
+    create_shortcut(expect_successful_shortcut=True)
+    # Clearing shortcut information for spokes
+    r1.vtysh_cmd("clear ip nhrp shortcut")
+    r4.vtysh_cmd("clear ip nhrp shortcut")
+
+    # Setting iptables rules to stop incoming packets on r4
+    # This should stop resolution requests from reaching
+    # the receiving router (r4) and hence stop the
+    # creation of a complete shortcut
+    r4.cmd("iptables -A INPUT -i r4-eth0 -j DROP")
+
+    # Make sure that nhrp debugging is enabled to read the retry logs
+    r1.vtysh_cmd(
+        """
+        configure
+           debug nhrp all
+    """
+    )
+    create_shortcut(expect_successful_shortcut=False)
+    # Look for retry logging output for resolution request retries
+    check_retry_debug_info()
+    # Undo iptables rule
+    r4.cmd("iptables -D INPUT -i r4-eth0 -j DROP")
+
+
 def test_memory_leak():
     "Run the memory leak test and report results."
     tgen = get_topogen()