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..b055a33c1 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 @@ -367,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. @@ -485,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" \ @@ -651,9 +767,17 @@ 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 $?)" + # 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..70fc8dd7c 100755 --- a/tools/mock-cf-dns/server.py +++ b/tools/mock-cf-dns/server.py @@ -10,10 +10,20 @@ 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 `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,7 +41,19 @@ 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 +# 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]]: @@ -66,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 "") @@ -106,10 +149,18 @@ 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}) 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": @@ -186,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( @@ -201,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, @@ -233,6 +294,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 +358,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 +378,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: