From a3cbaaef61b3b80b2d57328de1eec357a95be22e Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 4 Aug 2026 19:02:10 +0200 Subject: [PATCH] Abschluss-Review: fuenf Befunde, ein Fix-Durchgang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kritisch: ein doppelt genannter Name — APP_HOST auch in SITE_HOST, ein Name zweimal in SITE_HOST, oder einer davon gleich dem Konsolennamen — liess vpn-entrypoint.sh zwei identische Site-Bloecke schreiben. Caddy lehnt das nicht bloss ab, es startet dann ueberhaupt nicht ("ambiguous site definition"), der Container laeuft in eine Neustartschleife, und mit ihm ist die Konsole aus dem Tunnel verschwunden. Also genau der Ausfall, den dieser Zweig verhindern soll, erreicht durch einen Tippfehler in der .env. Belegt mit caddy validate in beide Richtungen. Dazu: clupilot:publish-tunnel-names wurde von nichts aufgerufen. Die Gateway-Haelfte liest die .env bei jedem up -d neu, die Resolver-Haelfte wurde einmal von Hand geschrieben und nie wieder — eine neue Installation, ein zusaetzlicher STATUS_HOST oder ein neu angelegtes dns-hosts-Volume haetten sie still veralten lassen. Sie laeuft jetzt im Deploy mit. Und drei Kleinigkeiten: die Spec beschrieb den Gesundheits-Port noch nach der alten Annahme, der Resolver uebernahm ungeprueft, was in der .env steht (auf der Produktivmaschine steht dort noch ein Markdown-Link), und die Testattrappe raeumte weniger auf als die echte Umsetzung. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/BindHosts.php | 15 ++++--- app/Console/Commands/PublishTunnelNames.php | 45 ++++++++++++++++++- app/Services/Dns/FakeHostDnsDirectory.php | 8 +++- app/Support/HostnamePattern.php | 29 ++++++++++++ deploy/update.sh | 17 +++++++ docker/caddy/vpn-entrypoint.sh | 30 ++++++++++++- ...04-oeffentliche-seiten-im-tunnel-design.md | 9 +++- tests/Feature/TunnelNamesTest.php | 24 +++++++++- tests/Feature/VpnGatewayConfigTest.php | 19 ++++++++ 9 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 app/Support/HostnamePattern.php diff --git a/app/Console/Commands/BindHosts.php b/app/Console/Commands/BindHosts.php index 585d372..15c9fb2 100644 --- a/app/Console/Commands/BindHosts.php +++ b/app/Console/Commands/BindHosts.php @@ -4,6 +4,7 @@ namespace App\Console\Commands; use App\Services\Env\EnvFileEditor; use App\Services\Env\InvalidEnvContentException; +use App\Support\HostnamePattern; use Illuminate\Console\Command; /** @@ -125,9 +126,12 @@ class BindHosts extends Command /** Eine Komma-Liste, in der JEDER Eintrag ein Hostname ist. */ private function isHostList(string $value): bool { + // `explode(',', $value)` liefert nie ein leeres Array — selbst für + // `''` steht `['']` da, und das faengt der `in_array('', …)` gleich + // darunter ab. $names = array_map('trim', explode(',', $value)); - if ($names === [] || in_array('', $names, true)) { + if (in_array('', $names, true)) { return false; } @@ -143,7 +147,9 @@ class BindHosts extends Command /** * Dasselbe Muster, das der root-eigene Helfer in `apply-proxy-hosts` * benutzt, bevor er einen Namen in die Proxy-Konfiguration schreibt - * (deploy/install-agent.sh). + * (deploy/install-agent.sh) — jetzt in HostnamePattern, denn + * PublishTunnelNames braucht dieselbe Prüfung fürs Lesen, ohne diese + * Klasse dafür kennen zu müssen. * * Der Anlass ist konkret: `[www.example.com](https://www.example.com)` ist * als Hostname offensichtlich Unsinn und wurde trotzdem geschrieben, weil @@ -153,10 +159,7 @@ class BindHosts extends Command */ private function isHost(string $name): bool { - return (bool) preg_match( - '/^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)+$/', - $name, - ); + return HostnamePattern::matches($name); } /** diff --git a/app/Console/Commands/PublishTunnelNames.php b/app/Console/Commands/PublishTunnelNames.php index 5b0fbac..8a9836f 100644 --- a/app/Console/Commands/PublishTunnelNames.php +++ b/app/Console/Commands/PublishTunnelNames.php @@ -3,7 +3,9 @@ namespace App\Console\Commands; use App\Services\Dns\HostDnsDirectory; +use App\Support\HostnamePattern; use Illuminate\Console\Command; +use Symfony\Component\Console\Output\ConsoleOutputInterface; /** * Macht Portal, Website und Statusseite im Management-Tunnel auflösbar. @@ -43,12 +45,31 @@ class PublishTunnelNames extends Command return self::SUCCESS; } - $names = array_values(array_unique(array_filter(array_merge( + $candidates = array_values(array_unique(array_filter(array_merge( [(string) config('admin_access.app_host')], array_map('strval', (array) config('admin_access.site_hosts', [])), [(string) config('admin_access.status_host')], )))); + // `bind-hosts` weist einen NEUEN Wert dieser Form inzwischen zurück, + // aendert aber nichts an einem, der schon in der .env steht. Auf der + // Produktivmaschine stand dort bereits ein Markdown-Link aus einem + // Kopiervorgang (`[www.clupilot.com](https://www.clupilot.com)`), von + // VOR dieser Prüfung — geschrieben, weil EnvFileEditor nur prüft, ob + // die ZEILE die Form KEY=value hat. Ungeprüft ginge so ein Wert in + // `platform.hosts`, und der Resolver böge einen Unsinns-Namen um. + $names = []; + + foreach ($candidates as $candidate) { + if (HostnamePattern::matches($candidate)) { + $names[] = $candidate; + + continue; + } + + $this->skipped($candidate); + } + $hub = (string) config('provisioning.wireguard.hub_address'); $dns->writeMany(self::KEY, $names, $hub); @@ -65,4 +86,26 @@ class PublishTunnelNames extends Command return self::SUCCESS; } + + /** + * Nicht still: eine ausgelassene `.env`-Altlast soll auffallen, auch wenn + * niemand die Ausgabe dieses Befehls Zeile für Zeile liest. + * + * Auf STDERR statt über `$this->error()` — das faerbt nur denselben Strom + * rot ein, den auch `$this->line()` benutzt. `deploy/update.sh` ruft + * diesen Befehl mit `2>&1` auf und sieht den Unterschied nicht, aber ein + * Betreiber, der ihn von Hand startet und nur STDOUT protokolliert, soll + * die Meldung trotzdem bekommen. `getOutput()` liefert in einem echten + * CLI-Lauf die `ConsoleOutput`, die einen echten zweiten Strom hat; im + * Testlauf ist es der gepufferte Mock aus PendingCommand, der keiner ist + * — dort faellt das auf den normalen Strom zurück, und Tests sehen die + * Meldung trotzdem. + */ + private function skipped(string $value): void + { + $stream = $this->output->getOutput(); + $target = $stream instanceof ConsoleOutputInterface ? $stream->getErrorOutput() : $stream; + + $target->writeln(" uebersprungen: „{$value}“ ist kein Hostname."); + } } diff --git a/app/Services/Dns/FakeHostDnsDirectory.php b/app/Services/Dns/FakeHostDnsDirectory.php index 639476e..e1f08f2 100644 --- a/app/Services/Dns/FakeHostDnsDirectory.php +++ b/app/Services/Dns/FakeHostDnsDirectory.php @@ -45,7 +45,13 @@ class FakeHostDnsDirectory implements HostDnsDirectory } if ($fqdns === []) { - unset($this->groups[$key], $this->ips[$key]); + // FileHostDnsDirectory::writeMany() ruft bei einer leeren Liste + // remove() auf, und remove() loescht die Datei komplett — nicht + // nur die Zeilen darin, die als IP-Zuordnung durchgingen. Ohne + // $fqdns hier mit zu leeren, wich der Fake vom echten Verhalten + // ab: ein Test haette `$fake->fqdns[$key]` noch stehen sehen, + // waehrend die echte Datei laengst weg war. + unset($this->groups[$key], $this->ips[$key], $this->fqdns[$key]); return; } diff --git a/app/Support/HostnamePattern.php b/app/Support/HostnamePattern.php new file mode 100644 index 0000000..4220c0d --- /dev/null +++ b/app/Support/HostnamePattern.php @@ -0,0 +1,29 @@ +/de docker compose --profile vpn restart vpn-dns vpn-gateway >/dev/null 2>&1 || true fi +# Die Namen leben in der .env, und die .env aendert sich zwischen Deployments, +# ohne dass jemand von Hand einen Befehl nachfaehrt — ein neu gesetzter +# STATUS_HOST, ein per bind-hosts nachgetragener Name, eine frisch installierte +# Maschine, ein neu angelegtes dns-hosts-Volume. Bisher stand der Befehl nur im +# Plan als manueller Einzeiler, und der Aufloeser blieb genau so lange aktuell, +# wie zuletzt jemand daran gedacht hatte — das war: nie automatisch. +# +# Nach dem Cache-Rebuild oben, damit `config('admin_access…')` die eben +# geschriebenen Werte liest und nicht den Stand von vorher. Vor der +# Bereitschaftspruefung, damit ein frisch gebundener Name schon aufloest, wenn +# `reconcile_vpn_readiness` gleich danach den Tunnel als bereit meldet. +# +# `|| true`, wie bei den Nachbaraufrufen in dieser Funktion: ein Deployment darf +# daran nicht scheitern, und ein Fehlschlag hier heisst hoechstens, dass die +# Namen bis zum naechsten Lauf stehen bleiben, wie sie waren. +in_app php artisan clupilot:publish-tunnel-names >/dev/null 2>&1 || true + reconcile_vpn_readiness # ── Host packages the update can install itself ────────────────────────────── diff --git a/docker/caddy/vpn-entrypoint.sh b/docker/caddy/vpn-entrypoint.sh index 7531c2d..271b8ea 100755 --- a/docker/caddy/vpn-entrypoint.sh +++ b/docker/caddy/vpn-entrypoint.sh @@ -18,6 +18,9 @@ HEALTH="${VPN_HEALTH_PORT:-8081}" CERT_DIR="${VPN_CERT_DIR:-/certs}" OUT="${VPN_CONFIG_OUT:-/tmp/vpn.Caddyfile}" +# Die Namen, fuer die schon ein Site-Block geschrieben wurde — siehe emit_site(). +EMITTED="" + # Die Zertifikate, die dieser Lauf wirklich geladen hat. Der Update-Agent # ueberwacht sie und startet den Gateway nach einer Erneuerung neu — Caddys # `tls` liest die Datei EINMAL beim Start, und ohne Neustart liefe der Tunnel @@ -37,6 +40,21 @@ CERT_LIST="${VPN_CERT_LIST:-/tmp/vpn-certs.list}" # den der Gateway keinen Zweck hat. emit_site() { name="$1" + + # Schon bedient? Caddy lehnt eine doppelte Site-Definition nicht bloss ab — + # es startet dann ueberhaupt nicht („ambiguous site definition"), der + # Container laeuft in eine Neustartschleife, und mit ihm ist die Konsole aus + # dem Tunnel verschwunden. Ein doppelter Name ist kein exotischer Fall: + # APP_HOST auch in SITE_HOST, SITE_HOST zweimal derselbe Name, oder einer + # davon gleich dem Konsolennamen — alles Tippfehler, die in einer .env + # vorkommen. + case " $EMITTED " in + *" $name "*) + echo " uebersprungen: $name — bereits als Site-Block geschrieben" >&2 + return 0 + ;; + esac + # `-print -quit`, nicht `| head -1`: eine Pipe, aus der head aussteigt, # waehrend find noch schreibt, liefert SIGPIPE — install-agent.sh hat sich # daran schon einmal selbst beendet. `|| true`, damit ein leeres Ergebnis @@ -62,6 +80,7 @@ emit_site() { } >> "$OUT" echo "$crt" >> "$CERT_LIST" + EMITTED="$EMITTED $name" } # `if`, nicht `[ … ] && …`. Bei leerem Wert gibt die AND-OR-Liste 1 zurueck, und @@ -75,10 +94,17 @@ fi # Portal, Website und Statusseite. files. steht hier nie drin: dort holt ein # Server im Rettungssystem sein Archiv, und der ist nicht im Tunnel. -echo "${VPN_TUNNEL_HOSTS:-}" | tr ',' '\n' | while read -r host; do +# +# Als Here-Dokument, nicht als Pipe: eine Pipe haengt die Schleife in eine +# Subshell, und EMITTED — dort veraendert — waere nach der Zeile wieder leer. +# `read -r` mit der Standard-IFS trimmt umgebende Leerzeichen weiterhin, worauf +# Eintraege wie " www.x" sich verlassen. +while read -r host; do [ -n "$host" ] || continue emit_site "$host" -done +done <toBe("10.66.0.1 a.test\n10.66.0.1 b.test\n"); }); +it('überspringt einen Wert, der schon vor der bind-hosts-Prüfung in der .env stand', function () { + // `bind-hosts` weist einen NEUEN Wert dieser Form inzwischen zurück — aber + // auf einer Produktivmaschine stand genau das schon in der .env, von VOR + // dieser Prüfung. Ungeprüft ginge der Unsinn hier in platform.hosts, und + // der Resolver böge einen Namen um, den keine Anfrage je trifft. + config()->set('admin_access.app_host', 'app.clupilot.test'); + config()->set('admin_access.site_hosts', ['[www.clupilot.test](https://www.clupilot.test)']); + + $this->artisan('clupilot:publish-tunnel-names') + ->expectsOutputToContain('www.clupilot.test') + ->assertSuccessful(); + + $written = file_get_contents($this->dir.'/platform.hosts'); + + expect($written)->toContain('10.66.0.1 app.clupilot.test') + ->and($written)->not->toContain('[www.clupilot.test]'); +}); + it('ist dieselbe Datei-Umsetzung wie für die Host-Namen', function () { // Nicht Fake gegen Fake geprüft: die Rechte und das Anlegen des // Verzeichnisses sind der Teil, der in Produktion schiefgeht. + // + // Kein ->skip() hier: die Bedingung `! app()->environment('testing')` kann + // in einem Testlauf nie zutreffen — der Test lief seit seiner Einführung + // also immer, und ein Skip, der nie greift, ist bloss eine Behauptung. expect(app(HostDnsDirectory::class))->toBeInstanceOf(FileHostDnsDirectory::class); -})->skip(fn () => ! app()->environment('testing'), 'nur im Testlauf'); +}); diff --git a/tests/Feature/VpnGatewayConfigTest.php b/tests/Feature/VpnGatewayConfigTest.php index 17d3926..8b86940 100644 --- a/tests/Feature/VpnGatewayConfigTest.php +++ b/tests/Feature/VpnGatewayConfigTest.php @@ -170,6 +170,25 @@ it('schreibt die geladenen Zertifikate mit, damit die Erneuerung greift', functi ->and($written)->not->toContain('www.clupilot.test'); }); +it('schreibt einen doppelt genannten Namen nur als EINEN Site-Block', function () { + // Der Anlass fuer diesen Test: Caddy lehnt eine doppelte Site-Definition + // nicht bloss ab, es startet dann UEBERHAUPT NICHT + // ("ambiguous site definition: https://…:443") — der Container laeuft in + // eine Neustartschleife, und mit ihm ist die Konsole aus dem Tunnel weg. + // Ein Tippfehler in der .env reicht: APP_HOST auch in SITE_HOST gelistet, + // SITE_HOST mit demselben Namen zweimal, oder einer davon gleich dem + // Konsolennamen. + $config = renderVpnConfig([ + // admin.clupilot.test ist zugleich der Konsolenname UND in + // VPN_TUNNEL_HOSTS gelistet. + 'VPN_INTERNAL_HOST' => 'admin.clupilot.test', + 'VPN_TUNNEL_HOSTS' => 'admin.clupilot.test,app.clupilot.test,app.clupilot.test', + ], ['admin.clupilot.test', 'app.clupilot.test']); + + expect(substr_count($config, 'https://admin.clupilot.test:443 {'))->toBe(1) + ->and(substr_count($config, 'https://app.clupilot.test:443 {'))->toBe(1); +}); + it('gibt jedem Block die Weiterleitung mit der echten Quelladresse', function () { // Ohne X-Forwarded-For sähe die Anwendung den Gateway statt des Anrufers, // und die Freigabeliste prüfte die falsche Adresse.