diff --git a/app/Livewire/Admin/ConfirmDeleteVpnPeer.php b/app/Livewire/Admin/ConfirmDeleteVpnPeer.php index b9aa9a0..e8ca79f 100644 --- a/app/Livewire/Admin/ConfirmDeleteVpnPeer.php +++ b/app/Livewire/Admin/ConfirmDeleteVpnPeer.php @@ -34,10 +34,13 @@ class ConfirmDeleteVpnPeer extends ModalComponent $peer = VpnPeer::query()->where('uuid', $this->uuid)->first(); if ($peer !== null) { - // Remove from the hub first: if the row went first and the job then - // failed, the peer would keep its tunnel with nothing left to show it. - ApplyVpnPeer::dispatch($peer->public_key, null, false); + // Soft-delete, so the row survives as a tombstone until the hub has + // actually dropped the peer. A hard delete here would let the next + // sync adopt the still-present peer back as a live access — + // silently restoring what was just revoked. ApplyVpnPeer purges the + // tombstone once removal succeeded; SyncVpnPeers retries if not. $peer->delete(); + ApplyVpnPeer::dispatch($peer->public_key, null, false); } $this->dispatch('vpn-peer-deleted'); diff --git a/app/Livewire/Admin/Vpn.php b/app/Livewire/Admin/Vpn.php index 7b1be51..de245d2 100644 --- a/app/Livewire/Admin/Vpn.php +++ b/app/Livewire/Admin/Vpn.php @@ -55,8 +55,12 @@ class Vpn extends Component $keypair = $ownKey ? null : Keypair::generate(); $publicKey = $ownKey ? trim($this->publicKey) : $keypair->publicKey; - if (VpnPeer::query()->where('public_key', $publicKey)->exists()) { - $this->addError('publicKey', __('vpn.duplicate_key')); + // withTrashed: a revoked peer still holds the unique key until the hub + // confirms removal, so re-adding it now would hit the index instead of + // telling the operator what is actually going on. + $existing = VpnPeer::withTrashed()->where('public_key', $publicKey)->first(); + if ($existing !== null) { + $this->addError('publicKey', $existing->trashed() ? __('vpn.pending_removal') : __('vpn.duplicate_key')); return; } diff --git a/app/Models/VpnPeer.php b/app/Models/VpnPeer.php index 9dd59e8..6d78403 100644 --- a/app/Models/VpnPeer.php +++ b/app/Models/VpnPeer.php @@ -6,11 +6,13 @@ use Illuminate\Database\Eloquent\Concerns\HasUuids; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\SoftDeletes; class VpnPeer extends Model { use HasFactory; use HasUuids; + use SoftDeletes; /** A tunnel is considered live if the last handshake is recent. WireGuard * re-handshakes about every two minutes, so three is the first value that diff --git a/app/Provisioning/Jobs/ApplyVpnPeer.php b/app/Provisioning/Jobs/ApplyVpnPeer.php index 6e6a53e..b35f4cf 100644 --- a/app/Provisioning/Jobs/ApplyVpnPeer.php +++ b/app/Provisioning/Jobs/ApplyVpnPeer.php @@ -38,17 +38,38 @@ class ApplyVpnPeer implements ShouldQueue public function handle(WireguardHub $hub): void { - if ($this->enabled && $this->allowedIp !== null) { - $hub->addPeer($this->publicKey, $this->allowedIp); + // Read the CURRENT desired state rather than the one captured at + // dispatch time. A retried block job would otherwise undo an unblock + // that happened in the meantime, and nothing would ever put it back: + // SyncVpnPeers only observes, it never re-applies intent. + $peer = VpnPeer::withTrashed()->where('public_key', $this->publicKey)->first(); + + // No row, or a revoked one, means the peer must be off the hub. The + // constructor payload only matters when the row is already gone. + $enabled = $peer === null + ? $this->enabled && $this->allowedIp !== null + : (! $peer->trashed() && $peer->enabled); + $ip = $peer?->allowed_ip ?? $this->allowedIp; + + if ($enabled && $ip !== null) { + $hub->addPeer($this->publicKey, $ip); } else { $hub->removePeer($this->publicKey); } + if ($peer === null) { + return; + } + + // The tombstone has served its purpose once the hub has dropped the peer. + if ($peer->trashed()) { + $peer->forceDelete(); + + return; + } + // Reflect the change immediately instead of waiting for the next sync, // so the console does not sit on "wird angewendet" for a whole minute. - VpnPeer::query()->where('public_key', $this->publicKey)->update([ - 'present' => $this->enabled, - 'observed_at' => now(), - ]); + $peer->update(['present' => $enabled, 'observed_at' => now()]); } } diff --git a/app/Provisioning/Jobs/SyncVpnPeers.php b/app/Provisioning/Jobs/SyncVpnPeers.php index d47e01e..79ffb2c 100644 --- a/app/Provisioning/Jobs/SyncVpnPeers.php +++ b/app/Provisioning/Jobs/SyncVpnPeers.php @@ -50,7 +50,16 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue ->pluck('id', 'wg_pubkey'); foreach ($snapshots as $publicKey => $snapshot) { - $peer = VpnPeer::query()->where('public_key', $publicKey)->first(); + $peer = VpnPeer::withTrashed()->where('public_key', $publicKey)->first(); + + // Revoked, yet still on the hub: the removal never landed. Retry it + // rather than adopting the peer back into the console — otherwise a + // failed removal quietly becomes a live access again. + if ($peer?->trashed()) { + ApplyVpnPeer::dispatch($publicKey, null, false); + + continue; + } $hostId = $hostsByKey[$publicKey] ?? null; $observed = [ @@ -83,8 +92,13 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue $peer->update($observed + ['host_id' => $peer->host_id ?? $hostId]); } + $seen = array_keys($snapshots); + VpnPeer::query() - ->whereNotIn('public_key', array_keys($snapshots)) + ->whereNotIn('public_key', $seen) ->update(['present' => false, 'observed_at' => $now]); + + // A tombstone whose peer is gone from the hub has done its job. + VpnPeer::onlyTrashed()->whereNotIn('public_key', $seen)->forceDelete(); } } diff --git a/app/Services/Wireguard/LocalWireguardHub.php b/app/Services/Wireguard/LocalWireguardHub.php index 3b70ea5..27d04a6 100644 --- a/app/Services/Wireguard/LocalWireguardHub.php +++ b/app/Services/Wireguard/LocalWireguardHub.php @@ -21,7 +21,8 @@ class LocalWireguardHub implements WireguardHub // access and a host peer would otherwise be given the same tunnel IP. $used = array_flip(array_merge( Host::query()->whereNotNull('wg_ip')->pluck('wg_ip')->all(), - VpnPeer::query()->pluck('allowed_ip')->all(), + // withTrashed: a revoked peer keeps its address until the hub drops it. + VpnPeer::withTrashed()->pluck('allowed_ip')->all(), )); $hubIp = (string) config('provisioning.wireguard.hub_ip'); diff --git a/database/migrations/2026_07_25_200000_create_vpn_peers_table.php b/database/migrations/2026_07_25_200000_create_vpn_peers_table.php index 8037d5c..0098906 100644 --- a/database/migrations/2026_07_25_200000_create_vpn_peers_table.php +++ b/database/migrations/2026_07_25_200000_create_vpn_peers_table.php @@ -34,6 +34,11 @@ return new class extends Migration $table->timestamp('observed_at')->nullable(); // when the hub last reported it $table->foreignId('created_by')->nullable()->constrained('users')->nullOnDelete(); $table->timestamps(); + // Tombstone: a revoked access keeps its row until the hub has + // actually dropped the peer. Deleting outright would let the next + // sync see the still-present peer and adopt it back as a live + // access — silently restoring what an operator just revoked. + $table->softDeletes(); }); } diff --git a/lang/de/vpn.php b/lang/de/vpn.php index ce8e800..c450d78 100644 --- a/lang/de/vpn.php +++ b/lang/de/vpn.php @@ -32,6 +32,7 @@ return [ 'own_key_placeholder' => 'optional', 'invalid_key' => 'Das ist kein gültiger WireGuard-Schlüssel (32 Byte, Base64).', 'duplicate_key' => 'Für diesen Schlüssel gibt es bereits einen Zugang.', + 'pending_removal' => 'Dieser Zugang wird gerade entfernt. Bitte kurz warten.', 'created' => 'Zugang angelegt.', 'blocked' => 'Zugang gesperrt.', 'unblocked' => 'Zugang wieder freigegeben.', diff --git a/lang/en/vpn.php b/lang/en/vpn.php index 0e69260..b365a90 100644 --- a/lang/en/vpn.php +++ b/lang/en/vpn.php @@ -32,6 +32,7 @@ return [ 'own_key_placeholder' => 'optional', 'invalid_key' => 'That is not a valid WireGuard key (32 bytes, base64).', 'duplicate_key' => 'An access already exists for this key.', + 'pending_removal' => 'This access is currently being removed. Please wait a moment.', 'created' => 'Access created.', 'blocked' => 'Access blocked.', 'unblocked' => 'Access re-enabled.', diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 3bece32..3f97348 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -207,3 +207,69 @@ it('does not hand the same tunnel address to a host and a VPN access', function // The real allocator, not the fake — this is the collision the fake cannot show. expect((new LocalWireguardHub)->allocateIp())->toBe('10.66.0.4'); }); + +it('does not let a failed removal resurrect a revoked access', function () { + $hub = vpnHub(); + Queue::fake(); + $peer = VpnPeer::factory()->create(); + $hub->addPeer($peer->public_key, $peer->allowed_ip); + + Livewire::actingAs(operator('Owner')) + ->test(ConfirmDeleteVpnPeer::class, ['uuid' => $peer->uuid]) + ->call('delete'); + + // Removal is queued but has not run — the peer is still on the hub. + expect(VpnPeer::query()->count())->toBe(0) + ->and(VpnPeer::withTrashed()->count())->toBe(1); + + (new SyncVpnPeers)->handle($hub); + + // The sync must NOT adopt it back as a live access… + expect(VpnPeer::query()->count())->toBe(0); + // …it must retry the removal instead. + Queue::assertPushed(ApplyVpnPeer::class, fn ($job) => $job->publicKey === $peer->public_key && ! $job->enabled); +}); + +it('clears the tombstone once the peer is really gone', function () { + $hub = vpnHub(); + $peer = VpnPeer::factory()->create(); + $hub->addPeer($peer->public_key, $peer->allowed_ip); + $peer->delete(); + + (new ApplyVpnPeer($peer->public_key, null, false))->handle($hub); + + expect($hub->peerIps())->toBe([]) + ->and(VpnPeer::withTrashed()->count())->toBe(0); +}); + +it('ignores a stale job that would undo a newer decision', function () { + $hub = vpnHub(); + $peer = VpnPeer::factory()->create(); + $hub->addPeer($peer->public_key, $peer->allowed_ip); + + // Operator blocks, then immediately unblocks: two jobs are now in flight. + $blockJob = new ApplyVpnPeer($peer->public_key, $peer->allowed_ip, false); + $peer->update(['enabled' => false]); + $peer->update(['enabled' => true]); // newer intent wins + + // The older block job runs late (a retry, say). It must not cut access. + $blockJob->handle($hub); + + expect($hub->peerIps())->toHaveKey($peer->public_key) + ->and($peer->fresh()->present)->toBeTrue(); +}); + +it('keeps a revoked address and key reserved until the hub confirms', function () { + vpnHub(); + $peer = VpnPeer::factory()->create(['allowed_ip' => '10.66.0.2']); + $peer->delete(); + + // The address is still configured on the hub — handing it out again would + // give two machines the same tunnel IP. + expect((new LocalWireguardHub)->allocateIp())->not->toBe('10.66.0.2'); + + Livewire::actingAs(operator('Owner'))->test(Vpn::class) + ->set('name', 'wieder da')->set('publicKey', $peer->public_key) + ->call('create') + ->assertHasErrors('publicKey'); +});