From 08670ecbee77ef7b2de29e0d86e3f22d045b7e56 Mon Sep 17 00:00:00 2001 From: nexxo Date: Sat, 25 Jul 2026 21:41:03 +0200 Subject: [PATCH] fix(vpn): stop revoked accesses coming back and stale jobs undoing intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two lifecycle holes Codex found in the new VPN management: 1. Delete removed the row before the hub knew. If the removal job was delayed or failed, SyncVpnPeers saw a peer it did not recognise and adopted it back as a live access — silently restoring what an operator had just revoked, and with no row left to revoke it again. Deletion is now a soft-delete tombstone: the row survives until the hub confirms the peer is gone, the sync retries the removal instead of adopting, and the tombstone is purged only once the peer is really absent. It also keeps the key and the tunnel address reserved meanwhile, so neither is handed out twice. 2. ApplyVpnPeer applied the state captured at dispatch. A retried block job could therefore undo a later unblock, and nothing would repair it — the sync only observes state, it never re-applies intent. The job now resolves the current desired state from the row and treats its payload as a fallback for the case where the row is already gone. Co-Authored-By: Claude Opus 4.8 --- app/Livewire/Admin/ConfirmDeleteVpnPeer.php | 9 ++- app/Livewire/Admin/Vpn.php | 8 ++- app/Models/VpnPeer.php | 2 + app/Provisioning/Jobs/ApplyVpnPeer.php | 33 ++++++++-- app/Provisioning/Jobs/SyncVpnPeers.php | 18 ++++- app/Services/Wireguard/LocalWireguardHub.php | 3 +- ...26_07_25_200000_create_vpn_peers_table.php | 5 ++ lang/de/vpn.php | 1 + lang/en/vpn.php | 1 + tests/Feature/Admin/VpnTest.php | 66 +++++++++++++++++++ 10 files changed, 132 insertions(+), 14 deletions(-) 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'); +});