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

nhrp: add cisco-authentication password support #14788

Closed
wants to merge 1 commit into from

Conversation

1337kerberos
Copy link
Contributor

@1337kerberos 1337kerberos commented Nov 13, 2023

Implemented:

  • handling 8 char long password, aka Cisco style.
  • minimal error inidication routine
  • test case, password change affects connection

Implemented in the context - https://vyos.dev/T2326

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Missing documentation, some contributing guidelines, and styling issues yet.

nhrpd/nhrp_vty.c Outdated Show resolved Hide resolved
nhrpd/nhrp_vty.c Outdated Show resolved Hide resolved
nhrpd/nhrp_vty.c Outdated Show resolved Hide resolved
nhrpd/nhrp_vty.c Outdated Show resolved Hide resolved
nhrpd/nhrp_vty.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_packet.c Outdated Show resolved Hide resolved
@pguibert6WIND
Copy link
Member

general remark: apply 'git clang-format HEAD^' and squash the proposed changes.

@donaldsharp
Copy link
Member

@louberger can you take a look at this?

@github-actions github-actions bot added the rebase PR needs rebase label Nov 22, 2023
@1337kerberos 1337kerberos force-pushed the NHRP_CISCO_AUTH branch 2 times, most recently from eeae138 to 282a6e5 Compare November 22, 2023 21:28
@1337kerberos
Copy link
Contributor Author

The latest update:

  • addressed the PR comments
  • fixed styling with clang-format
  • added a short doc for the password config

The internal testing by my team has shown issues when connecting with Cisco Spoke; keep PR as a draft until addressed

Implemented:
- handling 8 char long password, aka Cisco style.
- minimal error inidication routine
- test case, password change affects conection

Signed-off-by: Volodymyr Huti <[email protected]>
@fett0
Copy link

fett0 commented Feb 15, 2024

@ton31337 @pguibert6WIND is anything else needed to tmerge this PR ?

@ton31337
Copy link
Member

It won't be reviewed until it's marked as a draft.

@dleroy
Copy link
Contributor

dleroy commented May 23, 2024

@volodymyrhuti I also have an interest in seeing this committed. I tested your PR against a Cisco router as spoke and found/fixed a few issues. (see attached file). Wireshark is still complaining. I'm wondering if its because the packet causing the error indication does not include the extensions when copying the packet that caused the indication. Wireshark just recursively parses the included original packet, so the checksum and length would be incorrect.

Issues I found/fixed:

  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 were invalid on the wire (confirmed in wireshark)

Happy to discuss, let me know if you have any questions.

auth.txt

dleroy added a commit to LabNConsulting/frr that referenced this pull request Jun 5, 2024
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 <[email protected]>
@dleroy
Copy link
Contributor

dleroy commented Jun 5, 2024

I have coordinated with @volodymyrhuti and he has agreed to let me carry this work forward. I am closing this PR and taking up the work in #16172

@Jafaral
Copy link
Member

Jafaral commented Jun 5, 2024

Replaced by #16172

@Jafaral Jafaral closed this Jun 5, 2024
dleroy added a commit to LabNConsulting/frr that referenced this pull request Jun 6, 2024
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 <[email protected]>
dleroy added a commit to LabNConsulting/frr that referenced this pull request Jun 6, 2024
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 <[email protected]>
dleroy added a commit to LabNConsulting/frr that referenced this pull request Jun 10, 2024
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 <[email protected]>
Co-authored-by: Volodymyr Huti <[email protected]>
dleroy added a commit to LabNConsulting/frr that referenced this pull request Jun 11, 2024
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 <[email protected]>
Co-authored-by: Volodymyr Huti <[email protected]>
aapostoliuk pushed a commit to aapostoliuk/frr that referenced this pull request Sep 17, 2024
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 <[email protected]>
Co-authored-by: Volodymyr Huti <[email protected]>
aapostoliuk pushed a commit to aapostoliuk/frr that referenced this pull request Sep 19, 2024
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 <[email protected]>
Co-authored-by: Volodymyr Huti <[email protected]>
aapostoliuk pushed a commit to aapostoliuk/frr that referenced this pull request Sep 19, 2024
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 <[email protected]>
Co-authored-by: Volodymyr Huti <[email protected]>
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.

7 participants