bugs.md #7: Isolation kappt jetzt auch bereits bestehende Verbindungen

Bisher wirkten die neuen Firewall-Isolationsregeln nur auf neue Verbindungen - eine bereits offene Verbindung zwischen zwei gerade isolierten Netzen lief unbeeinflusst weiter (Standard-Verhalten jeder stateful Firewall). Neue SSHTransport.flushConnections/ ConnectionService.flushConnections entfernen per /ip firewall connection remove [find where (src-address in A) and (dst-address in B)] bereits getrackte Verbindungen zwischen isolierten Netzpaaren, aufgerufen direkt nach den Firewall-Befehlen in SetupViewModel.apply(). FirewallConfig.NetworkSegment um networkAddress (CIDR) erweitert, Paar-Logik in eine wiederverwendbare isolatedNetworkPairs-Property extrahiert. networkA/networkB werden vor der SSH-Interpolation als reine CIDR-Notation validiert (dieselbe Vorsicht wie bei der zuvor gefixten CLI-Injection). Ehrlicher Verifikationsstand dokumentiert statt Überclaiming: die kombinierte remove-Bedingung ließ sich mangels zweier echter Testnetze nicht end-to-end beweisen - ein erster scheinbarer Erfolg stellte sich als Messfehler heraus (natürlicher ICMP-Conntrack-Timeout, nicht der remove-Befehl selbst). Details in bugs.md #7. Build + alle 103 Unit-Tests grün (1 neuer Regressionstest). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 21:30:51 +02:00
parent 06ddd837f1
commit dcb9d9e03c
7 changed files with 226 additions and 30 deletions
@@ -20,6 +20,14 @@ struct FirewallConfig: Equatable {
    struct NetworkSegment: Equatable {
        var interfaceName: String
        var isolated: Bool
+        /// CIDR range (e.g. "192.168.88.0/24") — not used for the filter rules themselves
+        /// (those match on interface, not address), only for `isolatedNetworkPairs`, which
+        /// `SetupViewModel.apply()` uses to flush already-open connections between two networks
+        /// that just became isolated (bugs.md #7 / Gitea #19). Empty for a network with no
+        /// address filled in yet (e.g. mid-wizard-editing) — such a network is simply skipped by
+        /// the connection-flush, same as it already is by the isolation filter rules once
+        /// applied (an unreachable address isolates itself).
+        var networkAddress: String = ""
    }

    var wanInterface: String
@@ -91,37 +99,49 @@ struct FirewallConfig: Equatable {
        return [natCommand] + filterCommands + isolationCommands
    }

-    /// Forward-drop rules between every network marked `isolated` and every other configured
-    /// network (both directions). Pairs are deduplicated so two mutually isolated networks
-    /// still only get one pair of rules, not two.
-    private func buildIsolationCommands(startingPlaceBefore: Int) -> [RouterOSCommand] {
+    /// Every pair of networks where at least one side is marked `isolated`, deduplicated so two
+    /// mutually isolated networks still only produce one pair, not two. Shared by
+    /// `buildIsolationCommands` (drop rules, by interface) and `SetupViewModel.apply()`'s
+    /// post-apply connection-flush (by address range, bugs.md #7 / Gitea #19) — kept as one
+    /// source of truth so the two can never disagree about which pairs count as isolated.
+    var isolatedNetworkPairs: [(NetworkSegment, NetworkSegment)] {
        var seenPairs = Set<Set<String>>()
-        var commands: [RouterOSCommand] = []
-        var placeBefore = startingPlaceBefore
+        var pairs: [(NetworkSegment, NetworkSegment)] = []

        for network in networks where network.isolated {
            for other in networks where other.interfaceName != network.interfaceName {
                let pair = Set([network.interfaceName, other.interfaceName])
                guard !seenPairs.contains(pair) else { continue }
                seenPairs.insert(pair)
+                pairs.append((network, other))
+            }
+        }

-                for (from, to) in [(network.interfaceName, other.interfaceName), (other.interfaceName, network.interfaceName)] {
-                    commands.append(
-                        RouterOSCommand.add(
-                            menuPath: "/ip firewall filter",
-                            restPath: "ip/firewall/filter",
-                            arguments: [
-                                "chain": "forward",
-                                "in-interface": from,
-                                "out-interface": to,
-                                "action": "drop",
-                                "place-before": "\(placeBefore)"
-                            ],
-                            summary: "Netzwerk \"\(from)\" von \"\(to)\" isolieren"
-                        )
+        return pairs
+    }
+
+    /// Forward-drop rules between every isolated network pair (both directions).
+    private func buildIsolationCommands(startingPlaceBefore: Int) -> [RouterOSCommand] {
+        var commands: [RouterOSCommand] = []
+        var placeBefore = startingPlaceBefore
+
+        for (network, other) in isolatedNetworkPairs {
+            for (from, to) in [(network.interfaceName, other.interfaceName), (other.interfaceName, network.interfaceName)] {
+                commands.append(
+                    RouterOSCommand.add(
+                        menuPath: "/ip firewall filter",
+                        restPath: "ip/firewall/filter",
+                        arguments: [
+                            "chain": "forward",
+                            "in-interface": from,
+                            "out-interface": to,
+                            "action": "drop",
+                            "place-before": "\(placeBefore)"
+                        ],
+                        summary: "Netzwerk \"\(from)\" von \"\(to)\" isolieren"
                    )
-                    placeBefore += 1
-                }
+                )
+                placeBefore += 1
            }
        }

@@ -208,6 +208,58 @@ final class SSHTransport: RouterOSTransport {
        }
    }

+    /// Forcibly ends already-tracked connections between two networks (bugs.md #7 / Gitea #19) —
+    /// RouterOS' firewall rules only affect *new* connections, so a connection already open
+    /// between two networks at the moment they're marked isolated would otherwise keep flowing
+    /// through the pre-existing "forward established/related -> accept" rule indefinitely.
+    /// `/ip firewall connection` has no interface field, only address fields, so this matches by
+    /// CIDR-range membership instead. No REST equivalent exists for this CIDR-membership query
+    /// language (REST mirrors CRUD over menu paths, not scripting), so — same reasoning as
+    /// `BackupService`/`NetworkToolsService`/`UpdateService` — this always runs over SSH.
+    ///
+    /// **Verification status, honestly (2026-09-17):** `print count-only where src-address in
+    /// <cidr>` is live-confirmed to filter by real subnet containment (40 matches for a
+    /// populated /24 vs. 0 for an empty one, and `remove [find where dst-address=<exact-ip>]` —
+    /// single equality condition, no `in` — is live-confirmed to actually delete an entry (a
+    /// disposable ICMP ping's tracked connection genuinely disappeared). The compound
+    /// `remove [find where (src-address in A) and (dst-address in B)]` form this function
+    /// actually uses could NOT be cleanly proven live: a first attempt looked successful, but
+    /// that was later traced to ICMP conntrack's very short natural timeout (a few seconds)
+    /// coinciding with the multi-second gap between separate manual SSH round-trips, not the
+    /// `remove` itself. A cleaner retest against a still-*active* long-lived TCP connection (an
+    /// open SSH session) showed the tracked entry reappearing immediately after `remove` — which
+    /// is expected/correct behavior for connection tracking in general (removing the tracking
+    /// state doesn't RST the socket; the very next packet on an actively-flowing connection just
+    /// gets re-tracked as "new"), not proof the `remove` itself is a no-op, but this app has no
+    /// two genuinely separate test networks available to observe the one behavior that actually
+    /// matters here: whether a fresh isolation drop rule catches that re-tracked "new" packet
+    /// instead of silently re-admitting it. The `remove [find where ... in ...]` compound-CIDR
+    /// technique itself is real and community-documented (MikroTik forum), just not end-to-end
+    /// live-verified against this app's specific isolation scenario. Best-effort by design either
+    /// way (see `ConnectionService.flushConnections`'s caller in `SetupViewModel`) — a live
+    /// multi-network test is the natural next verification step.
+    ///
+    /// `networkA`/`networkB` are interpolated directly into the script, so they're validated as
+    /// plain CIDR notation first (digits/dots/slash only) — an unvalidated value here would
+    /// reopen the exact command-injection class already fixed once in
+    /// `RouterOSCommand.quoteIfNeeded` (bugs.md #5). Both directions are removed since either
+    /// isolated network could be the connection's source or destination.
+    func flushConnections(between networkA: String, and networkB: String) async throws {
+        guard Self.isPlainCIDR(networkA), Self.isPlainCIDR(networkB) else {
+            throw RouterOSError.invalidResponse("Ungültiges Netzwerkformat: \(networkA) / \(networkB)")
+        }
+        let script = """
+        /ip firewall connection remove [find where (src-address in \(networkA)) and (dst-address in \(networkB))]
+        /ip firewall connection remove [find where (src-address in \(networkB)) and (dst-address in \(networkA))]
+        """
+        _ = try await run(script)
+    }
+
+    private static func isPlainCIDR(_ value: String) -> Bool {
+        let allowed = CharacterSet(charactersIn: "0123456789./")
+        return !value.isEmpty && value.unicodeScalars.allSatisfy(allowed.contains)
+    }
+
    /// Restores RouterOS' own vendor-default configuration and reboots the device. See
    /// FactoryResetService for why this bypasses the RouterOSCommand add/set model entirely.
    func resetToFactoryDefaults() async throws {
@@ -216,6 +216,25 @@ final class ConnectionService: ObservableObject {
        }
    }

+    /// Forcibly ends already-tracked connections between two networks that were just marked
+    /// isolated — see `SSHTransport.flushConnections(between:and:)`'s doc comment for the full
+    /// reasoning and live verification (bugs.md #7 / Gitea #19). Best-effort: called by
+    /// `SetupViewModel.apply()` right after the isolation filter rules themselves are in place,
+    /// so any connection that re-establishes immediately after being flushed hits the new drop
+    /// rule instead of reopening freely.
+    func flushConnections(between networkA: String, and networkB: String) async throws {
+        guard let credentials else { throw RouterOSError.notConnected }
+        let transport = SSHTransport(credentials: credentials)
+        try await transport.connect()
+        do {
+            try await transport.flushConnections(between: networkA, and: networkB)
+            await transport.disconnect()
+        } catch {
+            await transport.disconnect()
+            throw error
+        }
+    }
+
    func fetchFirewallRuleCounts() async throws -> FirewallRuleCounts {
        guard let activeTransport else { throw RouterOSError.notConnected }
        return try await activeTransport.fetchFirewallRuleCounts()
@@ -73,8 +73,12 @@ final class SetupViewModel: ObservableObject {

    /// All configured LAN + VLAN networks, for the firewall isolation rules.
    private var networkSegments: [FirewallConfig.NetworkSegment] {
-        lanConfigs.map { FirewallConfig.NetworkSegment(interfaceName: $0.interfaceName, isolated: $0.isolated) }
-            + vlans.map { FirewallConfig.NetworkSegment(interfaceName: $0.interfaceName, isolated: $0.isolated) }
+        lanConfigs.map {
+            FirewallConfig.NetworkSegment(interfaceName: $0.interfaceName, isolated: $0.isolated, networkAddress: $0.networkAddress)
+        }
+            + vlans.map {
+                FirewallConfig.NetworkSegment(interfaceName: $0.interfaceName, isolated: $0.isolated, networkAddress: $0.networkAddress)
+            }
    }

    /// Interface names of networks marked isolated, for display on the Firewall step.
@@ -372,6 +376,10 @@ final class SetupViewModel: ObservableObject {
                    try await applyIdempotently(command)
                }

+                if firewallSectionEnabled {
+                    await flushIsolatedNetworkConnections()
+                }
+
                didApplySuccessfully = true
            } catch {
                applyError = error.localizedDescription
@@ -380,6 +388,28 @@ final class SetupViewModel: ObservableObject {
        }
    }

+    /// Ends already-open connections between networks that just became isolated (bugs.md #7 /
+    /// Gitea #19) — without this, the new "forward … drop" rules just added only block *new*
+    /// connections; anything already flowing between the two networks at apply-time would
+    /// otherwise keep going through the existing "forward established/related -> accept" rule
+    /// until it ends on its own. Runs after the firewall commands themselves so a connection that
+    /// re-establishes right after being flushed immediately hits the new drop rule.
+    ///
+    /// Best-effort and silent on failure (`try?`), same as the rest of this best-effort area
+    /// (`checkPortConflict`): a flush that fails (e.g. a transient SSH hiccup) must not fail the
+    /// whole apply — the isolation rules themselves are already in place either way, this only
+    /// affects whether a pre-existing connection gets cut immediately or lingers until it times
+    /// out on its own. A pair where either side has no network address yet (e.g. left blank) is
+    /// skipped — there's no address range to match connections against.
+    private func flushIsolatedNetworkConnections() async {
+        let pairs = FirewallConfig(wanInterface: wanConfig.interfaceName, networks: networkSegments).isolatedNetworkPairs
+        for (network, other) in pairs {
+            guard !network.networkAddress.isEmpty, !other.networkAddress.isEmpty else { continue }
+            applyLog.append("Bestehende Verbindungen zwischen \"\(network.interfaceName)\" und \"\(other.interfaceName)\" trennen…")
+            _ = try? await connectionService.flushConnections(between: network.networkAddress, and: other.networkAddress)
+        }
+    }
+
    /// Menus where a duplicate `.add` should instead reconfigure the existing entry in place —
    /// re-running any wizard step against an already-configured router hits this on every menu
    /// that enforces uniqueness (dhcp-client/pppoe-client: one per interface, live-confirmed