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.
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:The reload failure is swallowed and does not set
failed, so the pass reportssuccess. Twelve hours later the ACME client says "nothing to renew",
changedis 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.stampand acerts_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 onlydns.googleand refuses to start when therecord 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:
grep -F "accounturi=…"on.datacompounds it: that matches presentation-formrdata, which is what
dns.googlereturns. Adding a second resolver naively wouldbreak 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 withunion 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_MODEfor free —waitblocks untilthe operator's records appear instead of failing on the first look, and
webhookPOSTs them to an automation endpoint with an HMAC and a TDX quote. One record
type has to be added to
build_records: the_acme-challengeCNAME. The onlyawkward part is
ALLOW_MISSING_CAA, which has no dnsguide equivalent; mapping itto
--mode printwould also skip CNAME/TXT verification, so it likely needs itsown flag.
P2 —
unset_txt_recordreports success when the zone lookup failedget_dns_records()returns[]both when a name has no records and when thezone could not be resolved, so
unset_txt_recorditerates nothing and returnsTrue. Observed:
Exit code 0. The hook discards cleanup failures deliberately, so the impact is a
stale TXT in the delegation zone (the next
authoverwrites it) — but the logstates 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_compileplusbash -n. Reviewing after the fact,the mechanism does hold up:
_get_zone_infodoes longest-suffix matching, so_acme-challenge.svc.example.com.deleg.example.netresolves todeleg.example.netand not to the served zone whose name it contains;dnsman.pyonPATH, and usesthe venv interpreter;
authexits 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.mddescribes how to setthis up; it needs a delegation zone.
P3 —
set_txt_recorddeletes same-name records, which SAN will breakIt removes every existing TXT at the name before creating the new one. Today
process_domainpasses a single-dper certbot invocation, so only onechallenge 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.complus*.example.com, or #86's SANsupport. certbot then calls the auth hook once per identifier with the same
CERTBOT_DOMAINand differentCERTBOT_VALIDATION, both records have to existat 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_idfalls back to a stale cached zone on lookup failureA 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.pyruns as a fresh process per call, so the cache is empty at the pointthat matters. Worth closing anyway, since delegation mode is precisely the
feature that makes "which zone are we writing to" a security property.