Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nhrpd: add cisco-authentication password support #16172

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

dleroy
Copy link
Contributor

@dleroy dleroy commented Jun 5, 2024

This PR takes over work done on #14788 with agreement from @volodymyrhuti .

In addition to the work in 14788 I have addressed 4 issues I found while testing interop between Cisco and FRR:

  1. FRR would accept messages from a spoke without authentication configured 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)

@Jafaral
Copy link
Member

Jafaral commented Jun 6, 2024

@dleroy dleroy force-pushed the dleroy/nhrpd-auth-support branch from 7a3e672 to bb0fbf5 Compare June 6, 2024 17:41
doc/user/nhrpd.rst Outdated Show resolved Hide resolved
nhrpd/nhrp_peer.c Outdated Show resolved Hide resolved
nhrpd/nhrp_peer.c Outdated Show resolved Hide resolved
nhrpd/nhrp_peer.c Outdated Show resolved Hide resolved
nhrpd/nhrp_peer.c Outdated Show resolved Hide resolved
nhrpd/nhrp_vty.c Outdated Show resolved Hide resolved
nhrpd/nhrp_vty.c Show resolved Hide resolved
nhrpd/nhrp_vty.c Outdated Show resolved Hide resolved
tests/topotests/nhrp_topo/test_nhrp_topo.py Outdated Show resolved Hide resolved
tests/topotests/nhrp_topo/test_nhrp_topo.py Outdated Show resolved Hide resolved
@dleroy dleroy force-pushed the dleroy/nhrpd-auth-support branch from bb0fbf5 to 792d1dd Compare June 10, 2024 20:29
@github-actions github-actions bot added the rebase PR needs rebase label Jun 10, 2024
Volodymyr Huti and others added 2 commits June 10, 2024 16:39
Implemented:
- handling 8 char long password, aka Cisco style.
- minimal error inidication routine
- test case, password change affects conection

Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
Taking over this development from FRRouting#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>
@dleroy dleroy force-pushed the dleroy/nhrpd-auth-support branch from 792d1dd to b5540d3 Compare June 11, 2024 01:41
@dleroy dleroy requested a review from ton31337 June 11, 2024 01:43
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks okay, but the memory leak needs to be looked at before merging ...

@dleroy
Copy link
Contributor Author

dleroy commented Jun 11, 2024

A new issue poped up with this PR: https://ci1.netdef.org/browse/FRR-PULLREQ3-3588/artifact/ASAN3/AddressSanitizerError/AddressSanitzer.txt

I have addressed this issue and pushed fix.

@dleroy
Copy link
Contributor Author

dleroy commented Jun 11, 2024

code looks okay, but the memory leak needs to be looked at before merging ...

Hi Russ, I've addressed the address sanitizer failure. Is that what you were referring to?

@riw777 riw777 merged commit 66ad4aa into FRRouting:master Jun 18, 2024
10 checks passed
@gotthardp gotthardp mentioned this pull request Sep 24, 2024
Jafaral added a commit that referenced this pull request Nov 12, 2024
New Features Highlight:

- PIM candidate BSR/RP [#16438]
- Static IGMP join without an IGMP report [1#6450]
- PIM AutoRP discovery/announcements [#16634]
- IGMP proxy [#16861]
- SRv6 SID Manager [#15604]
- Add `bgp ipv6-auto-ra` command [#16354]
- Implement `neighbor x remote-as auto` for BGP [#16345]
- Implement `bgp dual-as` for BGP [#16816]
- Implement BGP-wide configuration for graceful restart [#16099]
- Handle kernel routes appropriately (should fix recent NOPREFIXROUTE issue) [#16300]
- Add `cisco-authentication` password support for NHRP [#16172]

Signed-off-by: Jafar Al-Gharaibeh <jafar@atcorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants