From 65717bd3bd35282d7e00da452b4f95f8ecb4cffd Mon Sep 17 00:00:00 2001 From: nexxo Date: Sat, 25 Jul 2026 10:20:59 +0200 Subject: [PATCH] fix(engine): address Codex review (auth token bootstrap, tunnel recheck, ssh) - CreateAutomationToken now bootstraps the pveum role/user/token over the authenticated SSH session (a fresh host has no API token yet); ProxmoxClient is read-only in A. - ConfigureWireguard re-verifies the handshake on the idempotent replay path, never advancing over a dead tunnel. - PhpseclibRemoteShell treats a missing SSH exit status as failure (255). - connectWithKey verifies the pinned host key fingerprint on later logins. Co-Authored-By: Claude Opus 4.8 --- .../Steps/Host/ConfigureWireguard.php | 45 +++++++++-------- .../Steps/Host/CreateAutomationToken.php | 50 +++++++++++++------ app/Provisioning/Steps/Host/HostStep.php | 7 ++- app/Services/Proxmox/FakeProxmoxClient.php | 23 --------- app/Services/Proxmox/HttpProxmoxClient.php | 49 ++---------------- app/Services/Proxmox/ProxmoxClient.php | 10 ++-- app/Services/Ssh/FakeRemoteShell.php | 2 +- app/Services/Ssh/PhpseclibRemoteShell.php | 11 +++- app/Services/Ssh/RemoteShell.php | 7 ++- .../HostOnboardingEndToEndTest.php | 6 +++ tests/Feature/Provisioning/HostStepsTest.php | 17 ++++--- tests/Feature/Provisioning/ServicesTest.php | 6 +-- 12 files changed, 105 insertions(+), 128 deletions(-) diff --git a/app/Provisioning/Steps/Host/ConfigureWireguard.php b/app/Provisioning/Steps/Host/ConfigureWireguard.php index 8b4bd55..8875d0d 100644 --- a/app/Provisioning/Steps/Host/ConfigureWireguard.php +++ b/app/Provisioning/Steps/Host/ConfigureWireguard.php @@ -24,33 +24,34 @@ class ConfigureWireguard extends HostStep public function execute(ProvisioningRun $run): StepResult { $host = $this->host($run); - - // Idempotent: tunnel already provisioned and peer registered. - if (filled($host->wg_ip) && $this->hasResource($run, 'wg_peer')) { - return StepResult::advance(); - } + $alreadyProvisioned = filled($host->wg_ip) && $this->hasResource($run, 'wg_peer'); $this->keyLogin($this->shell, $host); - $this->shell->run('export DEBIAN_FRONTEND=noninteractive; apt-get install -y wireguard'); - $this->shell->run('test -f /etc/wireguard/privatekey || (umask 077; wg genkey > /etc/wireguard/privatekey)'); - $publicKey = trim($this->shell->run('wg pubkey < /etc/wireguard/privatekey')->stdout); - if (blank($publicKey)) { - return StepResult::retry(20, 'could not read host WireGuard public key'); + if (! $alreadyProvisioned) { + $this->shell->run('export DEBIAN_FRONTEND=noninteractive; apt-get install -y wireguard'); + $this->shell->run('test -f /etc/wireguard/privatekey || (umask 077; wg genkey > /etc/wireguard/privatekey)'); + + $publicKey = trim($this->shell->run('wg pubkey < /etc/wireguard/privatekey')->stdout); + if (blank($publicKey)) { + return StepResult::retry(20, 'could not read host WireGuard public key'); + } + + $wgIp = $host->wg_ip ?: $this->hub->allocateIp(); + $privateKey = trim($this->shell->run('cat /etc/wireguard/privatekey')->stdout); + + $this->shell->putFile('/etc/wireguard/wg0.conf', $this->renderConfig($wgIp, $privateKey)); + $this->shell->run('systemctl enable --now wg-quick@wg0 || wg-quick up wg0'); + + $this->hub->addPeer($publicKey, $wgIp); + + // Persist external identity BEFORE verifying/advancing (crash-safe). + $host->update(['wg_ip' => $wgIp, 'wg_pubkey' => $publicKey]); + $this->recordResource($run, $host, 'wg_peer', $publicKey); } - $wgIp = $host->wg_ip ?: $this->hub->allocateIp(); - $privateKey = trim($this->shell->run('cat /etc/wireguard/privatekey')->stdout); - - $this->shell->putFile('/etc/wireguard/wg0.conf', $this->renderConfig($wgIp, $privateKey)); - $this->shell->run('systemctl enable --now wg-quick@wg0 || wg-quick up wg0'); - - $this->hub->addPeer($publicKey, $wgIp); - - // Persist external identity BEFORE verifying/advancing (crash-safe). - $host->update(['wg_ip' => $wgIp, 'wg_pubkey' => $publicKey]); - $this->recordResource($run, $host, 'wg_peer', $publicKey); - + // Always verify the tunnel is up — including the idempotent replay path, + // so we never advance onto Proxmox API calls over a dead tunnel. $hubIp = (string) config('provisioning.wireguard.hub_ip', '10.66.0.1'); if (! $this->shell->run('ping -c1 -W2 '.escapeshellarg($hubIp))->ok()) { return StepResult::retry(15, 'WireGuard handshake not up yet'); diff --git a/app/Provisioning/Steps/Host/CreateAutomationToken.php b/app/Provisioning/Steps/Host/CreateAutomationToken.php index 97334a5..4e6b46f 100644 --- a/app/Provisioning/Steps/Host/CreateAutomationToken.php +++ b/app/Provisioning/Steps/Host/CreateAutomationToken.php @@ -4,16 +4,18 @@ namespace App\Provisioning\Steps\Host; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; -use App\Services\Proxmox\ProxmoxClient; +use App\Services\Ssh\RemoteShell; /** - * Creates the automation role + user + API token on the host and stores the - * (encrypted) token reference. The secret is shown only at creation, so the - * token id is persisted before advancing. + * Creates the automation role + user + API token on the host via `pveum` over + * SSH. A fresh host has no API token yet, so this bootstrap must run through the + * authenticated root SSH session, not the (token-less) REST API. The secret is + * shown only once, so the token id is persisted before advancing; a re-run + * deletes and re-mints the token to stay idempotent after a crash. */ class CreateAutomationToken extends HostStep { - public function __construct(private ProxmoxClient $pve) {} + public function __construct(private RemoteShell $shell) {} public function key(): string { @@ -29,23 +31,39 @@ class CreateAutomationToken extends HostStep return StepResult::advance(); } - $client = $this->pve->forHost($host); - $client->createRole( - config('provisioning.proxmox.role_id'), - config('provisioning.proxmox.role_privs'), + $role = (string) config('provisioning.proxmox.role_id'); + $privs = (string) config('provisioning.proxmox.role_privs'); + $user = (string) config('provisioning.proxmox.user'); + $tokenName = (string) config('provisioning.proxmox.token_name'); + + $this->keyLogin($this->shell, $host); + + // Role / user / ACL are idempotent (ignore "already exists"). + $this->shell->run('pveum role add '.escapeshellarg($role).' -privs '.escapeshellarg($privs).' || true'); + $this->shell->run('pveum user add '.escapeshellarg($user).' || true'); + $this->shell->run('pveum acl modify / -user '.escapeshellarg($user).' -role '.escapeshellarg($role).' || true'); + + // Drop any half-created token from a prior crashed attempt, then mint fresh. + $this->shell->run('pveum user token remove '.escapeshellarg($user).' '.escapeshellarg($tokenName).' || true'); + $result = $this->shell->run( + 'pveum user token add '.escapeshellarg($user).' '.escapeshellarg($tokenName).' -privsep 0 --output-format json' ); - $token = $client->createUserAndToken( - config('provisioning.proxmox.user'), - config('provisioning.proxmox.role_id'), - ); + if (! $result->ok()) { + return StepResult::retry(20, 'Proxmox token creation failed'); + } - if (blank($token['secret'] ?? null)) { + $data = json_decode($result->stdout, true); + $secret = $data['value'] ?? null; + + if (blank($secret)) { return StepResult::retry(20, 'Proxmox token secret was empty'); } - $host->update(['api_token_ref' => $token['token_id'].'='.$token['secret']]); - $this->recordResource($run, $host, 'pve_token', $token['token_id']); + $tokenId = $data['full-tokenid'] ?? "{$user}!{$tokenName}"; + + $host->update(['api_token_ref' => $tokenId.'='.$secret]); + $this->recordResource($run, $host, 'pve_token', $tokenId); return StepResult::advance(); } diff --git a/app/Provisioning/Steps/Host/HostStep.php b/app/Provisioning/Steps/Host/HostStep.php index b23a307..fa66b95 100644 --- a/app/Provisioning/Steps/Host/HostStep.php +++ b/app/Provisioning/Steps/Host/HostStep.php @@ -34,7 +34,12 @@ abstract class HostStep implements ProvisioningStep protected function keyLogin(RemoteShell $shell, Host $host): void { - $shell->connectWithKey($host->public_ip, 'root', (string) config('provisioning.ssh.private_key')); + $shell->connectWithKey( + $host->public_ip, + 'root', + (string) config('provisioning.ssh.private_key'), + $host->ssh_host_key, // pinned during EstablishSshTrust + ); } protected function recordResource(ProvisioningRun $run, Host $host, string $kind, string $externalId): void diff --git a/app/Services/Proxmox/FakeProxmoxClient.php b/app/Services/Proxmox/FakeProxmoxClient.php index c7684e6..56017c5 100644 --- a/app/Services/Proxmox/FakeProxmoxClient.php +++ b/app/Services/Proxmox/FakeProxmoxClient.php @@ -23,14 +23,6 @@ class FakeProxmoxClient implements ProxmoxClient ['storage' => 'local-lvm', 'type' => 'lvmthin', 'total' => 1099511627776], // 1 TiB ]; - /** @var array */ - public array $createdRoles = []; - - /** @var array{token_id: string, secret: string}|null */ - public ?array $createdToken = null; - - public int $tokenCalls = 0; - public function forHost(Host $host): static { $this->host = $host; @@ -52,19 +44,4 @@ class FakeProxmoxClient implements ProxmoxClient { return $this->storage; } - - public function createRole(string $roleId, string $privs): void - { - $this->createdRoles[] = $roleId; - } - - public function createUserAndToken(string $user, string $roleId): array - { - $this->tokenCalls++; - - return $this->createdToken = [ - 'token_id' => $user.'!clupilot', - 'secret' => 'fake-secret-'.substr(md5($user.$roleId), 0, 12), - ]; - } } diff --git a/app/Services/Proxmox/HttpProxmoxClient.php b/app/Services/Proxmox/HttpProxmoxClient.php index 85e4226..bf7f966 100644 --- a/app/Services/Proxmox/HttpProxmoxClient.php +++ b/app/Services/Proxmox/HttpProxmoxClient.php @@ -5,12 +5,13 @@ namespace App\Services\Proxmox; use App\Models\Host; use Illuminate\Http\Client\PendingRequest; use Illuminate\Support\Facades\Http; -use Illuminate\Support\Str; /** - * Real Proxmox VE REST client. Token auth over the WireGuard tunnel; the PVE - * certificate is self-signed so TLS verification is disabled (traffic already - * runs inside the encrypted tunnel). Not unit-tested (live I/O). + * Real Proxmox VE REST client (read-only in A: capacity + reachability). Token + * auth over the WireGuard tunnel; the PVE certificate is self-signed so TLS + * verification is disabled (traffic already runs inside the encrypted tunnel). + * Not unit-tested (live I/O). The automation token itself is bootstrapped via + * SSH `pveum` (see CreateAutomationToken), not here. */ class HttpProxmoxClient implements ProxmoxClient { @@ -48,44 +49,4 @@ class HttpProxmoxClient implements ProxmoxClient { return $this->http()->get("/nodes/{$node}/storage")->throw()->json('data', []); } - - public function createRole(string $roleId, string $privs): void - { - $response = $this->http()->asForm()->post('/access/roles', [ - 'roleid' => $roleId, - 'privs' => $privs, - ]); - - // Idempotent: an existing role is not an error. - if ($response->failed() && ! str_contains($response->body(), 'already exists')) { - $response->throw(); - } - } - - public function createUserAndToken(string $user, string $roleId): array - { - // Ensure the user exists (ignore "already exists"), grant the role, mint a token. - $this->http()->asForm()->post('/access/users', [ - 'userid' => $user, - 'password' => Str::random(40), - ]); - - $this->http()->asForm()->put('/access/acl', [ - 'path' => '/', - 'users' => $user, - 'roles' => $roleId, - ])->throw(); - - $tokenName = config('provisioning.proxmox.token_name', 'clupilot'); - - $data = $this->http()->asForm() - ->post("/access/users/{$user}/token/{$tokenName}", ['privsep' => 0]) - ->throw() - ->json('data', []); - - return [ - 'token_id' => $data['full-tokenid'] ?? "{$user}!{$tokenName}", - 'secret' => $data['value'] ?? '', - ]; - } } diff --git a/app/Services/Proxmox/ProxmoxClient.php b/app/Services/Proxmox/ProxmoxClient.php index 0fb3a4b..3ae0984 100644 --- a/app/Services/Proxmox/ProxmoxClient.php +++ b/app/Services/Proxmox/ProxmoxClient.php @@ -6,8 +6,9 @@ use App\Models\Host; /** * Proxmox VE REST client, addressed per host over its wg_ip with a token. - * A v1.0 reads capacity and provisions the automation token; Subsystem B - * extends it with VM lifecycle methods (clone, cloud-init, guestExec …). + * A v1.0 uses it read-only (capacity + reachability); the automation token is + * bootstrapped over SSH (pveum). Subsystem B extends it with VM lifecycle + * methods (clone, cloud-init, guestExec …). */ interface ProxmoxClient { @@ -22,9 +23,4 @@ interface ProxmoxClient /** @return array> */ public function nodeStorage(string $node): array; - - public function createRole(string $roleId, string $privs): void; - - /** @return array{token_id: string, secret: string} */ - public function createUserAndToken(string $user, string $roleId): array; } diff --git a/app/Services/Ssh/FakeRemoteShell.php b/app/Services/Ssh/FakeRemoteShell.php index 8909f9d..1611c81 100644 --- a/app/Services/Ssh/FakeRemoteShell.php +++ b/app/Services/Ssh/FakeRemoteShell.php @@ -53,7 +53,7 @@ class FakeRemoteShell implements RemoteShell $this->connections[] = ['password', $host, $user]; } - public function connectWithKey(string $host, string $user, string $privateKey): void + public function connectWithKey(string $host, string $user, string $privateKey, ?string $expectedFingerprint = null): void { if ($this->failConnect) { throw new \RuntimeException("connection refused: {$host}"); diff --git a/app/Services/Ssh/PhpseclibRemoteShell.php b/app/Services/Ssh/PhpseclibRemoteShell.php index 9ab4b72..2065722 100644 --- a/app/Services/Ssh/PhpseclibRemoteShell.php +++ b/app/Services/Ssh/PhpseclibRemoteShell.php @@ -26,7 +26,7 @@ class PhpseclibRemoteShell implements RemoteShell $this->ssh = $ssh; } - public function connectWithKey(string $host, string $user, string $privateKey): void + public function connectWithKey(string $host, string $user, string $privateKey, ?string $expectedFingerprint = null): void { $key = PublicKeyLoader::load($privateKey); $ssh = new SSH2($host); @@ -36,6 +36,12 @@ class PhpseclibRemoteShell implements RemoteShell } $this->ssh = $ssh; + + // Host-key pinning: reject a server whose key changed since onboarding. + if (filled($expectedFingerprint) && $this->hostKeyFingerprint() !== $expectedFingerprint) { + $this->ssh = null; + throw new RuntimeException("SSH host key mismatch for {$host} (possible interception or re-provision)."); + } } public function run(string $command): CommandResult @@ -43,7 +49,8 @@ class PhpseclibRemoteShell implements RemoteShell $stdout = (string) $this->ssh()->exec($command); $exit = $this->ssh()->getExitStatus(); - return new CommandResult(is_int($exit) ? $exit : 0, $stdout, ''); + // No exit status = interrupted/closed channel → treat as failure, never success. + return new CommandResult(is_int($exit) ? $exit : 255, $stdout, ''); } public function putFile(string $remotePath, string $contents): void diff --git a/app/Services/Ssh/RemoteShell.php b/app/Services/Ssh/RemoteShell.php index e78eceb..8784eaa 100644 --- a/app/Services/Ssh/RemoteShell.php +++ b/app/Services/Ssh/RemoteShell.php @@ -10,7 +10,12 @@ interface RemoteShell { public function connectWithPassword(string $host, string $user, string $password): void; - public function connectWithKey(string $host, string $user, string $privateKey): void; + /** + * Connect with a private key. When $expectedFingerprint is non-empty the + * server host key must match it, otherwise the connection is rejected + * (host-key pinning). + */ + public function connectWithKey(string $host, string $user, string $privateKey, ?string $expectedFingerprint = null): void; public function run(string $command): CommandResult; diff --git a/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php b/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php index 0abc671..6e926a7 100644 --- a/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php +++ b/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php @@ -12,6 +12,9 @@ it('drives a fresh host all the way to active (mocked)', function () { $s = fakeServices(); $s['shell']->script('wg pubkey', CommandResult::success('HOSTPUBKEY0000=')); $s['shell']->script('uname -r', CommandResult::success('6.8.12-4-pve')); + $s['shell']->script('pveum user token add', CommandResult::success( + json_encode(['full-tokenid' => 'automation@pve!clupilot', 'value' => 'tok-secret-123']) + )); $host = app(StartHostOnboarding::class)->run([ 'name' => 'pve-fsn-9', @@ -53,6 +56,9 @@ it('does not duplicate external resources when a step re-runs after a crash', fu $s = fakeServices(); $s['shell']->script('wg pubkey', CommandResult::success('HOSTPUBKEY0000=')); $s['shell']->script('uname -r', CommandResult::success('6.8.12-4-pve')); + $s['shell']->script('pveum user token add', CommandResult::success( + json_encode(['full-tokenid' => 'automation@pve!clupilot', 'value' => 'tok-secret-123']) + )); $host = app(StartHostOnboarding::class)->run([ 'name' => 'pve-fsn-10', diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php index c4e2bf9..4a9a983 100644 --- a/tests/Feature/Provisioning/HostStepsTest.php +++ b/tests/Feature/Provisioning/HostStepsTest.php @@ -180,20 +180,25 @@ it('fails when the host does not return before the reboot deadline', function () // --- CreateAutomationToken --- -it('creates and persists the automation token exactly once', function () { +it('creates and persists the automation token via pveum exactly once', function () { $s = fakeServices(); + $s['shell']->script('pveum user token add', CommandResult::success( + json_encode(['full-tokenid' => 'automation@pve!clupilot', 'value' => 'tok-secret-123']) + )); $host = Host::factory()->create(); $run = hostRun($host); expect(app(CreateAutomationToken::class)->execute($run)->type)->toBe('advance'); $host->refresh(); - expect($host->api_token_ref)->toContain('automation@pve') - ->and($host->api_token_ref)->toContain('fake-secret') - ->and($s['pve']->tokenCalls)->toBe(1); + expect($host->api_token_ref)->toContain('automation@pve!clupilot') + ->and($host->api_token_ref)->toContain('tok-secret-123'); - // Re-run: idempotent short-circuit, no second token call. + $tokenCalls = fn () => count(array_filter($s['shell']->recorded(), fn ($c) => str_contains($c, 'pveum user token add'))); + expect($tokenCalls())->toBe(1); + + // Re-run: idempotent short-circuit, no second token creation. app(CreateAutomationToken::class)->execute($run->fresh()); - expect($s['pve']->tokenCalls)->toBe(1) + expect($tokenCalls())->toBe(1) ->and(RunResource::where('run_id', $run->id)->where('kind', 'pve_token')->count())->toBe(1); }); diff --git a/tests/Feature/Provisioning/ServicesTest.php b/tests/Feature/Provisioning/ServicesTest.php index b3ecf9a..904e3d6 100644 --- a/tests/Feature/Provisioning/ServicesTest.php +++ b/tests/Feature/Provisioning/ServicesTest.php @@ -34,15 +34,11 @@ it('allocates ips and tracks peers (FakeWireguardHub)', function () { expect($hub->peers())->toBe([]); }); -it('reports capacity and mints a token for a host (FakeProxmoxClient)', function () { +it('reports node capacity for a host (FakeProxmoxClient)', function () { $host = Host::factory()->create(['wg_ip' => '10.66.0.5']); $client = (new FakeProxmoxClient)->forHost($host); expect($client->listNodes())->not->toBeEmpty() ->and($client->nodeStatus('pve')['memory']['total'])->toBeGreaterThan(0) ->and($client->nodeStorage('pve')[0]['total'])->toBeGreaterThan(0); - - $token = $client->createUserAndToken('automation@pve', 'CluPilotAutomation'); - expect($token)->toHaveKeys(['token_id', 'secret']) - ->and($client->tokenCalls)->toBe(1); });