boban
/
CluPilotCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(vpn): a replaced key must not stall the reconciliation 

Codex found that a sync landing between the key swap and its removal would see
the old key as unknown. Writing the test showed something worse than the
duplicate he predicted: the adoption cannot even happen — the address still
belongs to the live access — so the insert violated the unique index and took
the ENTIRE reconciliation down with it. One stale key would have stopped every
peer's state from updating.

The sync now recognises that case and queues the removal instead of adopting,
and a key with no row of its own is revoked with force: it can only ever be
removed, never kept, so the removal cannot be talked out of it by a row that
should not exist.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
feat/portal-design
nexxo 2026-07-26 01:23:15 +02:00
parent 556a560506
commit 4f9ca27732
4 changed files with 61 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/Livewire/Admin/Vpn.php
View File

@ -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();

21
app/Provisioning/Jobs/ApplyVpnPeer.php
View File

@ -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;
}

16
app/Provisioning/Jobs/SyncVpnPeers.php
View File

@ -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.

26
tests/Feature/Admin/VpnTest.php
View File

@ -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();
});