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([]); +});