From 53d305340fb19dae6c35f14bf3b35393c3f63145 Mon Sep 17 00:00:00 2001
From: nexxo <nexxo@clupilot.local>
Date: Sun, 26 Jul 2026 01:24:55 +0200
Subject: [PATCH] fix(vpn): a late revocation must not disconnect whoever holds
 the key now
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Once a key is free, someone can legitimately create a new access with it. The
forced revocation looked the key up and would have deleted that person's access
and pulled their key off the hub. It now acts only on an adoption artifact — a
system peer with no owner and no creator — and leaves anything else alone.

Re-issuing also threw when VPN_CONFIG_KEY was missing, which is exactly the
situation the console tells people to fix by re-issuing. It now hands the new
config over once instead of storing it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 app/Livewire/Admin/Vpn.php             |  7 +++++-
 app/Provisioning/Jobs/ApplyVpnPeer.php | 16 +++++++++++--
 tests/Feature/Admin/VpnTest.php        | 32 ++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 3 deletions(-)

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