From c78866e360935c728ad4103b2c2275a73fffb2c3 Mon Sep 17 00:00:00 2001 From: nexxo Date: Mon, 3 Aug 2026 17:28:28 +0200 Subject: [PATCH] fix(security): Aufheben einer Sperre sagt die Wahrheit und wird nachgeholt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mein eigener Befund "die Zeile erneuert sich nach dem Aufheben nicht" war falsch — eine Pruefung ueber den ganzen Weg (Modal schickt das Ereignis, Seite faengt es) zeigt, dass die Zeile sehr wohl in den Verlauf wandert. Dabei fiel der echte Fehler auf, der daneben lag: HostFirewall::release() schluckt jeden Fehlschlag und meldet ihn nur ins Log. Beide Aufrufer verwarfen den Rueckgabewert und meldeten in jedem Fall "Sperre aufgehoben." War der Host im Moment des Aufhebens nicht erreichbar, stand der Datensatz auf aufgehoben und die Regel noch drin: Portal und Konsole zeigten "Aufgehoben", waehrend die Adresse weiter ausgesperrt blieb — bis zum Ablauf der urspruenglichen Sperrzeit, ohne dass es jemand sagen konnte. - release() gibt zurueck, ob die Firewall schon nachgezogen hat; alle drei Stellen (Portal, Host-Ansicht, Kunden-Ansicht) sagen es, wenn nicht. - releaseMany() als Gegenstueck zu blockMany(): eine SSH-Sitzung statt einer je Adresse. - ScanForIntrusions gleicht jetzt in BEIDE Richtungen ab. Bisher trug er nur ein; nichts nahm je einen haengengebliebenen Eintrag wieder heraus. Eine Adresse, die eine ANDERE aktive Sperre desselben Hosts noch traegt, bleibt stehen. - Jeder Loeschbefehl traegt `2>/dev/null || true`: nft scheitert am Loeschen eines Elements, das es nicht gibt, und weg ist genau das Ziel. Ohne das meldete der Abgleich bei jedem Lauf einen Fehlschlag. 8 neue Pruefungen, Suite 2625 gruen. --- app/Livewire/Admin/CustomerDetail.php | 7 +- app/Livewire/Admin/HostDetail.php | 7 +- app/Livewire/Security.php | 10 ++- app/Models/SecurityBlock.php | 22 +++++- app/Provisioning/Jobs/ScanForIntrusions.php | 48 ++++++++++++ app/Services/Security/HostFirewall.php | 54 ++++++++++++-- lang/de/admin.php | 1 + lang/de/security.php | 1 + lang/en/admin.php | 1 + lang/en/security.php | 1 + tests/Feature/Security/HostFirewallTest.php | 50 +++++++++++++ .../Security/ScanForIntrusionsTest.php | 73 +++++++++++++++++++ tests/Feature/Security/SecurityPageTest.php | 46 ++++++++++++ 13 files changed, 304 insertions(+), 17 deletions(-) diff --git a/app/Livewire/Admin/CustomerDetail.php b/app/Livewire/Admin/CustomerDetail.php index ea52c60..aa02b1d 100644 --- a/app/Livewire/Admin/CustomerDetail.php +++ b/app/Livewire/Admin/CustomerDetail.php @@ -173,8 +173,11 @@ class CustomerDetail extends Component return; } - $block->release(auth('operator')->user()); - $this->dispatch('notify', message: __('admin.security_block.released')); + // Siehe SecurityBlock::release() — der Rueckgabewert sagt, ob die + // Firewall schon nachgezogen hat, nicht ob der Datensatz steht. + $this->dispatch('notify', message: $block->release(auth('operator')->user()) + ? __('admin.security_block.released') + : __('admin.security_block.released_pending')); } /** diff --git a/app/Livewire/Admin/HostDetail.php b/app/Livewire/Admin/HostDetail.php index 88cc3d7..8348886 100644 --- a/app/Livewire/Admin/HostDetail.php +++ b/app/Livewire/Admin/HostDetail.php @@ -126,8 +126,11 @@ class HostDetail extends Component return; } - $block->release(auth('operator')->user()); - $this->dispatch('notify', message: __('admin.security_block.released')); + // Siehe SecurityBlock::release() — der Rueckgabewert sagt, ob die + // Firewall schon nachgezogen hat, nicht ob der Datensatz steht. + $this->dispatch('notify', message: $block->release(auth('operator')->user()) + ? __('admin.security_block.released') + : __('admin.security_block.released_pending')); } public function retry(): void diff --git a/app/Livewire/Security.php b/app/Livewire/Security.php index a0b6dfd..5d71059 100644 --- a/app/Livewire/Security.php +++ b/app/Livewire/Security.php @@ -39,9 +39,13 @@ class Security extends Component abort_if($block === null, 403); - $block->release(auth()->user()); - - $this->dispatch('notify', message: __('security.blocks_released')); + // Der Datensatz steht in jedem Fall; nur die Firewall kann nachhinken, + // wenn der Host gerade nicht erreichbar ist. Dann sagen wir das — + // sonst versucht sich jemand gleich wieder anzumelden und wird + // wortlos erneut abgewiesen. + $this->dispatch('notify', message: $block->release(auth()->user()) + ? __('security.blocks_released') + : __('security.blocks_released_pending')); } public function render() diff --git a/app/Models/SecurityBlock.php b/app/Models/SecurityBlock.php index a988a20..3a30aae 100644 --- a/app/Models/SecurityBlock.php +++ b/app/Models/SecurityBlock.php @@ -67,7 +67,21 @@ class SecurityBlock extends Model * aufgehoben hat; sonst der Operator oder Customer, der den Knopf gedrückt * hat (R23-Modal auf der aufrufenden Seite, nicht hier). */ - public function release(?Model $by): void + /** + * Hebt die Sperre auf — und sagt, ob die Regel wirklich gefallen ist. + * + * Der Datensatz wird IMMER zuerst gespeichert, auch wenn der Host gerade + * nicht erreichbar ist: er ist die Sperre, die Firewall-Menge nur ihr + * Abdruck (siehe Klassenkopf), und `reapplyActiveBlocks()` traegt beim + * naechsten Lauf nur noch ein, was hier aktiv steht. + * + * Der Rueckgabewert ist deshalb NICHT „hat es geklappt", sondern „steht + * die Firewall schon nach". Er wird gebraucht, weil `HostFirewall::apply()` + * jeden Fehlschlag schluckt und nur ins Log meldet — ohne ihn faende der + * Mensch am Bildschirm „Sperre aufgehoben." vor, waehrend die Adresse + * weiter ausgesperrt ist und er sich das nicht erklaeren kann. + */ + public function release(?Model $by): bool { $this->releasedBy()->associate($by); $this->released_at = now(); @@ -75,8 +89,10 @@ class SecurityBlock extends Model $host = $this->host ?? $this->instance?->host; - if ($host !== null) { - app(HostFirewall::class)->release($host, $this->ip); + if ($host === null) { + return true; // Kein Host, keine Regel — nichts, was nachhinken koennte. } + + return app(HostFirewall::class)->release($host, $this->ip); } } diff --git a/app/Provisioning/Jobs/ScanForIntrusions.php b/app/Provisioning/Jobs/ScanForIntrusions.php index 0645ce4..d1a26e5 100644 --- a/app/Provisioning/Jobs/ScanForIntrusions.php +++ b/app/Provisioning/Jobs/ScanForIntrusions.php @@ -261,5 +261,53 @@ class ScanForIntrusions implements ShouldQueue foreach ($perHost as $entry) { $firewall->blockMany($entry['host'], $entry['seconds']); } + + $this->removeLiftedBlocks($firewall, $perHost); + } + + /** + * Und wieder heraus, was aufgehoben wurde, aber noch drinstehen könnte. + * + * `release()` scheitert still, wenn der Host im Moment des Aufhebens nicht + * erreichbar war — der Datensatz steht dann auf aufgehoben, die Regel aber + * noch. Ohne diesen Abgleich bliebe die Adresse bis zum Ablauf der + * ursprünglichen Sperrzeit ausgesperrt, während Portal und Konsole + * „Aufgehoben" zeigen. Genau die Lücke, die den Menschen ratlos lässt. + * + * Betrachtet werden nur aufgehobene Sperren, deren Ablaufzeit noch in der + * Zukunft liegt: danach hat der Kernel den Eintrag selbst herausgenommen + * (`flags timeout`), und es gäbe nichts mehr zu tun. + * + * Eine Adresse, die eine ANDERE aktive Sperre desselben Hosts noch trägt, + * bleibt stehen — sonst hebt das Aufheben der einen Sperre die andere + * gleich mit auf. + * + * @param array}> $perHost die soeben eingetragenen + */ + private function removeLiftedBlocks(HostFirewall $firewall, array $perHost): void + { + $lifted = SecurityBlock::query() + ->whereNotNull('released_at') + ->where('expires_at', '>', now()) + ->with(['host', 'instance.host']) + ->get(); + + /** @var array}> */ + $perHostRemove = []; + + foreach ($lifted as $block) { + $host = $block->host ?? $block->instance?->host; + + if ($host === null || isset($perHost[$host->id]['seconds'][$block->ip])) { + continue; + } + + $perHostRemove[$host->id] ??= ['host' => $host, 'ips' => []]; + $perHostRemove[$host->id]['ips'][$block->ip] = $block->ip; // je Adresse einmal + } + + foreach ($perHostRemove as $entry) { + $firewall->releaseMany($entry['host'], array_values($entry['ips'])); + } } } diff --git a/app/Services/Security/HostFirewall.php b/app/Services/Security/HostFirewall.php index 39e3ef6..4397940 100644 --- a/app/Services/Security/HostFirewall.php +++ b/app/Services/Security/HostFirewall.php @@ -112,15 +112,55 @@ class HostFirewall public function release(Host $host, string $ip): bool { - if (! $this->isWellFormed($host, $ip)) { - return false; + return $this->releaseMany($host, [$ip]); + } + + /** + * Mehrere Adressen desselben Hosts wieder heraus, in EINER SSH-Sitzung — + * das Gegenstück zu blockMany(), aus demselben Grund. + * + * `|| true` hinter jedem Befehl ist hier kein Wegsehen, sondern die + * richtige Bedeutung: `nft delete element` scheitert, wenn das Element + * schon weg ist — und weg ist genau das Ziel. Der Kernel nimmt abgelaufene + * Einträge selbst heraus (`flags timeout`), und eine Sperre, die wegen + * eines nicht erreichbaren Hosts nie eingetragen wurde, kann man auch + * nicht löschen. Ohne dieses `|| true` meldete der Abgleich unten bei + * JEDEM Lauf einen Fehlschlag, und der Mensch am Bildschirm bekäme + * „Server nicht erreichbar" zu lesen, während in Wahrheit alles stimmt. + * + * Was bleibt, ist die Aussage, um die es geht: `false` heißt, der Host war + * nicht erreichbar — das wirft `connectWithKey()`, und `apply()` fängt es. + * + * @param array $ips + */ + public function releaseMany(Host $host, array $ips): bool + { + /** @var array> */ + $elements = []; + + foreach ($ips as $ip) { + if (! $this->isWellFormed($host, $ip)) { + return false; + } + + $elements[$this->setFor($ip)][] = $ip; } - return $this->apply($host, [sprintf( - 'nft delete element inet clupilot_filter %s { %s }', - $this->setFor($ip), - $ip, - )], $ip); + if ($elements === []) { + return true; + } + + $commands = []; + + foreach ($elements as $set => $entries) { + $commands[] = sprintf( + 'nft delete element inet clupilot_filter %s { %s } 2>/dev/null || true', + $set, + implode(', ', $entries), + ); + } + + return $this->apply($host, $commands, implode(', ', $ips)); } /** diff --git a/lang/de/admin.php b/lang/de/admin.php index 3b4d39c..0769c34 100644 --- a/lang/de/admin.php +++ b/lang/de/admin.php @@ -131,6 +131,7 @@ return [ 'status_expired' => 'Abgelaufen', 'release' => 'Aufheben', 'released' => 'Sperre aufgehoben.', + 'released_pending' => 'Sperre aufgehoben. Der Host ist gerade nicht erreichbar — die Firewall zieht nach, sobald er wieder antwortet.', 'release_title' => 'Sperre aufheben?', 'release_body' => 'Die Adresse :ip wird sofort wieder zugelassen.', 'release_confirm' => 'Aufheben', diff --git a/lang/de/security.php b/lang/de/security.php index 2b3af2e..36e2aca 100644 --- a/lang/de/security.php +++ b/lang/de/security.php @@ -97,4 +97,5 @@ return [ 'release_body' => 'Die Adresse :ip wird sofort wieder zugelassen. Heben Sie eine Sperre nur auf, wenn Sie sicher sind, dass die Anmeldeversuche von Ihnen selbst stammten.', 'release_confirm' => 'Aufheben', 'blocks_released' => 'Sperre aufgehoben.', + 'blocks_released_pending' => 'Sperre aufgehoben. Der Server ist gerade nicht erreichbar — die Adresse wird zugelassen, sobald er wieder antwortet.', ]; diff --git a/lang/en/admin.php b/lang/en/admin.php index 2bfb723..aca0ec7 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -131,6 +131,7 @@ return [ 'status_expired' => 'Expired', 'release' => 'Lift', 'released' => 'Block lifted.', + 'released_pending' => 'Block lifted. The host is not reachable right now — the firewall will follow as soon as it answers again.', 'release_title' => 'Lift this block?', 'release_body' => 'Address :ip will be allowed through again immediately.', 'release_confirm' => 'Lift', diff --git a/lang/en/security.php b/lang/en/security.php index cbb24a6..28538c0 100644 --- a/lang/en/security.php +++ b/lang/en/security.php @@ -98,4 +98,5 @@ return [ 'release_body' => 'Address :ip will be allowed through again immediately. Only lift a block if you are sure the sign-in attempts were your own.', 'release_confirm' => 'Lift', 'blocks_released' => 'Block lifted.', + 'blocks_released_pending' => 'Block lifted. The server is not reachable right now — the address will be allowed through as soon as it answers again.', ]; diff --git a/tests/Feature/Security/HostFirewallTest.php b/tests/Feature/Security/HostFirewallTest.php index 7ece6a4..a78b334 100644 --- a/tests/Feature/Security/HostFirewallTest.php +++ b/tests/Feature/Security/HostFirewallTest.php @@ -204,3 +204,53 @@ it('fuehrt aus einem Buendel gar nichts aus, wenn eine einzige Adresse keine ist expect($ok)->toBeFalse() ->and($shell->recorded())->toBe([]); }); + +it('nimmt mehrere Adressen desselben Hosts in EINER Verbindung heraus', function () { + $shell = new FakeRemoteShell; + app()->instance(\App\Services\Ssh\RemoteShell::class, $shell); + $host = Host::factory()->active()->create(['ssh_host_key' => 'SHA256:abc']); + + app(HostFirewall::class)->releaseMany($host, ['203.0.113.7', '203.0.113.8', '2001:db8::1']); + + // Zwei Befehle, weil v4 und v6 in getrennten Mengen leben — aber EINE + // Verbindung. Das ist der Grund, aus dem es diese Methode gibt. + expect($shell->connections)->toHaveCount(1) + ->and($shell->ran('delete element inet clupilot_filter clupilot_blocked { 203.0.113.7, 203.0.113.8 }'))->toBeTrue() + ->and($shell->ran('delete element inet clupilot_filter clupilot_blocked6 { 2001:db8::1 }'))->toBeTrue(); +}); + +it('traegt jedem Loeschbefehl sein eigenes Auffangnetz mit', function () { + // nft scheitert beim Loeschen eines Elements, das es nicht gibt — und genau + // das ist der Normalfall: der Kernel hat es selbst herausgenommen + // (`flags timeout`), oder es stand nie drin, weil der Host beim Sperren + // nicht erreichbar war. Ohne das `|| true` meldete der Abgleich in + // ScanForIntrusions bei JEDEM Lauf einen Fehlschlag, und der Mensch am + // Bildschirm bekaeme „Server nicht erreichbar" zu lesen, obwohl alles + // stimmt. + // + // Geprueft am erzeugten BEFEHL, nicht am Verhalten des Fakes: dieser fuehrt + // keine Shell aus, `|| true` kann er also gar nicht abbilden. Wirksam ist + // es genau dort, wo der Befehl hingeht — in /bin/sh auf dem Host. + $shell = new FakeRemoteShell; + app()->instance(\App\Services\Ssh\RemoteShell::class, $shell); + $host = Host::factory()->active()->create(['ssh_host_key' => 'SHA256:abc']); + + app(HostFirewall::class)->releaseMany($host, ['203.0.113.7', '2001:db8::1']); + + $loeschbefehle = array_filter($shell->recorded(), fn ($b) => str_contains($b, 'delete element')); + + expect($loeschbefehle)->toHaveCount(2); + + foreach ($loeschbefehle as $befehl) { + expect($befehl)->toEndWith('2>/dev/null || true'); + } +}); + +it('meldet sehr wohl, wenn der Host gar nicht erreichbar ist', function () { + $shell = new FakeRemoteShell; + $shell->failConnect = true; + app()->instance(\App\Services\Ssh\RemoteShell::class, $shell); + $host = Host::factory()->active()->create(['ssh_host_key' => 'SHA256:abc']); + + expect(app(HostFirewall::class)->release($host, '203.0.113.7'))->toBeFalse(); +}); diff --git a/tests/Feature/Security/ScanForIntrusionsTest.php b/tests/Feature/Security/ScanForIntrusionsTest.php index 81dcdcc..f106df5 100644 --- a/tests/Feature/Security/ScanForIntrusionsTest.php +++ b/tests/Feature/Security/ScanForIntrusionsTest.php @@ -393,3 +393,76 @@ it('ueberspringt einen Gast, der nicht antwortet, ohne den Versatz zu verlieren' expect($instance->fresh()->security_log_offset)->toBe(4711); }); + +it('nimmt eine aufgehobene Sperre nachtraeglich aus der Firewall', function () { + // Die Luecke, um die es geht: war der Host im Moment des Aufhebens nicht + // erreichbar, steht der Datensatz auf aufgehoben und die Regel noch drin. + // Portal und Konsole zeigen dann "Aufgehoben", waehrend die Adresse weiter + // ausgesperrt bleibt — bis zum Ablauf der urspruenglichen Sperrzeit, ohne + // dass es dem Menschen davor jemand sagt. + $shell = new FakeRemoteShell; + app()->instance(RemoteShell::class, $shell); + + $host = Host::factory()->active()->create(['ssh_host_key' => 'SHA256:def']); + SecurityBlock::factory()->forHost($host)->create([ + 'ip' => '198.51.100.9', + 'blocked_at' => now()->subMinutes(40), + 'expires_at' => now()->addMinutes(20), // noch nicht abgelaufen + 'released_at' => now()->subMinute(), // aber schon aufgehoben + ]); + + app(ScanForIntrusions::class)->handle(); + + expect($shell->ran('delete element inet clupilot_filter clupilot_blocked { 198.51.100.9 }'))->toBeTrue(); +}); + +it('ruehrt eine abgelaufene Sperre nicht mehr an', function () { + // Nach der Ablaufzeit hat der Kernel den Eintrag selbst herausgenommen + // (`flags timeout`). Jede Minute erneut loeschen zu wollen, was seit Wochen + // weg ist, waere unnoetiger SSH-Verkehr auf demselben Arbeiter, der die + // bezahlte Bereitstellung faehrt. + $shell = new FakeRemoteShell; + app()->instance(RemoteShell::class, $shell); + + $host = Host::factory()->active()->create(['ssh_host_key' => 'SHA256:def']); + SecurityBlock::factory()->forHost($host)->create([ + 'ip' => '198.51.100.9', + 'blocked_at' => now()->subDays(3), + 'expires_at' => now()->subDays(2), + 'released_at' => now()->subDays(2), + ]); + + app(ScanForIntrusions::class)->handle(); + + expect($shell->ran('delete element'))->toBeFalse(); +}); + +it('laesst eine Adresse stehen, die eine ANDERE aktive Sperre noch traegt', function () { + // Dieselbe Adresse kann an zwei Subjekten desselben Hosts haengen (Host und + // Instanz, oder zwei Instanzen). Wer eine davon aufhebt, darf die andere + // nicht gleich mit aufheben — sonst oeffnet das Aufheben einer harmlosen + // Sperre die Tuer, die eine ganz andere zugehalten hat. + $shell = new FakeRemoteShell; + app()->instance(RemoteShell::class, $shell); + + $host = Host::factory()->active()->create(['ssh_host_key' => 'SHA256:def']); + $instanz = Instance::factory()->create(['host_id' => $host->id]); + + SecurityBlock::factory()->forHost($host)->create([ + 'ip' => '198.51.100.9', + 'blocked_at' => now()->subMinutes(40), + 'expires_at' => now()->addMinutes(20), + 'released_at' => now()->subMinute(), + ]); + + SecurityBlock::factory()->for($instanz)->create([ + 'ip' => '198.51.100.9', // dieselbe Adresse, noch aktiv + 'blocked_at' => now()->subMinutes(10), + 'expires_at' => now()->addMinutes(50), + ]); + + app(ScanForIntrusions::class)->handle(); + + expect($shell->ran('delete element'))->toBeFalse() + ->and($shell->ran('add element inet clupilot_filter clupilot_blocked { 198.51.100.9'))->toBeTrue(); +}); diff --git a/tests/Feature/Security/SecurityPageTest.php b/tests/Feature/Security/SecurityPageTest.php index 45a84fb..dbe4343 100644 --- a/tests/Feature/Security/SecurityPageTest.php +++ b/tests/Feature/Security/SecurityPageTest.php @@ -46,6 +46,52 @@ it('laesst einen Inhaber eine fremde Sperre nicht aufheben', function () { expect($fremd->fresh()->released_at)->toBeNull(); }); +// Der Weg, den ein Mensch nimmt: das Modal mutiert nichts, es schickt nur +// 'block-release-confirmed' (R23). Die bestehenden Pruefungen rufen +// onReleaseConfirmed() direkt auf — damit steht das Ereignis selbst nie auf +// dem Pruefstand, und die Zeile danach auch nicht. Genau das war der Verdacht: +// dass die Zeile nach dem Aufheben weiter auf "Aktiv" steht, bis jemand neu +// laedt. Diese Pruefung nimmt den ganzen Weg. +it('raeumt die Zeile nach dem Aufheben aus der aktiven Liste in den Verlauf', function () { + $customer = Customer::factory()->create(); + $user = $customer->ensureUser(); + $instance = Instance::factory()->for($customer)->create(); + $block = SecurityBlock::factory()->for($instance)->create(['ip' => '203.0.113.7']); + + $seite = Livewire::actingAs($user)->test(Security::class) + ->assertSee('active-'.$block->uuid, escape: false) + ->assertDontSee('history-'.$block->uuid, escape: false); + + $seite->dispatch('block-release-confirmed', uuid: $block->uuid) + ->assertDontSee('active-'.$block->uuid, escape: false) + ->assertSee('history-'.$block->uuid, escape: false) + ->assertSee(__('security.status_released')); +}); + +it('sagt es, wenn die Firewall beim Aufheben nicht mitkam', function () { + // HostFirewall schluckt jeden Fehlschlag und meldet ihn nur ins Log. Ohne + // diesen Unterschied faende der Kunde „Sperre aufgehoben." vor, versuchte + // sich gleich wieder anzumelden — und wuerde wortlos erneut abgewiesen, + // ohne dass ihm irgendjemand sagen kann, warum. + $shell = new \App\Services\Ssh\FakeRemoteShell; + $shell->failConnect = true; + app()->instance(\App\Services\Ssh\RemoteShell::class, $shell); + + $customer = Customer::factory()->create(); + $user = $customer->ensureUser(); + $host = \App\Models\Host::factory()->active()->create(['ssh_host_key' => 'SHA256:abc']); + $instance = Instance::factory()->for($customer)->create(['host_id' => $host->id]); + $block = SecurityBlock::factory()->for($instance)->create(); + + Livewire::actingAs($user)->test(Security::class) + ->call('onReleaseConfirmed', $block->uuid) + ->assertDispatched('notify', message: __('security.blocks_released_pending')); + + // Der Datensatz steht trotzdem — er IST die Sperre, die Firewall-Menge nur + // ihr Abdruck. ScanForIntrusions traegt das Loeschen nach. + expect($block->fresh()->released_at)->not->toBeNull(); +}); + it('hebt eine eigene Sperre auf und traegt ein, wer es war', function () { $customer = Customer::factory()->create(); $user = $customer->ensureUser();