diff --git a/custom-domain/dstack-ingress/README.md b/custom-domain/dstack-ingress/README.md index 20c188c..3fa658b 100644 --- a/custom-domain/dstack-ingress/README.md +++ b/custom-domain/dstack-ingress/README.md @@ -192,7 +192,8 @@ environment: | `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) | -| `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_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. The certbot run timeout is sized from this, so raising it is safe | +| `CERTBOT_TIMEOUT` | propagation wait + 180s, never below 300s | Seconds a single certbot run may take. The default is derived from the provider's propagation wait (or `DELEGATION_PROPAGATION_SECONDS`), which is what dominates it. Set this only if a run needs longer still; an explicit value is used as given, floor included | For DNS provider credentials, see [DNS_PROVIDERS.md](DNS_PROVIDERS.md). diff --git a/custom-domain/dstack-ingress/scripts/certman.py b/custom-domain/dstack-ingress/scripts/certman.py index 12f08a7..e0d048e 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -24,6 +24,17 @@ def staging_enabled() -> bool: return value == "true" +# Time a certbot run needs on top of the DNS propagation wait: ACME round +# trips, plugin setup, the cleanup hook, and certbot's own bookkeeping. +CERTBOT_TIMEOUT_HEADROOM = 180 + +# Never allow less than the cap this replaced. route53 declares no propagation +# wait at all, so sizing purely from the wait would cut its budget from 300s to +# 180s and fail renewals that used to fit. The sizing exists to raise the cap +# where a provider needs more, not to lower it anywhere. +CERTBOT_TIMEOUT_FLOOR = 300 + + class CertManager: """Certificate management using DNS provider infrastructure.""" @@ -303,6 +314,8 @@ def _build_certbot_command(self, action: str, domain: str, email: str) -> List[s # For `renew`, certbot reuses the authenticator + hooks saved in the # renewal config from the initial `certonly`, so we don't re-specify # them here (and must not fall back to the DNS plugin). + if action == "renew": + base_cmd.extend(["--cert-name", self._lineage_name(domain)]) if staging_enabled(): base_cmd.append("--staging") masked = [a if not (i > 0 and base_cmd[i - 1] == "--email") else "" @@ -342,6 +355,12 @@ def _build_certbot_command(self, action: str, domain: str, email: str) -> List[s else: base_cmd.append("--register-unsafely-without-email") base_cmd.extend(["-d", domain]) + if action == "renew": + # Without this, `certbot renew` renews *every* lineage in + # /etc/letsencrypt/renewal. run_pass calls this once per domain, so + # N domains meant N passes over all N lineages, and the result was + # then reported against whichever domain happened to ask. + base_cmd.extend(["--cert-name", self._lineage_name(domain)]) if staging_enabled(): base_cmd.extend(["--staging"]) @@ -380,10 +399,11 @@ def obtain_certificate(self, domain: str, email: str) -> bool: return False cmd = self._build_certbot_command("certonly", domain, email) + timeout = self._certbot_timeout() try: result = subprocess.run( - cmd, capture_output=True, text=True, timeout=300) + cmd, capture_output=True, text=True, timeout=timeout) if result.returncode == 0: print(f"✓ Certificate obtained successfully for {domain}") @@ -412,7 +432,8 @@ def obtain_certificate(self, domain: str, email: str) -> bool: return False except subprocess.TimeoutExpired: - print(f"Certbot command timed out after 300 seconds", file=sys.stderr) + print(f"Certbot command timed out after {timeout} seconds", + file=sys.stderr) return False except Exception as e: print(f"Error running certbot: {e}", file=sys.stderr) @@ -433,10 +454,11 @@ def renew_certificate(self, domain: str) -> Tuple[bool, bool]: self.apply_renewal_window(domain) cmd = self._build_certbot_command("renew", domain, "") + timeout = self._certbot_timeout() try: result = subprocess.run( - cmd, capture_output=True, text=True, timeout=300) + cmd, capture_output=True, text=True, timeout=timeout) stdout_output = result.stdout.strip() if result.stdout else "" error_output = result.stderr.strip() if result.stderr else "" @@ -468,6 +490,14 @@ def renew_certificate(self, domain: str) -> Tuple[bool, bool]: return False, False + except subprocess.TimeoutExpired: + print( + f"Certbot renew for {domain} timed out after {timeout} seconds. " + f"If this provider needs a longer DNS propagation wait, raise " + f"CERTBOT_TIMEOUT.", + file=sys.stderr, + ) + return False, False except Exception as e: print(f"Error running certbot: {e}", file=sys.stderr) return False, False @@ -486,6 +516,35 @@ def _renewal_conf_path(self, domain: str) -> str: """Where certbot keeps this lineage's renewal config.""" return f"/etc/letsencrypt/renewal/{self._lineage_name(domain)}.conf" + def _certbot_timeout(self) -> int: + """How long one certbot run may take. + + The dominant term is the DNS propagation wait, which the plugin -- or, + under delegation, the auth hook -- sleeps through inside the process. + A single fixed cap cannot fit every provider: linode's own default + propagation is 300s, so a 300s cap could never be met and dns-01 on + linode timed out every time, on issuance as well as renewal. + + Size the cap from the wait instead, and let a deployment override it. + Never below CERTBOT_TIMEOUT_FLOOR: a provider that declares no + propagation wait (route53) still spends time on ACME round trips, and + this change should not shorten anyone's budget. An explicit + CERTBOT_TIMEOUT is taken literally -- that one is the operator's call. + """ + override = os.environ.get("CERTBOT_TIMEOUT", "").strip() + if override.isdigit() and int(override) > 0: + return int(override) + + if os.environ.get("DELEGATION_ZONE", "").strip(): + # Kept in step with acme-dns-alias-hook.sh, which does the sleeping. + wait = os.environ.get("DELEGATION_PROPAGATION_SECONDS", "").strip() + propagation = int(wait) if wait.isdigit() else 120 + else: + propagation = getattr( + self.provider, "CERTBOT_PROPAGATION_SECONDS", None) or 0 + + return max(CERTBOT_TIMEOUT_FLOOR, propagation + CERTBOT_TIMEOUT_HEADROOM) + def apply_renewal_window(self, domain: str) -> None: """Make RENEW_DAYS_BEFORE mean the same thing here as it does for lego. diff --git a/custom-domain/dstack-ingress/scripts/tests/test_certman.py b/custom-domain/dstack-ingress/scripts/tests/test_certman.py index 3bbef2d..d8d238c 100644 --- a/custom-domain/dstack-ingress/scripts/tests/test_certman.py +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -149,6 +149,146 @@ def test_wildcard_uses_the_bare_lineage_name(self): ) +class FakeProvider: + CERTBOT_PLUGIN = "dns-cloudflare" + CERTBOT_CREDENTIALS_FILE = None + CERTBOT_PROPAGATION_SECONDS = 120 + + +def _command_manager(propagation=120) -> certman.CertManager: + """A CertManager that can build commands without a real provider or venv.""" + mgr = object.__new__(certman.CertManager) + provider = FakeProvider() + provider.CERTBOT_PROPAGATION_SECONDS = propagation + mgr.provider = provider + mgr.provider_type = "cloudflare" + mgr._get_certbot_command = lambda: ["certbot"] # noqa: SLF001 + return mgr + + +class EnvTestCase(unittest.TestCase): + """Restore every environment variable a test touches.""" + + VARS = ( + "CERTBOT_TIMEOUT", + "DELEGATION_ZONE", + "DELEGATION_PROPAGATION_SECONDS", + "ACME_STAGING", + "CERTBOT_STAGING", + ) + + def setUp(self): + self._saved = {v: os.environ.get(v) for v in self.VARS} + for v in self.VARS: + os.environ.pop(v, None) + + def tearDown(self): + for v, old in self._saved.items(): + if old is None: + os.environ.pop(v, None) + else: + os.environ[v] = old + + +class RenewScopeTest(EnvTestCase): + """`certbot renew` renews every lineage unless told otherwise. + + run_pass calls this once per domain, so without --cert-name N domains meant + N runs over all N lineages, and each run's result was reported against + whichever domain happened to ask for it. + """ + + def test_renew_is_scoped_to_one_lineage(self): + cmd = _command_manager()._build_certbot_command("renew", "a.example.com", "") + self.assertIn("--cert-name", cmd) + self.assertEqual(cmd[cmd.index("--cert-name") + 1], "a.example.com") + + def test_renew_uses_the_bare_name_for_a_wildcard(self): + cmd = _command_manager()._build_certbot_command("renew", "*.example.com", "") + self.assertEqual(cmd[cmd.index("--cert-name") + 1], "example.com") + + def test_certonly_is_scoped_by_d_not_cert_name(self): + cmd = _command_manager()._build_certbot_command( + "certonly", "a.example.com", "") + self.assertNotIn("--cert-name", cmd) + self.assertIn("-d", cmd) + + def test_delegation_renew_is_scoped_too(self): + os.environ["DELEGATION_ZONE"] = "deleg.example.net" + cmd = _command_manager()._build_certbot_command("renew", "a.example.com", "") + self.assertEqual(cmd[cmd.index("--cert-name") + 1], "a.example.com") + # renew must not re-specify the authenticator; it is in the lineage. + self.assertNotIn("--manual", cmd) + + +class CertbotTimeoutTest(EnvTestCase): + """A fixed 300s cap could not fit every provider's own propagation wait.""" + + def test_timeout_covers_the_propagation_wait(self): + self.assertEqual( + _command_manager(propagation=400)._certbot_timeout(), + 400 + certman.CERTBOT_TIMEOUT_HEADROOM, + ) + + def test_linode_default_propagation_fits(self): + # linode's CERTBOT_PROPAGATION_SECONDS is 300, so the old 300s cap could + # never be met: issuance and renewal timed out every time. + timeout = _command_manager(propagation=300)._certbot_timeout() + self.assertGreater(timeout, 300) + + def test_delegation_uses_its_own_propagation_setting(self): + os.environ["DELEGATION_ZONE"] = "deleg.example.net" + os.environ["DELEGATION_PROPAGATION_SECONDS"] = "240" + self.assertEqual( + _command_manager()._certbot_timeout(), + 240 + certman.CERTBOT_TIMEOUT_HEADROOM, + ) + + def test_delegation_falls_back_to_the_hook_default(self): + os.environ["DELEGATION_ZONE"] = "deleg.example.net" + self.assertEqual( + _command_manager()._certbot_timeout(), + max(certman.CERTBOT_TIMEOUT_FLOOR, + 120 + certman.CERTBOT_TIMEOUT_HEADROOM), + ) + + def test_explicit_override_wins(self): + os.environ["CERTBOT_TIMEOUT"] = "900" + self.assertEqual(_command_manager(propagation=300)._certbot_timeout(), 900) + + def test_invalid_override_is_ignored(self): + for bad in ("0", "-5", "soon", ""): + os.environ["CERTBOT_TIMEOUT"] = bad + self.assertEqual( + _command_manager(propagation=400)._certbot_timeout(), + 400 + certman.CERTBOT_TIMEOUT_HEADROOM, + f"{bad!r} should be ignored", + ) + + def test_an_override_below_the_floor_is_still_honoured(self): + os.environ["CERTBOT_TIMEOUT"] = "60" + self.assertEqual(_command_manager(propagation=300)._certbot_timeout(), 60) + + def test_provider_without_propagation_keeps_the_old_budget(self): + """route53 declares no propagation wait. + + Sizing purely from the wait would cut it from 300s to 180s and fail + renewals that used to fit -- a regression introduced by the fix. + """ + self.assertEqual( + _command_manager(propagation=None)._certbot_timeout(), + certman.CERTBOT_TIMEOUT_FLOOR, + ) + + def test_no_provider_ends_up_below_the_previous_fixed_cap(self): + for propagation in (None, 0, 30, 120, 300): + self.assertGreaterEqual( + _command_manager(propagation=propagation)._certbot_timeout(), + 300, + f"propagation={propagation} shortened the budget", + ) + + class NoDuplicateDefinitionsTest(unittest.TestCase): """Python shadows a repeated class or method silently; unittest counts the later one and the total still goes up, so a duplicated block looks like