From 8321e825eb62b2b336a9898377d337f33cbccba4 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 14:19:44 +0200 Subject: [PATCH] Fix round: CGNAT, guarded write/delete, and a silent host is not a missing template Co-Authored-By: Claude Opus 5 --- app/Services/Dns/DnsTokenCheck.php | 55 +++++-- app/Services/Proxmox/VmTemplateCheck.php | 42 +++++- app/Services/Vpn/WireguardEndpointCheck.php | 57 ++++++-- tests/Feature/Readiness/ActiveChecksTest.php | 143 +++++++++++++++++++ 4 files changed, 265 insertions(+), 32 deletions(-) diff --git a/app/Services/Dns/DnsTokenCheck.php b/app/Services/Dns/DnsTokenCheck.php index 8c7f2f2..7d34bc6 100644 --- a/app/Services/Dns/DnsTokenCheck.php +++ b/app/Services/Dns/DnsTokenCheck.php @@ -56,27 +56,52 @@ final class DnsTokenCheck // The actual point of this whole check. A read-only token gets this // far and looks in the console like a working one; it fails only when // a customer VM needs its A record — after payment. - $probe = Http::withHeaders(['Auth-API-Token' => $token])->acceptJson()->timeout(15) - ->post('https://dns.hetzner.com/api/v1/records', [ - 'zone_id' => $zoneId, - 'type' => 'TXT', - 'name' => '_clupilot-write-probe-'.Str::lower(Str::random(12)), - 'value' => 'clupilot readiness probe', - 'ttl' => 60, - ]); + // + // Guarded the same way as the GET above: this call is just as capable + // of hitting a broken network as the first one, and "the network + // died mid-write" must stay distinct from "the token cannot write" + // (read_only, below) — both are failures, but only one of them says + // anything about the token. + try { + $probe = Http::withHeaders(['Auth-API-Token' => $token])->acceptJson()->timeout(15) + ->post('https://dns.hetzner.com/api/v1/records', [ + 'zone_id' => $zoneId, + 'type' => 'TXT', + 'name' => '_clupilot-write-probe-'.Str::lower(Str::random(12)), + 'value' => 'clupilot readiness probe', + 'ttl' => 60, + ]); + } catch (Throwable) { + return ['ok' => false, 'reason' => 'unreachable']; + } if (! $probe->successful()) { return ['ok' => false, 'reason' => 'read_only', 'status' => $probe->status()]; } - // Always clean up, even when the delete itself fails: a leftover TXT - // record with this name is harmless, but has to be named in the - // result so someone can remove it by hand. + // The write already proved the token can write — that is the whole + // point of this check, and nothing past this line changes that + // verdict. Always attempt to clean up, and guard the attempt itself: + // a delete that throws is handled exactly like one that comes back + // non-2xx, because both leave the same TXT record sitting in the + // zone. Named in the result either way, so someone can remove it by + // hand instead of a record nobody knows exists. $recordId = $probe->json('record.id'); - $removed = Http::withHeaders(['Auth-API-Token' => $token])->acceptJson()->timeout(15) - ->delete('https://dns.hetzner.com/api/v1/records/'.$recordId) - ->successful(); - return ['ok' => true, 'reason' => 'writable', 'probe_removed' => $removed]; + try { + $removed = Http::withHeaders(['Auth-API-Token' => $token])->acceptJson()->timeout(15) + ->delete('https://dns.hetzner.com/api/v1/records/'.$recordId) + ->successful(); + } catch (Throwable) { + $removed = false; + } + + $result = ['ok' => true, 'reason' => 'writable', 'probe_removed' => $removed]; + + if (! $removed) { + $result['leftover_record_id'] = $recordId; + } + + return $result; } } diff --git a/app/Services/Proxmox/VmTemplateCheck.php b/app/Services/Proxmox/VmTemplateCheck.php index c3c4dca..c6c0ceb 100644 --- a/app/Services/Proxmox/VmTemplateCheck.php +++ b/app/Services/Proxmox/VmTemplateCheck.php @@ -43,9 +43,11 @@ final class VmTemplateCheck } $missing = []; + $unverifiable = []; foreach ($required as $vmid) { $found = false; + $aHostFailedToAnswer = false; foreach ($hosts as $host) { try { @@ -55,18 +57,48 @@ final class VmTemplateCheck } } catch (Throwable) { // A host that is not answering right now is not proof of - // a missing template. Keep looking at the others. + // a missing template — it might be sitting on exactly + // this host. Keep looking at the others; a later "yes" + // still wins outright (see $found above). + $aHostFailedToAnswer = true; + continue; } } - if (! $found) { + if ($found) { + continue; + } + + // Two different verdicts, not one: every host answered "no" is a + // confirmed gap (build the template); some host never answered + // is merely unverified (ask again once it is back). Telling an + // operator to build something that may already exist on a host + // that is simply not responding right now sends them to fix the + // wrong problem. + if ($aHostFailedToAnswer) { + $unverifiable[] = $vmid; + } else { $missing[] = $vmid; } } - return $missing === [] - ? ['ok' => true, 'reason' => 'present'] - : ['ok' => false, 'reason' => 'template_missing', 'vmids' => $missing]; + if ($missing !== []) { + $result = ['ok' => false, 'reason' => 'template_missing', 'vmids' => $missing]; + + if ($unverifiable !== []) { + // Named separately, never merged into vmids: one is + // confirmed and actionable, the other merely unanswered. + $result['unverifiable'] = $unverifiable; + } + + return $result; + } + + if ($unverifiable !== []) { + return ['ok' => false, 'reason' => 'hosts_unreachable', 'vmids' => $unverifiable]; + } + + return ['ok' => true, 'reason' => 'present']; } } diff --git a/app/Services/Vpn/WireguardEndpointCheck.php b/app/Services/Vpn/WireguardEndpointCheck.php index 6ab3f6b..9c2301f 100644 --- a/app/Services/Vpn/WireguardEndpointCheck.php +++ b/app/Services/Vpn/WireguardEndpointCheck.php @@ -22,18 +22,51 @@ use Illuminate\Support\Str; final class WireguardEndpointCheck { /** - * RFC 5737 "TEST-NET" ranges: documentation and example addresses, never - * routed on the real internet. PHP's own FILTER_FLAG_NO_RES_RANGE does - * NOT reject these (verified: 203.0.113.11 passes filter_var with that - * flag) — exactly the range a seed value or a copy-pasted example lands - * in, which is precisely the failure mode this check exists to catch. + * IPv4 ranges FILTER_FLAG_NO_PRIV_RANGE|FILTER_FLAG_NO_RES_RANGE does NOT + * reject, checked one by one against this PHP build (`php -r + * 'var_dump(filter_var($ip, FILTER_VALIDATE_IP, + * FILTER_FLAG_NO_PRIV_RANGE|FILTER_FLAG_NO_RES_RANGE));'`) so the next + * reader does not have to repeat the research: + * + * Included below, because each is a shape a real installation can + * actually end up with: + * - 192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24 (RFC 5737 "TEST-NET"): + * documentation/example addresses — seed data and copy-pasted examples + * land here. This was the original gap (Task 9 review). + * - 100.64.0.0/10 (RFC 6598, Carrier-Grade NAT): an ISP hands this out to + * a real customer connection; it is exactly as unreachable from the + * outside as RFC 1918, just not covered by NO_PRIV_RANGE. Confirmed + * passing filter_var's flags before this fix (fix-round finding). + * - 224.0.0.0/4 (multicast): not a shape a *mistake* produces, but never + * a single host's own reachable address either — no WireGuard peer's + * endpoint can legitimately be a multicast address, so it belongs in + * the same bucket as the others rather than being let through by + * omission. + * + * Already rejected by the filter_var flags above WITHOUT this list, so + * deliberately NOT duplicated here (verified empirically, not assumed): + * 0.0.0.0/8, 127.0.0.0/8 (loopback), 169.254.0.0/16 (link-local), + * 240.0.0.0/4 (reserved — this range also covers the 255.255.255.255 + * broadcast address, confirmed separately). + * + * Deliberately left UNCHECKED, a considered omission and not an + * oversight: 192.0.0.0/24 (IETF protocol assignments), 192.88.99.0/24 + * (deprecated 6to4 relay anycast), 198.18.0.0/15 (RFC 2544 benchmark + * testing). All three pass filter_var's flags today, but none of them is + * a shape an operator's real configuration plausibly lands in by + * accident or by a real provider's assignment — unlike the TEST-NET + * ranges (copy-pasted examples) and CGNAT (an ISP genuinely assigns it). + * Adding checks for ranges nobody will ever hit is effort spent on the + * task's shape, not its risk (R22). * * @var array */ - private const DOCUMENTATION_RANGES = [ - '192.0.2.0/24', // TEST-NET-1 - '198.51.100.0/24', // TEST-NET-2 - '203.0.113.0/24', // TEST-NET-3 + private const NON_PUBLIC_RANGES = [ + '192.0.2.0/24', // TEST-NET-1 (RFC 5737) + '198.51.100.0/24', // TEST-NET-2 (RFC 5737) + '203.0.113.0/24', // TEST-NET-3 (RFC 5737) + '100.64.0.0/10', // Carrier-Grade NAT (RFC 6598) + '224.0.0.0/4', // Multicast ]; /** @return array */ @@ -63,14 +96,14 @@ final class WireguardEndpointCheck // at one host on 2026-07-30. $public = filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE); - if ($public === false || $this->isDocumentationAddress($host)) { + if ($public === false || $this->isKnownNonPublicRange($host)) { return ['ok' => false, 'reason' => 'not_public', 'address' => $host]; } return ['ok' => true, 'reason' => 'public', 'address' => $host]; } - private function isDocumentationAddress(string $ip): bool + private function isKnownNonPublicRange(string $ip): bool { if (! filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { return false; @@ -78,7 +111,7 @@ final class WireguardEndpointCheck $long = ip2long($ip); - foreach (self::DOCUMENTATION_RANGES as $cidr) { + foreach (self::NON_PUBLIC_RANGES as $cidr) { [$subnet, $bits] = explode('/', $cidr); $mask = -1 << (32 - (int) $bits); diff --git a/tests/Feature/Readiness/ActiveChecksTest.php b/tests/Feature/Readiness/ActiveChecksTest.php index b538c75..7158952 100644 --- a/tests/Feature/Readiness/ActiveChecksTest.php +++ b/tests/Feature/Readiness/ActiveChecksTest.php @@ -9,6 +9,7 @@ use App\Services\Proxmox\ProxmoxClient; use App\Services\Proxmox\VmTemplateCheck; use App\Services\Vpn\WireguardEndpointCheck; use App\Support\Settings; +use Illuminate\Http\Client\ConnectionException; use Illuminate\Support\Facades\Http; use Illuminate\Support\Str; @@ -68,6 +69,27 @@ it('rejects a blank endpoint', function () { expect((new WireguardEndpointCheck)->run('')['reason'])->toBe('missing'); }); +/** + * Fix-Runde (Befund 1): FILTER_FLAG_NO_PRIV_RANGE|FILTER_FLAG_NO_RES_RANGE + * does not reject RFC 6598 Carrier-Grade NAT — verified live against + * `100.64.0.5:51820` before this fix, which came back `ok: true`. A host + * behind CGNAT is exactly as unreachable from outside as one behind RFC 1918, + * it is just an ISP handing the address out instead of an operator. + */ +it('rejects a carrier-grade NAT address', function () { + Http::fake(); + expect((new WireguardEndpointCheck)->run('100.64.0.5:51820')['ok'])->toBeFalse(); +}); + +/** + * Fix-Runde (Befund 1, vollständiger Durchgang): kein Mehrfach-Empfänger ist + * je der eigene Endpunkt eines einzelnen WireGuard-Peers. + */ +it('rejects a multicast address', function () { + Http::fake(); + expect((new WireguardEndpointCheck)->run('224.1.1.1:51820')['ok'])->toBeFalse(); +}); + // --- DnsTokenCheck -------------------------------------------------------- // // SICHERHEITSAUFLAGE: jeder dieser Tests benutzt Http::fake() und darf unter @@ -141,6 +163,53 @@ it('tells a read-only token apart from one that can write', function () { Http::assertNotSent(fn ($request) => $request->method() === 'DELETE'); }); +/** + * Fix-Runde (Befund 2): Das GET auf /zones ist gegen Throwable abgesichert + * (Reason "unreachable"), der Schreibversuch danach war es nicht — ein + * Netzwerkfehler genau dort hätte die Ausnahme aus run() herausgeworfen, auf + * einer Seite, deren einziger Zweck ist, ruhig zu berichten. "Netz kaputt" + * bleibt dabei von "Token darf nicht schreiben" (read_only, 403 s.o.) und von + * "Zone falsch benannt" getrennt — dieselbe Reason wie beim GET, weil es + * dieselbe Fehlerklasse ist, nur an einer anderen Stelle der Kette. + */ +it('reports unreachable when the write attempt itself cannot reach the network', function () { + Http::fake([ + 'dns.hetzner.com/api/v1/zones' => Http::response(['zones' => [['id' => 'zone-1', 'name' => 'probe.example']]]), + 'dns.hetzner.com/api/v1/records' => function () { + throw new ConnectionException('Could not resolve host'); + }, + ]); + + $result = (new DnsTokenCheck)->run('rw-token'); + + expect($result['ok'])->toBeFalse() + ->and($result['reason'])->toBe('unreachable'); +}); + +/** + * Fix-Runde (Befund 2, Sonderfall Aufräumen): der Schreibversuch selbst hat + * bereits bewiesen, dass der Token schreiben darf — das ist der Zweck der + * ganzen Prüfung, und ein kaputtes Netz beim Löschen ändert daran nichts. + * `probe_removed` muss trotzdem false sein, und der liegengebliebene Eintrag + * muss benennbar bleiben, sonst weiß niemand, dass er in der Zone liegt. + */ +it('still proves write access when the cleanup delete cannot reach the network', function () { + Http::fake([ + 'dns.hetzner.com/api/v1/zones' => Http::response(['zones' => [['id' => 'zone-1', 'name' => 'probe.example']]]), + 'dns.hetzner.com/api/v1/records' => Http::response(['record' => ['id' => 'rec-orphaned']]), + 'dns.hetzner.com/api/v1/records/*' => function () { + throw new ConnectionException('Could not resolve host'); + }, + ]); + + $result = (new DnsTokenCheck)->run('rw-token'); + + expect($result['ok'])->toBeTrue() + ->and($result['reason'])->toBe('writable') + ->and($result['probe_removed'])->toBeFalse() + ->and($result['leftover_record_id'])->toBe('rec-orphaned'); +}); + it('does not attempt anything without a token or a zone', function () { // Zurückgesetzt statt sich auf den .env-Wert dieser Installation zu // verlassen — genau die Falle, die phpunit.xml zu CLUPILOT_DNS_ZONE @@ -219,6 +288,80 @@ it('names the missing vmid when the template is not on any active host', functio ->and($result['vmids'])->toContain(4002); }); +/** + * Fix-Runde (Befund 3): ein Host, der gerade nicht antwortet, ist kein + * Beweis dafür, dass die Vorlage fehlt — sie könnte genau auf diesem Host + * liegen. Der bestehende FakeProxmoxClient kann das nicht abbilden (jede + * Antwort ist entweder true oder false, nie eine Ausnahme), deshalb eine + * anonyme Unterklasse nach dem Muster aus TrafficTest.php + * ($flaky/$blind), die vmExists() gezielt scheitern lässt. + */ +it('does not confuse a silent host with a confirmed-missing template', function () { + Http::fake(); + + $silent = new class extends FakeProxmoxClient + { + public function vmExists(string $node, int $vmid): bool + { + throw new RuntimeException('proxmox unreachable'); + } + }; + app()->instance(ProxmoxClient::class, $silent); + + Host::factory()->create(['status' => 'active', 'api_token_ref' => 'ref-1', 'node' => 'pve']); + makePublishedVersion(4004); + + $result = app(VmTemplateCheck::class)->run(); + + // "build the template" and "your hosts are not answering" are two + // different instructions — a reason of 'template_missing' here would + // send an operator to build a template that might already exist. + expect($result['ok'])->toBeFalse() + ->and($result['reason'])->toBe('hosts_unreachable') + ->and($result['reason'])->not->toBe('template_missing') + ->and($result['vmids'])->toContain(4004); +}); + +/** + * Fix-Runde (Befund 3, Gegenprobe): ein Host, der antwortet und die Vorlage + * bestätigt, darf das Ergebnis nicht kippen lassen, nur weil ein ANDERER + * Host in derselben Runde schweigt — "gefunden" sticht "still", und die + * Schleife bricht bei einem Treffer ab, statt den stillen Host danach noch + * als Beweis für irgendetwas zu werten. + * + * Eine einzige ProxmoxClient-Instanz bedient in VmTemplateCheck ALLE Hosts + * (`forHost($host)` wird pro Host auf demselben `$this->pve` aufgerufen), also + * unterscheidet dieser Fake anhand dessen, welcher Host zuletzt über + * `forHost()` hereinkam — genau das Feld, das `FakeProxmoxClient::forHost()` + * ohnehin schon merkt. + */ +it('still confirms a template that a working host has, despite a silent one', function () { + Http::fake(); + + $mixed = new class extends FakeProxmoxClient + { + public function vmExists(string $node, int $vmid): bool + { + if ($this->host?->node === 'silent-node') { + throw new RuntimeException('proxmox unreachable'); + } + + return parent::vmExists($node, $vmid); + } + }; + $mixed->clonedVmids = [4005]; // what the WORKING host actually has + app()->instance(ProxmoxClient::class, $mixed); + + Host::factory()->create(['status' => 'active', 'api_token_ref' => 'ref-1', 'node' => 'silent-node']); + Host::factory()->create(['status' => 'active', 'api_token_ref' => 'ref-2', 'node' => 'working-node']); + makePublishedVersion(4005); + + $result = app(VmTemplateCheck::class)->run(); + + expect($result['ok'])->toBeTrue() + ->and($result['reason'])->toBe('present'); +}); + it('reports no usable host when nothing is left to ask', function () { Http::fake(); app()->instance(ProxmoxClient::class, new FakeProxmoxClient);