From dcb9d9e03c13cca5b3bf717a434baa97a18dbedf Mon Sep 17 00:00:00 2001 From: Kay Date: Thu, 17 Sep 2026 21:30:51 +0200 Subject: [PATCH] bugs.md #7: Isolation kappt jetzt auch bereits bestehende Verbindungen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Core/Models/FirewallConfig.swift | 64 ++++++++++++------- .../Core/Networking/SSHTransport.swift | 52 +++++++++++++++ .../Core/Services/ConnectionService.swift | 19 ++++++ .../Wizard/Steps/Setup/SetupViewModel.swift | 34 +++++++++- .../FirewallConfigTests.swift | 21 ++++++ bugs.md | 41 ++++++++++-- found.md | 25 ++++++++ 7 files changed, 226 insertions(+), 30 deletions(-) diff --git a/RouterOSAssistant/Core/Models/FirewallConfig.swift b/RouterOSAssistant/Core/Models/FirewallConfig.swift index 49c8f96..af4c80b 100644 --- a/RouterOSAssistant/Core/Models/FirewallConfig.swift +++ b/RouterOSAssistant/Core/Models/FirewallConfig.swift @@ -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>() - 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 } } diff --git a/RouterOSAssistant/Core/Networking/SSHTransport.swift b/RouterOSAssistant/Core/Networking/SSHTransport.swift index fcb5681..a914167 100644 --- a/RouterOSAssistant/Core/Networking/SSHTransport.swift +++ b/RouterOSAssistant/Core/Networking/SSHTransport.swift @@ -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 + /// ` 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=]` — + /// 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 { diff --git a/RouterOSAssistant/Core/Services/ConnectionService.swift b/RouterOSAssistant/Core/Services/ConnectionService.swift index 823cf23..a973cb9 100644 --- a/RouterOSAssistant/Core/Services/ConnectionService.swift +++ b/RouterOSAssistant/Core/Services/ConnectionService.swift @@ -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() diff --git a/RouterOSAssistant/Features/Wizard/Steps/Setup/SetupViewModel.swift b/RouterOSAssistant/Features/Wizard/Steps/Setup/SetupViewModel.swift index 7c0b552..a4e51f4 100644 --- a/RouterOSAssistant/Features/Wizard/Steps/Setup/SetupViewModel.swift +++ b/RouterOSAssistant/Features/Wizard/Steps/Setup/SetupViewModel.swift @@ -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 diff --git a/RouterOSAssistantTests/FirewallConfigTests.swift b/RouterOSAssistantTests/FirewallConfigTests.swift index 82a82ca..5b57999 100644 --- a/RouterOSAssistantTests/FirewallConfigTests.swift +++ b/RouterOSAssistantTests/FirewallConfigTests.swift @@ -81,4 +81,25 @@ final class FirewallConfigTests: XCTestCase { XCTAssertEqual(config.buildCommands().count, 8) } + + /// Regression test for bugs.md #7 / Gitea #19: `isolatedNetworkPairs` is the shared source of + /// truth `SetupViewModel.flushIsolatedNetworkConnections()` uses to know which network pairs + /// to flush already-open connections between — must report the same pairs + /// `buildIsolationCommands` derives its drop rules from, including the network's address + /// range (not used by the drop rules themselves, only by the connection flush). + func testIsolatedNetworkPairsCarriesAddressesForConnectionFlush() { + let config = FirewallConfig( + wanInterface: "ether1", + networks: [ + FirewallConfig.NetworkSegment(interfaceName: "bridge", isolated: true, networkAddress: "192.168.88.0/24"), + FirewallConfig.NetworkSegment(interfaceName: "vlan20", isolated: false, networkAddress: "192.168.20.0/24") + ] + ) + + let pairs = config.isolatedNetworkPairs + + XCTAssertEqual(pairs.count, 1) + XCTAssertEqual(Set([pairs[0].0.interfaceName, pairs[0].1.interfaceName]), ["bridge", "vlan20"]) + XCTAssertEqual(Set([pairs[0].0.networkAddress, pairs[0].1.networkAddress]), ["192.168.88.0/24", "192.168.20.0/24"]) + } } diff --git a/bugs.md b/bugs.md index 20a61fb..1688e52 100644 --- a/bugs.md +++ b/bugs.md @@ -175,10 +175,10 @@ gelesen — deutlich seltener als die vorherige 100%-Fehlerquote bei jedem mehrw Regressionstest `testParseGenericItemsPreservesUnquotedMultiWordValue`, abgeleitet vom echten Live-Dump. Build + alle 101 Unit-Tests grün. -### 7. Beobachtung (kein Fix): Isolation wirkt nicht rückwirkend auf bereits bestehende Verbindungen -**Status:** offen (bewusst nicht automatisch gefixt — Risikoabwägung, siehe unten) +### 7. Isolation wirkte nicht rückwirkend auf bereits bestehende Verbindungen +**Status:** fixed (best-effort, Mechanismus teilweise live verifiziert — Details unten) **Gitea-Issue:** [#19](http://192.168.178.222:3500/kay/RouterOS/issues/19) -**Confidence:** hoch (Regel-Reihenfolge im Code nachvollzogen), nicht live reproduziert +**Confidence:** mittel — siehe ehrliche Verifikationslage unten `FirewallConfig.buildCommands()` (`FirewallConfig.swift`) setzt die Regel "forward established,related → accept" auf Position 4, die Isolations-Drop-Regeln erst ab Position @@ -186,10 +186,39 @@ established,related → accept" auf Position 4, die Isolations-Drop-Regeln erst korrekt (erstes Paket hat `connection-state=new`, trifft also nicht Regel 4, sondern die Isolations-Regel weiter hinten). Für eine zum Zeitpunkt des Anwendens bereits **bestehende** (im Conntrack getrackte) Verbindung zwischen zwei gerade erst als isoliert markierten Netzen -greift dagegen weiterhin Regel 4 zuerst — sie bleibt offen, bis sie von selbst endet. Das ist +griff dagegen weiterhin Regel 4 zuerst — sie blieb offen, bis sie von selbst endete. Das ist Standardverhalten jeder stateful/conntrack-basierten Firewall (RouterOS, iptables, pf, …), kein -App-spezifischer Fehler, und der bisher einzige Live-Test dieser Funktion (M8, 2026-09-15) betraf -einen frisch aufgeteilten Port ohne bestehende Verbindung — dieser Randfall wurde nie geprüft. +App-spezifischer Fehler. + +**Fix:** neue `SSHTransport.flushConnections(between:and:)` + `ConnectionService.flushConnections` +(dedizierte SSH-Verbindung, kein REST-Äquivalent für RouterOS' CIDR-Mitgliedschafts-Abfragesprache +vorhanden) — entfernt per `/ip firewall connection remove [find where (src-address in A) and +(dst-address in B)]` (beide Richtungen) bereits getrackte Verbindungen zwischen zwei gerade +isolierten Netzen, aufgerufen von `SetupViewModel.apply()` direkt nach den Firewall-Befehlen +selbst (damit eine sofort neu aufgebaute Verbindung schon auf die neue Drop-Regel trifft). +`FirewallConfig.NetworkSegment` um `networkAddress` (CIDR) erweitert, Paar-Logik in eine +wiederverwendbare `isolatedNetworkPairs`-Property extrahiert. `networkA`/`networkB` werden vor +der Interpolation ins Skript als reine CIDR-Notation validiert (dieselbe Vorsicht wie bei Fund +#5 — sonst neue Injection-Fläche). + +**Ehrliche Verifikationslage:** `print count-only where src-address in ` ist live bestätigt, +korrekt nach Subnetz zu filtern (40 Treffer bei besetztem /24 vs. 0 bei leerem). Ein einfaches +`remove [find where dst-address=]` (Gleichheit, kein `in`) ist live bestätigt, einen +Eintrag wirklich zu löschen (Test-ICMP-Verbindung verschwand). Die kombinierte Form `remove [find +where (a in X) and (b in Y)]`, die der Fix tatsächlich nutzt, ließ sich NICHT sauber live +beweisen: ein erster Testlauf sah erfolgreich aus, erwies sich aber als Messfehler (ICMP-Conntrack +verfällt von selbst in wenigen Sekunden — der Mehrfach-SSH-Testablauf mit Verzögerung dazwischen +täuschte ein Löschen nur vor). Ein sauberer Nachtest an einer echten, weiterhin aktiven +TCP-Verbindung zeigte den Eintrag sofort wieder auftauchen — erwartbares Verhalten für Connection- +Tracking allgemein (kein RST, nächstes Paket einer aktiven Verbindung wird einfach neu getrackt), +kein Beweis, dass `remove` wirkungslos ist, aber eben auch kein Beweis, dass die neue +Isolations-Regel das neu getrackte Paket tatsächlich abfängt. Dafür bräuchte es zwei echte, getrennte +Testnetze mit echten Endgeräten, die hier nicht verfügbar sind. Die `remove [find where ... in +...]`-Technik selbst ist real und community-dokumentiert (MikroTik-Forum), nur nicht +Ende-zu-Ende gegen dieses konkrete App-Szenario bewiesen. Best-effort per Design (Fehler bricht +den Apply-Vorgang nicht ab) — ein echter Mehrnetz-Test ist der natürliche nächste Schritt. + +Build + alle 103 Unit-Tests grün (1 neuer Regressionstest für die Paar-Logik). Nicht automatisch gefixt: ein Fix würde bedeuten, beim Aktivieren der Isolation gezielt `/ip firewall connection remove` für die betroffenen Netzpaare auszulösen — ein zusätzlicher, diff --git a/found.md b/found.md index cf1b453..cf40c27 100644 --- a/found.md +++ b/found.md @@ -336,3 +336,28 @@ Build grün, alle 102 Unit-Tests grün. Der ursprünglich gemeldete Milestone (P selbst, inkl. Warndialoge, "Weiter"-Sperre, "Fertig"-Button) bleibt beim Status "Code-Review bestätigt" — ein echter Live-Klicktest durch den Nutzer in der App-UI steht weiterhin aus, da UI-Automatisierung in dieser Session nicht verfügbar ist. + +### 15. bugs.md #7 bearbeitet: Isolation trennt jetzt auch bereits bestehende Verbindungen +**Status:** fixed (best-effort, Mechanismus teilweise live verifiziert) +**Gitea-Issue:** [#19](http://192.168.178.222:3500/kay/RouterOS/issues/19) + +Auf Nutzerwunsch ("bearbeite #7") den zuvor bewusst zurückgestellten Punkt jetzt umgesetzt: 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 zwei Netzen, sobald sie als isoliert angewendet werden — aufgerufen direkt +nach den Firewall-Befehlen in `SetupViewModel.apply()`. + +Bemerkenswert am Weg dorthin: eine erste Live-Verifikation sah erfolgreich aus (Test-Verbindung +verschwand nach `remove`), erwies sich bei genauerem Hinsehen aber als Messfehler — die +ICMP-Test-Verbindung war einfach von selbst abgelaufen (RouterOS' sehr kurzer ICMP-Conntrack- +Timeout), nicht durch den `remove`-Befehl entfernt worden. Ein sauberer Nachtest an einer +tatsächlich noch aktiven TCP-Verbindung zeigte den Eintrag sofort wieder auftauchen. Root Cause +geklärt (Web-Recherche + Verhalten selbst nachvollzogen): Connection-Tracking-Removal sendet kein +RST, eine aktiv weiterlaufende Verbindung wird beim nächsten Paket einfach neu getrackt — kein +Beweis, dass `remove` nichts tut, aber auch kein Beweis, dass die neue Isolations-Regel das neu +getrackte Paket abfängt. Für den vollständigen Beweis fehlen zwei echte, getrennte Testnetze mit +echten Endgeräten. Dokumentation entsprechend ehrlich mit dem tatsächlichen Verifikationsstand +statt einer überzogenen "live bestätigt"-Behauptung versehen (siehe `bugs.md` #7 für die volle +Herleitung). + +Build grün, alle 103 Unit-Tests grün (1 neuer Regressionstest für `FirewallConfig.isolatedNetworkPairs`).