refactor(dstack-ingress): remove what the delegation rework left behind

Publishing the CAA from inside the container made a chain of things dead
without anything failing, which is how they survived. A review found:

- delegation_verify_caa, and with it the only caller of ALLOW_MISSING_CAA
  and of dnsguide's --caa-required / --caa-advisory. Wildcards were the
  last user; once the container published their CAA too, the whole path
  was unreachable. The README still advertised ALLOW_MISSING_CAA.

- check_caa's require_present parameter and its three tests. The
  semantics it existed for -- "an absent CAA must block, because we
  cannot create it" -- stopped being true when we started creating it.
  Absence now just means "not published yet", and the next pass fixes it.

- Two test classes defined twice over. Python shadows the earlier
  definition silently, unittest counts the later one, and the total still
  goes up, so duplicated blocks look like passing tests. A guard now
  walks the file's AST and fails on any repeated class or method name;
  it was checked against a deliberately duplicated block.

- Stale naming: --challenge-alias and args.challenge_alias outlived the
  variable rename, and ACME_CHALLENGE_PROPAGATION_SECONDS with them. Both
  now say delegation. The README still claimed ACME_CHALLENGE_ALIAS
  "still works", which had not been true since it was removed.

Also reject contradictory --include values. `delegated` is a shape rather
than a record kind -- it replaces the per-kind records instead of adding
to them -- so `--include cname,delegated` would emit two CNAMEs claiming
the same name. Unknown values are now rejected too, instead of being
silently ignored.

Author: kvinwang

custom-domain/dstack-ingress/README.md

@@ -191,10 +191,9 @@ environment:
 | `EVIDENCE_SERVER` | `true` | Serve evidence files at `/evidences/` on the TLS port |
 | `EVIDENCE_PORT` | `80` | Internal port for evidence HTTP server |
 | `ALPN` | | TLS ALPN protocols (e.g. `h2,http/1.1`). Only set if backends support h2c |
-| `DELEGATION_ZONE` | | Zone this container writes into, so the DNS token needs no access to the served domain's own zone (see below). `ACME_CHALLENGE_ALIAS` is the original name and still works |
-| `ACME_CHALLENGE_PROPAGATION_SECONDS` | `120` | Wait after writing the delegated challenge TXT before validation. Must outlast the record TTL (60s), or a resolver still serving the previous attempt's value fails validation. Keep well under ~250s — certbot is killed after a 300s per-run timeout |
+| `DELEGATION_ZONE` | | Zone this container writes into, so the DNS token needs no access to the served domain's own zone (see below) |
+| `DELEGATION_PROPAGATION_SECONDS` | `120` | Wait after writing the delegated challenge TXT before validation. Must outlast the record TTL (60s), or a resolver still serving the previous attempt's value fails validation. Keep well under ~250s — certbot is killed after a 300s per-run timeout |
 | `DELEGATION_GATEWAY_RECORD` | per provider | How the delegated name points at the gateway: `cname` (Cloudflare default; follows a gateway that moves) or `a` (default elsewhere, for providers that refuse a CAA beside a CNAME) |
-| `ALLOW_MISSING_CAA` | `false` | In delegation mode, treat an unconfirmed `accounturi` CAA as a warning instead of a blocker. Default fails closed (see below) |
 
 For DNS provider credentials, see [DNS_PROVIDERS.md](DNS_PROVIDERS.md).
 

custom-domain/dstack-ingress/scripts/acme-dns-alias-hook.sh

@@ -46,7 +46,7 @@ case "$action" in
         # attempts. At 30s Let's Encrypt's multi-perspective check fails with
         # "During secondary validation: Incorrect TXT record found". The dns-01
         # plugin path waits 120s for the same reason.
-        sleep "${ACME_CHALLENGE_PROPAGATION_SECONDS:-120}"
+        sleep "${DELEGATION_PROPAGATION_SECONDS:-120}"
         ;;
     cleanup)
         echo "acme-dns-alias-hook: removing challenge TXT $record"

custom-domain/dstack-ingress/scripts/dns01.sh

@@ -214,36 +214,12 @@ delegation_guide() {
         --caa-name "$caa_domain" \
         --caa-tag "$caa_tag" \
         --challenge dns-01 \
-        --challenge-alias "$DELEGATION_ZONE" \
+        --delegation-zone "$DELEGATION_ZONE" \
         --mode "${DNS_SETUP_MODE:-wait}" \
         --include "$include" \
         "$@"
 }
 
-# The accounturi CAA is what stops anyone else who can satisfy the delegated
-# challenge from getting a certificate for this name, and we cannot write it, so
-# a confirmed-absent record is fatal. ALLOW_MISSING_CAA downgrades it to a
-# warning for operators who accept that risk.
-delegation_verify_caa() {
-    local domain="$1" account_file account_uri
-    if ! account_file=$(get_letsencrypt_account_file); then
-        echo "ERROR: cannot read the ACME account file to determine the required CAA" >&2
-        return 1
-    fi
-    account_uri=$(jq -j '.uri' "$account_file")
-    # Absent is the state to block on here, not "absent means unrestricted":
-    # we cannot create this record, so nothing else protects the name.
-    local gate=(--caa-required)
-    if [ "${ALLOW_MISSING_CAA:-false}" = "true" ]; then
-        gate=(--caa-required --caa-advisory)
-    fi
-
-    delegation_guide "$domain" caa \
-        --caa-value "letsencrypt.org;validationmethods=dns-01;accounturi=$account_uri" \
-        --account-uri "$account_uri" \
-        "${gate[@]}"
-}
-
 set_caa_record() {
     local domain="$1"
 

custom-domain/dstack-ingress/scripts/dnsguide.py

@@ -274,19 +274,12 @@ def check_caa(
     want_method: str,
     account_uri: str,
     resolvers: str,
-    require_present: bool = False,
 ) -> Tuple[bool, str]:
     """Check CAA for domain, walking up to the closest ancestor that has one.
 
     An absent CAA record set means every CA may issue (RFC 8659), so "no CAA
-    anywhere" is normally a pass: we are asking whether *we* may issue, and
-    nothing forbids it.
-
-    `require_present` inverts that for challenge delegation, where the question
-    is different. There the record is not a formality but the only thing stopping
-    someone else who can satisfy the delegated challenge from getting a
-    certificate for this name, and we hold no token to create it ourselves. An
-    unrestricted name is exactly the state that must block.
+    anywhere" is a pass: we are asking whether *we* may issue, and nothing
+    forbids it.
     """
     labels = _fqdn(domain).split(".")
     for i in range(len(labels) - 1):
@@ -320,11 +313,6 @@ def check_caa(
             reasons.append(reason)
         return False, f"CAA at {candidate} does not permit issuance: {'; '.join(reasons)}"
 
-    if require_present:
-        return False, (
-            "no CAA record set, so any account that can satisfy the delegated "
-            "challenge could obtain a certificate for this name"
-        )
     return True, "ok (no CAA record set; issuance is unrestricted)"
 
 
@@ -357,8 +345,6 @@ def verify_once(
     caa_method: str,
     account_uri: str,
     resolvers: str,
-    caa_advisory: bool = False,
-    caa_required: bool = False,
 ) -> List[Tuple[str, bool, str]]:
     results = []
     for rec in records:
@@ -371,18 +357,27 @@ def verify_once(
             else:
                 ok, why = check_alias(rec, resolvers)
             results.append((f"CNAME {rec.name}", ok, why))
-    ok, why = check_caa(
-        domain, caa_tag, caa_method, account_uri, resolvers, caa_required
-    )
-    if not ok and caa_advisory:
-        results.append((f"CAA {domain}", True, f"WARNING: {why} (continuing: CAA is advisory)"))
-    else:
-        results.append((f"CAA {domain}", ok, why))
+    ok, why = check_caa(domain, caa_tag, caa_method, account_uri, resolvers)
+    results.append((f"CAA {domain}", ok, why))
     return results
 
 
+# `delegated` is a shape, not a record kind: it replaces the per-kind records
+# rather than adding to them. Asking for both would emit two CNAMEs for the same
+# name -- one at the gateway, one at the delegation zone -- so refuse instead.
+PER_KIND = {"cname", "txt", "caa", "challenge-cname"}
+
+
 def build_records(args: argparse.Namespace) -> List[Record]:
     include = {part.strip() for part in args.include.split(",") if part.strip()}
+    unknown = include - PER_KIND - {"delegated"}
+    if unknown:
+        raise SystemExit(f"unknown --include value(s): {', '.join(sorted(unknown))}")
+    if "delegated" in include and include & (PER_KIND - {"caa"}):
+        raise SystemExit(
+            "--include delegated cannot be combined with cname/txt/challenge-cname: "
+            "delegated already covers them, and both would claim the same names"
+        )
     records = []
     if "cname" in include:
         records.append(
@@ -402,7 +397,7 @@ def build_records(args: argparse.Namespace) -> List[Record]:
                 note="tells the gateway which instance to route this hostname to",
             )
         )
-    if "delegated" in include and args.challenge_alias:
+    if "delegated" in include and args.delegation_zone:
         # Full delegation: every name this deployment needs is aliased into a
         # zone the container can write, so the operator creates these three once
         # and never touches DNS again -- not when the app id changes, not when
@@ -412,7 +407,7 @@ def build_records(args: argparse.Namespace) -> List[Record]:
         # address to fall back to and a wrong target must be reported as wrong
         # rather than as "does not resolve".
         base = args.domain[2:] if args.domain.startswith("*.") else args.domain
-        alias = args.challenge_alias
+        alias = args.delegation_zone
         wanted = [
             (args.domain, "routes traffic for this hostname into the delegated zone"),
             (args.txt_name, "lets the container publish the gateway routing target"),
@@ -438,7 +433,7 @@ def build_records(args: argparse.Namespace) -> List[Record]:
                     exact=True,
                 )
             )
-    if "challenge-cname" in include and args.challenge_alias:
+    if "challenge-cname" in include and args.delegation_zone:
         # Challenge delegation: the CA follows this CNAME when it looks for
         # _acme-challenge, so the TXT can live in a zone whose token we hold and
         # the served zone never needs one. certbot validates at the base name
@@ -448,7 +443,7 @@ def build_records(args: argparse.Namespace) -> List[Record]:
             Record(
                 type="CNAME",
                 name=f"_acme-challenge.{base}",
-                value=f"_acme-challenge.{base}.{args.challenge_alias}",
+                value=f"_acme-challenge.{base}.{args.delegation_zone}",
                 note="delegates the ACME challenge to a zone this container can write",
                 exact=True,
             )
@@ -480,20 +475,9 @@ def main() -> int:
     parser.add_argument("--account-uri", default="")
     parser.add_argument("--challenge", default="tls-alpn-01")
     parser.add_argument(
-        "--challenge-alias",
+        "--delegation-zone",
         default=os.environ.get("DELEGATION_ZONE", ""),
-        help="delegation zone for the _acme-challenge CNAME (dns-01 delegation)",
-    )
-    parser.add_argument(
-        "--caa-required",
-        action="store_true",
-        help="treat an absent CAA record set as a failure (challenge delegation: "
-             "the record is the only protection and we cannot create it)",
-    )
-    parser.add_argument(
-        "--caa-advisory",
-        action="store_true",
-        help="report a failing CAA check as a warning instead of blocking",
+        help="zone this deployment is delegated into (dns-01 delegation)",
     )
     parser.add_argument(
         "--mode",
@@ -566,8 +550,7 @@ def main() -> int:
         try:
             results = verify_once(
                 records, args.domain.lstrip("*."), args.caa_tag, args.challenge,
-                args.account_uri, args.resolver, args.caa_advisory,
-                args.caa_required,
+                args.account_uri, args.resolver,
             )
         except ResolveError as exc:
             print(f"[dns-check #{attempt}] resolver problem: {exc}", flush=True)

custom-domain/dstack-ingress/scripts/tests/test_dnsguide.py

@@ -128,7 +128,7 @@ def _args(self, domain, alias):
             domain=domain, alias_target="gw.example.net",
             txt_name="_dstack-app-address." + domain.lstrip("*."),
             txt_value="appid:443", caa_name="", caa_tag="issue", caa_value="",
-            account_uri="", challenge="dns-01", challenge_alias=alias,
+            account_uri="", challenge="dns-01", delegation_zone=alias,
             include="challenge-cname",
         )
 
@@ -154,7 +154,7 @@ def _records(self, domain, txt):
         return dnsguide.build_records(argparse.Namespace(
             domain=domain, alias_target="gw", txt_name=txt, txt_value="x",
             caa_name="", caa_tag="issue", caa_value="", account_uri="",
-            challenge="dns-01", challenge_alias="deleg.net", include="delegated"))
+            challenge="dns-01", delegation_zone="deleg.net", include="delegated"))
 
     def test_wildcard_aliases_its_base_as_well(self):
         names = [r.name for r in self._records(
@@ -199,48 +199,6 @@ def test_reports_absence_without_claiming_it_does_not_resolve(self):
         self.assertIn("no CNAME record found", why)
 
 
-class TestCaaRequiredForDelegation(unittest.TestCase):
-    """Delegation inverts the usual CAA question.
-
-    Normally we ask "does anything forbid us from issuing", and no CAA at all
-    means no. Under challenge delegation the record is the only thing stopping
-    someone else who can satisfy the delegated challenge, and we cannot create
-    it, so its absence has to block.
-    """
-
-    def _no_caa(self):
-        saved = dnsguide.query_union
-        dnsguide.query_union = lambda name, rr, r: []
-        self.addCleanup(lambda: setattr(dnsguide, "query_union", saved))
-
-    def test_absent_caa_passes_by_default(self):
-        self._no_caa()
-        ok, why = dnsguide.check_caa("app.example.com", "issue", "dns-01", "", "r")
-        self.assertTrue(ok)
-        self.assertIn("unrestricted", why)
-
-    def test_absent_caa_blocks_when_required(self):
-        self._no_caa()
-        ok, why = dnsguide.check_caa(
-            "app.example.com", "issue", "dns-01", "", "r", require_present=True
-        )
-        self.assertFalse(ok)
-        self.assertIn("no CAA record set", why)
-
-    def test_present_and_matching_passes_when_required(self):
-        saved = dnsguide.query_union
-        dnsguide.query_union = lambda name, rr, r: (
-            ['0 issue "letsencrypt.org;validationmethods=dns-01;accounturi=https://acme/1"']
-            if name == "app.example.com" else []
-        )
-        self.addCleanup(lambda: setattr(dnsguide, "query_union", saved))
-        ok, _ = dnsguide.check_caa(
-            "app.example.com", "issue", "dns-01", "https://acme/1", "r",
-            require_present=True,
-        )
-        self.assertTrue(ok)
-
-
 class TestTxtNormalisation(unittest.TestCase):
     def test_strips_quotes(self):
         self.assertEqual(dnsguide._unquote_txt('"abc:443"'), "abc:443")
@@ -283,5 +241,25 @@ def test_caa_omitted_without_value(self):
         self.assertEqual(types, ["CNAME", "TXT"])
 
 
+class TestNoShadowedDefinitions(unittest.TestCase):
+    """A duplicated class or method silently shadows the earlier one.
+
+    Python does not complain, unittest does not notice, and the count still
+    goes up -- so a merge that lands the same block twice leaves dead tests
+    that look like passing ones. This caught exactly that.
+    """
+
+    def test_every_definition_is_unique(self):
+        import ast, collections, pathlib
+
+        tree = ast.parse(pathlib.Path(__file__).read_text())
+        names = [n.name for n in tree.body if isinstance(n, ast.ClassDef)]
+        for cls in (n for n in tree.body if isinstance(n, ast.ClassDef)):
+            names += [f"{cls.name}.{m.name}" for m in cls.body
+                      if isinstance(m, (ast.FunctionDef, ast.AsyncFunctionDef))]
+        dupes = [n for n, c in collections.Counter(names).items() if c > 1]
+        self.assertEqual(dupes, [], f"defined more than once: {dupes}")
+
+
 if __name__ == "__main__":
     unittest.main(verbosity=2)