diff --git a/app/Livewire/Admin/Vpn.php b/app/Livewire/Admin/Vpn.php index 4b85c19..862d663 100644 --- a/app/Livewire/Admin/Vpn.php +++ b/app/Livewire/Admin/Vpn.php @@ -256,7 +256,7 @@ class Vpn extends Component // Old key off the hub first, then the new one on: the other order would // briefly leave two peers claiming the same tunnel address. - ApplyVpnPeer::dispatch($oldKey, null, false); + ApplyVpnPeer::dispatch($oldKey, null, false, force: true); ApplyVpnPeer::dispatch($peer->public_key, $peer->allowed_ip, true); $this->dismissConfig(); diff --git a/app/Provisioning/Jobs/ApplyVpnPeer.php b/app/Provisioning/Jobs/ApplyVpnPeer.php index 570a13d..36629c7 100644 --- a/app/Provisioning/Jobs/ApplyVpnPeer.php +++ b/app/Provisioning/Jobs/ApplyVpnPeer.php @@ -28,6 +28,17 @@ class ApplyVpnPeer implements ShouldQueue public string $publicKey, public ?string $allowedIp, public bool $enabled, + /** + * Revoke this key no matter what the database says. + * + * Needed when a key is replaced: for a moment the old key is live on the + * hub with no row behind it, and a sync running in that window adopts it + * as an unknown peer — enabled. Without this flag the removal would then + * read that row, decide the key should stay, and leave two peers + * claiming the same tunnel address. Forcing can only ever remove, so it + * cannot resurrect anything. + */ + public bool $force = false, ) { $this->onQueue('provisioning'); } @@ -56,7 +67,7 @@ class ApplyVpnPeer implements ShouldQueue // already purged. A stale enable payload must never be able to re-add a // key that no longer has a row to revoke it with, so the captured state // can only ever remove, never enable. - $enabled = $peer !== null && ! $peer->trashed() && $peer->enabled; + $enabled = ! $this->force && $peer !== null && ! $peer->trashed() && $peer->enabled; $ip = $peer?->allowed_ip ?? $this->allowedIp; if ($enabled && $ip !== null) { @@ -65,6 +76,14 @@ class ApplyVpnPeer implements ShouldQueue $hub->removePeer($this->publicKey); } + // A row still carrying a forcibly revoked key can only be an adoption + // artifact — the access itself has moved on to a new key. + if ($this->force && $peer !== null) { + $peer->forceDelete(); + + return; + } + if ($peer === null) { return; } diff --git a/app/Provisioning/Jobs/SyncVpnPeers.php b/app/Provisioning/Jobs/SyncVpnPeers.php index 8ad5b56..798fa99 100644 --- a/app/Provisioning/Jobs/SyncVpnPeers.php +++ b/app/Provisioning/Jobs/SyncVpnPeers.php @@ -54,7 +54,8 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue // Dispatched outside the lock: with the sync queue driver this executes // inline, and ApplyVpnPeer takes the very same lock. foreach ($retryRemoval as $publicKey) { - ApplyVpnPeer::dispatch($publicKey, null, false); + // force: a key with no row of its own can only be revoked, never kept. + ApplyVpnPeer::dispatch($publicKey, null, false, force: true); } } @@ -93,11 +94,22 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue ]; if ($peer === null) { + // The address already belongs to a live access: this is a key + // that was just replaced and whose removal has not run yet. + // Adopting it would violate the unique index and abort the whole + // reconciliation — one stale key would stop every peer's state + // from being updated. + if (VpnPeer::query()->where('allowed_ip', $ip = strtok($snapshot->allowedIps, '/') ?: $snapshot->allowedIps)->exists()) { + $retryRemoval[] = $publicKey; + + continue; + } + VpnPeer::create($observed + [ 'public_key' => $publicKey, // Fall back to the dump's allowed-ips for peers that were // never created through the console. - 'allowed_ip' => strtok($snapshot->allowedIps, '/') ?: $snapshot->allowedIps, + 'allowed_ip' => $ip, 'host_id' => $host?->id, // Never "staff": a staff access has an owner, and nobody is // behind a peer we merely found on the interface. diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 7a410f1..8894659 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -821,3 +821,29 @@ it('does not re-issue a host peer from the VPN page', function () { expect($peer->fresh()->public_key)->toBe($oldKey); Queue::assertNotPushed(ApplyVpnPeer::class); }); + +it('revokes the replaced key even after a sync adopted it', function () { + $hub = vpnHub(); + $owner = operator('Owner'); + $peer = VpnPeer::factory()->ownedBy($owner)->create(); + $oldKey = $peer->public_key; + $hub->addPeer($oldKey, $peer->allowed_ip); + + // Queue faked on purpose: the window only exists while the removal is still + // pending, which a synchronous queue would close instantly. + Queue::fake(); + Livewire::actingAs($owner)->test(Vpn::class)->call('reissue', $peer->uuid); + + // A sync lands before the removal runs. It must neither adopt the old key + // (its address belongs to the live access) nor blow up on the unique index, + // which would stop every other peer's state from being updated too. + (new SyncVpnPeers)->handle($hub); + + expect(VpnPeer::query()->where('public_key', $oldKey)->exists())->toBeFalse() + ->and(VpnPeer::query()->count())->toBe(1); + + (new ApplyVpnPeer($oldKey, null, false, force: true))->handle($hub); + + expect($hub->peerIps())->not->toHaveKey($oldKey) + ->and(VpnPeer::withTrashed()->where('public_key', $oldKey)->exists())->toBeFalse(); +});