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

HA Network Routes: prevent routing directly-accessible networks through VPN interface #612

Merged
merged 3 commits into from
Dec 8, 2022

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Dec 6, 2022

Describe your changes

It prevents Server from adding rules to route directly accessible/own networks through VPN when the route is in HA mode (has more than 1 entry):

  1. the code was checking membership against a single Network Route entry with equality operator on Peer attribute:
    1. there is 1 Route per Peer
    2. second (HA) Route entry did not pass the check because it was not the server
    3. rule for routing the traffic through VPN interface is created
    4. host X is routing through host Y
    5. host Y is routing through host X
    6. (i assume) we have a routing loop passing packets back and forth between HA hosts instead of going to the real network
  2. The code fixes above issue by checking NetID instead of Peer values

see the linked issue for more details

Issue ticket number and link

fixes: #598

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2022

CLA assistant check
All committers have signed the CLA.

@nazarewk nazarewk changed the title fix High Availability routing fix High Availability routing through VPN when routes are directly accessible Dec 6, 2022
@nazarewk nazarewk changed the title fix High Availability routing through VPN when routes are directly accessible HA Network Routes: prevent routing directly-accessible networks through VPN interface Dec 6, 2022
Copy link
Collaborator

@mlsmaycon mlsmaycon left a comment

Choose a reason for hiding this comment

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

Thank you so much for your PR @nazarewk, it is definitely helping with the case you described.

I have a few notes that I took into consideration on my request for changes:

In multi-hop routing cases, you want to route traffic via the VPN interface as the routing peer might be an intermediate router. In that case, will make sense to have different network routes for each hop.

Also, to document, with PR #606 we will be able to prevent similar case by using different distribution groups for routes, but only when different groups are used with routes and peers.

client/internal/routemanager/manager.go Outdated Show resolved Hide resolved
client/internal/routemanager/manager.go Outdated Show resolved Hide resolved
client/internal/routemanager/manager_test.go Outdated Show resolved Hide resolved
@nazarewk nazarewk force-pushed the fix-ha-routes branch 2 times, most recently from 59f67b1 to 4e6823f Compare December 7, 2022 09:46
nazarewk added a commit to nazarewk/netbird that referenced this pull request Dec 8, 2022
nazarewk added a commit to nazarewk/netbird that referenced this pull request Dec 8, 2022
@nazarewk nazarewk requested a review from mlsmaycon December 8, 2022 08:42
nazarewk added a commit to nazarewk/netbird that referenced this pull request Dec 8, 2022
Copy link
Collaborator

@mlsmaycon mlsmaycon left a comment

Choose a reason for hiding this comment

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

@nazarewk thank you for the changes and hanging thought all the requests.

@mlsmaycon mlsmaycon merged commit 1204bbd into netbirdio:main Dec 8, 2022
@nazarewk nazarewk deleted the fix-ha-routes branch December 8, 2022 12:39
braginini pushed a commit that referenced this pull request Dec 11, 2022
…gh VPN interface (#612)

Prevent routing peer to add routes from the same HA group as client routes
pulsastrix pushed a commit to pulsastrix/netbird that referenced this pull request Dec 24, 2023
…gh VPN interface (netbirdio#612)

Prevent routing peer to add routes from the same HA group as client routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Server should not create ip routes to networks it is routing to
3 participants