From b41bf32ea0c1840bb2491ca522eae20e1c9989bf Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 28 Jul 2026 21:19:23 -0700 Subject: [PATCH 1/2] fix(dstack-ingress): renew one lineage per run, and size the timeout to the wait Two problems that compound each other. `certbot renew` renews *every* lineage in /etc/letsencrypt/renewal unless given --cert-name, and we never gave it one. run_pass calls this once per domain, so N domains meant N runs each covering all N lineages -- and renew_certificate then reported the result against whichever domain happened to ask, so domain A could be recorded as renewed because domain B was. Scope each run to the lineage it is named for. The run timeout was a hard-coded 300s. The dominant term inside a run is the DNS propagation wait, which the plugin sleeps through in-process -- and linode's own CERTBOT_PROPAGATION_SECONDS is 300, so on linode the cap could never be met and dns-01 timed out every time, on issuance as well as renewal. Before --cert-name it was worse still: the cap covered all lineages at once, so three cloudflare domains (120s each) could not fit either. Size it from the wait, and let CERTBOT_TIMEOUT override. --cert-name does not change the no-op wording renew_certificate parses: "No renewals were attempted." comes from _renew_describe_results, which runs for `renew` whether or not the lineage set was filtered. Checked against certbot 5.7.0, the version the image installs. Tested: 11 new cases in scripts/tests/test_certman.py covering scope (plain, wildcard, delegation, and certonly staying on -d) and the timeout (per-provider, delegation, override, invalid override, and a provider with no propagation setting). All five that assert the new behaviour fail against the previous code. --- custom-domain/dstack-ingress/README.md | 3 +- .../dstack-ingress/scripts/certman.py | 55 +++++++- .../scripts/tests/test_certman.py | 123 ++++++++++++++++++ 3 files changed, 177 insertions(+), 4 deletions(-) diff --git a/custom-domain/dstack-ingress/README.md b/custom-domain/dstack-ingress/README.md index 20c188c..515ad72 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 | 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 | 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..9ed0040 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -24,6 +24,11 @@ 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 + + class CertManager: """Certificate management using DNS provider infrastructure.""" @@ -303,6 +308,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 +349,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 +393,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 +426,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 +448,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 +484,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 +510,31 @@ 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. + """ + 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 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..97660a5 100644 --- a/custom-domain/dstack-ingress/scripts/tests/test_certman.py +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -149,6 +149,129 @@ 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=120)._certbot_timeout(), + 120 + 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"] = "200" + self.assertEqual( + _command_manager()._certbot_timeout(), + 200 + 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(), + 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=120)._certbot_timeout(), + 120 + certman.CERTBOT_TIMEOUT_HEADROOM, + f"{bad!r} should be ignored", + ) + + def test_provider_without_propagation_still_gets_headroom(self): + # route53 sets CERTBOT_PROPAGATION_SECONDS = None. + self.assertEqual( + _command_manager(propagation=None)._certbot_timeout(), + certman.CERTBOT_TIMEOUT_HEADROOM, + ) + + 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 From 4d62c579fa949bd75796e8ccb4ba019a39cc1da9 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 28 Jul 2026 22:07:13 -0700 Subject: [PATCH 2/2] fix(dstack-ingress): never let the sized timeout fall below the old cap Review follow-up, and a regression this PR introduced. route53 declares CERTBOT_PROPAGATION_SECONDS = None, so sizing purely from the wait gave it 0 + 180 = 180s where the fixed cap had allowed 300s. A route53 renewal that used to complete in between would now be killed. The point of sizing was to raise the cap where a provider needs more than 300s, not to lower it anywhere. Floor the derived value at the 300s it replaced. Nothing gets a smaller budget than before; linode goes 300 -> 480. An explicit CERTBOT_TIMEOUT is still taken literally, floor included, since that one is the operator's call. Tested: the route53 case now asserts the floor rather than the bare headroom, and a new case walks every propagation value we ship (None, 0, 30, 120, 300) asserting none of them ends up under 300. 24 cases in test_certman.py. --- custom-domain/dstack-ingress/README.md | 2 +- .../dstack-ingress/scripts/certman.py | 12 +++++- .../scripts/tests/test_certman.py | 37 ++++++++++++++----- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/custom-domain/dstack-ingress/README.md b/custom-domain/dstack-ingress/README.md index 515ad72..3fa658b 100644 --- a/custom-domain/dstack-ingress/README.md +++ b/custom-domain/dstack-ingress/README.md @@ -193,7 +193,7 @@ environment: | `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. The certbot run timeout is sized from this, so raising it is safe | -| `CERTBOT_TIMEOUT` | propagation wait + 180s | 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 | +| `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 9ed0040..e0d048e 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -28,6 +28,12 @@ def staging_enabled() -> bool: # 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.""" @@ -520,6 +526,10 @@ def _certbot_timeout(self) -> int: 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: @@ -533,7 +543,7 @@ def _certbot_timeout(self) -> int: propagation = getattr( self.provider, "CERTBOT_PROPAGATION_SECONDS", None) or 0 - return propagation + CERTBOT_TIMEOUT_HEADROOM + 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 97660a5..d8d238c 100644 --- a/custom-domain/dstack-ingress/scripts/tests/test_certman.py +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -226,8 +226,8 @@ class CertbotTimeoutTest(EnvTestCase): def test_timeout_covers_the_propagation_wait(self): self.assertEqual( - _command_manager(propagation=120)._certbot_timeout(), - 120 + certman.CERTBOT_TIMEOUT_HEADROOM, + _command_manager(propagation=400)._certbot_timeout(), + 400 + certman.CERTBOT_TIMEOUT_HEADROOM, ) def test_linode_default_propagation_fits(self): @@ -238,17 +238,18 @@ def test_linode_default_propagation_fits(self): def test_delegation_uses_its_own_propagation_setting(self): os.environ["DELEGATION_ZONE"] = "deleg.example.net" - os.environ["DELEGATION_PROPAGATION_SECONDS"] = "200" + os.environ["DELEGATION_PROPAGATION_SECONDS"] = "240" self.assertEqual( _command_manager()._certbot_timeout(), - 200 + certman.CERTBOT_TIMEOUT_HEADROOM, + 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(), - 120 + certman.CERTBOT_TIMEOUT_HEADROOM, + max(certman.CERTBOT_TIMEOUT_FLOOR, + 120 + certman.CERTBOT_TIMEOUT_HEADROOM), ) def test_explicit_override_wins(self): @@ -259,18 +260,34 @@ def test_invalid_override_is_ignored(self): for bad in ("0", "-5", "soon", ""): os.environ["CERTBOT_TIMEOUT"] = bad self.assertEqual( - _command_manager(propagation=120)._certbot_timeout(), - 120 + certman.CERTBOT_TIMEOUT_HEADROOM, + _command_manager(propagation=400)._certbot_timeout(), + 400 + certman.CERTBOT_TIMEOUT_HEADROOM, f"{bad!r} should be ignored", ) - def test_provider_without_propagation_still_gets_headroom(self): - # route53 sets CERTBOT_PROPAGATION_SECONDS = None. + 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_HEADROOM, + 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