lmagyar / homeassistant-addon-tailscale

Adds some functionality to the Tailscale Home Assistant Community Add-on https://github.com/hassio-addons/addon-tailscale
MIT License
56 stars 7 forks source link

Code adaption required for site-to-site networking since version v1.66.0 (--stateful-filtering) #127

Closed elcajon closed 3 months ago

elcajon commented 4 months ago

Hi @lmagyar,

Since version 1.66.0, a new parameter for tailscale up was introduced to fix the previous version related security vulnerability. This new parameter allows a connection to be established from outside.

To set up site-to-site networking, it is now necessary to set --stateful-filtering=false (defaults to true) in addition to the --snat-subnet-routes=false option.

My question about this: I'm struggling about how to implement this (in terms of UX and HA add-on maintainability). I don't currently see any use for it except in connection with site-to-site networking. So you could either set the parameter depending on snat-subnet-routes or create an extra add-on option for it. The former would be possible without major documentation changes while the latter leads to problems if the end user does not know what he is doing...

Maybe it would also be possible to think about a general extension of the add-on for rarely used tailscale up options (a la additional_run_options) which are simply appended 1:1 to tailscale up at runtime

Don't see this as a request to implement it, I'm especially interested in your opinion. I can create the PR myself if you don't have the time. šŸ˜Š I would be happy to hear your opinion on this. šŸ˜‰

elcajon commented 4 months ago

Another thought... Maybe this is also a discussion for the hassio-addons repo as frenck has to decide which way he wants to provide officially in the end.

lmagyar commented 4 months ago

Background

Source: https://tailscale.com/security-bulletins#ts-2024-005

Stateful packet filtering on packet-forwarding nodes

On Linux packet-forwarding nodes we added stateful packet filtering. This means that these nodes keep track of forwarded connections and only allow return packets for existing outbound connections. Inbound packets that don't belong to an existing connection are dropped. Stateful filtering is enabled by default, except for existing subnet routers that set --snat-subnet-routes=false. You can disable stateful filtering using tailscale up --stateful-filtering=false.

Add-on docs:

Option: userspace_networking

If you need to access other clients on your tailnet from your Home Assistant instance, disable userspace networking mode, which will create a tailscale0 network interface on your host. If you want to access other clients on your tailnet even from your local subnet, execute steps 2 and 3 as described on Site-to-site networking.

Source: https://tailscale.com/kb/1214/site-to-site

S2S

Opinion

So I think there are 2 levels of site-to-site:

And I'm thinking whether stateful-filtering=true and snat-subnet-routes=false makes sense on eg. LAN1 router? Maybe you want your LAN1 nodes to know who accessed them from LAN2, though you do not allow initiating traffic from LAN1 toward LAN2?

So, my suggestions are:

see: a6c7160

lmagyar commented 4 months ago

Oh, and maybe frenck did not released the new TS version because it was failing on some test environment???

lmagyar commented 4 months ago

And I released it on my beta repo, you can test it: https://github.com/lmagyar/homeassistant-addon-tailscale-beta

After some smoke test, I will release it here also. :)

lmagyar commented 4 months ago

Hmmm, maybe this also makes sense:

userspace_networking: true
stateful_filtering: true
snat_subnet_routes: false

So I think we should remove the "Note:" in the docs and the notice in the log about the correlation of stateful-filtering=false and snat-subnet-routes=false. If stateful-filtering has "mandatory" correlation, it would have been with userspace_networking, if userspace_networking is enabled, there is no filtering going on to be disabled (but snat is active). Note: I'm just talking, I didn't make any test about this.

lmagyar commented 4 months ago

And I've reread your first comment again, and I forget to answer one topic. :)

additional_run_options: I think/assume frenck wouldn't like it. Isn't it possible currently? As I remember Tailscale is stateful, if we configure something in the commandline and restart the add-on, it will remember it. But I can be totally wrong, as I remember this was the case some time ago for some options, I didn't test it now.

elcajon commented 4 months ago

I would agree on that --stateful-filter topic šŸ‘šŸ» Thanks for providing the PR!

And I've reread your first comment again, and I forget to answer one topic. :)

additional_run_options: I think/assume frenck wouldn't like it. Isn't it possible currently? As I remember Tailscale is stateful, if we configure something in the commandline and restart the add-on, it will remember it. But I can be totally wrong, as I remember this was the case some time ago for some options, I didn't test it now.

With add-on options, I am always unsure whether it makes sense to provide everything that a service (in this case Tailscale) offers or not. Many of the options that have already been added are probably only used by a handful of users and they are probably also able to set the parameters correctly if there is an option to set them elsewhere.

If I understand it correctly, with each add-on start we perform a new tailscale up with the corresponding currently valid values and would override manual changes in the Docker container? (This was also the problem with manual customizations in the new web UI if I remember correctly)

lmagyar commented 4 months ago

Oh, yes, you are right: any option that is configured by the add-on is overwritten on next start, like the (now readonly) UI. But, eg. if you want to create a complex serve config, you can use the cli, and TS will remember and won't be overwritten if the add-on proxy/funnel settings are turned off.

Hmmm, if we do not add the options as add-on options, we should mention that they should use docker exec, and that is a 100% no from frenck, and I have to aggree with him (let only those use it, who can figure out that this is possible, because if we mention it anywhere in the docs, more people will use it than should, I assume this was the reason that the Portainer add-on got revoked) . And for example the userspace-networking=false + subnet routing the same 192.168.x.x at multiple places can soft-brick the whole HA device, the protection workaround is quite complex.

The best would be if TS provides a thorough UI and we delete 90% of the add-on's code. :) See: https://github.com/hassio-addons/addon-tailscale/issues/314#issuecomment-1890445727

lmagyar commented 3 months ago

Closing, because PR is accepted. :)