fix(vpn): never let an access hijack a host key; name adopted peers correctly
- A host's wg_pubkey may not be in vpn_peers yet, so the duplicate check passed and an operator could create an access with it. ApplyVpnPeer would then run wg set with a freshly allocated address, rewriting that host's allowed-ip and cutting CluPilot's management tunnel to a live machine. Creation now rejects keys already held by a host, checked under the allocation lock. - A sync landing between addPeer() and the step storing wg_pubkey adopted the peer as "unknown", and a later sync filled in host_id but left the name, so the page showed that host as unknown forever. The name is now filled in when the link becomes known — only for sync-adopted rows, so an access an operator named keeps its name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>feat/portal-design
parent
28e681b9df
commit
c54b718566
|
|
@ -2,6 +2,7 @@
|
||||||
|
|
||||||
namespace App\Livewire\Admin;
|
namespace App\Livewire\Admin;
|
||||||
|
|
||||||
|
use App\Models\Host;
|
||||||
use App\Models\VpnPeer;
|
use App\Models\VpnPeer;
|
||||||
use App\Provisioning\Jobs\ApplyVpnPeer;
|
use App\Provisioning\Jobs\ApplyVpnPeer;
|
||||||
use App\Provisioning\Jobs\SyncVpnPeers;
|
use App\Provisioning\Jobs\SyncVpnPeers;
|
||||||
|
|
@ -83,6 +84,14 @@ class Vpn extends Component
|
||||||
return $existing->trashed() ? 'pending_removal' : 'duplicate_key';
|
return $existing->trashed() ? 'pending_removal' : 'duplicate_key';
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A host's key may not have been adopted into vpn_peers yet.
|
||||||
|
// Re-using it would make `wg set` rewrite that host's allowed-ip
|
||||||
|
// to the address allocated here — cutting the management tunnel
|
||||||
|
// to a live machine.
|
||||||
|
if (Host::query()->where('wg_pubkey', $publicKey)->exists()) {
|
||||||
|
return 'host_key';
|
||||||
|
}
|
||||||
|
|
||||||
return VpnPeer::create([
|
return VpnPeer::create([
|
||||||
'name' => trim($this->name),
|
'name' => trim($this->name),
|
||||||
'public_key' => $publicKey,
|
'public_key' => $publicKey,
|
||||||
|
|
|
||||||
|
|
@ -68,10 +68,12 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue
|
||||||
// under their host name instead of as an unknown key.
|
// under their host name instead of as an unknown key.
|
||||||
$hostsByKey = Host::query()
|
$hostsByKey = Host::query()
|
||||||
->whereNotNull('wg_pubkey')
|
->whereNotNull('wg_pubkey')
|
||||||
->pluck('id', 'wg_pubkey');
|
->get(['id', 'name', 'wg_pubkey'])
|
||||||
|
->keyBy('wg_pubkey');
|
||||||
|
|
||||||
foreach ($snapshots as $publicKey => $snapshot) {
|
foreach ($snapshots as $publicKey => $snapshot) {
|
||||||
$peer = VpnPeer::withTrashed()->where('public_key', $publicKey)->first();
|
$peer = VpnPeer::withTrashed()->where('public_key', $publicKey)->first();
|
||||||
|
$host = $hostsByKey[$publicKey] ?? null;
|
||||||
|
|
||||||
// Revoked, yet still on the hub: the removal never landed. Retry it
|
// Revoked, yet still on the hub: the removal never landed. Retry it
|
||||||
// rather than adopting the peer back into the console — otherwise a
|
// rather than adopting the peer back into the console — otherwise a
|
||||||
|
|
@ -81,8 +83,6 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue
|
||||||
|
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
$hostId = $hostsByKey[$publicKey] ?? null;
|
|
||||||
|
|
||||||
$observed = [
|
$observed = [
|
||||||
'present' => true,
|
'present' => true,
|
||||||
'endpoint' => $snapshot->endpoint,
|
'endpoint' => $snapshot->endpoint,
|
||||||
|
|
@ -98,19 +98,28 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue
|
||||||
// Fall back to the dump's allowed-ips for peers that were
|
// Fall back to the dump's allowed-ips for peers that were
|
||||||
// never created through the console.
|
// never created through the console.
|
||||||
'allowed_ip' => strtok($snapshot->allowedIps, '/') ?: $snapshot->allowedIps,
|
'allowed_ip' => strtok($snapshot->allowedIps, '/') ?: $snapshot->allowedIps,
|
||||||
'host_id' => $hostId,
|
'host_id' => $host?->id,
|
||||||
'name' => $hostId !== null
|
'name' => $host?->name ?? __('vpn.unknown_peer'),
|
||||||
? (Host::find($hostId)?->name ?? __('vpn.unknown_peer'))
|
|
||||||
: __('vpn.unknown_peer'),
|
|
||||||
'enabled' => true,
|
'enabled' => true,
|
||||||
]);
|
]);
|
||||||
|
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A sync that ran between addPeer() and the step storing wg_pubkey
|
||||||
|
// adopts the peer as "unknown". Once the link is known, name it
|
||||||
|
// after its host — but only if no operator ever named it
|
||||||
|
// (created_by is null exactly for sync-adopted rows).
|
||||||
|
if ($peer->host_id === null && $host !== null) {
|
||||||
|
$observed['host_id'] = $host->id;
|
||||||
|
if ($peer->created_by === null) {
|
||||||
|
$observed['name'] = $host->name;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Never overwrite `enabled`: that is the operator's intent, and a
|
// Never overwrite `enabled`: that is the operator's intent, and a
|
||||||
// peer being present only says the hub has not caught up yet.
|
// peer being present only says the hub has not caught up yet.
|
||||||
$peer->update($observed + ['host_id' => $peer->host_id ?? $hostId]);
|
$peer->update($observed);
|
||||||
}
|
}
|
||||||
|
|
||||||
$seen = array_keys($snapshots);
|
$seen = array_keys($snapshots);
|
||||||
|
|
|
||||||
|
|
@ -33,6 +33,7 @@ return [
|
||||||
'invalid_key' => 'Das ist kein gültiger WireGuard-Schlüssel (32 Byte, Base64).',
|
'invalid_key' => 'Das ist kein gültiger WireGuard-Schlüssel (32 Byte, Base64).',
|
||||||
'duplicate_key' => 'Für diesen Schlüssel gibt es bereits einen Zugang.',
|
'duplicate_key' => 'Für diesen Schlüssel gibt es bereits einen Zugang.',
|
||||||
'pending_removal' => 'Dieser Zugang wird gerade entfernt. Bitte kurz warten.',
|
'pending_removal' => 'Dieser Zugang wird gerade entfernt. Bitte kurz warten.',
|
||||||
|
'host_key' => 'Dieser Schlüssel gehört bereits zu einem Host. Ein zweiter Zugang darauf würde dessen Tunnel kappen.',
|
||||||
'created' => 'Zugang angelegt.',
|
'created' => 'Zugang angelegt.',
|
||||||
'blocked' => 'Zugang gesperrt.',
|
'blocked' => 'Zugang gesperrt.',
|
||||||
'unblocked' => 'Zugang wieder freigegeben.',
|
'unblocked' => 'Zugang wieder freigegeben.',
|
||||||
|
|
|
||||||
|
|
@ -33,6 +33,7 @@ return [
|
||||||
'invalid_key' => 'That is not a valid WireGuard key (32 bytes, base64).',
|
'invalid_key' => 'That is not a valid WireGuard key (32 bytes, base64).',
|
||||||
'duplicate_key' => 'An access already exists for this key.',
|
'duplicate_key' => 'An access already exists for this key.',
|
||||||
'pending_removal' => 'This access is currently being removed. Please wait a moment.',
|
'pending_removal' => 'This access is currently being removed. Please wait a moment.',
|
||||||
|
'host_key' => 'This key already belongs to a host. A second access on it would cut that host\'s tunnel.',
|
||||||
'created' => 'Access created.',
|
'created' => 'Access created.',
|
||||||
'blocked' => 'Access blocked.',
|
'blocked' => 'Access blocked.',
|
||||||
'unblocked' => 'Access re-enabled.',
|
'unblocked' => 'Access re-enabled.',
|
||||||
|
|
|
||||||
|
|
@ -384,3 +384,49 @@ it('keeps host peer removal behind the same hub lock', function () {
|
||||||
expect(microtime(true) - $started)->toBeGreaterThan(0.8)
|
expect(microtime(true) - $started)->toBeGreaterThan(0.8)
|
||||||
->and($hub->peerIps())->toBe([]);
|
->and($hub->peerIps())->toBe([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('refuses a public key that already belongs to a host', function () {
|
||||||
|
vpnHub();
|
||||||
|
$key = Keypair::generate()->publicKey;
|
||||||
|
// The host exists but the scheduled sync has not adopted its peer yet.
|
||||||
|
Host::factory()->active()->create(['name' => 'pve-fsn1', 'wg_pubkey' => $key]);
|
||||||
|
|
||||||
|
Livewire::actingAs(operator('Owner'))->test(Vpn::class)
|
||||||
|
->set('name', 'entführt')
|
||||||
|
->set('publicKey', $key)
|
||||||
|
->call('create')
|
||||||
|
->assertHasErrors('publicKey');
|
||||||
|
|
||||||
|
expect(VpnPeer::withTrashed()->count())->toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('renames a peer it adopted before the host link was known', function () {
|
||||||
|
$hub = vpnHub();
|
||||||
|
$key = Keypair::generate()->publicKey;
|
||||||
|
$hub->addPeer($key, '10.66.0.7');
|
||||||
|
|
||||||
|
// First sync: the step has not stored wg_pubkey yet → unknown peer.
|
||||||
|
(new SyncVpnPeers)->handle($hub);
|
||||||
|
$peer = VpnPeer::query()->where('public_key', $key)->firstOrFail();
|
||||||
|
expect($peer->name)->toBe(__('vpn.unknown_peer'))
|
||||||
|
->and($peer->host_id)->toBeNull();
|
||||||
|
|
||||||
|
Host::factory()->active()->create(['name' => 'pve-fsn1', 'wg_pubkey' => $key]);
|
||||||
|
(new SyncVpnPeers)->handle($hub);
|
||||||
|
|
||||||
|
expect($peer->fresh()->name)->toBe('pve-fsn1')
|
||||||
|
->and($peer->fresh()->host_id)->not->toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not rename an access an operator named themselves', function () {
|
||||||
|
$hub = vpnHub();
|
||||||
|
$user = operator('Owner');
|
||||||
|
$key = Keypair::generate()->publicKey;
|
||||||
|
$peer = VpnPeer::factory()->create(['public_key' => $key, 'name' => 'Notebook Boban', 'created_by' => $user->id]);
|
||||||
|
$hub->addPeer($key, $peer->allowed_ip);
|
||||||
|
|
||||||
|
Host::factory()->active()->create(['name' => 'pve-fsn1', 'wg_pubkey' => $key]);
|
||||||
|
(new SyncVpnPeers)->handle($hub);
|
||||||
|
|
||||||
|
expect($peer->fresh()->name)->toBe('Notebook Boban');
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue