Abschluss-Review: fuenf Befunde, ein Fix-Durchgang

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 <noreply@anthropic.com>
main
nexxo 2026-08-04 19:02:10 +02:00
parent 620539a512
commit a3cbaaef61
9 changed files with 183 additions and 13 deletions

View File

@ -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);
}
/**

View File

@ -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.");
}
}

View File

@ -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;
}

View File

@ -0,0 +1,29 @@
<?php
namespace App\Support;
/**
* Ob eine Zeichenkette ein Hostname sein kann an EINER Stelle geprüft.
*
* Vorher stand das Muster nur in `BindHosts::isHost()`. `PublishTunnelNames`
* liest dieselben `.env`-Werte weiter, nachdem `bind-hosts` sie schon
* geschrieben hat und auf einer Produktivmaschine stand dort bereits
* `SITE_HOST=[www.clupilot.com](https://www.clupilot.com)`, ein Markdown-Link
* aus einem Kopiervorgang, von VOR dieser Prüfung. `bind-hosts` weist einen
* neuen Wert dieser Form inzwischen zurück, ändert aber nichts an einem, der
* schon in der Datei steht und ohne dieselbe Prüfung beim Veröffentlichen
* würde genau dieser Unsinn in `platform.hosts` landen.
*
* Hier statt in `BindHosts` oder `PublishTunnelNames` selbst: beide Befehle
* brauchen das Muster, und keiner der beiden soll den anderen kennen müssen,
* nur um daran zu kommen.
*/
final class HostnamePattern
{
private const PATTERN = '/^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)+$/';
public static function matches(string $name): bool
{
return (bool) preg_match(self::PATTERN, $name);
}
}

View File

@ -701,6 +701,23 @@ if [[ "$hub_rebuilt" == true ]] && grep -qE '^COMPOSE_PROFILES=.*vpn' .env 2>/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 ──────────────────────────────

View File

@ -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 <<EOF
$(printf '%s' "${VPN_TUNNEL_HOSTS:-}" | tr ',' '\n')
EOF
# Antwortet der Gesundheits-Port mit 204, gilt der Tunnel als BEREIT — daran
# haengt, ob ausgegebene Client-Konfigurationen den Resolver ueberhaupt nennen.

View File

@ -86,8 +86,13 @@ Das Zertifikat bleibt das, was der öffentliche Caddy ohnehin erneuert. Es häng
am Namen, nicht an der Adresse, die ihn ausliefert — dieselbe Begründung, die im
Kopf der `vpn.Caddyfile` schon steht.
Der Gesundheits-Port bleibt unverändert und unabhängig von jedem Zertifikat: er
ist das Signal, an dem die Bereitschaft des Tunnels hängt.
Der Gesundheits-Port antwortet nur dann mit 204, wenn der Site-Block der
Konsole selbst gerendert wurde — er hängt also gerade NICHT von jedem
Zertifikat ab, sondern ausdrücklich vom Konsolen-Zertifikat, dem einzigen, ohne
den ausgegebene Client-Konfigurationen den Resolver gar nicht erst nennen
dürften. Ein 204, das nur „irgendein Caddy läuft" bedeutete, wäre eine
Falschmeldung: `VPN_READY` würde wahr, und ein Client bekäme einen Resolver
genannt, der ihn auf eine Adresse schickt, die die Verbindung ablehnt.
### 3. `VPN_CERT_PATH` / `VPN_KEY_PATH` bleiben stehen

View File

@ -89,8 +89,30 @@ it('schreibt genau eine Zeile je Name, im hosts-Format', function () {
->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');
});

View File

@ -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.