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

Protect local subnets from being routed toward Tailscale subnets if they collide #201

Merged
merged 2 commits into from
Oct 15, 2023

Conversation

lmagyar
Copy link
Contributor

@lmagyar lmagyar commented May 11, 2023

Note: this is based on / continuation of #199, but I will rebase this PR as required.

Proposed Changes

TLDR:

  • when tailscaled starts: ip rule add to "${route}" priority 5000 table main for each advertised local subnets
  • when stops: ip rule del to "${route}" for each previously added subnets
  • when nm-dispatcher notifies about networking config change: delete rules and recalculate again

Idea is stolen from: tailscale/tailscale#6231 (comment) (Workaround 3)

Reason: Tailscale adds it's own routes with higher priority than the "normal" route settings, so we add back the local subnets with even higher priority. Tailscale wants to forward the local domain in case it is a coffee shop WiFi that collides with our tailnet settings, but in a server's case, it causes issues.

When Tailscale provides a solution for this old issue (tailscale/tailscale#1227), we can remove this workaround.

ip rule list when the add-on is active/started:

0:      from all lookup local
5000:   from all to 192.168.1.0/24 lookup main                 <- added by this PR
5210:   from all fwmark 0x80000/0xff0000 lookup main
5230:   from all fwmark 0x80000/0xff0000 lookup default
5250:   from all fwmark 0x80000/0xff0000 lookup unspec unreachable
5270:   from all lookup 52
32766:  from all lookup main
32767:  from all lookup default

Tested on real HA OS, RPI 3 config, it didn't lost its network access when there were active colliding subnet routing, though I didn't make a "negative" test, that without this PR my RPI gets "bricked" with the same config. UPDATE: I made this "negative" test unintentionally, so this PR really protects the access to the device.

Related Issues

@lmagyar
Copy link
Contributor Author

lmagyar commented May 15, 2023

I've added a warning into the log, that there are colliding subnets when the add-on starts (this is independent of the permanently active, higher priority ip rule redirection). Tested on 3 real devices, seems to be solid.

@lmagyar
Copy link
Contributor Author

lmagyar commented May 25, 2023

Fixed IPv6 handling.

@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 28, 2023

Added handling local network changes (eg. from the UI changing IP, mask, etc.). Moved the functionality to a new s6 service.

Eg. when NetworkManager reported the manual removal of IPv6 IP address, the ip rules (protecting the local subnets from collision/redirection) got recalculated.

    [21:44:32] INFO: Removing route 192.168.1.0/24 from ip rules
    [21:44:32] INFO: Removing route fd19:8efe:2a99::/64 from ip rules
    [21:44:36] INFO: Adding local subnets to ip rules with higher priority than Tailscale's routing,
    [21:44:36] INFO: to prevent routing local subnets if the same subnet is routed within your tailnet.
    [21:44:36] INFO: Adding route 192.168.1.0/24 to ip rules

@lmagyar lmagyar force-pushed the pr-add-local-subnets-to-ip-rules branch from 3a4093a to b0191b8 Compare August 6, 2023 11:14
@ss89

This comment was marked as off-topic.

@lmagyar

This comment was marked as off-topic.

@lmagyar lmagyar force-pushed the pr-add-local-subnets-to-ip-rules branch from de64b21 to 765875a Compare September 7, 2023 19:33
@lmagyar lmagyar force-pushed the pr-add-local-subnets-to-ip-rules branch from 765875a to 1fa2f89 Compare October 5, 2023 08:35
@frenck frenck added the enhancement Enhancement of the code, not introducing new features. label Oct 15, 2023
@frenck frenck force-pushed the pr-add-local-subnets-to-ip-rules branch from 1fa2f89 to 8a11c91 Compare October 15, 2023 19:29
tailscale/DOCS.md Outdated Show resolved Hide resolved
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Made a small tiny docs tweak. Otherwise: Beautiful!

@frenck frenck merged commit c912774 into hassio-addons:main Oct 15, 2023
13 checks passed
@lmagyar
Copy link
Contributor Author

lmagyar commented Oct 16, 2023

Made a small tiny docs tweak. Otherwise: Beautiful!

Thank you. :)

Though it has 2 logical bugs/issues:

  1. Real bug: It protects the advertised subnets, what was originally identical with the local subnets, but after the advertised subnets are user configurable, I think this stuff should always protect local subnets, independently of what users configured to be advertised. Because collisions will be between local and accepted routes not between advertised and accepted. It's only a few lines of code, I can make a PR.
  2. Maybe not a bug: It protects subnet domains, maybe should only protect HA's local IPs only. But I'm not sure whether it solves or creates more problems.

I think 1. needs a new PR, 2. can stay as is. What do you think?

@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2023
@lmagyar lmagyar deleted the pr-add-local-subnets-to-ip-rules branch October 18, 2023 07:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhancement of the code, not introducing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants