dstack-ingress: follow-ups after the delegation (#104) and tls-alpn-01 (#105) merges

Aperta
#106 3 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
5/5
Tempo stimato
Più di una settimana
Idoneità per principianti
28/100
Tipo di issue
Bug
Chiarezza
Abbastanza chiara
Stato di attività
Tranquilla
Stack tecnologico
bash, python

Direzione di ricerca

Inizia con dns01.sh, tlsalpn.sh, dnsman.py, dnsguide.py e TESTING.md, quindi segui i percorsi di delega e rinnovo ed esegui i controlli di sintassi e unitari esistenti. Conferma i comportamenti relativi a nuovi tentativi, resolver, pulizia, selezione della zona e SAN descritti qui, e usa TESTING.md per tentare un'emissione delegata reale. Il lavoro è completato quando i follow-up prioritari sono implementati con test e il percorso delegato ha esito positivo end-to-end.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

Follow-ups from reviewing #104 and #105 after both landed. Ordered by impact.
Nothing here needs a revert; all are incremental.

P1 — A failed HAProxy reload is never retried

dns01.sh / tlsalpn.sh:

if [ "$changed" -eq 1 ]; then
    collect_evidence     || echo "Evidence generation failed" >&2
    build_combined_pems  || echo "Combined PEM build failed" >&2
    haproxy_reload       || true
fi
[ "$failed" -eq 0 ]

The reload failure is swallowed and does not set failed, so the pass reports
success. Twelve hours later the ACME client says "nothing to renew", changed
is 0, and the reload is never attempted again. HAProxy keeps serving the
previous certificate — which was renewed precisely because it was near expiry
— until it expires. Same for a failed build_combined_pems.

#102 solved this with a renewal-applied.stamp and a certs_pending_apply()
check: certificate material newer than the stamp means the renewal has not
reached HAProxy yet, so the apply step retries on the next cycle. That PR
predates the restructure and targets renewal-daemon.sh / renew-certificate.sh,
which #105 removed, so the mechanism needs porting rather than merging — and it
can be simpler here: #102 needed a file because bootstrap and the daemon were
two processes, whereas one process now owns the whole lifecycle, so an in-memory
"apply pending" flag is enough. Credit to @pacoyang for finding it.

P1 — Delegation CAA verification is single-resolver and fails closed

verify_delegation_caa() queries only dns.google and refuses to start when the
record is not found. Public resolvers cache negative answers for the zone's SOA
minimum — up to an hour — so a resolver queried before the operator created the
record keeps reporting it absent well after it exists. That is a false-positive
refusal to start, observed during #105's testing on exactly this kind of check.

The failure asymmetry also runs the wrong way:

Situation Current behaviour
DoH unreachable / non-zero DNS status warn and continue — no CAA protection
Record genuinely present but cached negative hard fail, container will not start

grep -F "accounturi=…" on .data compounds it: that matches presentation-form
rdata, which is what dns.google returns. Adding a second resolver naively would
break it, because Cloudflare returns CAA in RFC 3597 generic hex
(\# 47 00 05 69 73 73 75 65 …) and the literal match silently fails — which,
being fail-closed, is an outage.

dnsguide.py (added in #105) already does this properly: two resolvers with
union quorum for CAA, both rdata encodings, unit-tested against strings real
resolvers returned. Delegation should call it instead of rolling its own check.

That also gets the delegation path DNS_SETUP_MODE for free — wait blocks until
the operator's records appear instead of failing on the first look, and webhook
POSTs them to an automation endpoint with an HMAC and a TDX quote. One record
type has to be added to build_records: the _acme-challenge CNAME. The only
awkward part is ALLOW_MISSING_CAA, which has no dnsguide equivalent; mapping it
to --mode print would also skip CNAME/TXT verification, so it likely needs its
own flag.

P2 — unset_txt_record reports success when the zone lookup failed

get_dns_records() returns [] both when a name has no records and when the
zone could not be resolved, so unset_txt_record iterates nothing and returns
True. Observed:

acme-dns-alias-hook: removing challenge TXT _acme-challenge.svc.example.com.deleg.example.net
Error: Could not find zone for domain _acme-challenge.svc.example.com.deleg.example.net
Successfully unset TXT record for _acme-challenge.svc.example.com.deleg.example.net

Exit code 0. The hook discards cleanup failures deliberately, so the impact is a
stale TXT in the delegation zone (the next auth overwrites it) — but the log
states the opposite of what happened, which will cost someone an afternoon.

P2 — The delegated issuance path has never been executed

#104 states its testing as py_compile plus bash -n. Reviewing after the fact,
the mechanism does hold up:

  • _get_zone_info does longest-suffix matching, so
    _acme-challenge.svc.example.com.deleg.example.net resolves to
    deleg.example.net and not to the served zone whose name it contains;
  • the hook computes the right record name, finds dnsman.py on PATH, and uses
    the venv interpreter;
  • a failed auth exits non-zero, so certbot does see the challenge fail.

But no one has run an actual delegated issuance — hook writes the TXT, Let's
Encrypt follows the CNAME, validation passes. TESTING.md describes how to set
this up; it needs a delegation zone.

P3 — set_txt_record deletes same-name records, which SAN will break

It removes every existing TXT at the name before creating the new one. Today
process_domain passes a single -d per certbot invocation, so only one
challenge value ever exists at _acme-challenge.<name> and this is fine.

It stops being fine as soon as one certificate covers several identifiers that
validate at the same name — example.com plus *.example.com, or #86's SAN
support. certbot then calls the auth hook once per identifier with the same
CERTBOT_DOMAIN and different CERTBOT_VALIDATION, both records have to exist
at once, and the second call deletes the first. Worth fixing before #86 lands:
the challenge hook wants add-without-delete semantics, distinct from the
set-and-replace the routing records want.

P3 — _ensure_zone_id falls back to a stale cached zone on lookup failure

zone_info = self._get_zone_info(domain)
if zone_info:
    self.zone_id, self.zone_domain = zone_info
return self.zone_id      # previous zone's id when the lookup failed

A failed lookup returns whichever zone was cached last, so a subsequent write
could land in the wrong zone. Pre-existing, and not currently reachable —
dnsman.py runs as a fresh process per call, so the cache is empty at the point
that matters. Worth closing anyway, since delegation mode is precisely the
feature that makes "which zone are we writing to" a security property.

Lingua principale
Python
Stelle
27
Fork
26
Merge medio
1h 25m
PR unite (30g)
7

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di Dstack-TEE/dstack-examples

Tutte le issue di Dstack-TEE/dstack-examples

Issue simili

Altre issue su Python

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.