fix(vpn): store the config inside the creation transaction

Encryption and the write ran after the transaction had committed and the peer
had been dispatched to the hub. A failure there left a live access whose config
was never stored — unrecoverable, and the operator would create a second one
not knowing why. Both now happen in the same insert; nothing is dispatched
unless it succeeded.

Re-verified in the browser afterwards: create with storage, wrong password
refused, right password reveals the config.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
feat/portal-design
nexxo 2026-07-25 22:53:30 +02:00
parent 4ffdafc614
commit 17d7dc2ad9
1 changed files with 25 additions and 10 deletions

View File

@ -108,7 +108,11 @@ class Vpn extends Component
// tables, so neither unique index can catch the other's insert — the // tables, so neither unique index can catch the other's insert — the
// shared lock is what keeps a host and an access off the same tunnel IP. // shared lock is what keeps a host and an access off the same tunnel IP.
try { try {
$peer = Cache::lock('wireguard:allocate', 30)->block(10, fn () => DB::transaction(function () use ($hub, $publicKey) { // Filled inside the transaction; only the handoff needs it afterwards.
$plainConfig = null;
$peer = Cache::lock('wireguard:allocate', 30)->block(10, function () use ($hub, $publicKey, $keypair, &$plainConfig) {
return DB::transaction(function () use ($hub, $publicKey, $keypair, &$plainConfig) {
// The owner row is locked for the whole insert, and revokeStaff() // The owner row is locked for the whole insert, and revokeStaff()
// locks the same row: without that, a revocation could commit // locks the same row: without that, a revocation could commit
// between the check and the insert, find no peer to remove, and // between the check and the insert, find no peer to remove, and
@ -135,17 +139,33 @@ class Vpn extends Component
return 'host_key'; return 'host_key';
} }
$ip = $hub->allocateIp();
// Built and encrypted here so the stored config is part of the
// same insert. Doing it afterwards could leave a live access
// whose config was never stored — unrecoverable, and the
// operator would create a second one not knowing why.
$secret = null;
if ($keypair !== null) {
$plainConfig = $this->clientConfig($keypair, $ip);
if ($this->storeConfig) {
$secret = ConfigVault::encrypt($plainConfig);
}
}
return VpnPeer::create([ return VpnPeer::create([
'name' => trim($this->name), 'name' => trim($this->name),
'kind' => VpnPeer::KIND_STAFF, 'kind' => VpnPeer::KIND_STAFF,
'user_id' => $this->ownerId, 'user_id' => $this->ownerId,
'public_key' => $publicKey, 'public_key' => $publicKey,
'allowed_ip' => $hub->allocateIp(), 'allowed_ip' => $ip,
'config_secret' => $secret,
'enabled' => true, 'enabled' => true,
'present' => false, 'present' => false,
'created_by' => auth()->id(), 'created_by' => auth()->id(),
]); ]);
})); });
});
} catch (QueryException) { } catch (QueryException) {
// Backstop: the unique index caught a writer that did not take this // Backstop: the unique index caught a writer that did not take this
// lock. Report it like any other duplicate instead of a 500. // lock. Report it like any other duplicate instead of a 500.
@ -169,14 +189,9 @@ class Vpn extends Component
$this->dismissConfig(); $this->dismissConfig();
// Only a key we generated can be turned into a ready-to-use config. // Only a key we generated can be turned into a ready-to-use config.
if ($keypair !== null) { if ($plainConfig !== null) {
$config = $this->clientConfig($keypair, $peer->allowed_ip); $this->configToken = ConfigHandoff::put($plainConfig);
$this->configToken = ConfigHandoff::put($config);
$this->newConfigName = $peer->name; $this->newConfigName = $peer->name;
if ($this->storeConfig) {
$peer->forceFill(['config_secret' => ConfigVault::encrypt($config)])->saveQuietly();
}
} }
$this->reset('name', 'publicKey', 'storeConfig'); $this->reset('name', 'publicKey', 'storeConfig');