From 28e681b9dfe896371886b827c39af7bcfa7efc6f Mon Sep 17 00:00:00 2001 From: nexxo Date: Sat, 25 Jul 2026 21:55:57 +0200 Subject: [PATCH] fix(vpn): lock every hub mutation; resolve duplicate keys inside the lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ConfigureWireguard and RemoveWireguardPeer mutate the hub too, and were not taking the lock, so a sync could still read the interface around them and write stale state. Both now hold wireguard:hub, which is what makes the read/mutation guarantee actually hold. - The duplicate-key check ran before the allocation lock, so two concurrent creations with the same key both passed it and the loser hit the unique index as an unhandled 500. The check moved inside the lock, with the index kept as a caught backstop for any writer that does not take it — reproduced in a test by inserting a colliding row from within the allocation call. Co-Authored-By: Claude Opus 4.8 --- app/Livewire/Admin/Vpn.php | 52 ++++++++++++------- app/Provisioning/Jobs/RemoveWireguardPeer.php | 6 ++- .../Steps/Host/ConfigureWireguard.php | 4 +- tests/Feature/Admin/VpnTest.php | 47 +++++++++++++++++ 4 files changed, 89 insertions(+), 20 deletions(-) diff --git a/app/Livewire/Admin/Vpn.php b/app/Livewire/Admin/Vpn.php index 749bbc2..0f0d4c7 100644 --- a/app/Livewire/Admin/Vpn.php +++ b/app/Livewire/Admin/Vpn.php @@ -7,6 +7,7 @@ use App\Provisioning\Jobs\ApplyVpnPeer; use App\Provisioning\Jobs\SyncVpnPeers; use App\Services\Wireguard\Keypair; use App\Services\Wireguard\WireguardHub; +use Illuminate\Database\QueryException; use Illuminate\Support\Facades\Cache; use Livewire\Attributes\Layout; use Livewire\Attributes\On; @@ -65,30 +66,45 @@ class Vpn extends Component $keypair = $ownKey ? null : Keypair::generate(); $publicKey = $ownKey ? trim($this->publicKey) : $keypair->publicKey; - // 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; - } - $hub = app(WireguardHub::class); // Same lock the host pipeline holds while it reserves an address // (ConfigureWireguard). Addresses come from one subnet but live in two // tables, so neither unique index can catch the other's insert — the // shared lock is what keeps a host and an access off the same tunnel IP. - $peer = Cache::lock('wireguard:allocate', 30)->block(10, fn () => VpnPeer::create([ - 'name' => trim($this->name), - 'public_key' => $publicKey, - 'allowed_ip' => $hub->allocateIp(), - 'enabled' => true, - 'present' => false, - 'created_by' => auth()->id(), - ])); + try { + $peer = Cache::lock('wireguard:allocate', 30)->block(10, function () use ($hub, $publicKey) { + // Inside the lock: checking before it would let two concurrent + // requests both pass and the loser hit the unique index as a + // 500. withTrashed, because a revoked peer keeps its key until + // the hub confirms removal. + $existing = VpnPeer::withTrashed()->where('public_key', $publicKey)->first(); + if ($existing !== null) { + return $existing->trashed() ? 'pending_removal' : 'duplicate_key'; + } + + return VpnPeer::create([ + 'name' => trim($this->name), + 'public_key' => $publicKey, + 'allowed_ip' => $hub->allocateIp(), + 'enabled' => true, + 'present' => false, + 'created_by' => auth()->id(), + ]); + }); + } catch (QueryException) { + // Backstop: the unique index caught a writer that did not take this + // lock. Report it like any other duplicate instead of a 500. + $this->addError('publicKey', __('vpn.duplicate_key')); + + return; + } + + if (is_string($peer)) { + $this->addError('publicKey', __('vpn.'.$peer)); + + return; + } ApplyVpnPeer::dispatch($peer->public_key, $peer->allowed_ip, true); diff --git a/app/Provisioning/Jobs/RemoveWireguardPeer.php b/app/Provisioning/Jobs/RemoveWireguardPeer.php index f2abf49..3095f79 100644 --- a/app/Provisioning/Jobs/RemoveWireguardPeer.php +++ b/app/Provisioning/Jobs/RemoveWireguardPeer.php @@ -3,6 +3,7 @@ namespace App\Provisioning\Jobs; use App\Services\Wireguard\WireguardHub; +use Illuminate\Support\Facades\Cache; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; @@ -30,6 +31,9 @@ class RemoveWireguardPeer implements ShouldQueue public function handle(WireguardHub $hub): void { - $hub->removePeer($this->publicKey); + // Every hub mutation takes this lock so SyncVpnPeers cannot read the + // interface around it and then write what it saw — a peer removed here + // would otherwise be reported back as a live access. + Cache::lock('wireguard:hub', 30)->block(10, fn () => $hub->removePeer($this->publicKey)); } } diff --git a/app/Provisioning/Steps/Host/ConfigureWireguard.php b/app/Provisioning/Steps/Host/ConfigureWireguard.php index 304bdb9..9058ef2 100644 --- a/app/Provisioning/Steps/Host/ConfigureWireguard.php +++ b/app/Provisioning/Steps/Host/ConfigureWireguard.php @@ -61,7 +61,9 @@ class ConfigureWireguard extends HostStep return StepResult::retry(20, 'bringing up wg0 failed'); } - $this->hub->addPeer($publicKey, $wgIp); + // See RemoveWireguardPeer: hub mutations are serialized against the + // reconciliation that feeds the VPN console. + Cache::lock('wireguard:hub', 30)->block(10, fn () => $this->hub->addPeer($publicKey, $wgIp)); // Store the pubkey now (so removal can always clean up the hub peer), but // record the wg_peer resource only AFTER the handshake verifies — the diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 8630522..9b57361 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -337,3 +337,50 @@ it('serializes hub reads against hub mutations', function () { expect(microtime(true) - $started)->toBeGreaterThan(0.8) ->and($peer->fresh()->present)->toBeTrue(); }); + +it('reports a lost creation race as a validation error, not a 500', function () { + // A hub whose allocation inserts a colliding row mid-flight: exactly what a + // concurrent request that skipped the lock would do between our check and + // our insert. + $key = Keypair::generate()->publicKey; + $hub = new class($key) extends FakeWireguardHub + { + public function __construct(private string $collidingKey) {} + + public function allocateIp(): string + { + VpnPeer::create([ + 'name' => 'gleichzeitig', + 'public_key' => $this->collidingKey, + 'allowed_ip' => '10.66.0.90', + 'enabled' => true, + ]); + + return '10.66.0.91'; + } + }; + app()->instance(WireguardHub::class, $hub); + + Livewire::actingAs(operator('Owner'))->test(Vpn::class) + ->set('name', 'verloren') + ->set('publicKey', $key) + ->call('create') + ->assertHasErrors('publicKey'); + + // Only the winner's row exists. + expect(VpnPeer::query()->count())->toBe(1) + ->and(VpnPeer::query()->first()->name)->toBe('gleichzeitig'); +}); + +it('keeps host peer removal behind the same hub lock', function () { + $hub = vpnHub(); + $hub->addPeer('HOSTKEY', '10.66.0.30'); + + expect(Illuminate\Support\Facades\Cache::lock('wireguard:hub', 1)->get())->toBeTrue(); + + $started = microtime(true); + (new App\Provisioning\Jobs\RemoveWireguardPeer('HOSTKEY'))->handle($hub); + + expect(microtime(true) - $started)->toBeGreaterThan(0.8) + ->and($hub->peerIps())->toBe([]); +});