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

fix Windows firewall rules and allow to disable firewall manipulation #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dyhkwong
Copy link
Contributor

@dyhkwong dyhkwong commented Mar 4, 2025

  • use api to create windows firewall rules #12 introduced a TCP-only firewall rule to make system stack work for newbies. Manipulating firewall is not a good bahavior, so an option to disable this should be introduced.
  • In [Bug] 上游依赖项 sing-tun 的行为导致 mihomo NAT 降级 MetaCubeX/mihomo#1875, someone claims that the TCP-only firewall rule causes "NAT type degradation" and adding a UDP firewall rule can fix this. Although a user can add corresponding UDP rule manually, no firewall pop-up is considered a bug in that issue.
  • I don't think one can achieve "endpoint independent" NAT filtering bahavior with a firewall blocking most of UDP ports, so I suspect it is a false positive result of "NatTypeTester" (known to have false positive results with continuous testing) rather than a real bug. Issue reporter is unwilling to further investigate so this change needs someone with local open NAT environment to confirm.

@dyhkwong dyhkwong changed the base branch from dev to main March 4, 2025 07:53
@TGSAN
Copy link

TGSAN commented Mar 5, 2025

In my understanding, as a foundational library, sing-tun itself should not be modifying firewall rules; that logic is a dirty code. A TUN is a layer 3 network device, while the Windows Firewall only governs layer 4 and above. TCP is allowed solely to facilitate communication between the TUN and the application and has nothing to do with the TUN itself. Similarly, UDP is required for inbound connections and communication with the application, and it is also unrelated to the TUN. Therefore, instead of adding Windows Firewall rules within sing-tun, it would be better to completely remove this logic. If necessary, the calling party (such as mihomo or sing-box) should handle it.
Furthermore, the typical method for adding Windows firewall rules should be to listen directly on IPv4Zero/IPv6Zero or on the non-reserved address assigned to the network interface. This approach will cause Windows to immediately display an explicit firewall addition wizard for Workstation-type systems (non-server versions), unless the user has disabled this feature. Of course, Server-type systems will not trigger this explicit wizard, but users of such systems are usually professionals who know how to manage these settings.

@dyhkwong
Copy link
Contributor Author

dyhkwong commented Mar 5, 2025

sing-tun itself should not be modifying firewall rules; that logic is a dirty code

I agree with this. You can see in #12 that the original PR was opt-in, but was modified later.

TCP is allowed solely to facilitate communication between the TUN and the application and has nothing to do with the TUN itself

The TCP firewall rule is for "二次 NAT", which got another name "system stack" from Clash. It has nothing to do with "communication between the TUN and the application". The "fake TCP server" is listening on TUN address and that's the only reason for the TCP firewall rule. It's not related to UDP. "gVisor stack" does not add any explicit firewall rules and does not trigger firewall pop-up but this does not broke its UDP.

Furthermore, the typical method for adding Windows firewall rules should be to listen directly on IPv4Zero/IPv6Zero or on the non-reserved address assigned to the network interface

IIRC there is no pop-up if it is wrapped as a Windows service. Those Clash GUIs wrap Clash core as a Windows service. There may be also some reports about no pop-up but the repositories have been deleted and I can't find them. Not sure about that.

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.

2 participants