From 354f9f5275ef5e0326d0fa87a6449fc651e0afd4 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Wed, 26 Aug 2026 00:09:19 -0700 Subject: [PATCH 1/2] test: resolve challenge records through the mock, and check that it happened certbot resolves each challenge name itself before telling the CA to look, so that a record which has not propagated is reported by name rather than as an order failure. Nothing in the suite exercised that: the gateway resolved through Docker's embedded DNS, which has never heard of the test zones, so every self-check ran its full budget, warned, and proceeded -- for dns-01 too, seconds after certbot itself wrote the record. WARN no authoritative nameserver for _acme-challenge.test0.local, using the system resolver DEBUG challenge not found, waiting for 500ms tries=2 max_wait=30s Pointing the gateways at the mock needs the mock to stop being a black hole for everything else: a name it does not know was answered NOERROR with no records, which a resolver reads as authoritative and does not retry elsewhere, so the gateway would lose the other containers. It now forwards those upstream, which is what makes it usable as a client's only resolver rather than only as a CA's `-dnsserver`. The check is advisory, so nothing downstream reveals whether it worked -- an order succeeds either way. The mock therefore logs every question it is asked and serves them at /api/dns-queries, and the two cases are asserted directly: - Answered, for a record the suite published. Observed rather than triggered: the periodic renewal picks a domain up as soon as it is added, so forcing one races it and can find nothing left to do. - Asked repeatedly and never answered, for a name with no record. Polling is the point -- a record may still be propagating -- and giving up must not stop the order. Each keys off a name nothing else asks for, so they need no clearing and do not depend on running in any order. Incidentally the suite gets its time back: a self-check that resolves settles in seconds instead of spending the whole advisory wait on every order. --- .../gateway/test-run/e2e/docker-compose.yml | 20 ++++- dstack/gateway/test-run/e2e/test.sh | 67 ++++++++++++++++ tools/mock-cf-dns/server.py | 76 +++++++++++++++++++ 3 files changed, 162 insertions(+), 1 deletion(-) diff --git a/dstack/gateway/test-run/e2e/docker-compose.yml b/dstack/gateway/test-run/e2e/docker-compose.yml index bfe8dc378..3808323e4 100644 --- a/dstack/gateway/test-run/e2e/docker-compose.yml +++ b/dstack/gateway/test-run/e2e/docker-compose.yml @@ -51,7 +51,7 @@ services: - PORT=8080 - DEBUG=true # The zones certbot writes into and Pebble reads back out of. - - MOCK_CF_ZONES=test0.local,test1.local,test2.local,persist0.local,persist1.local,persist2.local + - MOCK_CF_ZONES=test0.local,test1.local,test2.local,persist0.local,persist1.local,persist2.local,selfcheck0.local healthcheck: test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://localhost:8080/health')"] interval: 5s @@ -117,6 +117,12 @@ services: timeout: 3s retries: 10 start_period: 30s + # Resolve through the mock, which owns the test zones and forwards + # everything else. certbot's pre-order self-check reads DNS like any + # client; without this it queries a resolver that has never heard of the + # challenge names, so the check can only ever time out. + dns: + - 172.30.0.10 cap_add: - NET_ADMIN extra_hosts: @@ -152,6 +158,12 @@ services: timeout: 3s retries: 10 start_period: 30s + # Resolve through the mock, which owns the test zones and forwards + # everything else. certbot's pre-order self-check reads DNS like any + # client; without this it queries a resolver that has never heard of the + # challenge names, so the check can only ever time out. + dns: + - 172.30.0.10 cap_add: - NET_ADMIN @@ -184,6 +196,12 @@ services: timeout: 3s retries: 10 start_period: 30s + # Resolve through the mock, which owns the test zones and forwards + # everything else. certbot's pre-order self-check reads DNS like any + # client; without this it queries a resolver that has never heard of the + # challenge names, so the check can only ever time out. + dns: + - 172.30.0.10 cap_add: - NET_ADMIN diff --git a/dstack/gateway/test-run/e2e/test.sh b/dstack/gateway/test-run/e2e/test.sh index 04962c93e..59e9f0ca7 100755 --- a/dstack/gateway/test-run/e2e/test.sh +++ b/dstack/gateway/test-run/e2e/test.sh @@ -348,6 +348,67 @@ test_persist_record_for_another_account_is_refused() { ! neg_domain_issued "${PERSIST_NEG2_DOMAIN}" } +# ---- The pre-order DNS self-check ----------------------------------------- +# +# certbot resolves the challenge name itself before telling the CA to go and +# look, so that a record that has not propagated is reported by name instead of +# as an order failure. The check is advisory: it warns and proceeds either way, +# which is exactly why nothing downstream reveals whether it worked. The mock +# logs the questions it is asked, so the check is observed directly. + +dns_queries_for() { + curl -sf "${MOCK_CF_API}/api/dns-queries" 2>/dev/null \ + | tr '{' '\n' \ + | grep -F "\"name\": \"$1\"" || true +} + +answered_queries_for() { + dns_queries_for "$1" | grep -cvF '"answers": 0' +} + +# The happy path, observed rather than triggered. By the time this runs the +# dns-persist-01 domain has issued from a record this suite published, so the +# self-check must have resolved it -- and an answered question is the only +# direct evidence, because the check warns and proceeds either way. +# +# Deliberately passive: forcing another renewal would race the periodic one, +# which picks a domain up as soon as it is added and can leave nothing for the +# forced run to do. +test_self_check_resolves_a_published_record() { + [ "$(answered_queries_for "_validation-persist.${PERSIST_DOMAIN}")" -gt 0 ] +} + +# The unhappy path, and the reason the check is advisory at all. A name with no +# record has to be polled and given up on -- the record may still be +# propagating -- rather than asked once and abandoned. +# +# Its own domain, and one nothing else queries, so this needs no clearing and +# does not care what ran before it. +test_self_check_gives_up_on_a_missing_record() { + local domain="selfcheck0.local" + local name="_validation-persist.${domain}" + admin_post DeleteZtDomain '{"domain": "'"${domain}"'"}' > /dev/null 2>&1 || true + # dns-persist-01 because nothing writes its record: the name stays empty for + # the whole wait without the test racing certbot's own cleanup. + admin_post AddZtDomain \ + '{"domain": "'"${domain}"'", "port": 443, "challenge": "dns-persist-01"}' \ + > /dev/null || return 1 + admin_post RenewZtDomainCert \ + '{"domain": "'"${domain}"'", "force": true}' > /dev/null 2>&1 || true + + local i=0 asked=0 + while [ $i -lt 45 ]; do + asked=$(dns_queries_for "$name" | wc -l) + [ "$asked" -ge 3 ] && break + sleep 2 + i=$((i + 1)) + done + # Polled, not asked once: the retry loop is what makes the wait a wait. + [ "$asked" -ge 3 ] || return 1 + # Every one a miss, or the record was not actually absent. + [ "$(answered_queries_for "$name")" -eq 0 ] +} + # ---- Gateway operations that change shape for such a domain --------------- # SetCaa reconciles CAA through the DNS provider, which the gateway has no @@ -654,6 +715,12 @@ main() { run_test "Rotation reports the records to republish" \ "$(test_rotation_reports_the_records_to_republish; echo $?)" + # The pre-order self-check, both ways round. + run_test "Self-check resolves a published challenge record" \ + "$(test_self_check_resolves_a_published_record; echo $?)" + run_test "Self-check polls and gives up when the record is absent" \ + "$(test_self_check_gives_up_on_a_missing_record; echo $?)" + # Summary log_section "Test Summary" log_info "Passed: $TESTS_PASSED" diff --git a/tools/mock-cf-dns/server.py b/tools/mock-cf-dns/server.py index 9208f9adb..d8c0e9253 100755 --- a/tools/mock-cf-dns/server.py +++ b/tools/mock-cf-dns/server.py @@ -14,6 +14,12 @@ DNS-01 and dns-persist-01 for real rather than being run with PEBBLE_VA_ALWAYS_VALID=1. TCP is not optional: Pebble sets its DNS client to `Net = "tcp"` whenever it is given `-dnsserver`. + +Questions outside the configured zones are forwarded upstream, so this can be a +client's only resolver rather than only a CA's `-dnsserver`: certbot resolves +its own challenge records through it while still reaching the other containers +by name. Every question is logged and served at /api/dns-queries, so a test can +assert that a name was actually looked up. """ from __future__ import annotations @@ -31,6 +37,10 @@ STATE_LOCK = threading.RLock() RECORDS: list[dict[str, Any]] = [] +# Every DNS question this mock was asked, so a test can assert that the client +# under test resolved a name rather than inferring it from what happened next. +QUERIES: list[dict[str, Any]] = [] +MAX_QUERIES = 2000 NEXT_ID = 1 @@ -110,6 +120,10 @@ def do_GET(self) -> None: # noqa: N802 with STATE_LOCK: _json(self, 200, {"records": RECORDS}) return + if path == "/api/dns-queries": + with STATE_LOCK: + _json(self, 200, {"queries": QUERIES}) + return if path.startswith("/client/v4/") and not _authorized(self): return if path == "/client/v4/zones": @@ -233,6 +247,55 @@ def txt_rdata(text: str) -> bytes: return b"".join(bytes([len(c)]) + c for c in chunks) +def _upstream() -> str: + """Where to send questions this mock is not authoritative for. + + Docker's embedded resolver, which is what this container's own + `/etc/resolv.conf` points at, so service names keep resolving for whoever + is pointed here. + """ + return os.environ.get("MOCK_DNS_UPSTREAM", "127.0.0.11") + + +def _is_ours(name: str) -> bool: + """Whether `name` falls inside one of the configured mock zones.""" + return any( + name == zone["name"] or name.endswith("." + zone["name"]) for zone in _zones() + ) + + +def _forward(packet: bytes) -> bytes: + """Ask the upstream resolver and hand back its answer verbatim. + + Without this the mock is only usable as a CA's `-dnsserver`, because a name + it does not know is answered NOERROR with no records -- which a resolver + reads as an authoritative "no such record" and does not retry elsewhere. + A client configured to use this as its only resolver would then fail to + resolve the other containers. + """ + sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + sock.settimeout(5) + try: + sock.sendto(packet, (_upstream(), 53)) + return sock.recvfrom(65535)[0] + finally: + sock.close() + + +def _record_query(name: str, qtype: int, answers: int, forwarded: bool) -> None: + with STATE_LOCK: + QUERIES.append( + { + "name": name, + "type": qtype, + "answers": answers, + "forwarded": forwarded, + "at": int(time.time()), + } + ) + del QUERIES[:-MAX_QUERIES] + + def dns_response(packet: bytes) -> bytes: """Build a DNS response containing matching TXT records.""" if len(packet) < 12: @@ -248,6 +311,18 @@ def dns_response(packet: bytes) -> bytes: if qend + 4 <= len(packet) else 16 ) + # Only answer for the zones this mock owns; anything else is the caller's + # ordinary name resolution and belongs upstream. + if not _is_ours(name): + try: + reply = _forward(packet) + _record_query(name, qtype, -1, True) + return reply + except Exception as exc: # pragma: no cover - diagnostic only + _debug(f"forwarding {name} failed: {exc}") + _record_query(name, qtype, 0, True) + return txid + struct.pack("!HHHHH", 0x8182, 1, 0, 0, 0) + packet[12:qend + 4] + with STATE_LOCK: answers = [ r @@ -256,6 +331,7 @@ def dns_response(packet: bytes) -> bytes: ] if qtype not in (16, 255): answers = [] + _record_query(name, qtype, len(answers), False) header = txid + struct.pack("!HHHHH", 0x8180, 1, len(answers), 0, 0) body = question for record in answers: From 5a825641609efbf1ffcfd1631ed6890dc80d8b77 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Thu, 27 Aug 2026 09:16:13 +0800 Subject: [PATCH 2/2] test(gateway): add the port a ZT domain needs, and cover the CAA guard rerun (#1146) Phase 4 has been adding the dns-01 domains without a port, which the server refuses -- `proto_to_zt_domain_config` requires 1..65535 -- and the call swallowed the error with `|| true`. So `test0.local` and friends were never configured, and everything downstream of them failed: CAA records reconciled for 0 domains (3 on dns-persist-01 left ...) That is 22 of the suite's assertions, reported as failures for long enough to read as the environment. They are not: nothing had been added to fail on. The port is supplied and the swallow becomes a warning, so the next time this breaks it says so. With a dns-01 domain that exists, the CAA guard sequence can be exercised. A run that dies between installing the `;` guards and dropping them leaves them behind, and the operator is told to rerun; the rerun has to work, and has to get there without ever lifting the deny-all, because a name with no issuer CAA is one any CA may issue for. The stranded state is planted directly -- it is exactly what the dead run left -- so this needs no way to kill a run midway. The half that matters is the second one. Both reusing the stranded guard and deleting it to add a fresh one end with the same records, and only one of them stays denied throughout, so the gap is invisible in the result. The mock therefore records names whose issuer CAA set emptied out, and the test asserts the domain is not among them. Reverting to delete-then-add fails it. --- dstack/gateway/test-run/e2e/test.sh | 59 ++++++++++++++++++++++++++++- tools/mock-cf-dns/server.py | 49 +++++++++++++++++++++++- 2 files changed, 106 insertions(+), 2 deletions(-) diff --git a/dstack/gateway/test-run/e2e/test.sh b/dstack/gateway/test-run/e2e/test.sh index 59e9f0ca7..b055a33c1 100755 --- a/dstack/gateway/test-run/e2e/test.sh +++ b/dstack/gateway/test-run/e2e/test.sh @@ -428,6 +428,60 @@ test_set_caa_skips_a_persist_domain() { | grep -qF '"type": "CAA"' } +# A run of SetCaa that dies between installing the `;` guards and dropping them +# leaves the guards behind, and the operator is told to rerun. The rerun has to +# work -- re-adding a byte-identical guard is what a provider that refuses +# duplicates rejects -- and it has to get there without ever lifting the +# deny-all, because a name with no issuer CAA is one any CA may issue for. +# +# The stranded state is planted directly: that is exactly what the dead run +# left, and it needs no way to kill a run mid-flight. +test_caa_rerun_recovers_without_lifting_deny_all() { + local domain="${CERT_DOMAINS%% *}" + local zone="zone-${domain//./-}" + local id tag + + # Plant the guards a dead run would have left, and remove the real records + # it had already deleted by that point. + for id in $(curl -sf "${MOCK_CF_API}/api/records" 2>/dev/null \ + | tr '{' '\n' \ + | grep -F "\"name\": \"${domain}\"" \ + | grep -F '"type": "CAA"' \ + | sed -e 's/.*"id": "//' -e 's/".*//'); do + curl -sf -X DELETE "${MOCK_CF_API}/client/v4/zones/${zone}/dns_records/${id}" \ + -H "Authorization: Bearer ${CF_API_TOKEN}" > /dev/null 2>&1 || true + done + for tag in issue issuewild; do + curl -sf -X POST "${MOCK_CF_API}/client/v4/zones/${zone}/dns_records" \ + -H "Authorization: Bearer ${CF_API_TOKEN}" \ + -H "Content-Type: application/json" \ + -d '{"type": "CAA", "name": "'"${domain}"'", "content": "0 '"${tag}"' \";\"", "ttl": 60}' \ + > /dev/null || return 1 + done + + # From here the name is deny-all, and must stay that way. + curl -sf -X DELETE "${MOCK_CF_API}/api/caa-gaps" > /dev/null 2>&1 || true + admin_post SetCaa '{}' > /dev/null || return 1 + + # Recovered: real issuer records, and none of the guards left behind. + local caa + caa=$(curl -sf "${MOCK_CF_API}/api/records" 2>/dev/null \ + | tr '{' '\n' \ + | grep -F "\"name\": \"${domain}\"" \ + | grep -F '"type": "CAA"') + echo "$caa" | grep -qF 'accounturi=' || return 1 + # An `if`, not `&& return 1`: under `set -e` a failing left-hand side makes + # the whole list non-zero and aborts the function. + if echo "$caa" | grep -qF '0 issue \";\"'; then + return 1 + fi + + # And never fell open on the way. This is the half that separates reusing + # the stranded guard from deleting it and adding a fresh one: both end here, + # only one of them stays denied throughout. + ! curl -sf "${MOCK_CF_API}/api/caa-gaps" 2>/dev/null | grep -qF "\"${domain}\"" +} + # Rotation registers a new account, and every published record names the old # one. The response has to carry the replacements, because the gateway cannot # publish them and nothing else reports them. @@ -546,7 +600,8 @@ setup_certbot_config() { curl -sf -X POST "${GATEWAY_ADMIN}/prpc/Admin.AddZtDomain" \ -H "${ADMIN_AUTH_HEADER}" \ -H "Content-Type: application/json" \ - -d '{"domain": "'"${domain}"'"}' > /dev/null || true + -d '{"domain": "'"${domain}"'", "port": 443}' > /dev/null \ + || log_warn "AddZtDomain failed for $domain (may already exist)" log_info "Triggering renewal for: $domain" curl -sf -X POST "${GATEWAY_ADMIN}/prpc/Admin.RenewZtDomainCert" \ @@ -712,6 +767,8 @@ main() { # Gateway operations that change shape for a domain it cannot write. run_test "SetCaa skips it instead of failing or writing" \ "$(test_set_caa_skips_a_persist_domain; echo $?)" + run_test "A stranded CAA guard is recovered without falling open" \ + "$(test_caa_rerun_recovers_without_lifting_deny_all; echo $?)" run_test "Rotation reports the records to republish" \ "$(test_rotation_reports_the_records_to_republish; echo $?)" diff --git a/tools/mock-cf-dns/server.py b/tools/mock-cf-dns/server.py index d8c0e9253..70fc8dd7c 100755 --- a/tools/mock-cf-dns/server.py +++ b/tools/mock-cf-dns/server.py @@ -10,6 +10,10 @@ POST /client/v4/zones//dns_records DELETE /client/v4/zones//dns_records/ +It tracks one thing beyond storing records: names whose issuer CAA set was +emptied, served at /api/caa-gaps, because replacing CAA is meant to leave one +record standing throughout and a gap is invisible once the replacement is done. + It also serves TXT answers on 53, over both UDP and TCP, so Pebble can validate DNS-01 and dns-persist-01 for real rather than being run with PEBBLE_VA_ALWAYS_VALID=1. TCP is not optional: Pebble sets its DNS client to @@ -42,6 +46,14 @@ QUERIES: list[dict[str, Any]] = [] MAX_QUERIES = 2000 NEXT_ID = 1 +# Names whose issuer CAA set went from non-empty to empty at some point. +# +# A client replacing those records is meant to keep at least one in place +# throughout: absent CAA is not "no issuer may" but "any issuer may", so a gap +# is a window in which any CA could have issued. It closes as soon as the +# replacement finishes and is invisible in the result, so it is recorded as it +# happens. +CAA_GAPS: set[str] = set() def _zones() -> list[dict[str, str]]: @@ -76,6 +88,27 @@ def _json(handler: BaseHTTPRequestHandler, code: int, body: Any) -> None: handler.wfile.write(data) +def _issuer_caa_count(name: str) -> int: + """How many issue/issuewild CAA records currently sit at `name`.""" + wanted = name.strip(".").lower() + return sum( + 1 + for r in RECORDS + if r["type"] == "CAA" + and r["name"].strip(".").lower() == wanted + and (r["content"].split() + ["", ""])[1] in ("issue", "issuewild") + ) + + +def _note_caa_gap(name: str, before: int) -> None: + """Record `name` if its issuer CAA set just emptied out. + + Called with STATE_LOCK held, `before` sampled ahead of the mutation. + """ + if before > 0 and _issuer_caa_count(name) == 0: + CAA_GAPS.add(name.strip(".").lower()) + + def _record_content(payload: dict[str, Any]) -> str: if "content" in payload: return str(payload.get("content") or "") @@ -116,6 +149,10 @@ def do_GET(self) -> None: # noqa: N802 if path == "/health": _json(self, 200, {"ok": True}) return + if path == "/api/caa-gaps": + with STATE_LOCK: + _json(self, 200, {"names": sorted(CAA_GAPS)}) + return if path == "/api/records": with STATE_LOCK: _json(self, 200, {"records": RECORDS}) @@ -200,9 +237,14 @@ def do_POST(self) -> None: # noqa: N802 _json(self, 200, {"success": True, "result": record}) def do_DELETE(self) -> None: # noqa: N802 - """Delete a mock DNS record.""" + """Delete a mock DNS record, or forget the CAA gaps seen so far.""" parsed = urllib.parse.urlparse(self.path) path = parsed.path.rstrip("/") or "/" + if path == "/api/caa-gaps": + with STATE_LOCK: + CAA_GAPS.clear() + _json(self, 200, {"success": True}) + return m = re.fullmatch(r"/client/v4/zones/([^/]+)/dns_records/([^/]+)", path) if not m: _json( @@ -215,9 +257,14 @@ def do_DELETE(self) -> None: # noqa: N802 return record_id = urllib.parse.unquote(m.group(2)) with STATE_LOCK: + doomed = next((r for r in RECORDS if r["id"] == record_id), None) + caa_name = doomed["name"] if doomed and doomed["type"] == "CAA" else None + caa_before = _issuer_caa_count(caa_name) if caa_name else 0 before = len(RECORDS) RECORDS[:] = [r for r in RECORDS if r["id"] != record_id] removed = before != len(RECORDS) + if caa_name: + _note_caa_gap(caa_name, caa_before) _json( self, 200,