diff --git a/app/Models/Host.php b/app/Models/Host.php index 98c2bb7..15e02a7 100644 --- a/app/Models/Host.php +++ b/app/Models/Host.php @@ -4,6 +4,7 @@ namespace App\Models; use App\Models\Concerns\HasUuid; use App\Provisioning\Contracts\ProvisioningSubject; +use App\Services\Secrets\SecretCipher; use Database\Factories\HostFactory; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; @@ -28,7 +29,6 @@ class Host extends Model implements ProvisioningSubject protected function casts(): array { return [ - 'api_token_ref' => 'encrypted', 'last_seen_at' => 'datetime', 'total_gb' => 'integer', 'total_ram_mb' => 'integer', @@ -38,6 +38,32 @@ class Host extends Model implements ProvisioningSubject ]; } + /** + * The Proxmox API token, stored under SECRETS_KEY — NOT through Laravel's + * `encrypted` cast. + * + * That cast uses APP_KEY, and SecretVault's docblock states the rule this + * column was breaking: rotating APP_KEY is ordinary maintenance, and every + * credential that lives behind it would become unreadable at once. For this + * column that means every host's automation token in the same moment — no + * Proxmox API on any host, discovered when provisioning stops rather than + * when the key is rotated. Same handling as Mailbox::password, through the + * same SecretCipher, so there is one key story and not two. + */ + public function setApiTokenRefAttribute(?string $value): void + { + $this->attributes['api_token_ref'] = ($value === null || $value === '') + ? null + : app(SecretCipher::class)->encrypt($value); + } + + public function getApiTokenRefAttribute(?string $value): ?string + { + return ($value === null || $value === '') + ? null + : app(SecretCipher::class)->decrypt($value); + } + /** Placement runs (polymorphic subject). */ public function runs(): MorphMany { diff --git a/app/Provisioning/Steps/Host/ConfigureProxmox.php b/app/Provisioning/Steps/Host/ConfigureProxmox.php index 395f0c2..f4d7d08 100644 --- a/app/Provisioning/Steps/Host/ConfigureProxmox.php +++ b/app/Provisioning/Steps/Host/ConfigureProxmox.php @@ -7,8 +7,23 @@ use App\Provisioning\StepResult; use App\Services\Ssh\RemoteShell; /** - * Baseline Proxmox configuration after the kernel switch: ensure the default - * bridge exists and drop the enterprise repo. Idempotent and best-effort. + * Baseline Proxmox configuration after the kernel switch: confirm the default + * bridge exists, turn the datacenter firewall on so per-VM rules mean something, + * and drop the enterprise repo the proxmox-ve package brings back with it. + * + * This step used to do none of those things. It ran `ip link show vmbr0` and + * threw the result away — its comment claimed it "records the absence", and it + * recorded nothing — then repeated a `rm -f` and always advanced. Two real + * consequences followed from that: + * + * 1. Proxmox installed ON TOP OF DEBIAN does not create vmbr0; only the ISO + * installer writes that bridge into /etc/network/interfaces. So onboarding + * reported `active` on a host that had no bridge to attach a customer VM to, + * and the first paid order died in ConfigureNetwork. + * 2. Proxmox applies a guest's firewall rules only while the firewall is enabled + * at DATACENTER level (cluster.fw `[OPTIONS] enable`). It ships disabled, so + * the "80/443 only" rules HttpProxmoxClient::applyFirewall() writes for every + * customer VM were inert on every host this pipeline ever onboarded. */ class ConfigureProxmox extends HostStep { @@ -24,10 +39,120 @@ class ConfigureProxmox extends HostStep $host = $this->host($run); $this->keyLogin($this->shell, $host); - // Most PVE installs create vmbr0; record its absence for real-host follow-up. - $this->shell->run('ip link show vmbr0'); - $this->shell->run('rm -f /etc/apt/sources.list.d/pve-enterprise.list'); + if (($refusal = $this->refuseWithoutBridge()) !== null) { + return $refusal; + } + + if (($failure = $this->enableDatacenterFirewall()) !== null) { + return $failure; + } + + // AFTER the install, not before: the proxmox-ve package ships the + // enterprise repository itself, so InstallProxmoxVe's removal (which + // happens before apt runs) cannot be the one that sticks. Both spellings, + // because PVE 9 ships the deb822 file and PVE 8 the one-line one. + $this->shell->run('rm -f /etc/apt/sources.list.d/pve-enterprise.list /etc/apt/sources.list.d/pve-enterprise.sources'); return StepResult::advance(); } + + /** + * vmbr0 must exist — and this step will NOT create it. + * + * Creating a bridge over the primary NIC from a remote shell means editing + * /etc/network/interfaces and reloading the network on the same link the + * shell is running over. Getting it even slightly wrong (a provider that + * needs a pointopoint route, a MAC the upstream switch will not accept on a + * bridge, an IPv6 gateway on the wrong device) leaves the machine off the + * network with no way back except the provider's console. The failure below + * costs an operator ten minutes; the alternative costs a dead server. + * + * The message carries the default route, because that is the one fact the + * operator needs and the one they would otherwise go and look up. + */ + private function refuseWithoutBridge(): ?StepResult + { + if ($this->shell->run('ip link show vmbr0')->ok()) { + return null; + } + + $route = trim($this->shell->run('ip -4 route show default')->stdout); + + return StepResult::fail( + 'No vmbr0 bridge on this host. Proxmox installed on top of Debian does not create one — only the '. + 'ISO installer does — and CluPilot deliberately will not build it over the primary NIC from a remote '. + 'shell, because a mistake there takes the machine off the network for good. Create it by hand in '. + '/etc/network/interfaces (bridge_ports = the NIC carrying the default route, the host address and '. + 'gateway moved onto vmbr0), apply it with `ifreload -a`, confirm you still have SSH, then retry this '. + 'run. Current default route: '.($route === '' ? '(none reported)' : $route).'.' + ); + } + + /** + * Enable the datacenter firewall WITHOUT locking the host out. + * + * The plain `enable 1` is a trap. Proxmox's datacenter input policy defaults + * to DROP, the node's own host firewall defaults to ENABLED, and the only + * sources it lets reach 8006 and 22 are the members of the auto-generated + * `management` ipset — which is seeded from `local_network`, the subnet of + * the node's own management address, i.e. the PUBLIC subnet. The WireGuard + * management address every later step and every maintenance run comes from is + * not in it. Enabling the firewall blindly therefore drops CluPilot's own SSH + * and API access the moment pve-firewall next compiles, on a host whose + * public port 22 SecureHostFirewall is about to close as well. + * + * So three settings, in this order, each doing one job: + * + * - `policy_in`/`policy_out` ACCEPT at datacenter level. This is the HOST + * input policy only: a guest reads its own vm.fw `policy_in`, which + * applyFirewall() sets to DROP explicitly for every customer VM, so ACCEPT + * here cannot loosen a single customer rule. What it does do is make sure + * that if anyone ever re-enables the node firewall from the GUI, the host + * does not become unreachable over the tunnel in the same instant. + * - The node's host firewall off. The host policy lives in nftables, written + * by SecureHostFirewall — see that step's docblock for why that was chosen + * over pve-firewall. Two filters both claiming to own the input chain is + * how a host ends up unreachable for a reason nobody can find: with both + * active a packet must be accepted by BOTH, so the WireGuard rules in + * nftables would be silently overruled by an ipset compiled from the public + * subnet. One owner, named in one place. + * - `enable 1` last, once neither of the above can bite. + * + * `policy_forward` is deliberately left alone: it only applies on a node + * running the new nftables-based pve-firewall backend, which is opt-in and + * off here, and setting a knob whose scope depends on which backend is + * active would be guessing. + * + * The node is addressed by `$(hostname)` rather than by hosts.name because + * that is exactly how Proxmox names the node; if the two ever diverge, the + * machine is right and the database row is not. + */ + private function enableDatacenterFirewall(): ?StepResult + { + $commands = [ + 'pvesh set /cluster/firewall/options --policy_in ACCEPT --policy_out ACCEPT', + 'pvesh set /nodes/"$(hostname)"/firewall/options --enable 0', + 'pvesh set /cluster/firewall/options --enable 1', + ]; + + foreach ($commands as $command) { + if (! $this->shell->run($command)->ok()) { + return StepResult::retry(20, 'could not configure the Proxmox datacenter firewall: '.$command); + } + } + + // Read it back. `pvesh set` exiting 0 is not the same as the setting + // being in cluster.fw: /etc/pve is a replicated filesystem and a write + // that lost quorum is precisely the case where every customer VM's rules + // would go on being inert while onboarding reported success. + $options = $this->shell->run('pvesh get /cluster/firewall/options --output-format json')->stdout; + if (! str_contains(preg_replace('/\s+/', '', $options) ?? '', '"enable":1')) { + return StepResult::retry( + 20, + 'the Proxmox datacenter firewall does not read back as enabled; per-VM rules would have no effect' + ); + } + + return null; + } } diff --git a/app/Provisioning/Steps/Host/ConfigureWireguard.php b/app/Provisioning/Steps/Host/ConfigureWireguard.php index 9058ef2..b725a21 100644 --- a/app/Provisioning/Steps/Host/ConfigureWireguard.php +++ b/app/Provisioning/Steps/Host/ConfigureWireguard.php @@ -2,7 +2,6 @@ namespace App\Provisioning\Steps\Host; -use App\Models\Host; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Ssh\RemoteShell; @@ -13,9 +12,18 @@ use Illuminate\Support\Str; /** * Installs WireGuard on the host, allocates a management IP, registers the host * as a peer on the CluPilot hub, and verifies the tunnel is up. + * + * SSH for this step runs over the PUBLIC IP, not the tunnel — see + * HostStep::keyLogin(), which only prefers the tunnel once a handshake has + * actually succeeded. That is what makes the interface handling below safe to + * restart: the session it would otherwise cut does not go through wg0. */ class ConfigureWireguard extends HostStep { + private const CONFIG_PATH = '/etc/wireguard/wg0.conf'; + + private const UNIT = 'wg-quick@wg0'; + public function __construct(private RemoteShell $shell, private WireguardHub $hub) {} public function key(): string @@ -28,11 +36,30 @@ class ConfigureWireguard extends HostStep $host = $this->host($run); $this->keyLogin($this->shell, $host); - // Idempotent replay: tunnel already fully provisioned — just re-verify it. - if (filled($host->wg_ip) && $this->hasResource($run, 'wg_peer')) { + // Idempotent replay: the tunnel has already handshaked once (`wg_peer`) + // — just re-verify it. Asking tunnelProven() rather than checking + // wg_ip + the run's own breadcrumb keeps ONE definition of "the tunnel + // works" in the pipeline, the same one keyLogin() dials on. + if ($this->tunnelProven($host)) { return $this->verifyHandshake(); } + // The hub identity, checked BEFORE anything is installed, allocated or + // written. renderConfig() would otherwise happily emit `PublicKey = ` / + // `Endpoint = `, which is the likeliest way to reach a never-handshaking + // tunnel at all. Checked here rather than in ValidateHostInput because + // this is where the values are actually consumed: they come from the + // WireguardHub (settings table, .env fallback) and can be changed between + // step 1 and step 4, so a check further upstream would be reading + // something other than what gets written into wg0.conf. Nothing on the + // host and nothing in the database has been touched at this point, so the + // run fails clean and an operator can fill the setting in and retry. + $hubPublicKey = trim($this->hub->publicKey()); + $hubEndpoint = trim($this->hub->endpoint()); + if (blank($hubPublicKey) || blank($hubEndpoint)) { + return StepResult::fail($this->missingHubSettingsMessage($hubPublicKey, $hubEndpoint)); + } + if (! $this->shell->run('export DEBIAN_FRONTEND=noninteractive; apt-get install -y wireguard')->ok()) { return StepResult::retry(30, 'installing wireguard failed'); } @@ -55,10 +82,23 @@ class ConfigureWireguard extends HostStep }); $privateKey = trim($this->shell->run('cat /etc/wireguard/privatekey')->stdout); - $this->shell->putFile('/etc/wireguard/wg0.conf', $this->renderConfig($wgIp, $privateKey)); + $desiredConfig = $this->renderConfig($wgIp, $privateKey, $hubPublicKey, $hubEndpoint); - if (! $this->shell->run('systemctl enable --now wg-quick@wg0 || wg-quick up wg0')->ok()) { - return StepResult::retry(20, 'bringing up wg0 failed'); + // Compare before writing. The file used to be rewritten unconditionally + // with nothing reloading it, so an operator who corrected a wrong hub key + // and pressed Retry saw the new file on disk and no change in behaviour — + // the running interface still held the old peer. Knowing whether the file + // CHANGED is what lets the interface handling below decide between + // leaving a working tunnel alone and actually applying a correction. + $currentConfig = $this->shell->run('cat '.escapeshellarg(self::CONFIG_PATH).' 2>/dev/null')->stdout; + $configChanged = trim($currentConfig) !== trim($desiredConfig); + + if ($configChanged) { + $this->shell->putFile(self::CONFIG_PATH, $desiredConfig); + } + + if (($failure = $this->bringUpInterface($configChanged)) !== null) { + return $failure; } // See RemoveWireguardPeer: hub mutations are serialized against the @@ -67,7 +107,8 @@ class ConfigureWireguard extends HostStep // Store the pubkey now (so removal can always clean up the hub peer), but // record the wg_peer resource only AFTER the handshake verifies — the - // idempotent short-circuit must not skip an un-verified configuration. + // idempotent short-circuit must not skip an un-verified configuration, + // and keyLogin() must not dial an unproven tunnel. $host->update(['wg_pubkey' => $publicKey]); if ($this->verifyHandshake()->type !== StepResult::ADVANCE) { @@ -79,6 +120,62 @@ class ConfigureWireguard extends HostStep return StepResult::advance(); } + /** + * Get wg0 into the state onboarding needs: up now AND up after the step-6 + * reboot, running the configuration we just wrote. + * + * Three states, handled separately, because the single line this replaces + * (`systemctl enable --now wg-quick@wg0 || wg-quick up wg0`) got two of them + * wrong. Its fallback brought the interface up WITHOUT the systemd + * enablement, so the tunnel did not come back after the reboot; and on the + * next attempt both halves failed with "wg0 already exists", so the step + * retried forever against a tunnel that was in fact working. + * + * The order — enable before start — matters for the reboot: `enable` is what + * survives it, and it is checked and repaired even when the interface is + * already up by other means (an earlier `wg-quick up`, or an operator). + */ + private function bringUpInterface(bool $configChanged): ?StepResult + { + $present = $this->shell->run('ip link show wg0')->ok(); + + // `is-enabled` is asked for its exit status, not its wording, so a + // template unit that has never been enabled ("disabled", exit 1) and one + // that does not exist yet are treated the same: enable it. + if (! $this->shell->run('systemctl is-enabled --quiet '.self::UNIT)->ok()) { + if (! $this->shell->run('systemctl enable '.self::UNIT)->ok()) { + return StepResult::retry(20, 'could not enable '.self::UNIT.' (wg0 would not survive a reboot)'); + } + } + + if (! $present) { + if (! $this->shell->run('systemctl start '.self::UNIT)->ok()) { + return StepResult::retry(20, 'bringing up wg0 failed'); + } + + return null; + } + + // Present and unchanged: the running interface already matches the file. + // Restarting here is what used to make a working tunnel look broken. + if (! $configChanged) { + return null; + } + + // Present with a changed configuration: a restart, not `wg syncconf`. + // syncconf applies peers only, and a corrected Address or AllowedIPs + // would silently not take effect — the exact class of "the file changed + // and nothing happened" bug this method exists to remove. The brief + // interruption costs nothing: this step's own SSH session runs over the + // public IP (see the class docblock), and the short-circuit above means + // a proven tunnel never reaches this line. + if (! $this->shell->run('systemctl restart '.self::UNIT)->ok()) { + return StepResult::retry(20, 'restarting wg0 with the corrected configuration failed'); + } + + return null; + } + private function verifyHandshake(): StepResult { $hubIp = (string) config('provisioning.wireguard.hub_ip'); @@ -88,7 +185,23 @@ class ConfigureWireguard extends HostStep : StepResult::retry(15, 'WireGuard handshake not up yet'); } - private function renderConfig(string $wgIp, string $privateKey): string + /** Names the setting that is missing, so the console shows what to fill in. */ + private function missingHubSettingsMessage(string $publicKey, string $endpoint): string + { + $missing = []; + if (blank($publicKey)) { + $missing[] = 'the hub public key (CLUPILOT_WG_HUB_PUBKEY)'; + } + if (blank($endpoint)) { + $missing[] = 'the hub endpoint (CLUPILOT_WG_ENDPOINT, host:port)'; + } + + return 'Refusing to write wg0.conf: this installation has not been told '. + implode(' or ', $missing).'. Set it under Integrations and retry — '. + 'a peer section without those two values can never handshake.'; + } + + private function renderConfig(string $wgIp, string $privateKey, string $hubPublicKey, string $hubEndpoint): string { $subnet = (string) config('provisioning.wireguard.subnet', '10.66.0.0/24'); $prefix = Str::afterLast($subnet, '/'); // honour the configured prefix length @@ -99,8 +212,8 @@ class ConfigureWireguard extends HostStep "PrivateKey = {$privateKey}", '', '[Peer]', - 'PublicKey = '.$this->hub->publicKey(), - 'Endpoint = '.$this->hub->endpoint(), + 'PublicKey = '.$hubPublicKey, + 'Endpoint = '.$hubEndpoint, "AllowedIPs = {$subnet}", 'PersistentKeepalive = 25', '', diff --git a/app/Provisioning/Steps/Host/CreateAutomationToken.php b/app/Provisioning/Steps/Host/CreateAutomationToken.php index 4e6b46f..0f28ab8 100644 --- a/app/Provisioning/Steps/Host/CreateAutomationToken.php +++ b/app/Provisioning/Steps/Host/CreateAutomationToken.php @@ -26,11 +26,6 @@ class CreateAutomationToken extends HostStep { $host = $this->host($run); - // Idempotent: token already minted and persisted. - if ($this->hasResource($run, 'pve_token') && filled($host->api_token_ref)) { - return StepResult::advance(); - } - $role = (string) config('provisioning.proxmox.role_id'); $privs = (string) config('provisioning.proxmox.role_privs'); $user = (string) config('provisioning.proxmox.user'); @@ -38,8 +33,35 @@ class CreateAutomationToken extends HostStep $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'); + // The role is converged BEFORE the token short-circuit below, deliberately. + // + // `pveum role add … || true` only ever applied the privilege list on the + // run that first created the role, so adding a privilege to + // provisioning.role_privs reached new hosts and no existing one. That is + // how Sys.Modify would have been missing on every host onboarded before + // it was added, with the symptom appearing much later and somewhere else + // entirely: a 403 from POST /cluster/backup failing a paying customer's + // provisioning at register_backup. `role modify` without -append REPLACES + // the list, so this line makes the host match the config rather than + // accumulating whatever any past version of it once granted. + // Two separate commands rather than one `add || modify` line, so a + // failure can be attributed: `add` failing means the role is already + // there, which is the ordinary case and not an error. + $roleArgs = escapeshellarg($role).' -privs '.escapeshellarg($privs); + if (! $this->shell->run('pveum role add '.$roleArgs)->ok() + && ! $this->shell->run('pveum role modify '.$roleArgs)->ok()) { + return StepResult::retry(20, 'could not converge the Proxmox automation role privileges'); + } + + // Idempotent: token already minted and persisted. Below the role + // convergence, so a replay still repairs the privileges. + if ($this->hasResource($run, 'pve_token') && filled($host->api_token_ref)) { + return StepResult::advance(); + } + + // User and ACL are idempotent and have nothing to converge (unlike the + // role, whose CONTENT changes as the pipeline grows), so "already exists" + // stays tolerated here. $this->shell->run('pveum user add '.escapeshellarg($user).' || true'); $this->shell->run('pveum acl modify / -user '.escapeshellarg($user).' -role '.escapeshellarg($role).' || true'); diff --git a/app/Provisioning/Steps/Host/HostStep.php b/app/Provisioning/Steps/Host/HostStep.php index 88b5860..8914f3c 100644 --- a/app/Provisioning/Steps/Host/HostStep.php +++ b/app/Provisioning/Steps/Host/HostStep.php @@ -4,6 +4,7 @@ namespace App\Provisioning\Steps\Host; use App\Models\Host; use App\Models\ProvisioningRun; +use App\Models\RunResource; use App\Provisioning\Contracts\ProvisioningStep; use App\Provisioning\Steps\ManagesRunResources; use App\Services\Secrets\SecretVault; @@ -36,11 +37,32 @@ abstract class HostStep implements ProvisioningStep } /** - * Prefer the WireGuard management address once the tunnel exists, so every - * step after ConfigureWireguard — and every later maintenance run — reaches - * the host over the tunnel rather than its public IP. Falls back to - * public_ip for the handful of steps that run before wg_ip is set (the - * tunnel cannot carry SSH before it exists). + * Reach the host over the WireGuard tunnel once the tunnel is PROVEN, and + * over its public IP until then. + * + * The distinction is the whole point. `wg_ip` is written inside the + * allocation lock in ConfigureWireguard, before the handshake has been + * verified even once — so "wg_ip is filled" means "an address has been + * reserved", not "SSH works over the tunnel". Dialling the tunnel on the + * strength of the reservation alone produced an unrecoverable dead end: + * attempt 1 reserved the address and then failed at the handshake, and every + * later attempt — including an operator pressing Retry in the console — died + * connecting to a tunnel that did not work, before reaching the only code + * that could have repaired wg0.conf. Recovery meant nulling hosts.wg_ip by + * hand. The same shape sat under RebootIntoPveKernel, ConfigureProxmox, + * CreateAutomationToken and SecureHostFirewall, which all call this method. + * + * tunnelProven() reads the `wg_peer` breadcrumb, which ConfigureWireguard + * records ONLY after a successful handshake — the one fact in the database + * that means "this tunnel has carried traffic". + * + * Falling back to the public IP is not a way around the firewall: port 22 on + * the public IP is only open until SecureHostFirewall runs, that step runs + * last, and by the time it has run the tunnel is necessarily proven (it is + * the same `wg_peer` breadcrumb, recorded several steps earlier, and + * SecureHostFirewall additionally re-checks the handshake from the host's own + * side before writing a single rule). So the fallback only ever applies while + * 22 is still open anyway, and stops applying exactly when it closes. */ protected function keyLogin(RemoteShell $shell, Host $host): void { @@ -48,10 +70,31 @@ abstract class HostStep implements ProvisioningStep // one, else CLUPILOT_SSH_PRIVATE_KEY(_PATH) — read here, at the // point of use, per SecretVault's docblock (rule 3). $shell->connectWithKey( - filled($host->wg_ip) ? $host->wg_ip : $host->public_ip, + $this->tunnelProven($host) ? $host->wg_ip : $host->public_ip, 'root', (string) app(SecretVault::class)->get('ssh.private_key'), $host->ssh_host_key, // pinned during EstablishSshTrust ); } + + /** + * Has a WireGuard handshake to this host ever succeeded? + * + * Scoped to the HOST, not to one run: the tunnel is a property of the + * machine, and every maintenance run made after onboarding must keep using + * it — a run-scoped lookup would send a later run to a public IP where 22 is + * closed. Nothing prunes provisioning runs or their resources, so the + * breadcrumb outlives the run that wrote it. + */ + protected function tunnelProven(Host $host): bool + { + if (blank($host->wg_ip)) { + return false; + } + + return RunResource::query() + ->where('host_id', $host->id) + ->where('kind', 'wg_peer') + ->exists(); + } } diff --git a/app/Provisioning/Steps/Host/InstallProxmoxVe.php b/app/Provisioning/Steps/Host/InstallProxmoxVe.php index 36d0dad..0ba8d5b 100644 --- a/app/Provisioning/Steps/Host/InstallProxmoxVe.php +++ b/app/Provisioning/Steps/Host/InstallProxmoxVe.php @@ -10,9 +10,62 @@ use App\Services\Ssh\RemoteShell; * Installs Proxmox VE on the Debian base: add the no-subscription repo + key, * full-upgrade, install proxmox-ve. Long-running; apt/network failures retry, * a bad repo/key fails. + * + * The Debian release is READ FROM THE HOST, never assumed. Proxmox publishes one + * suite per Debian release and the two must match: pointing a Debian 13 (trixie) + * base at the bookworm suite installs a PVE built against a different libc and + * kernel, which is the kind of breakage that ends with a machine that has no + * working package manager and no working hypervisor. A server ordered today + * boots trixie, so the codename this step used to hard-code was already wrong. + * + * An unrecognised release FAILS and names what it found. Guessing a suite for an + * unknown base — "trixie is newest, so use trixie" — is exactly the mistake this + * replaces: the next Debian will be unknown too, and by then nobody is watching. */ class InstallProxmoxVe extends HostStep { + /** + * Debian codename -> the Proxmox suite and release key that go with it. + * + * `keyring` is the filename upstream's own install guide for that release + * uses; `key_url` is the URL from that same guide. Both are kept per release + * because Proxmox signs each suite with its own key and renamed the file + * between PVE 8 and PVE 9 — one shared name would eventually be pointed at + * the wrong key without anything looking wrong. + * + * Bullseye/PVE 7 is deliberately absent: onboarding a Debian 11 machine + * today means somebody picked the wrong image, and the loud failure below is + * a better answer than installing a hypervisor that is out of support. + * + * @var array + */ + private const RELEASES = [ + 'bookworm' => [ + 'suite' => 'bookworm', + 'key_url' => 'https://enterprise.proxmox.com/debian/proxmox-release-bookworm.gpg', + 'keyring' => 'proxmox-release-bookworm.gpg', + ], + 'trixie' => [ + 'suite' => 'trixie', + 'key_url' => 'https://enterprise.proxmox.com/debian/proxmox-archive-keyring-trixie.gpg', + 'keyring' => 'proxmox-archive-keyring.gpg', + ], + ]; + + /** + * Keys go here and are named by `Signed-By`, not into + * /etc/apt/trusted.gpg.d/. A key in trusted.gpg.d is trusted for EVERY + * repository on the machine, so the Proxmox key would also vouch for + * anything else that later appears in sources.list — which is why apt has + * been warning about that directory for several releases. + */ + private const KEYRING_DIR = '/usr/share/keyrings'; + + private const SOURCES_FILE = '/etc/apt/sources.list.d/pve-install-repo.sources'; + + /** What this step wrote before it used deb822; removed so the suite is stated once. */ + private const LEGACY_SOURCES_FILE = '/etc/apt/sources.list.d/pve-install-repo.list'; + public function __construct(private RemoteShell $shell) {} public function key(): string @@ -35,18 +88,40 @@ class InstallProxmoxVe extends HostStep return StepResult::advance(); } - $key = $this->shell->run( - 'curl -fsSL https://enterprise.proxmox.com/debian/proxmox-release-bookworm.gpg '. - '-o /etc/apt/trusted.gpg.d/proxmox-release-bookworm.gpg' - ); - if (! $key->ok()) { - return StepResult::fail('Could not fetch the Proxmox release key.'); + [$id, $codename, $prettyName] = $this->readOsRelease(); + + if ($id !== 'debian' || ! isset(self::RELEASES[$codename])) { + return StepResult::fail( + 'Refusing to install Proxmox VE: this host does not run a Debian release CluPilot '. + 'knows a matching Proxmox suite for. /etc/os-release reports '. + 'ID='.($id === '' ? '(empty)' : $id).', '. + 'VERSION_CODENAME='.($codename === '' ? '(empty)' : $codename).', '. + 'PRETTY_NAME='.($prettyName === '' ? '(empty)' : '"'.$prettyName.'"').'. '. + 'Supported: '.implode(', ', array_keys(self::RELEASES)).'. '. + 'Reinstall the server on a supported Debian release, or add the release to '. + 'InstallProxmoxVe::RELEASES once Proxmox publishes a suite for it.' + ); } - $this->shell->putFile( - '/etc/apt/sources.list.d/pve-install-repo.list', - "deb [arch=amd64] http://download.proxmox.com/debian/pve bookworm pve-no-subscription\n" + $release = self::RELEASES[$codename]; + $keyringPath = self::KEYRING_DIR.'/'.$release['keyring']; + + $key = $this->shell->run( + 'install -d -m 0755 '.escapeshellarg(self::KEYRING_DIR).' && '. + 'curl -fsSL '.escapeshellarg($release['key_url']).' -o '.escapeshellarg($keyringPath) ); + if (! $key->ok()) { + return StepResult::fail( + 'Could not fetch the Proxmox release key for '.$codename.' from '.$release['key_url'].'.' + ); + } + + $this->shell->putFile(self::SOURCES_FILE, $this->renderSourcesFile($release['suite'], $keyringPath)); + // A host onboarded before this step used deb822 still carries the old + // one-line file. Left in place it would name the suite a second time, + // and a stale bookworm line on a trixie machine is precisely the + // mismatch this step now refuses to create. + $this->shell->run('rm -f '.escapeshellarg(self::LEGACY_SOURCES_FILE)); $this->shell->run('rm -f /etc/apt/sources.list.d/pve-enterprise.list'); if (! $this->shell->run('export DEBIAN_FRONTEND=noninteractive; apt-get update')->ok()) { @@ -61,4 +136,48 @@ class InstallProxmoxVe extends HostStep return StepResult::advance(); } + + /** + * ID, VERSION_CODENAME and PRETTY_NAME, in one read. + * + * Sourced rather than grepped so the shell does the unquoting for us, and + * printed with a separator that cannot appear in any of the three values. + * PRETTY_NAME is carried purely so the failure message above can quote what + * the machine actually says about itself — a Debian unstable image has no + * VERSION_CODENAME at all, and "codename=(empty)" on its own tells an + * operator nothing about which image they booted. + * + * @return array{0: string, 1: string, 2: string} + */ + private function readOsRelease(): array + { + $raw = $this->shell->run( + '. /etc/os-release 2>/dev/null; printf "%s|%s|%s\n" "$ID" "$VERSION_CODENAME" "$PRETTY_NAME"' + )->stdout; + + $parts = array_pad(explode('|', trim($raw), 3), 3, ''); + + return [strtolower(trim($parts[0])), strtolower(trim($parts[1])), trim($parts[2])]; + } + + /** + * deb822 rather than the one-line format, because `Signed-By` is the only + * way to scope the Proxmox key to the Proxmox repository, and because this + * is the shape upstream's own PVE 9 guide ships. apt has understood it + * since well before bookworm, so both supported releases read the same file. + */ + private function renderSourcesFile(string $suite, string $keyringPath): string + { + return implode("\n", [ + '# Written by CluPilot\'s InstallProxmoxVe provisioning step.', + '# The suite is derived from /etc/os-release on this host, not assumed.', + 'Types: deb', + 'URIs: http://download.proxmox.com/debian/pve', + 'Suites: '.$suite, + 'Components: pve-no-subscription', + 'Architectures: amd64', + 'Signed-By: '.$keyringPath, + '', + ]); + } } diff --git a/app/Provisioning/Steps/Host/RebootIntoPveKernel.php b/app/Provisioning/Steps/Host/RebootIntoPveKernel.php index ea5a7f7..b01992e 100644 --- a/app/Provisioning/Steps/Host/RebootIntoPveKernel.php +++ b/app/Provisioning/Steps/Host/RebootIntoPveKernel.php @@ -12,6 +12,12 @@ use Throwable; * Removes the Debian kernel and reboots into the Proxmox kernel. The reboot is * modelled as a retry loop: issue once, then poll until `uname -r` reports a * -pve kernel. The state machine survives the reboot by design. + * + * The reboot is the one irreversible thing this pipeline does. Everything else + * can be retried from CluPilot; a machine that does not come back needs the + * provider's out-of-band console, and on a dedicated server that can mean hours. + * So the boot path is PROVEN before the reboot is issued — never assumed from + * "apt did not complain". */ class RebootIntoPveKernel extends HostStep { @@ -35,8 +41,18 @@ class RebootIntoPveKernel extends HostStep if (! $run->context('reboot_issued')) { $this->keyLogin($this->shell, $host); + + // Tolerated, deliberately: an image that never had the + // linux-image-amd64 metapackage installed makes apt exit non-zero + // with nothing wrong. What matters is not whether the Debian kernel + // went away but whether a Proxmox one can boot, and that is checked + // below on its own terms rather than inferred from this line. $this->shell->run('export DEBIAN_FRONTEND=noninteractive; apt-get -y remove linux-image-amd64 || true'); - $this->shell->run('update-grub'); + + if (($refusal = $this->refuseUnbootableReboot()) !== null) { + return $refusal; + } + $run->mergeContext([ 'reboot_issued' => true, 'reboot_deadline' => now()->addMinutes(15)->toIso8601String(), @@ -69,4 +85,68 @@ class RebootIntoPveKernel extends HostStep return StepResult::poll(15, 'waiting for the Proxmox kernel'); } + + /** + * Everything that has to be true for this machine to come back, or a fail + * naming what is not. + * + * The three checks are the three ways the old code could reboot into + * nothing. It removed the Debian kernel with `|| true` and ignored + * update-grub's exit status, so a /boot that had filled up — common enough + * on a provider image with a small boot partition, and made likelier by the + * full-upgrade the previous step runs — produced a truncated initramfs, a + * half-written grub.cfg, and a reboot issued on the strength of neither. + * + * A fail rather than a retry: none of these three conditions fixes itself, + * and an operator who is already watching the onboarding can clear /boot or + * reinstall the kernel. Retrying would only spend the budget and then say + * something vaguer. + */ + private function refuseUnbootableReboot(): ?StepResult + { + // A kernel image is not enough on its own — grub needs the initramfs + // that goes with it, and a truncated or absent initrd is exactly what a + // full /boot leaves behind. Both are listed so the message can say which + // half is missing. + $kernels = trim($this->shell->run('ls -1 /boot/vmlinuz-*-pve 2>/dev/null')->stdout); + $initrds = trim($this->shell->run('ls -1 /boot/initrd.img-*-pve 2>/dev/null')->stdout); + + if ($kernels === '') { + return StepResult::fail( + 'Refusing to reboot: no Proxmox kernel image (/boot/vmlinuz-*-pve) is installed on this host, '. + 'so it would come back on the Debian kernel at best and not at all if that one was removed. '. + 'Check `apt-get -y install proxmox-ve` and the free space in /boot, then retry.' + ); + } + + if ($initrds === '') { + return StepResult::fail( + 'Refusing to reboot: a Proxmox kernel is installed ('.$this->firstLine($kernels).') but it has no '. + 'initramfs (/boot/initrd.img-*-pve), which is what a full /boot leaves behind. Free space in /boot, '. + 'run `update-initramfs -u -k all`, then retry.' + ); + } + + // Checked for its EXIT STATUS, which the old code discarded. update-grub + // failing is the difference between a boot menu that lists the new + // kernel and one that does not mention it. + $grub = $this->shell->run('update-grub'); + if (! $grub->ok()) { + return StepResult::fail( + 'Refusing to reboot: update-grub failed, so the boot menu may not list the Proxmox kernel '. + '('.$this->firstLine($kernels).'). Output: '.$this->firstLine(trim($grub->stderr.' '.$grub->stdout)).'. '. + 'Fix that on the host (usually free space in /boot), then retry.' + ); + } + + return null; + } + + /** Keeps a multi-line `ls` or a long apt error from swamping the run's error field. */ + private function firstLine(string $output): string + { + $line = trim((string) preg_split('/\R/', $output)[0]); + + return $line === '' ? '(no output)' : $line; + } } diff --git a/app/Provisioning/Steps/Host/SecureHostFirewall.php b/app/Provisioning/Steps/Host/SecureHostFirewall.php index 5caea8a..9cd897c 100644 --- a/app/Provisioning/Steps/Host/SecureHostFirewall.php +++ b/app/Provisioning/Steps/Host/SecureHostFirewall.php @@ -32,11 +32,18 @@ use App\Services\Ssh\RemoteShell; * one). Chosen over Proxmox's own firewall (pve-firewall needs the /etc/pve * cluster filesystem plus a separate datacenter+node enable flag — two more * places this could silently stay off) and over iptables-persistent (nftables - * is bookworm's default packet-filtering framework and already ships the + * is Debian's default packet-filtering framework and already ships the * boot-time unit the package needs). Only the INPUT chain is touched — FORWARD * and OUTPUT are left at the kernel default so customer VM traffic through * the bridges is never affected by a host-management rule. * + * This ruleset is the ONLY owner of the host's input policy. ConfigureProxmox + * enables Proxmox's firewall at datacenter level — guest rules do nothing + * without it — but explicitly turns the NODE firewall off for exactly this + * reason: with both active a packet must be accepted by both, and pve-firewall's + * auto-generated management ipset is seeded from the public subnet, which would + * silently overrule the WireGuard-only rule below. See that step's docblock. + * * Idempotent: once applied, the resource breadcrumb short-circuits a re-run * exactly like the other steps' external-resource guards. * @@ -123,9 +130,35 @@ table inet clupilot_filter { ct state invalid drop ct state established,related accept + # ICMPv6 is not optional. Neighbour discovery (135/136) and router + # advertisements arrive in the INPUT chain, so a bare policy drop takes + # IPv6 down on this host as soon as the neighbour cache expires — minutes, + # not days. packet-too-big is the other half: it is how the far end of a + # TLS connection learns the path MTU, and dropping it black-holes large + # transfers rather than failing them. Debian's own example ruleset accepts + # all of ipv6-icmp; the types are listed instead so the intent is readable + # and so the legacy information-leak requests stay out. One rule per line, + # deliberately: a ruleset that can be read a line at a time can also be + # asserted on a line at a time. + meta nfproto ipv6 icmpv6 type { destination-unreachable, packet-too-big, time-exceeded, parameter-problem, echo-request, echo-reply, nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert, mld-listener-query, mld-listener-report, mld-listener-done } accept + + # IPv4 ICMP, for the same reason: destination-unreachable carries + # fragmentation-needed (code 4), which is what IPv4 path-MTU discovery + # runs on. echo is here so the host answers a ping — including the hub's. + meta nfproto ipv4 icmp type { destination-unreachable, time-exceeded, parameter-problem, echo-request, echo-reply } accept + # Public web — Traefik on this host serves customer traffic here. tcp dport { 80, 443 } accept + # DHCP client replies. Some providers hand out the IPv4 address by DHCP, + # and a lease REBIND arrives as a fresh inbound packet that conntrack has + # no established entry for — dropped, the lease expires and the host loses + # the address it is reached on. Narrow on purpose: only a reply from a + # server port to the client port. IPv6 needs no equivalent here because + # addressing on these hosts comes from router advertisements, which the + # nd-router-advert accept above already covers. + udp sport 67 udp dport 68 accept + # Management surfaces: only reachable over the WireGuard tunnel. ip saddr {$wgSubnet} tcp dport { 22, 8006 } accept } diff --git a/app/Provisioning/Steps/Host/VerifyVmTemplate.php b/app/Provisioning/Steps/Host/VerifyVmTemplate.php new file mode 100644 index 0000000..4f41a99 --- /dev/null +++ b/app/Provisioning/Steps/Host/VerifyVmTemplate.php @@ -0,0 +1,106 @@ +host($run); + $required = $this->requiredTemplateVmids(); + + // A catalogue with nothing published yet requires no template. This is a + // real state — an installation being set up for the first time onboards + // its host before it publishes its plans — and failing here would be + // inventing a requirement nobody has stated. + if ($required === []) { + return StepResult::advance(); + } + + $client = $this->pve->forHost($host); + + try { + $nodes = $client->listNodes(); + $node = $nodes[0]['node'] ?? 'pve'; + + $missing = array_values(array_filter( + $required, + fn (int $vmid) => ! $client->vmExists($node, $vmid), + )); + } catch (Throwable $e) { + // The API was answering one step ago; a hiccup here is transient. + return StepResult::retry(15, 'could not ask Proxmox about the VM templates: '.$e->getMessage()); + } + + if ($missing !== []) { + return StepResult::fail( + 'This host has no VM template to clone from, so it could not serve a single order. Missing on node '. + $node.': VMID '.implode(', ', $missing).', which '. + (count($missing) === 1 ? 'a published plan version points' : 'published plan versions point').' at. '. + 'Copy the golden template onto this host (or restore it from another node) and retry — otherwise the '. + 'first paid order placed here fails in clone_vm, after the customer has paid.' + ); + } + + return StepResult::advance(); + } + + /** + * Every template a currently sellable plan version points at. + * + * Published versions whose window has not closed, rather than + * PlanVersion::available() — a version scheduled to launch next week is not + * "available" yet and its template gap would then surface at its first order + * instead of here, which is the whole failure this step removes. A closed + * window is different: nobody can buy it any more, so its template is not + * this host's problem. + * + * @return array + */ + private function requiredTemplateVmids(): array + { + return PlanVersion::query() + ->whereNotNull('published_at') + ->whereNotNull('template_vmid') + ->where(fn ($q) => $q->whereNull('available_until')->orWhere('available_until', '>', now())) + ->pluck('template_vmid') + ->map(fn ($vmid) => (int) $vmid) + ->unique() + ->sort() + ->values() + ->all(); + } +} diff --git a/app/Services/Ssh/FakeRemoteShell.php b/app/Services/Ssh/FakeRemoteShell.php index 88f932c..4c3b795 100644 --- a/app/Services/Ssh/FakeRemoteShell.php +++ b/app/Services/Ssh/FakeRemoteShell.php @@ -74,6 +74,14 @@ class FakeRemoteShell implements RemoteShell return $this->default; } + /** + * Observable through files(), NOT through ran(): an uploaded file is not a + * command and is deliberately not recorded as one. Worth stating, because a + * test once asserted `ran('/usr/local/sbin/clupilot-…-firewall.sh')` believing + * it proved the script had been deployed — the only thing that substring + * matched was the `chmod 700` on the line above, so the same fact was + * asserted twice and the script's contents were never checked at all. + */ public function putFile(string $remotePath, string $contents): void { $this->files[$remotePath] = $contents; diff --git a/config/provisioning.php b/config/provisioning.php index bb6776c..dd8fd9c 100644 --- a/config/provisioning.php +++ b/config/provisioning.php @@ -39,6 +39,10 @@ return [ Host\ConfigureProxmox::class, Host\CreateAutomationToken::class, Host\VerifyProxmoxApi::class, + // Before the host is ever offered to a customer: a node with no + // template to clone from cannot serve one, and finding that out in + // CloneVirtualMachine means finding it out after somebody paid. + Host\VerifyVmTemplate::class, Host\RegisterHostDns::class, Host\RegisterCapacity::class, Host\SecureHostFirewall::class, @@ -360,7 +364,23 @@ return [ // Proxmox automation role/user created on each host. 'proxmox' => [ 'role_id' => 'CluPilotAutomation', - 'role_privs' => 'VM.Allocate,VM.Clone,VM.Config.Disk,VM.Config.CPU,VM.Config.Memory,VM.Config.Network,VM.Config.Options,VM.Config.Cloudinit,VM.PowerMgmt,VM.Monitor,VM.Audit,VM.Backup,VM.GuestAgent.Audit,VM.GuestAgent.Unrestricted,Datastore.AllocateSpace,Datastore.Audit,Sys.Audit', + /* + | Sys.Modify is not optional, and its absence broke every customer run. + | + | HttpProxmoxClient::createBackupJob() POSTs /cluster/backup, and that + | endpoint declares `check => ['perm', '/', ['Sys.Modify']]` in + | pve-manager's PVE/API2/Backup.pm — verified against the source, not + | inferred from a forum post. Without it the POST comes back 403, + | RegisterBackup rethrows anything whose body does not contain "already", + | and the run failed at register_backup for every single customer. + | + | Sys.Modify is also what PUT /cluster/firewall/options requires + | (pve-firewall's PVE/API2/Firewall/Cluster.pm), so the same privilege + | covers the datacenter firewall if that ever moves off SSH and onto the + | API. It is granted on `/` because both endpoints check `/` and nothing + | narrower would satisfy them. + */ + 'role_privs' => 'VM.Allocate,VM.Clone,VM.Config.Disk,VM.Config.CPU,VM.Config.Memory,VM.Config.Network,VM.Config.Options,VM.Config.Cloudinit,VM.PowerMgmt,VM.Monitor,VM.Audit,VM.Backup,VM.GuestAgent.Audit,VM.GuestAgent.Unrestricted,Datastore.AllocateSpace,Datastore.Audit,Sys.Audit,Sys.Modify', 'user' => 'automation@pve', 'token_name' => 'clupilot', ], diff --git a/database/migrations/2026_07_31_160000_move_host_api_tokens_onto_the_secrets_key.php b/database/migrations/2026_07_31_160000_move_host_api_tokens_onto_the_secrets_key.php new file mode 100644 index 0000000..853f01d --- /dev/null +++ b/database/migrations/2026_07_31_160000_move_host_api_tokens_onto_the_secrets_key.php @@ -0,0 +1,114 @@ +rewrite( + reader: fn (string $cipher) => Crypt::decryptString($cipher), + writer: fn (string $plain) => app(SecretCipher::class)->encrypt($plain), + alreadyDone: fn (string $cipher) => app(SecretCipher::class)->decrypt($cipher), + direction: 'APP_KEY -> SECRETS_KEY', + ); + } + + public function down(): void + { + $this->rewrite( + reader: fn (string $cipher) => app(SecretCipher::class)->decrypt($cipher), + writer: fn (string $plain) => Crypt::encryptString($plain), + alreadyDone: fn (string $cipher) => Crypt::decryptString($cipher), + direction: 'SECRETS_KEY -> APP_KEY', + ); + } + + /** + * One pass over the table, re-encrypting what it can read. + * + * `alreadyDone` is what makes this safe to run twice, and safe to run on a + * database somebody has already half-migrated: both keys produce ordinary + * Laravel encryption payloads, so the only way to tell them apart is which + * key validates the MAC. A row the TARGET key already opens is not touched + * and not counted as a failure. + * + * @param callable(string): string $reader + * @param callable(string): string $writer + * @param callable(string): string $alreadyDone + */ + private function rewrite(callable $reader, callable $writer, callable $alreadyDone, string $direction): void + { + if (! app(SecretCipher::class)->isUsable()) { + // Not a failure of the migration: a fresh install has no SECRETS_KEY + // yet and no hosts either. Throwing here would block the whole + // migration run on an installation that has nothing to migrate. + $pending = DB::table('hosts')->whereNotNull('api_token_ref')->count(); + + if ($pending > 0) { + Log::warning( + 'SECRETS_KEY is not usable, so '.$pending.' host API token(s) were left encrypted under the '. + 'old key. Set SECRETS_KEY and re-run this migration ('.$direction.') before rotating APP_KEY.' + ); + } + + return; + } + + $left = []; + + foreach (DB::table('hosts')->whereNotNull('api_token_ref')->get(['id', 'api_token_ref']) as $row) { + $cipher = (string) $row->api_token_ref; + + if ($cipher === '') { + continue; + } + + try { + $plain = $reader($cipher); + } catch (Throwable) { + try { + // Already in the target shape: nothing to do, nothing wrong. + $alreadyDone($cipher); + + continue; + } catch (Throwable) { + $left[] = $row->id; + + continue; + } + } + + DB::table('hosts')->where('id', $row->id)->update(['api_token_ref' => $writer($plain)]); + } + + if ($left !== []) { + Log::warning( + 'Left '.count($left).' host API token(s) untouched ('.$direction.'): neither key could read them. '. + 'Host ids: '.implode(', ', $left).'. Re-mint the token on those hosts, or restore the APP_KEY they '. + 'were written under — the stored value has NOT been overwritten.' + ); + } + } +}; diff --git a/lang/de/hosts.php b/lang/de/hosts.php index 4aa6984..c91b8f3 100644 --- a/lang/de/hosts.php +++ b/lang/de/hosts.php @@ -128,6 +128,8 @@ return [ 'configure_proxmox' => 'Proxmox konfigurieren', 'create_automation_token' => 'Automation-Token erstellen', 'verify_proxmox_api' => 'Proxmox-API prüfen', + 'verify_vm_template' => 'VM-Vorlage prüfen', + 'register_host_dns' => 'Internen DNS-Namen registrieren', 'register_capacity' => 'Kapazität registrieren', 'secure_host_firewall' => 'Host-Firewall absichern', 'complete_host_onboarding' => 'Onboarding abschließen', diff --git a/lang/en/hosts.php b/lang/en/hosts.php index ba4d75b..0527c9e 100644 --- a/lang/en/hosts.php +++ b/lang/en/hosts.php @@ -128,6 +128,8 @@ return [ 'configure_proxmox' => 'Configure Proxmox', 'create_automation_token' => 'Create automation token', 'verify_proxmox_api' => 'Verify Proxmox API', + 'verify_vm_template' => 'Verify VM template', + 'register_host_dns' => 'Register internal DNS name', 'register_capacity' => 'Register capacity', 'secure_host_firewall' => 'Secure host firewall', 'complete_host_onboarding' => 'Complete onboarding', diff --git a/tests/Feature/Provisioning/DataModelTest.php b/tests/Feature/Provisioning/DataModelTest.php index 140a2c9..92a7bf1 100644 --- a/tests/Feature/Provisioning/DataModelTest.php +++ b/tests/Feature/Provisioning/DataModelTest.php @@ -3,6 +3,9 @@ use App\Models\Host; use App\Models\ProvisioningRun; use App\Models\RunResource; +use App\Services\Secrets\SecretCipher; +use Illuminate\Contracts\Encryption\DecryptException; +use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; @@ -13,13 +16,34 @@ it('auto-assigns a uuid and routes by it', function () { ->and($host->getRouteKeyName())->toBe('uuid'); }); -it('encrypts api_token_ref at rest but reads it back in clear', function () { +it('encrypts api_token_ref under SECRETS_KEY, not APP_KEY', function () { + // The column used Laravel's `encrypted` cast, i.e. APP_KEY — against + // SecretVault's own written rule, which exists because rotating APP_KEY is + // ordinary maintenance. One rotation would have made every host's Proxmox + // token undecryptable in the same moment, discovered when provisioning + // stopped rather than when the key changed. + config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + $host = Host::factory()->create(['api_token_ref' => 'automation@pve!clupilot=s3cr3t']); - $raw = DB::table('hosts')->where('id', $host->id)->value('api_token_ref'); + $raw = (string) DB::table('hosts')->where('id', $host->id)->value('api_token_ref'); expect($raw)->not->toContain('s3cr3t') - ->and($host->fresh()->api_token_ref)->toBe('automation@pve!clupilot=s3cr3t'); + ->and($host->fresh()->api_token_ref)->toBe('automation@pve!clupilot=s3cr3t') + // The mutation that matters: APP_KEY must NOT open it. + ->and(fn () => Crypt::decryptString($raw))->toThrow(DecryptException::class) + ->and(app(SecretCipher::class)->decrypt($raw))->toBe('automation@pve!clupilot=s3cr3t'); +}); + +it('leaves a host without a token alone rather than needing a key for it', function () { + // Most hosts have no token for most of onboarding, and a model that reached + // for SECRETS_KEY to read a null column would make an unconfigured + // installation unable to so much as list its hosts. + config()->set('admin_access.secrets_key', ''); + + $host = Host::factory()->create(); + + expect($host->fresh()->api_token_ref)->toBeNull(); }); it('casts run context to an array and resolves the polymorphic subject', function () { diff --git a/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php b/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php index 996cae6..7c87b5b 100644 --- a/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php +++ b/tests/Feature/Provisioning/HostOnboardingEndToEndTest.php @@ -8,6 +8,17 @@ use App\Provisioning\Steps\Host\ConfigureWireguard; use App\Services\Ssh\CommandResult; use Illuminate\Support\Facades\Queue; +/* +| Both tests below drive the state machine against a fake whose default result is +| success, so what they prove is SEQUENCING — that the steps run in order, that +| the run reaches `completed`, and that external resources are created exactly +| once across a crash. They deliberately do not stand in for the per-step +| assertions in HostStepsTest: no command's content is checked here, and a step +| that stopped doing its work while still advancing would pass both. +*/ + +beforeEach(fn () => config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32)))); + it('drives a fresh host all the way to active (mocked)', function () { Queue::fake(); // drive the runner manually, one step per advance $s = fakeServices(); @@ -16,6 +27,16 @@ it('drives a fresh host all the way to active (mocked)', function () { $s['shell']->script('pveum user token add', CommandResult::success( json_encode(['full-tokenid' => 'automation@pve!clupilot', 'value' => 'tok-secret-123']) )); + // What a healthy host answers to the checks the pipeline now actually makes: + // a Debian release Proxmox publishes a suite for, a /boot it can reboot into, + // a datacenter firewall that reads back as enabled, and a golden template to + // clone from. None of these is scenery — every one of them is a step that + // fails loudly rather than advancing on the fake's default success. + $s['shell']->script('/etc/os-release', CommandResult::success("debian|trixie|Debian GNU/Linux 13 (trixie)\n")); + $s['shell']->script('ls -1 /boot/vmlinuz-*-pve', CommandResult::success("/boot/vmlinuz-6.8.12-4-pve\n")); + $s['shell']->script('ls -1 /boot/initrd.img-*-pve', CommandResult::success("/boot/initrd.img-6.8.12-4-pve\n")); + $s['shell']->script('pvesh get /cluster/firewall/options', CommandResult::success('{"enable":1}')); + $s['pve']->clonedVmids[] = 9000; // the template the seeded catalogue sells $host = app(StartHostOnboarding::class)->run([ 'name' => 'pve-fsn-9', @@ -69,6 +90,16 @@ it('does not duplicate external resources when a step re-runs after a crash', fu $s['shell']->script('pveum user token add', CommandResult::success( json_encode(['full-tokenid' => 'automation@pve!clupilot', 'value' => 'tok-secret-123']) )); + // What a healthy host answers to the checks the pipeline now actually makes: + // a Debian release Proxmox publishes a suite for, a /boot it can reboot into, + // a datacenter firewall that reads back as enabled, and a golden template to + // clone from. None of these is scenery — every one of them is a step that + // fails loudly rather than advancing on the fake's default success. + $s['shell']->script('/etc/os-release', CommandResult::success("debian|trixie|Debian GNU/Linux 13 (trixie)\n")); + $s['shell']->script('ls -1 /boot/vmlinuz-*-pve', CommandResult::success("/boot/vmlinuz-6.8.12-4-pve\n")); + $s['shell']->script('ls -1 /boot/initrd.img-*-pve', CommandResult::success("/boot/initrd.img-6.8.12-4-pve\n")); + $s['shell']->script('pvesh get /cluster/firewall/options', CommandResult::success('{"enable":1}')); + $s['pve']->clonedVmids[] = 9000; // the template the seeded catalogue sells $host = app(StartHostOnboarding::class)->run([ 'name' => 'pve-fsn-10', diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php index 0c6265d..7207806 100644 --- a/tests/Feature/Provisioning/HostStepsTest.php +++ b/tests/Feature/Provisioning/HostStepsTest.php @@ -3,6 +3,7 @@ use App\Models\Datacenter; use App\Models\Host; use App\Models\Operator; +use App\Models\PlanVersion; use App\Models\ProvisioningRun; use App\Models\RunResource; use App\Provisioning\Jobs\PurgeHost; @@ -19,8 +20,10 @@ use App\Provisioning\Steps\Host\RegisterHostDns; use App\Provisioning\Steps\Host\SecureHostFirewall; use App\Provisioning\Steps\Host\ValidateHostInput; use App\Provisioning\Steps\Host\VerifyProxmoxApi; +use App\Provisioning\Steps\Host\VerifyVmTemplate; use App\Services\Secrets\SecretVault; use App\Services\Ssh\CommandResult; +use App\Services\Ssh\FakeRemoteShell; use Illuminate\Support\Facades\Crypt; function hostRun(Host $host, array $context = []): ProvisioningRun @@ -28,6 +31,92 @@ function hostRun(Host $host, array $context = []): ProvisioningRun return ProvisioningRun::factory()->forHost($host)->create(['context' => $context]); } +/** + * The `wg_peer` breadcrumb ConfigureWireguard records after a handshake actually + * succeeds — the one fact that means "SSH may use the tunnel". + * + * Every step from ConfigureWireguard onwards runs with this present in real life, + * so a test that exercises one of them without it is testing the pre-tunnel + * fallback path by accident. See HostStep::keyLogin(). + */ +function proveTunnel(ProvisioningRun $run, Host $host): void +{ + RunResource::create([ + 'run_id' => $run->id, + 'host_id' => $host->id, + 'kind' => 'wg_peer', + 'external_id' => 'HOSTPUB=', + ]); +} + +/** + * What ConfigureProxmox needs from a healthy host: a vmbr0 bridge, and a + * datacenter firewall that reads back as enabled. The fake's default-success + * would otherwise report "pvesh set worked" and then hand back an empty options + * document, which the step correctly refuses to believe. + */ +function scriptHealthyProxmoxHost(FakeRemoteShell $shell): void +{ + $shell->script('pvesh get /cluster/firewall/options', CommandResult::success('{"enable":1,"policy_in":"ACCEPT"}')); +} + +/** A /boot RebootIntoPveKernel is willing to reboot into. */ +function scriptBootableProxmoxKernel(FakeRemoteShell $shell): void +{ + $shell->script('ls -1 /boot/vmlinuz-*-pve', CommandResult::success("/boot/vmlinuz-6.8.12-4-pve\n")); + $shell->script('ls -1 /boot/initrd.img-*-pve', CommandResult::success("/boot/initrd.img-6.8.12-4-pve\n")); +} + +/** A Debian release InstallProxmoxVe knows a Proxmox suite for. */ +function scriptDebianRelease(FakeRemoteShell $shell, string $id, string $codename, string $prettyName): void +{ + $shell->script('/etc/os-release', CommandResult::success($id.'|'.$codename.'|'.$prettyName."\n")); +} + +/** + * Every ACCEPT rule in an nftables ruleset, one per entry, comments stripped. + * + * Reading the ruleset as a LIST rather than searching it for expected lines is + * what lets a test catch a rule somebody ADDED — which is the mutation the old + * firewall tests missed: they asserted the WireGuard-scoped accept was present + * and said nothing about what else might be. + * + * @return array + */ +function nftAcceptRules(string $config): array +{ + $rules = []; + + foreach (preg_split('/\R/', $config) ?: [] as $line) { + $line = trim((string) preg_replace('/\s+/', ' ', $line)); + + if ($line === '' || str_starts_with($line, '#') || ! str_contains($line, 'accept')) { + continue; + } + + $rules[] = $line; + } + + return $rules; +} + +/** + * The destination ports an nftables ACCEPT rule opens — both spellings, a set + * (`dport { 80, 443 }`) and a bare port (`dport 68`). + * + * @return array + */ +function nftAcceptedPorts(string $rule): array +{ + if (! preg_match('/dport (?:\{([^}]*)\}|(\d+))/', $rule, $m)) { + return []; + } + + $list = trim($m[1] ?? '') !== '' ? $m[1] : ($m[2] ?? ''); + + return array_map('intval', array_filter(array_map('trim', explode(',', $list)), fn ($p) => $p !== '')); +} + // --- ValidateHostInput --- it('advances on valid host input and marks the host onboarding', function () { @@ -78,15 +167,18 @@ it('throws when the host is unreachable (runner turns it into a retry)', functio // --- HostStep::keyLogin address preference --- // // Mutation boundary: every step after ConfigureWireguard (and every later -// maintenance run) MUST reach the host over the tunnel once wg_ip is set — -// closing SSH to the world without this would strand the host permanently. -// ConfigureProxmox is used as a plain probe: keyLogin then two commands, no -// other branching that could obscure which address was actually dialled. +// maintenance run) MUST reach the host over the tunnel once the tunnel is +// PROVEN — closing SSH to the world without this would strand the host +// permanently. And it must NOT use the tunnel before then, which is the other +// half and the one that was broken. ConfigureProxmox is used as a plain probe: +// keyLogin then a handful of commands, no branching on the address. -it('uses the WireGuard address for ssh once the tunnel exists', function () { +it('uses the WireGuard address for ssh once the tunnel is proven', function () { $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); $host = Host::factory()->create(['public_ip' => '203.0.113.20', 'wg_ip' => '10.66.0.20']); $run = hostRun($host); + proveTunnel($run, $host); app(ConfigureProxmox::class)->execute($run); @@ -99,6 +191,7 @@ it('uses the WireGuard address for ssh once the tunnel exists', function () { it('falls back to the public IP before the tunnel exists', function () { $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); $host = Host::factory()->create(['public_ip' => '203.0.113.21', 'wg_ip' => null]); $run = hostRun($host); @@ -111,6 +204,47 @@ it('falls back to the public IP before the tunnel exists', function () { } }); +it('never dials the tunnel for ssh while it is only allocated, not proven', function () { + // The dead end this closes: wg_ip is persisted inside the allocation lock + // BEFORE the handshake is ever verified, so a run that failed at the + // handshake left a host whose row said "10.66.0.x" and whose tunnel did not + // work. Every later attempt — including an operator pressing Retry — + // connected to that address and died before reaching the only code that + // could have repaired wg0.conf. Recovery meant nulling hosts.wg_ip by hand. + $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); + $host = Host::factory()->create(['public_ip' => '203.0.113.22', 'wg_ip' => '10.66.0.22']); + $run = hostRun($host); + + // No wg_peer breadcrumb: an address was reserved, nothing handshaked. + app(ConfigureProxmox::class)->execute($run); + + $connections = $s['shell']->connectionsWith('key'); + expect($connections)->not->toBeEmpty(); + foreach ($connections as $connection) { + expect($connection[1])->toBe('203.0.113.22') + ->and($connection[1])->not->toBe('10.66.0.22'); + } +}); + +it('keeps using the tunnel on a maintenance run started long after onboarding', function () { + // tunnelProven() is HOST-scoped, not run-scoped, on purpose: a later run has + // no wg_peer breadcrumb of its own, and reading only its own run would send + // it to a public IP where SecureHostFirewall has since closed port 22. + $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); + $host = Host::factory()->create(['public_ip' => '203.0.113.23', 'wg_ip' => '10.66.0.23']); + $onboarding = hostRun($host); + proveTunnel($onboarding, $host); + + $later = hostRun($host); + app(ConfigureProxmox::class)->execute($later); + + foreach ($s['shell']->connectionsWith('key') as $connection) { + expect($connection[1])->toBe('10.66.0.23'); + } +}); + it('prefers the vault-stored SSH key over CLUPILOT_SSH_PRIVATE_KEY', function () { // Mutation boundary (Part B): the SSH identity is the most sensitive // value in the system — it opens every host CluPilot manages. Before @@ -122,6 +256,7 @@ it('prefers the vault-stored SSH key over CLUPILOT_SSH_PRIVATE_KEY', function () app(SecretVault::class)->put('ssh.private_key', 'vault-key-should-be-used', Operator::factory()->create()); $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); $run = hostRun(Host::factory()->create()); app(ConfigureProxmox::class)->execute($run); @@ -179,6 +314,103 @@ it('retries wireguard while the handshake is not up', function () { expect(app(ConfigureWireguard::class)->execute($run)->type)->toBe('retry'); }); +it('refuses to write wg0.conf without a hub public key, before touching anything', function () { + // renderConfig() used to emit `PublicKey = ` quite happily, which is the + // likeliest way to reach a tunnel that can never handshake in the first + // place. Nothing may be installed, allocated or written on the way to + // finding out — that is what makes it a clean report rather than another + // half-configured host. + $s = fakeServices(); + $s['hub']->publicKeyValue = ''; + $host = Host::factory()->create(); + $run = hostRun($host); + + $result = app(ConfigureWireguard::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('CLUPILOT_WG_HUB_PUBKEY') + ->and($s['shell']->ran('apt-get install -y wireguard'))->toBeFalse() + ->and($s['shell']->files())->not->toHaveKey('/etc/wireguard/wg0.conf') + ->and($host->fresh()->wg_ip)->toBeNull(); +}); + +it('refuses to write wg0.conf without a hub endpoint', function () { + $s = fakeServices(); + $s['hub']->endpointValue = ''; + $run = hostRun(Host::factory()->create()); + + $result = app(ConfigureWireguard::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('CLUPILOT_WG_ENDPOINT'); +}); + +it('enables wg-quick@wg0 so the tunnel comes back after the reboot', function () { + // The line this replaces was `systemctl enable --now wg-quick@wg0 || + // wg-quick up wg0`. Its fallback brought the interface up WITHOUT the + // systemd enablement, so the tunnel did not return after the step-6 reboot — + // and step 6 is the reboot this same pipeline performs two steps later. + $s = fakeServices(); + $s['shell']->script('wg pubkey', CommandResult::success('HOSTPUB=')); + $s['shell']->script('ip link show wg0', CommandResult::failure(1)); // fresh host + $s['shell']->script('systemctl is-enabled', CommandResult::failure(1)); // never enabled + $run = hostRun(Host::factory()->create()); + + expect(app(ConfigureWireguard::class)->execute($run)->type)->toBe('advance') + ->and($s['shell']->ran('systemctl enable wg-quick@wg0'))->toBeTrue() + ->and($s['shell']->ran('systemctl start wg-quick@wg0'))->toBeTrue() + // The old fallback: an interface up outside systemd, lost on reboot. + ->and($s['shell']->ran('wg-quick up wg0'))->toBeFalse(); +}); + +it('does not fight an interface that is already up and correct', function () { + // Both halves of the old line failed with "wg0 already exists" on the second + // attempt, so the step retried forever against a tunnel that was working. + $s = fakeServices(); + $s['shell']->script('wg pubkey', CommandResult::success('HOSTPUB=')); + $s['shell']->script('ip link show wg0', CommandResult::success('3: wg0: ')); + $s['shell']->script('systemctl is-enabled', CommandResult::success('enabled')); + $host = Host::factory()->create(); + $run = hostRun($host); + + // First pass writes the config, on a shell we then throw away. + app(ConfigureWireguard::class)->execute($run); + $written = $s['shell']->files()['/etc/wireguard/wg0.conf']; + + // Attempt 2 against exactly that file, on a fresh shell so `recorded` only + // holds this attempt. The breadcrumb is rewound so the step really re-runs + // its body instead of short-circuiting — the case that used to retry forever. + RunResource::where('run_id', $run->id)->where('kind', 'wg_peer')->delete(); + $again = fakeServices(); + $again['shell']->script('wg pubkey', CommandResult::success('HOSTPUB=')); + $again['shell']->script('ip link show wg0', CommandResult::success('3: wg0: ')); + $again['shell']->script('systemctl is-enabled', CommandResult::success('enabled')); + $again['shell']->script("cat '/etc/wireguard/wg0.conf'", CommandResult::success($written)); + + expect(app(ConfigureWireguard::class)->execute($run->fresh())->type)->toBe('advance') + ->and($again['shell']->ran('systemctl restart wg-quick@wg0'))->toBeFalse() + // Nor is the file rewritten for nothing. + ->and($again['shell']->files())->not->toHaveKey('/etc/wireguard/wg0.conf'); +}); + +it('actually applies a corrected hub key instead of only writing the file', function () { + // An operator who fixes a wrong hub key and retries used to see the new file + // on disk and no change in behaviour: nothing reloaded the interface, so the + // running tunnel kept the old peer. + $s = fakeServices(); + $s['shell']->script('wg pubkey', CommandResult::success('HOSTPUB=')); + $s['shell']->script('ip link show wg0', CommandResult::success('3: wg0: ')); + $s['shell']->script('systemctl is-enabled', CommandResult::success('enabled')); + $s['shell']->script('cat \'/etc/wireguard/wg0.conf\'', CommandResult::success( + "[Interface]\nAddress = 10.66.0.9/24\n\n[Peer]\nPublicKey = THEWRONGKEY=\n" + )); + $run = hostRun(Host::factory()->create()); + + expect(app(ConfigureWireguard::class)->execute($run)->type)->toBe('advance') + ->and($s['shell']->files()['/etc/wireguard/wg0.conf'])->toContain($s['hub']->publicKeyValue) + ->and($s['shell']->ran('systemctl restart wg-quick@wg0'))->toBeTrue(); +}); + // --- InstallProxmoxVe --- it('skips the install when proxmox-ve is already present', function () { @@ -190,19 +422,93 @@ it('skips the install when proxmox-ve is already present', function () { ->and($s['shell']->ran('apt-get -y install proxmox-ve'))->toBeFalse(); }); -it('installs proxmox-ve when it is absent', function () { +it('installs proxmox-ve against the suite the host actually runs', function () { + // A server ordered today boots Debian 13 trixie. The step used to hard-code + // bookworm, i.e. add the PVE 8 repo and keyring to a trixie base — a + // hypervisor built against a different libc and kernel. $s = fakeServices(); $s['shell']->script('dpkg -l proxmox-ve', CommandResult::failure(1)); + scriptDebianRelease($s['shell'], 'debian', 'trixie', 'Debian GNU/Linux 13 (trixie)'); $run = hostRun(Host::factory()->create()); expect(app(InstallProxmoxVe::class)->execute($run)->type)->toBe('advance') ->and($s['shell']->ran('apt-get -y install proxmox-ve'))->toBeTrue(); + + $sources = $s['shell']->files()['/etc/apt/sources.list.d/pve-install-repo.sources']; + expect($sources)->toContain('Suites: trixie') + ->and($sources)->not->toContain('bookworm') + // Scoped to this repository via Signed-By, not trusted for every repo on + // the machine the way /etc/apt/trusted.gpg.d would be. + ->and($sources)->toContain('Signed-By: /usr/share/keyrings/proxmox-archive-keyring.gpg') + ->and($s['shell']->ran('proxmox-archive-keyring-trixie.gpg'))->toBeTrue() + ->and($s['shell']->ran('/etc/apt/trusted.gpg.d/'))->toBeFalse(); +}); + +it('installs the bookworm suite on a bookworm host', function () { + $s = fakeServices(); + $s['shell']->script('dpkg -l proxmox-ve', CommandResult::failure(1)); + scriptDebianRelease($s['shell'], 'debian', 'bookworm', 'Debian GNU/Linux 12 (bookworm)'); + $run = hostRun(Host::factory()->create()); + + expect(app(InstallProxmoxVe::class)->execute($run)->type)->toBe('advance'); + + $sources = $s['shell']->files()['/etc/apt/sources.list.d/pve-install-repo.sources']; + expect($sources)->toContain('Suites: bookworm') + ->and($sources)->not->toContain('trixie') + ->and($sources)->toContain('Signed-By: /usr/share/keyrings/proxmox-release-bookworm.gpg') + ->and($s['shell']->ran('proxmox-release-bookworm.gpg'))->toBeTrue(); +}); + +it('refuses an unknown release by name rather than guessing a suite', function () { + $s = fakeServices(); + $s['shell']->script('dpkg -l proxmox-ve', CommandResult::failure(1)); + scriptDebianRelease($s['shell'], 'debian', 'forky', 'Debian GNU/Linux 14 (forky)'); + $run = hostRun(Host::factory()->create()); + + $result = app(InstallProxmoxVe::class)->execute($run); + + expect($result->type)->toBe('fail') + // Names what it found, so an operator does not have to go and look. + ->and($result->reason)->toContain('forky') + ->and($result->reason)->toContain('Debian GNU/Linux 14 (forky)') + // And nothing was installed on the guess that newest-is-closest. + ->and($s['shell']->ran('apt-get -y install proxmox-ve'))->toBeFalse() + ->and($s['shell']->files())->not->toHaveKey('/etc/apt/sources.list.d/pve-install-repo.sources'); +}); + +it('refuses a base system that is not Debian at all', function () { + $s = fakeServices(); + $s['shell']->script('dpkg -l proxmox-ve', CommandResult::failure(1)); + scriptDebianRelease($s['shell'], 'ubuntu', 'noble', 'Ubuntu 24.04.1 LTS'); + $run = hostRun(Host::factory()->create()); + + $result = app(InstallProxmoxVe::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('ubuntu') + ->and($s['shell']->ran('apt-get -y install proxmox-ve'))->toBeFalse(); +}); + +it('refuses an image whose os-release names no codename', function () { + // Debian unstable has no VERSION_CODENAME, so this is the branch that must + // not read "" as "close enough to the newest one I know". + $s = fakeServices(); + $s['shell']->script('dpkg -l proxmox-ve', CommandResult::failure(1)); + scriptDebianRelease($s['shell'], 'debian', '', 'Debian GNU/Linux trixie/sid'); + $run = hostRun(Host::factory()->create()); + + $result = app(InstallProxmoxVe::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('(empty)') + ->and($result->reason)->toContain('trixie/sid'); }); // --- RebootIntoPveKernel --- it('issues the reboot on first execution', function () { $s = fakeServices(); + scriptBootableProxmoxKernel($s['shell']); $run = hostRun(Host::factory()->create()); expect(app(RebootIntoPveKernel::class)->execute($run)->type)->toBe('poll') @@ -210,6 +516,51 @@ it('issues the reboot on first execution', function () { ->and($s['shell']->ran('reboot'))->toBeTrue(); }); +it('refuses to reboot a host with no Proxmox kernel installed', function () { + // Mutation boundary: the reboot is the one irreversible thing this pipeline + // does. `apt-get remove linux-image-amd64 || true` followed by an unchecked + // update-grub could leave a machine that only the provider's console can + // reach — so nothing may be issued on a guess. + $s = fakeServices(); + $s['shell']->script('ls -1 /boot/vmlinuz-*-pve', CommandResult::failure(2)); + $run = hostRun(Host::factory()->create()); + + $result = app(RebootIntoPveKernel::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('vmlinuz-*-pve') + ->and($s['shell']->ran('reboot'))->toBeFalse() + ->and($run->fresh()->context('reboot_issued'))->toBeNull(); +}); + +it('refuses to reboot when the Proxmox kernel has no initramfs', function () { + // What a /boot that filled up during the full-upgrade actually leaves behind. + $s = fakeServices(); + $s['shell']->script('ls -1 /boot/vmlinuz-*-pve', CommandResult::success("/boot/vmlinuz-6.8.12-4-pve\n")); + $s['shell']->script('ls -1 /boot/initrd.img-*-pve', CommandResult::failure(2)); + $run = hostRun(Host::factory()->create()); + + $result = app(RebootIntoPveKernel::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('initrd.img-*-pve') + ->and($s['shell']->ran('reboot'))->toBeFalse(); +}); + +it('refuses to reboot when update-grub failed', function () { + $s = fakeServices(); + scriptBootableProxmoxKernel($s['shell']); + $s['shell']->script('update-grub', CommandResult::failure(1, 'cannot write /boot/grub/grub.cfg: No space left')); + $run = hostRun(Host::factory()->create()); + + $result = app(RebootIntoPveKernel::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('update-grub') + ->and($result->reason)->toContain('No space left') + ->and($s['shell']->ran('reboot'))->toBeFalse(); +}); + it('advances once the proxmox kernel is up', function () { $s = fakeServices(); $s['shell']->script('uname -r', CommandResult::success('6.8.12-4-pve')); @@ -247,6 +598,7 @@ it('fails when the host does not return before the reboot deadline', function () // --- CreateAutomationToken --- it('creates and persists the automation token via pveum exactly once', function () { + config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); $s = fakeServices(); $s['shell']->script('pveum user token add', CommandResult::success( json_encode(['full-tokenid' => 'automation@pve!clupilot', 'value' => 'tok-secret-123']) @@ -268,6 +620,183 @@ it('creates and persists the automation token via pveum exactly once', function ->and(RunResource::where('run_id', $run->id)->where('kind', 'pve_token')->count())->toBe(1); }); +it('grants Sys.Modify, which every customer backup job needs', function () { + // POST /cluster/backup checks ['perm', '/', ['Sys.Modify']]. Without it the + // call comes back 403, RegisterBackup rethrows anything whose body does not + // contain "already", and EVERY customer provisioning run failed at + // register_backup — long after this host looked fine. + expect(config('provisioning.proxmox.role_privs'))->toContain('Sys.Modify'); +}); + +it('converges the role privileges on a host that was onboarded before they changed', function () { + // `pveum role add … || true` only ever applied the privilege list on the run + // that first created the role, so a privilege added to the config reached new + // hosts and no existing one. The convergence must also sit BEFORE the token + // short-circuit, or a replay never reaches it. + config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + $s = fakeServices(); + $s['shell']->script('pveum role add', CommandResult::failure(1, 'role CluPilotAutomation already exists')); + $host = Host::factory()->create(['api_token_ref' => 'automation@pve!clupilot=already-there']); + $run = hostRun($host); + RunResource::create([ + 'run_id' => $run->id, 'host_id' => $host->id, 'kind' => 'pve_token', + 'external_id' => 'automation@pve!clupilot', + ]); + + expect(app(CreateAutomationToken::class)->execute($run)->type)->toBe('advance'); + + $modify = collect($s['shell']->recorded())->first(fn ($c) => str_contains($c, 'pveum role modify')); + expect($modify)->not->toBeNull() + ->and($modify)->toContain('Sys.Modify') + // Replaces rather than appends: the host matches the config, not the + // accumulation of everything any past version of it once granted. + ->and($modify)->not->toContain('-append') + // And the short-circuit still holds for the token itself. + ->and($s['shell']->ran('pveum user token add'))->toBeFalse(); +}); + +it('retries rather than minting a token the role privileges did not converge for', function () { + $s = fakeServices(); + $s['shell']->script('pveum role', CommandResult::failure(1, 'permission denied')); + $run = hostRun(Host::factory()->create()); + + expect(app(CreateAutomationToken::class)->execute($run)->type)->toBe('retry') + ->and($s['shell']->ran('pveum user token add'))->toBeFalse(); +}); + +// --- ConfigureProxmox --- + +it('refuses to finish a host that has no vmbr0 bridge', function () { + // Proxmox installed on top of Debian does not create vmbr0 — only the ISO + // installer does. The step used to run `ip link show vmbr0`, throw the result + // away, and advance, so onboarding reported `active` on a host with no bridge + // to attach a customer VM to. + $s = fakeServices(); + $s['shell']->script('ip link show vmbr0', CommandResult::failure(1)); + $s['shell']->script('ip -4 route show default', CommandResult::success('default via 203.0.113.1 dev enp0s31f6')); + $run = hostRun(Host::factory()->create()); + + $result = app(ConfigureProxmox::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toContain('vmbr0') + // Tells the operator what to do, including which NIC carries the route. + ->and($result->reason)->toContain('/etc/network/interfaces') + ->and($result->reason)->toContain('enp0s31f6') + // And never tries to build the bridge itself: getting that wrong over the + // very link the shell runs on takes the machine off the network for good. + ->and($s['shell']->ran('ifreload'))->toBeFalse() + ->and($s['shell']->ran('brctl'))->toBeFalse(); +}); + +it('enables the datacenter firewall without locking the host out', function () { + // Proxmox applies a guest's firewall rules only while the firewall is enabled + // at datacenter level, so applyFirewall()'s "80/443 only" rules were inert on + // every host. Enabling it blindly is the other trap: the datacenter input + // policy defaults to DROP, the node firewall defaults to ON, and the only + // sources it admits to 22/8006 come from an ipset seeded with the PUBLIC + // subnet — not the tunnel every later step uses. + $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); + $host = Host::factory()->create(['public_ip' => '203.0.113.40', 'wg_ip' => '10.66.0.40']); + $run = hostRun($host); + proveTunnel($run, $host); + + expect(app(ConfigureProxmox::class)->execute($run)->type)->toBe('advance'); + + $commands = collect($s['shell']->recorded()); + $policy = $commands->search(fn ($c) => str_contains($c, '--policy_in ACCEPT')); + $nodeOff = $commands->search(fn ($c) => str_contains($c, '/firewall/options --enable 0')); + $enable = $commands->search(fn ($c) => str_contains($c, '/cluster/firewall/options --enable 1')); + + expect($policy)->not->toBeFalse() + ->and($nodeOff)->not->toBeFalse() + ->and($enable)->not->toBeFalse() + // Order is the safety property: both mitigations land before the master + // switch, so there is no window in which pve-firewall compiles a DROP + // policy that cannot see the tunnel. + ->and($policy)->toBeLessThan($enable) + ->and($nodeOff)->toBeLessThan($enable); +}); + +it('does not believe a datacenter firewall write it cannot read back', function () { + // /etc/pve is a replicated filesystem: `pvesh set` exiting 0 is not the same + // as the setting being there, and a lost write means every customer VM's + // rules go on being inert while onboarding reports success. + $s = fakeServices(); + $s['shell']->script('pvesh get /cluster/firewall/options', CommandResult::success('{"enable":0}')); + $run = hostRun(Host::factory()->create()); + + expect(app(ConfigureProxmox::class)->execute($run)->type)->toBe('retry'); +}); + +it('drops the enterprise repo the proxmox-ve package brings back with it', function () { + $s = fakeServices(); + scriptHealthyProxmoxHost($s['shell']); + $run = hostRun(Host::factory()->create()); + + app(ConfigureProxmox::class)->execute($run); + + expect($s['shell']->ran('pve-enterprise.list'))->toBeTrue() + // PVE 9 ships the deb822 spelling instead, and one of the two is always + // the file that is actually there. + ->and($s['shell']->ran('pve-enterprise.sources'))->toBeTrue(); +}); + +// --- VerifyVmTemplate --- +// +// The seeded catalogue every installation starts with points all four plans at +// template_vmid 9000, which is exactly the situation this step exists for. + +it('refuses to finish a host that has no template to clone from', function () { + // A host used to go `active` while it could not serve a single customer: + // every plan version points at a template_vmid, nothing in the pipeline + // creates or verifies it, and there is no console command for it either — so + // the first PAID order died in clone_vm, after the money had changed hands. + $s = fakeServices(); + $run = hostRun(Host::factory()->active()->create()); + + $result = app(VerifyVmTemplate::class)->execute($run); + + expect(PlanVersion::query()->whereNotNull('published_at')->where('template_vmid', 9000)->exists())->toBeTrue() + ->and($result->type)->toBe('fail') + ->and($result->reason)->toContain('9000') + ->and($result->reason)->toContain('clone_vm') + // It reports; it does not invent a golden image nobody chose. + ->and($s['pve']->clonedVmids)->toBe([]); +}); + +it('advances once the template a published plan points at is on the node', function () { + $s = fakeServices(); + $s['pve']->clonedVmids[] = 9000; // the fake's stand-in for "this VMID exists" + $run = hostRun(Host::factory()->active()->create()); + + expect(app(VerifyVmTemplate::class)->execute($run)->type)->toBe('advance'); +}); + +it('still checks a version that is published but not on sale until next week', function () { + // PlanVersion::available() would exclude it, and its template gap would then + // surface at its first order rather than here — the whole failure this step + // removes. A CLOSED window is different: nobody can buy it any more. + $s = fakeServices(); + PlanVersion::query()->update(['available_from' => now()->addWeek()]); + $run = hostRun(Host::factory()->active()->create()); + + expect(app(VerifyVmTemplate::class)->execute($run)->type)->toBe('fail'); + + PlanVersion::query()->update(['available_until' => now()->subDay()]); + expect(app(VerifyVmTemplate::class)->execute($run)->type)->toBe('advance') + ->and($s['pve']->clonedVmids)->toBe([]); +}); + +it('requires no template on an installation that has not published a plan yet', function () { + fakeServices(); + PlanVersion::query()->update(['published_at' => null]); + $run = hostRun(Host::factory()->active()->create()); + + expect(app(VerifyVmTemplate::class)->execute($run)->type)->toBe('advance'); +}); + // --- VerifyProxmoxApi --- it('advances when the api returns nodes', function () { @@ -330,6 +859,7 @@ it('applies the firewall once the tunnel is confirmed up, over the tunnel itself $s = fakeServices(); $host = Host::factory()->active()->create(['public_ip' => '203.0.113.30']); $run = hostRun($host); + proveTunnel($run, $host); expect(app(SecureHostFirewall::class)->execute($run)->type)->toBe('advance'); @@ -346,15 +876,96 @@ it('applies the firewall once the tunnel is confirmed up, over the tunnel itself ->and($config)->toContain('ct state established,related accept'); expect($s['shell']->files())->toHaveKey('/usr/local/sbin/clupilot-emergency-open-firewall.sh') - ->and($s['shell']->ran('chmod 700'))->toBeTrue() - ->and($s['shell']->ran('/usr/local/sbin/clupilot-emergency-open-firewall.sh'))->toBeTrue() + ->and($s['shell']->ran('chmod 700 \'/usr/local/sbin/clupilot-emergency-open-firewall.sh\''))->toBeTrue() ->and($s['shell']->ran('systemctl enable --now nftables'))->toBeTrue() ->and(RunResource::where('run_id', $run->id)->where('kind', 'host_firewall')->count())->toBe(1); }); +it('closes 8006 and 22 to everything that is not the management subnet', function () { + // The owner's stated requirement, and the half nothing asserted: the old + // tests only checked that the WireGuard-scoped ACCEPT was PRESENT, so a + // mutation adding a bare `tcp dport { 8006 } accept` passed the whole suite + // while handing every scanner on the internet a Proxmox login form. + // + // Read as "which destination ports does this ruleset accept from a source it + // has not restricted to the tunnel", rather than as a search for one + // expected line — that is what makes an ADDED rule fail it. + $s = fakeServices(); + $host = Host::factory()->active()->create(); + $run = hostRun($host); + proveTunnel($run, $host); + + app(SecureHostFirewall::class)->execute($run); + + $wgSubnet = (string) config('provisioning.wireguard.subnet'); + $config = $s['shell']->files()['/etc/nftables.conf']; + + $worldOpen = []; + $tunnelOnly = []; + foreach (nftAcceptRules($config) as $rule) { + // ICMP and the loopback/conntrack rules carry no destination port. + foreach (nftAcceptedPorts($rule) as $port) { + if (str_contains($rule, 'ip saddr '.$wgSubnet)) { + $tunnelOnly[] = $port; + } else { + $worldOpen[] = $port; + } + } + } + + sort($worldOpen); + sort($tunnelOnly); + + expect($tunnelOnly)->toBe([22, 8006]) + // Exactly these and nothing else: public web, and the DHCP client reply + // below. 22 and 8006 must NOT appear here. + ->and($worldOpen)->toBe([68, 80, 443]) + ->and($worldOpen)->not->toContain(8006) + ->and($worldOpen)->not->toContain(22) + // A DHCP reply, not "anything to port 68". + ->and($config)->toContain('udp sport 67 udp dport 68 accept') + // And the chain still ends in drop, so an unmatched packet is refused + // rather than accepted by omission. + ->and($config)->toContain('policy drop') + ->and($config)->not->toContain('policy accept'); +}); + +it('accepts the ICMP a working host cannot do without', function () { + // policy drop with no ICMP accept at all — grepped, zero mentions. On a real + // host that breaks IPv6 outright as soon as the neighbour cache expires (NDP + // 135/136 and router advertisements arrive in the INPUT chain) and + // black-holes large transfers, because path-MTU discovery runs on + // frag-needed / packet-too-big. Debian's own example ruleset accepts both. + $s = fakeServices(); + $host = Host::factory()->active()->create(); + $run = hostRun($host); + proveTunnel($run, $host); + + app(SecureHostFirewall::class)->execute($run); + + $config = $s['shell']->files()['/etc/nftables.conf']; + $icmp = implode("\n", array_filter( + nftAcceptRules($config), + fn (string $rule) => str_contains($rule, 'icmp'), + )); + + expect($icmp)->toContain('nd-neighbor-solicit') + ->and($icmp)->toContain('nd-neighbor-advert') + ->and($icmp)->toContain('nd-router-advert') + // Path MTU discovery, both families. + ->and($icmp)->toContain('packet-too-big') + ->and($icmp)->toContain('destination-unreachable') + // Reachability, including the handshake check this very step performs. + ->and($icmp)->toContain('echo-request') + // Still a policy-drop ruleset, not "ICMP made it open". + ->and($config)->toContain('policy drop'); +}); + it("keeps the WireGuard interface's established connections working (never a bare drop-all)", function () { $s = fakeServices(); - $run = hostRun(Host::factory()->active()->create()); + $host = Host::factory()->active()->create(); + $run = hostRun($host); + proveTunnel($run, $host); app(SecureHostFirewall::class)->execute($run); @@ -363,10 +974,53 @@ it("keeps the WireGuard interface's established connections working (never a bar ->and($config)->toContain('iif "lo" accept'); }); +it('deploys an emergency script that never reopens the firewall by itself', function () { + // The owner's explicit requirement, and what nothing tested: the old + // assertion was `ran('/usr/local/sbin/clupilot-emergency-open-firewall.sh')`, + // which putFile() does not record — the only thing that substring matched was + // the `chmod 700` on the line above, so the same fact was asserted twice and + // the script's CONTENTS were never looked at. + $s = fakeServices(); + $host = Host::factory()->active()->create(); + $run = hostRun($host); + proveTunnel($run, $host); + + app(SecureHostFirewall::class)->execute($run); + + $script = $s['shell']->files()['/usr/local/sbin/clupilot-emergency-open-firewall.sh']; + + // It does the one thing it exists for. + expect($script)->toStartWith('#!/bin/sh') + ->and($script)->toContain('systemctl disable --now nftables') + ->and($script)->toContain('nft flush ruleset'); + + // And nothing in it can fire without a human at the out-of-band console. A + // firewall that reopens itself under failure is not a firewall. + foreach (['sleep', 'systemd-run', 'OnCalendar', 'OnBootSec', '.timer', 'crontab', 'cron.d', 'at now'] as $selfTrigger) { + expect($script)->not->toContain($selfTrigger); + } + + // Nothing detached into the background either, which is the other way a + // script leaves something running after the operator has walked away. + foreach (preg_split('/\R/', $script) ?: [] as $line) { + $line = trim($line); + if ($line === '' || str_starts_with($line, '#')) { + continue; + } + expect($line)->not->toEndWith('&'); + } + + // Nor does the step arrange one on its behalf. + expect($s['shell']->ran('systemd-run'))->toBeFalse() + ->and($s['shell']->ran('crontab'))->toBeFalse() + ->and($s['shell']->ran('.timer'))->toBeFalse(); +}); + it('never reapplies the firewall on a re-run', function () { $s = fakeServices(); $host = Host::factory()->active()->create(); $run = hostRun($host); + proveTunnel($run, $host); app(SecureHostFirewall::class)->execute($run); app(SecureHostFirewall::class)->execute($run->fresh()); diff --git a/tests/Feature/Provisioning/HostTokenKeyMigrationTest.php b/tests/Feature/Provisioning/HostTokenKeyMigrationTest.php new file mode 100644 index 0000000..17dca5c --- /dev/null +++ b/tests/Feature/Provisioning/HostTokenKeyMigrationTest.php @@ -0,0 +1,97 @@ +where('id', $host->id)->update(['api_token_ref' => $cipher]); +} + +function rawToken(Host $host): ?string +{ + $value = DB::table('hosts')->where('id', $host->id)->value('api_token_ref'); + + return $value === null ? null : (string) $value; +} + +beforeEach(fn () => config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32)))); + +it('moves a token written under APP_KEY onto SECRETS_KEY', function () { + $host = Host::factory()->create(); + storeRawToken($host, Crypt::encryptString('automation@pve!clupilot=old-secret')); + + loadHostTokenKeyMigration()->up(); + + $raw = (string) rawToken($host); + + expect(app(SecretCipher::class)->decrypt($raw))->toBe('automation@pve!clupilot=old-secret') + // The point of the whole exercise: APP_KEY no longer opens it, so + // rotating APP_KEY cannot take every host's Proxmox access with it. + ->and(fn () => Crypt::decryptString($raw))->toThrow(DecryptException::class) + ->and($host->fresh()->api_token_ref)->toBe('automation@pve!clupilot=old-secret'); +}); + +it('leaves a row it has already migrated exactly as it is', function () { + $host = Host::factory()->create(['api_token_ref' => 'automation@pve!clupilot=already-moved']); + $before = rawToken($host); + + loadHostTokenKeyMigration()->up(); + + expect(rawToken($host))->toBe($before) + ->and($host->fresh()->api_token_ref)->toBe('automation@pve!clupilot=already-moved'); +}); + +it('never destroys a value neither key can read', function () { + // Mutation boundary: the plaintext is a credential that exists nowhere else. + // A token an operator can still recover by hand from an old APP_KEY is worth + // more than a tidy column, so the row is left alone and reported — never + // overwritten, never nulled. + $host = Host::factory()->create(); + storeRawToken($host, 'eyJpdiI6IndyaXR0ZW4tdW5kZXItYS1sb25nLWdvbmUta2V5In0='); + + loadHostTokenKeyMigration()->up(); + + expect(rawToken($host))->toBe('eyJpdiI6IndyaXR0ZW4tdW5kZXItYS1sb25nLWdvbmUta2V5In0='); +}); + +it('touches nothing at all when there is no key to re-encrypt with', function () { + $host = Host::factory()->create(); + $appKeyCipher = Crypt::encryptString('automation@pve!clupilot=still-under-app-key'); + storeRawToken($host, $appKeyCipher); + + config()->set('admin_access.secrets_key', ''); + + // Does not throw: a fresh install has no SECRETS_KEY yet, and blocking the + // whole migration run on that would stop an installation that has nothing + // to migrate from being created at all. + loadHostTokenKeyMigration()->up(); + + expect(rawToken($host))->toBe($appKeyCipher); +}); + +it('puts the column back under APP_KEY on a rollback', function () { + $host = Host::factory()->create(['api_token_ref' => 'automation@pve!clupilot=roundtrip']); + + loadHostTokenKeyMigration()->down(); + + expect(Crypt::decryptString((string) rawToken($host)))->toBe('automation@pve!clupilot=roundtrip'); +});