diff --git a/app/Livewire/Admin/Vpn.php b/app/Livewire/Admin/Vpn.php index 862d663..802e5e3 100644 --- a/app/Livewire/Admin/Vpn.php +++ b/app/Livewire/Admin/Vpn.php @@ -251,7 +251,12 @@ class Vpn extends Component $peer->forceFill([ 'public_key' => $keypair->publicKey, 'present' => false, - 'config_secret' => $peer->hasStoredConfig() ? ConfigVault::encrypt($config) : null, + // Only re-stored when the vault can actually be used: without the key + // this would throw, and the console tells people to re-issue + // precisely when a stored config has become unreadable. + 'config_secret' => $peer->hasStoredConfig() && ConfigVault::available() + ? ConfigVault::encrypt($config) + : null, ])->save(); // Old key off the hub first, then the new one on: the other order would diff --git a/app/Provisioning/Jobs/ApplyVpnPeer.php b/app/Provisioning/Jobs/ApplyVpnPeer.php index 36629c7..be00840 100644 --- a/app/Provisioning/Jobs/ApplyVpnPeer.php +++ b/app/Provisioning/Jobs/ApplyVpnPeer.php @@ -5,6 +5,7 @@ namespace App\Provisioning\Jobs; use App\Models\VpnPeer; use App\Services\Wireguard\WireguardHub; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Log; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; @@ -70,14 +71,25 @@ class ApplyVpnPeer implements ShouldQueue $enabled = ! $this->force && $peer !== null && ! $peer->trashed() && $peer->enabled; $ip = $peer?->allowed_ip ?? $this->allowedIp; + // Checked before touching the hub: a forced revocation whose key has + // since been re-issued to someone else must not disconnect them. + if ($this->force && $peer !== null + && ! ($peer->kind === VpnPeer::KIND_SYSTEM && $peer->created_by === null && $peer->user_id === null)) { + Log::info('forced revocation skipped: the key belongs to a live access again', [ + 'peer' => $peer->uuid, + ]); + + return; + } + if ($enabled && $ip !== null) { $hub->addPeer($this->publicKey, $ip); } else { $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. + // Adoption artifact cleaned up; the guard above already sent anything + // else home. if ($this->force && $peer !== null) { $peer->forceDelete(); diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 8894659..42b5a31 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -847,3 +847,35 @@ it('revokes the replaced key even after a sync adopted it', function () { expect($hub->peerIps())->not->toHaveKey($oldKey) ->and(VpnPeer::withTrashed()->where('public_key', $oldKey)->exists())->toBeFalse(); }); + +it('does not let a late revocation delete an access that reused the key', function () { + $hub = vpnHub(); + $owner = operator('Owner'); + $key = Keypair::generate()->publicKey; + + // Someone created a new access with a key that used to belong elsewhere. + $fresh = VpnPeer::factory()->ownedBy($owner)->create(['public_key' => $key]); + $hub->addPeer($key, $fresh->allowed_ip); + + // The old revocation job finally runs. + (new ApplyVpnPeer($key, null, false, force: true))->handle($hub); + + expect(VpnPeer::query()->whereKey($fresh->id)->exists())->toBeTrue() + ->and($hub->peerIps())->toHaveKey($key); // and they stay connected +}); + +it('re-issues even when the config vault is unavailable', function () { + vpnHub(); + Queue::fake(); + $owner = operator('Owner'); + $peer = VpnPeer::factory()->ownedBy($owner)->create([ + 'config_secret' => App\Services\Wireguard\ConfigVault::encrypt("[Interface]\n"), + ]); + + // The very situation the console tells people to fix by re-issuing. + config()->set('admin_access.vpn_config_key', ''); + + Livewire::actingAs($owner)->test(Vpn::class)->call('reissue', $peer->uuid); + + expect($peer->fresh()->config_secret)->toBeNull(); // shown once instead +});