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>

diff --git a/doc/user/nhrpd.rst b/doc/user/nhrpd.rst
index e0ba90fcc1395471d03c572f24470b869e0755d7..648d56d9c18a2b73027ae3d35078e391fab8925e 100644 (file)
--- 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 7d0ab9762f7158151c705c774ec6b62576f6b858..e81a2efbb68516f737a254abc7b76849c105d5a8 100644 (file)
--- 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 84fcdb06974470c4737f0742a2814f725e938389..2414541bfaf4abd933802b22f6d240468c40b3da 100644 (file)
--- 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 66659bdcdb920b869f7452943d1b92602c8c8ccc..b938ae4cf0e82248155218d3b8b3ac0edbc35f51 100644 (file)
--- 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 027312dcd53f9232127bc683b2854383284dc98d..8ade77d07de2228ad959e4901ed2268904cbe9f0 100644 (file)
--- 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 621db6abc8382add985e4ddb7b6c1ed2d8fd0c5e..d8e59936c86595ccdebb21342cc2fc652c7c6aaf 100644 (file)
--- 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 c57d28b709bc3f1e494914592cda253886ead02f..8833003107738ded8c68b2139b13bd70cce9638f 100644 (file)
--- 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)."