nhrpd: add cisco-authentication password support

Taking over this development from https://github.com/FRRouting/frr/pull/14788

This commit addresses 4 issues found in the previous PR

1) FRR would accept messages from a spoke without authentication when FRR NHRP had auth configured.
2) The error indication was not being sent in network byte order
3) The debug print in nhrp_connection_authorized was not correctly printing the received password
4) The addresses portion of the mandatory part of the error indication was invalid on the wire (confirmed in wireshark)

Signed-off-by: Dave LeRoy <dleroy@labn.net>
Co-authored-by: Volodymyr Huti <volodymyr.huti@gmail.com>

7 files changed, 51 insertions, 43 deletions

diff --git a/doc/user/nhrpd.rst b/doc/user/nhrpd.rst
index e0ba90fcc..648d56d9c 100644
--- a/doc/user/nhrpd.rst
+++ b/doc/user/nhrpd.rst
@@ -88,7 +88,7 @@ Configuring NHRP
 
    Enables Cisco style authentication on NHRP packets. This embeds the
    plaintext password to the outgoing NHRP packets.
-   Maximum length of the is 8 characters.
+   Maximum length of the password is 8 characters.
 
 .. clicmd:: ip nhrp map A.B.C.D|X:X::X:X A.B.C.D|local
 
diff --git a/nhrpd/nhrp_interface.c b/nhrpd/nhrp_interface.c
index 7d0ab9762..e81a2efbb 100644
--- a/nhrpd/nhrp_interface.c
+++ b/nhrpd/nhrp_interface.c
@@ -99,6 +99,8 @@ static int nhrp_if_delete_hook(struct interface *ifp)
 		free(nifp->ipsec_fallback_profile);
 	if (nifp->source)
 		free(nifp->source);
+	if (nifp->auth_token)
+		zbuf_free(nifp->auth_token);
 
 	XFREE(MTYPE_NHRP_IF, ifp->info);
 	return 0;
diff --git a/nhrpd/nhrp_peer.c b/nhrpd/nhrp_peer.c
index 84fcdb069..2414541bf 100644
--- a/nhrpd/nhrp_peer.c
+++ b/nhrpd/nhrp_peer.c
@@ -730,7 +730,7 @@ static void nhrp_handle_registration_request(struct nhrp_packet_parser *p)
 		}
 	}
 
-	// auth ext was validated and copied from the request
+	/* auth ext was validated and copied from the request */
 	nhrp_packet_complete_auth(zb, hdr, ifp, false);
 	nhrp_peer_send(p->peer, zb);
 err:
@@ -1064,7 +1064,6 @@ static void nhrp_peer_forward(struct nhrp_peer *p,
 		nhrp_ext_complete(zb, dst);
 	}
 
-	// XXX: auth already handled ???
 	nhrp_packet_complete_auth(zb, hdr, pp->ifp, false);
 	nhrp_peer_send(p, zb);
 	zbuf_free(zb);
@@ -1121,24 +1120,26 @@ static int nhrp_packet_send_error(struct nhrp_packet_parser *pp,
 		dst_proto = pp->src_proto;
 	}
 	/* Create reply */
-	zb = zbuf_alloc(1500); // XXX: hardcode -> calculation routine
+	zb = zbuf_alloc(1500);
 	hdr = nhrp_packet_push(zb, NHRP_PACKET_ERROR_INDICATION, &pp->src_nbma,
 			       &src_proto, &dst_proto);
 
-	hdr->u.error.code = indication_code;
+	hdr->u.error.code = htons(indication_code);
 	hdr->u.error.offset = htons(offset);
 	hdr->flags = pp->hdr->flags;
-	hdr->hop_count = 0; // XXX: cisco returns 255
+	hdr->hop_count = 0; /* XXX: cisco returns 255 */
 
 	/* Payload is the packet causing error */
 	/* Don`t add extension according to RFC */
-	/* wireshark gives bad checksum, without exts */
-	// pp->hdr->checksum = nhrp_packet_calculate_checksum(zbuf_used(&pp->payload))
 	zbuf_put(zb, pp->hdr, sizeof(*pp->hdr));
-	zbuf_copy(zb, &pp->payload, zbuf_used(&pp->payload));
+	zbuf_put(zb, sockunion_get_addr(&pp->src_nbma),
+		 hdr->src_nbma_address_len);
+	zbuf_put(zb, sockunion_get_addr(&pp->src_proto),
+		 hdr->src_protocol_address_len);
+	zbuf_put(zb, sockunion_get_addr(&pp->dst_proto),
+		 hdr->dst_protocol_address_len);
 	nhrp_packet_complete_auth(zb, hdr, pp->ifp, false);
 
-	/* nhrp_packet_debug(zb, "SEND_ERROR"); */
 	nhrp_peer_send(pp->peer, zb);
 	zbuf_free(zb);
 	return 0;
@@ -1151,8 +1152,7 @@ static bool nhrp_connection_authorized(struct nhrp_packet_parser *pp)
 	struct zbuf *auth = nifp->auth_token;
 	struct nhrp_extension_header *ext;
 	struct zbuf *extensions, pl;
-	int cmp = 0;
-
+	int cmp = 1;
 
 	extensions = zbuf_alloc(zbuf_used(&pp->extensions));
 	zbuf_copy_peek(extensions, &pp->extensions, zbuf_used(&pp->extensions));
@@ -1164,7 +1164,14 @@ static bool nhrp_connection_authorized(struct nhrp_packet_parser *pp)
 					   auth->buf;
 			debugf(NHRP_DEBUG_COMMON,
 			       "Processing Authentication Extension for (%s:%s|%d)",
-			       auth_ext->secret, (const char *)pl.buf, cmp);
+			       auth_ext->secret,
+			       ((struct nhrp_cisco_authentication_extension *)
+					pl.buf)
+				       ->secret,
+			       cmp);
+			break;
+		default:
+			/* Ignoring all received extensions except Authentication*/
 			break;
 		}
 	}
diff --git a/nhrpd/nhrp_vty.c b/nhrpd/nhrp_vty.c
index 66659bdcd..b938ae4cf 100644
--- a/nhrpd/nhrp_vty.c
+++ b/nhrpd/nhrp_vty.c
@@ -467,7 +467,7 @@ DEFPY(if_nhrp_authentication, if_nhrp_authentication_cmd,
       AFI_CMD "nhrp authentication PASSWORD$password",
       AFI_STR
       NHRP_STR
-      "Specify plaint text password used for authenticantion\n"
+      "Specify plain text password used for authenticantion\n"
       "Password, plain text, limited to 8 characters\n")
 {
 	VTY_DECLVAR_CONTEXT(interface, ifp);
@@ -490,8 +490,6 @@ DEFPY(if_nhrp_authentication, if_nhrp_authentication_cmd,
 	auth->type = htonl(NHRP_AUTHENTICATION_PLAINTEXT);
 	memcpy(auth->secret, password, pass_len);
 
-	// XXX RFC: reset active (non-authorized) connections?
-	/* vty_out(vty, "NHRP passwd (%s:%s)", nifp->ifp->name, auth->secret); */
 	return CMD_SUCCESS;
 }
 
@@ -501,11 +499,12 @@ DEFPY(if_no_nhrp_authentication, if_no_nhrp_authentication_cmd,
       NO_STR
       AFI_STR
       NHRP_STR
-      "Reset password used for authentication\n"
-      "Password, plain text\n")
+      "Specify plain text password used for authenticantion\n"
+	  "Password, plain text, limited to 8 characters\n")
 {
 	VTY_DECLVAR_CONTEXT(interface, ifp);
 	struct nhrp_interface *nifp = ifp->info;
+
 	if (nifp->auth_token)
 		zbuf_free(nifp->auth_token);
 	return CMD_SUCCESS;
diff --git a/tests/topotests/nhrp_topo/r1/nhrpd.conf b/tests/topotests/nhrp_topo/r1/nhrpd.conf
index 027312dcd..8ade77d07 100644
--- a/tests/topotests/nhrp_topo/r1/nhrpd.conf
+++ b/tests/topotests/nhrp_topo/r1/nhrpd.conf
@@ -2,7 +2,7 @@ log stdout debugging
 ! debug nhrp all
 interface r1-gre0
  ip nhrp authentication secret
- ip nhrp holdtime 500
+ ip nhrp holdtime 10
  ip nhrp shortcut
  ip nhrp network-id 42
  ip nhrp nhs dynamic nbma 10.2.1.2
diff --git a/tests/topotests/nhrp_topo/r2/nhrpd.conf b/tests/topotests/nhrp_topo/r2/nhrpd.conf
index 621db6abc..d8e59936c 100644
--- a/tests/topotests/nhrp_topo/r2/nhrpd.conf
+++ b/tests/topotests/nhrp_topo/r2/nhrpd.conf
@@ -3,7 +3,7 @@ log stdout debugging
 nhrp nflog-group 1
 interface r2-gre0
  ip nhrp authentication secret
- ip nhrp holdtime 500
+ ip nhrp holdtime 10
  ip nhrp redirect
  ip nhrp network-id 42
  ip nhrp registration no-unique
diff --git a/tests/topotests/nhrp_topo/test_nhrp_topo.py b/tests/topotests/nhrp_topo/test_nhrp_topo.py
index c57d28b70..883300310 100644
--- a/tests/topotests/nhrp_topo/test_nhrp_topo.py
+++ b/tests/topotests/nhrp_topo/test_nhrp_topo.py
@@ -28,7 +28,7 @@ sys.path.append(os.path.join(CWD, "../"))
 from lib import topotest
 from lib.topogen import Topogen, TopoRouter, get_topogen
 from lib.topolog import logger
-from lib.common_config import required_linux_kernel_version
+from lib.common_config import required_linux_kernel_version, retry
 
 # Required to instantiate the topology builder class.
 
@@ -231,14 +231,27 @@ def test_nhrp_connection():
             tgen.net[r].cmd("ip l del {}-gre0".format(r));
         _populate_iface();
 
+    @retry(retry_timeout=40, initial_wait=5)
+    def verify_same_password():
+        output = ping_helper()
+        if "100 packets transmitted, 100 received" not in output:
+            assertmsg = "expected ping IPv4 from R1 to R2 should be ok"
+            assert 0, assertmsg
+        else:
+            logger.info("Check Ping IPv4 from R1 to R2 OK")
+
+    @retry(retry_timeout=40, initial_wait=5)
+    def verify_mismatched_password():
+        output = ping_helper()
+        if "Network is unreachable" not in output:
+            assertmsg = "expected ping IPv4 from R1 to R2 - should be down"
+            assert 0, assertmsg
+        else:
+            logger.info("Check Ping IPv4 from R1 to R2 missing - OK")
+
     ### Passwords are the same
     logger.info("Check Ping IPv4 from  R1 to R2 = 10.255.255.2")
-    output = ping_helper()
-    if "100 packets transmitted, 100 received" not in output:
-        assertmsg = "expected ping IPv4 from R1 to R2 should be ok"
-        assert 0, assertmsg
-    else:
-        logger.info("Check Ping IPv4 from R1 to R2 OK")
+    verify_same_password()
 
     ### Passwords are different
     logger.info("Modify password and send ping again, should drop")
@@ -248,14 +261,8 @@ def test_nhrp_connection():
                 ip nhrp authentication secret12
     """)
     relink_session()
-    topotest.sleep(10, "Waiting for session to initialize")
-    output = ping_helper()
-    if "Network is unreachable" not in output:
-        assertmsg = "expected ping IPv4 from R1 to R2 - should be down"
-        assert 0, assertmsg
-    else:
-        logger.info("Check Ping IPv4 from R1 to R2 missing - OK")
-
+    verify_mismatched_password()
+    
     ### Passwords are the same - again
     logger.info("Recover password and verify conectivity is back")
     hubrouter.vtysh_cmd("""
@@ -264,14 +271,7 @@ def test_nhrp_connection():
                 ip nhrp authentication secret
     """)
     relink_session()
-    topotest.sleep(10, "Waiting for session to initialize")
-    output = pingrouter.run("ping 10.255.255.2 -f -c 100")
-    logger.info(output)
-    if "100 packets transmitted, 100 received" not in output:
-        assertmsg = "expected ping IPv4 from R1 to R2 should be ok"
-        assert 0, assertmsg
-    else:
-        logger.info("Check Ping IPv4 from R1 to R2 OK")
+    verify_same_password()
 
 def test_route_install():
     "Test use of NHRP routes by other protocols (sharpd here)."