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();