feat(tun): auto set system DNS to TUN gateway#58
feat(tun): auto set system DNS to TUN gateway#58iTsingchen wants to merge 2 commits intoSitoi:devfrom
Conversation
|
@iTsingchen is attempting to deploy a commit to the Sitoi Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR adds automatic system DNS management to ClashBar’s TUN lifecycle on macOS, aiming to prevent system DNS queries from bypassing the TUN interface (notably when another VPN is also active) and getting polluted.
Changes:
- Add privileged XPC helper commands to set/restore system DNS and wire them through the app’s SystemProxy layer.
- Integrate DNS set/restore into TUN enable/disable, startup reconciliation, and termination/cleanup paths.
- Add YAML scalar helpers to reliably parse DNS-related values from config content.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/ProxyHelperShared/ProxyHelperProtocol.swift | Extends XPC protocol with DNS set/restore commands. |
| Sources/ProxyHelper/Daemon/main.swift | Implements DNS backup/set/restore via networksetup in privileged helper. |
| Sources/ClashBar/Infrastructure/System/SystemProxy/SystemProxyService.swift | Adds async + blocking DNS operations via XPC helper and refactors blocking helper invocation. |
| Sources/ClashBar/Infrastructure/System/SystemProxy/DefaultSystemProxyRepository.swift | Exposes DNS methods through the repository implementation. |
| Sources/ClashBar/Domain/Repositories/SystemProxyRepository.swift | Expands repository protocol to include DNS operations. |
| Sources/ClashBar/App/Session/Extensions/AppSession+Tun.swift | Sets DNS on TUN enable and restores on disable/failure paths; adds config-based DNS resolution. |
| Sources/ClashBar/App/Session/Coordinators/AppSession+Lifecycle.swift | Restores DNS on termination and core transitions/recovery. |
| Sources/ClashBar/App/Session/Extensions/AppSession+Settings.swift | Refactors settings flow to apply TUN changes via runtime change helper (enabling DNS hooks). |
| Sources/ClashBar/App/Session/Extensions/AppSession+ControllerValidation.swift | Reuses new YAML scalar utilities and exposes whitespace helper used by TUN parsing. |
| Sources/ClashBar/App/Session/Extensions/AppSession+Config.swift | Uses refactored applyTunRuntimeChange path. |
| Sources/ClashBar/Shared/Utilities/YAMLScalarUtilities.swift | New shared helpers for stripping inline comments and normalizing YAML scalar values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "\(state.port)|\(state.servers)" | ||
| }.joined(separator: "\n") | ||
| let dir = (backupFile as NSString).deletingLastPathComponent | ||
| try? FileManager.default.createDirectory(atPath: dir, withIntermediateDirectories: true) |
There was a problem hiding this comment.
createDirectory(atPath:withIntermediateDirectories:) is missing the required attributes: parameter, which will not compile with Foundation’s FileManager API. Pass attributes: nil (or explicit permissions) to fix the build.
| try? FileManager.default.createDirectory(atPath: dir, withIntermediateDirectories: true) | |
| try? FileManager.default.createDirectory(atPath: dir, withIntermediateDirectories: true, attributes: nil) |
| private static let backupFilePath = "/var/db/clashbar/original-dns.txt" | ||
|
|
||
| private struct HardwarePortResult { | ||
| let port: String | ||
| let interface: String | ||
| } | ||
|
|
||
| private struct DNSState { | ||
| let port: String | ||
| let servers: String | ||
| } | ||
|
|
||
| func setDNSServers(dnsServer: String) throws { | ||
| guard Self.isValidIPv4(dnsServer) else { | ||
| throw ProxyHelperError.systemConfigurationFailure( | ||
| action: "Set DNS servers", | ||
| code: 0, | ||
| detail: "Invalid DNS server address: \(dnsServer)") | ||
| } | ||
|
|
||
| let ports = try detectPhysicalHardwarePorts() | ||
| guard !ports.isEmpty else { | ||
| throw ProxyHelperError.systemConfigurationFailure( | ||
| action: "Set DNS servers", | ||
| code: 0, | ||
| detail: "No physical network service found") | ||
| } | ||
|
|
||
| let currentStates = try ports.map { port in | ||
| DNSState(port: port.port, servers: try readCurrentDNS(port: port.port)) | ||
| } | ||
|
|
||
| let backupFile = Self.backupFilePath | ||
| if !FileManager.default.fileExists(atPath: backupFile) { | ||
| let backup = currentStates.map { state in | ||
| "\(state.port)|\(state.servers)" | ||
| }.joined(separator: "\n") | ||
| let dir = (backupFile as NSString).deletingLastPathComponent | ||
| try? FileManager.default.createDirectory(atPath: dir, withIntermediateDirectories: true) | ||
| try backup.write(toFile: backupFile, atomically: true, encoding: .utf8) | ||
| } |
There was a problem hiding this comment.
The DNS backup file is written under /var/db/clashbar without explicitly setting restrictive permissions/ownership. If that directory or file ends up writable by non-root, an unprivileged user could tamper with original-dns.txt and influence what the privileged helper restores later. Create the directory/file with explicit root-only permissions (e.g., 0700/0600) and consider validating file ownership/mode before trusting its contents.
| let listResult = runProcessSynchronously( | ||
| executable: "/usr/sbin/networksetup", | ||
| arguments: ["-listnetworkserviceorder"]) | ||
|
|
There was a problem hiding this comment.
detectPhysicalHardwarePorts() uses networksetup -listnetworkserviceorder output but doesn’t check exitCode. If networksetup fails, this will silently produce an empty ports list and throw the misleading "No physical network service found" error. Consider failing fast when exitCode != 0 and including combinedOutput in the error detail.
| guard listResult.exitCode == 0 else { | |
| let detail = listResult.combinedOutput.trimmingCharacters(in: .whitespacesAndNewlines) | |
| throw ProxyHelperError.systemConfigurationFailure( | |
| action: "List network service order", | |
| code: listResult.exitCode, | |
| detail: detail.isEmpty | |
| ? "networksetup -listnetworkserviceorder failed with no output" | |
| : detail) | |
| } |
| private func readCurrentDNS(port: String) throws -> String { | ||
| let result = runProcessSynchronously( | ||
| executable: "/usr/sbin/networksetup", | ||
| arguments: ["-getdnsservers", port]) | ||
|
|
||
| let output = result.stdout.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if output.hasPrefix("There aren't any DNS Servers set on") { | ||
| return "empty" | ||
| } | ||
| guard !output.isEmpty else { return "empty" } | ||
| return output |
There was a problem hiding this comment.
readCurrentDNS() ignores exitCode/stderr from networksetup -getdnsservers. On failure it can return empty (or partial stdout) and corrupt the backup/restore behavior. Treat non-zero exit codes as errors (using combinedOutput) so backup/restore doesn’t proceed with invalid data.
|
|
||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| let connection = NSXPCConnection( | ||
| machServiceName: ProxyHelperConstants.machServiceName, | ||
| options: .privileged) | ||
| connection.remoteObjectInterface = NSXPCInterface(with: ProxyHelperProtocol.self) | ||
| connection.activate() | ||
|
|
||
| guard let helper = connection.remoteObjectProxyWithErrorHandler({ _ in | ||
| semaphore.signal() | ||
| }) as? ProxyHelperProtocol else { | ||
| connection.invalidate() | ||
| semaphore.signal() | ||
| return | ||
| } | ||
|
|
||
| operation(helper) { _, _ in | ||
| connection.invalidate() | ||
| semaphore.signal() | ||
| } | ||
| } | ||
|
|
||
| _ = semaphore.wait(timeout: .now() + timeout) |
There was a problem hiding this comment.
invokeBlockingHelper waits with a timeout but does not invalidate the NSXPCConnection when the wait times out. That can leave a live privileged XPC connection running past shutdown/termination and leak resources. Consider invalidating/cancelling the connection on timeout (e.g., by capturing it and calling invalidate() after wait returns .timedOut).
| DispatchQueue.global(qos: .userInitiated).async { | |
| let connection = NSXPCConnection( | |
| machServiceName: ProxyHelperConstants.machServiceName, | |
| options: .privileged) | |
| connection.remoteObjectInterface = NSXPCInterface(with: ProxyHelperProtocol.self) | |
| connection.activate() | |
| guard let helper = connection.remoteObjectProxyWithErrorHandler({ _ in | |
| semaphore.signal() | |
| }) as? ProxyHelperProtocol else { | |
| connection.invalidate() | |
| semaphore.signal() | |
| return | |
| } | |
| operation(helper) { _, _ in | |
| connection.invalidate() | |
| semaphore.signal() | |
| } | |
| } | |
| _ = semaphore.wait(timeout: .now() + timeout) | |
| let connectionLock = NSLock() | |
| var connection: NSXPCConnection? | |
| DispatchQueue.global(qos: .userInitiated).async { | |
| let localConnection = NSXPCConnection( | |
| machServiceName: ProxyHelperConstants.machServiceName, | |
| options: .privileged) | |
| connectionLock.lock() | |
| connection = localConnection | |
| connectionLock.unlock() | |
| localConnection.remoteObjectInterface = NSXPCInterface(with: ProxyHelperProtocol.self) | |
| localConnection.activate() | |
| guard let helper = localConnection.remoteObjectProxyWithErrorHandler({ _ in | |
| localConnection.invalidate() | |
| semaphore.signal() | |
| }) as? ProxyHelperProtocol else { | |
| localConnection.invalidate() | |
| semaphore.signal() | |
| return | |
| } | |
| operation(helper) { _, _ in | |
| localConnection.invalidate() | |
| semaphore.signal() | |
| } | |
| } | |
| if semaphore.wait(timeout: .now() + timeout) == .timedOut { | |
| connectionLock.lock() | |
| let timedOutConnection = connection | |
| connectionLock.unlock() | |
| timedOutConnection?.invalidate() | |
| } |
| // Restore DNS AFTER disabling TUN, once the default route is back on the physical interface. | ||
| try await self.restoreTunDNS() |
There was a problem hiding this comment.
DNS is restored immediately after issuing the disable PATCH, before verifying the runtime TUN state. If the PATCH returns but TUN remains enabled (or verification fails), this will restore system DNS while TUN is still active, reintroducing the exact DNS-bypass problem this PR is trying to solve. Restore DNS only after verifyTunRuntimeState(expectedEnabled: false) succeeds (or re-set DNS if disable verification fails).
| // Restore DNS AFTER disabling TUN, once the default route is back on the physical interface. | |
| try await self.restoreTunDNS() | |
| // Restore DNS only AFTER the runtime confirms TUN is disabled, so we do not | |
| // reintroduce DNS bypass if the PATCH returns before the TUN state changes. | |
| do { | |
| try await self.verifyTunRuntimeState(expectedEnabled: false) | |
| try await self.restoreTunDNS() | |
| } catch { | |
| do { | |
| try await self.setTunDNSForEnable() | |
| } catch { | |
| appendLog( | |
| level: "error", | |
| message: "Failed to re-apply TUN DNS after disable verification failure: \(error.localizedDescription)" | |
| ) | |
| } | |
| throw error | |
| } |
| // the physical interface. This also covers the case where no en* service | ||
| // is found by falling back to the default route approach. |
There was a problem hiding this comment.
The comment claims a fallback "default route approach" when no en* service is found, but the helper currently throws when no physical network service is detected and there is no default-route-based fallback shown here. Please either implement the fallback or adjust the comment so it accurately describes the behavior.
| // the physical interface. This also covers the case where no en* service | |
| // is found by falling back to the default route approach. | |
| // the physical interface while DNS is being prepared for TUN enablement. | |
| // This path relies on setTunDNSForEnable() succeeding; there is no | |
| // default-route fallback implemented here when no physical service is found. |
| } else if self.isTunEnabled { | ||
| // TUN already active — ensure DNS is set since patchTunConfig was skipped |
There was a problem hiding this comment.
During core feature recovery, if runtimeConfig.tunEnabled is already true but self.isTunEnabled is false (which can happen because prepareCoreFeatureRecoveryBeforeCoreTransition sets it to false), this branch will skip tunRestored = true and won’t set DNS. That leaves the app thinking TUN isn’t restored even though it is. Consider treating runtimeConfig.tunEnabled == true as restored regardless of self.isTunEnabled, and ensure DNS is set in that case.
| } else if self.isTunEnabled { | |
| // TUN already active — ensure DNS is set since patchTunConfig was skipped | |
| } else { | |
| // TUN already active in runtime — ensure DNS is set since patchTunConfig was skipped, | |
| // even if the local isTunEnabled flag was reset during core transition preparation. |
| // Restore DNS before stopping core (TUN mode may have set system DNS) | ||
| if isTunEnabled { | ||
| self.systemProxyRepository.restoreDNSServersBlocking(timeout: 2.0) | ||
| } |
There was a problem hiding this comment.
Shutdown restores DNS only when isTunEnabled is true. If the app previously set DNS but the in-memory flag is stale/false (or recovery logic hasn’t run yet), DNS may not be restored on termination even though the helper backup file exists. Since restoreDNSServersBlocking is effectively a no-op when there’s no backup, consider calling it unconditionally during shutdown.
| // Restore DNS before stopping core (TUN mode may have set system DNS) | |
| if isTunEnabled { | |
| self.systemProxyRepository.restoreDNSServersBlocking(timeout: 2.0) | |
| } | |
| // Restore DNS before stopping core; this is safe to attempt even when no DNS backup exists. | |
| self.systemProxyRepository.restoreDNSServersBlocking(timeout: 2.0) |
- Add attributes parameter to createDirectory and set restrictive permissions (0700/0600) - Check exitCode in detectPhysicalHardwarePorts() and readCurrentDNS() - Invalidate XPC connection on timeout in invokeBlockingHelper - Restore DNS only after TUN disable verification succeeds - Fix misleading comment about fallback in patchTunConfig - Remove stale isTunEnabled guard in core feature recovery - Call DNS restore unconditionally during shutdown - Remove duplicate verification in applyTunRuntimeChange Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@copilot review |
- Add attributes parameter to createDirectory and set restrictive permissions (0700/0600) - Check exitCode in detectPhysicalHardwarePorts() and readCurrentDNS() - Invalidate XPC connection on timeout in invokeBlockingHelper - Restore DNS only after TUN disable verification succeeds - Fix misleading comment about fallback in patchTunConfig - Remove stale isTunEnabled guard in core feature recovery - Call DNS restore unconditionally during shutdown - Remove duplicate verification in applyTunRuntimeChange Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
iTsingchen
force-pushed
the
feat/tun-system-dns
branch
from
0a03dcf to
cee1767
Compare
…sable When TUN mode is enabled, ClashBar now automatically sets the system DNS to the TUN gateway address (198.18.0.1) via the privileged XPC helper. This prevents macOS scoped routing from bypassing TUN when multiple network interfaces (e.g. VPN) are active, which caused DNS pollution by GFW (Sitoi#55). - Add setDNS/restoreDNS to SystemProxyRepository and XPC helper protocol - Handle DNS setup in TUN lifecycle (enable/disable/cleanup) - Extract YAML scalar utility for config parsing - Simplify controller validation logic Closes Sitoi#55 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add attributes parameter to createDirectory and set restrictive permissions (0700/0600) - Check exitCode in detectPhysicalHardwarePorts() and readCurrentDNS() - Invalidate XPC connection on timeout in invokeBlockingHelper - Restore DNS only after TUN disable verification succeeds - Fix misleading comment about fallback in patchTunConfig - Remove stale isTunEnabled guard in core feature recovery - Call DNS restore unconditionally during shutdown - Remove duplicate verification in applyTunRuntimeChange Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
iTsingchen
force-pushed
the
feat/tun-system-dns
branch
from
cee1767 to
cfe35b6
Compare
Summary
Closes #55
When TUN mode is enabled alongside other VPN (e.g. Viscosity OpenVPN), macOS scoped routing causes system DNS queries to bypass the TUN interface and get polluted by GFW, making sites like YouTube unreachable.
This PR adds automatic system DNS management to ClashBar's TUN lifecycle:
Changes
SystemProxyRepository.swiftsetDNS/restoreDNSto protocolDefaultSystemProxyRepository.swiftSystemProxyService.swiftProxyHelperProtocol.swiftProxyHelper/Daemon/main.swiftsetDNS/restoreDNSvianetworksetupAppSession+Tun.swiftAppSession+Lifecycle.swiftAppSession+Settings.swiftAppSession+ControllerValidation.swiftYAMLScalarUtilities.swiftRoot Cause Analysis
macOS creates a scoped default route for each network service with DNS configured. When multiple interfaces have default routes,
mDNSResponderbinds DNS queries to the physical interface (en0), bypassing TUN route rules and getting polluted by GFW.Setting system DNS directly to the TUN gateway ensures all DNS queries flow through Clash, regardless of scoped routing behavior.
Testing
🤖 Generated with Claude Code