From ea44c2c8e4f6f658c0fdb5087b890e65ee203154 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 28 Jul 2026 21:14:58 -0700 Subject: [PATCH 1/3] fix(dstack-ingress): let RENEW_DAYS_BEFORE be unset again The value is written into certbot's lineage config, which lives in the certificate volume and outlives the container, and apply_renewal_window returned early when the variable was absent. So the setting could be turned on but never off: `renew_before_expiry = 365 days` survived, every subsequent pass performed a real renewal, and the certificate stayed permanently due. Its own docstring said "leaving it unset keeps certbot's own default", and both README and TESTING.md tell you to unset it once you have seen the renewal branch you came for. None of that was true. Clearing it now removes the active setting and leaves certbot's commented-out template alone, and the file is only rewritten when it would actually change -- a no-op pass no longer touches it. Found while testing 2.3: after exercising the forced-renewal branch, removing the variable left the lineage due, so the next passes renewed for real and two of them hit certbot's 300s cap. Tested: new scripts/tests/test_certman.py, 9 cases. The regression case fails against the previous code and passes against this one. --- custom-domain/dstack-ingress/README.md | 2 +- .../dstack-ingress/scripts/certman.py | 47 ++++-- .../scripts/tests/test_certman.py | 144 ++++++++++++++++++ 3 files changed, 182 insertions(+), 11 deletions(-) create mode 100644 custom-domain/dstack-ingress/scripts/tests/test_certman.py diff --git a/custom-domain/dstack-ingress/README.md b/custom-domain/dstack-ingress/README.md index 9db66d8..20c188c 100644 --- a/custom-domain/dstack-ingress/README.md +++ b/custom-domain/dstack-ingress/README.md @@ -181,7 +181,7 @@ environment: | `DOH_RESOLVERS` | Google + Cloudflare | Comma-separated DoH endpoints used to verify records | | `TLSALPN_PORT` | `9443` | Loopback port lego's ACME responder binds to | | `TLS_TERMINATE_PORT` | `9444` | Loopback port the TLS frontend moves to in tls-alpn-01 mode | -| `RENEW_DAYS_BEFORE` | client default | Days of remaining lifetime that trigger renewal. Applies to both modes: passed to lego as `--renew-days`, and written to certbot's `renew_before_expiry` | +| `RENEW_DAYS_BEFORE` | client default | Days of remaining lifetime that trigger renewal. Applies to both modes: passed to lego as `--renew-days`, and written to certbot's `renew_before_expiry`. Unsetting it removes that setting again, so the client's own default applies | | `RENEW_INTERVAL` | `43200` | Seconds between successful certificate passes | | `DNS_SETTLE_SECONDS` | `30` | Wait after DNS verifies, so the gateway's own TXT cache expires | | `MAXCONN` | `4096` | HAProxy max connections | diff --git a/custom-domain/dstack-ingress/scripts/certman.py b/custom-domain/dstack-ingress/scripts/certman.py index d3993ac..bd87019 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -482,6 +482,10 @@ def _lineage_name(domain: str) -> str: """certbot stores a wildcard lineage under the bare name.""" return domain[2:] if domain.startswith("*.") else domain + 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 apply_renewal_window(self, domain: str) -> None: """Make RENEW_DAYS_BEFORE mean the same thing here as it does for lego. @@ -491,12 +495,14 @@ def apply_renewal_window(self, domain: str) -> None: runs. Without this the variable is silently tls-alpn-01 only, which also left the dns-01 renewal branch with no way to be exercised on demand. - Leaving it unset keeps certbot's own default (30 days). + Unsetting it has to remove the setting again, not merely stop writing + it: the lineage config lives in the certificate volume and outlives the + container, so a value written once would otherwise be permanent, and + the documented way to leave the renewal window -- unset the variable -- + would do nothing while the certificate stayed permanently due. """ days = os.environ.get("RENEW_DAYS_BEFORE", "").strip() - if not days: - return - if not days.isdigit() or int(days) < 1: + if days and (not days.isdigit() or int(days) < 1): print( f"Warning: ignoring invalid RENEW_DAYS_BEFORE={days!r} " f"(expected a positive number of days)", @@ -504,13 +510,13 @@ def apply_renewal_window(self, domain: str) -> None: ) return - path = f"/etc/letsencrypt/renewal/{self._lineage_name(domain)}.conf" + path = self._renewal_conf_path(domain) if not os.path.isfile(path): # No lineage yet: the first issuance has not happened, and certbot # writes this file itself. Nothing to do. return - setting = f"renew_before_expiry = {days} days" + setting = f"renew_before_expiry = {days} days" if days else None try: with open(path, encoding="utf-8") as fh: lines = fh.read().splitlines() @@ -518,23 +524,37 @@ def apply_renewal_window(self, domain: str) -> None: print(f"Warning: cannot read {path}: {exc}", file=sys.stderr) return - out, replaced = [], False + out, replaced, removed = [], False, False for line in lines: - # The key ships commented out, so match both forms. - if re.match(r"\s*#?\s*renew_before_expiry\s*=", line): + # The key ships commented out, so match both forms when writing. + # When clearing, drop only the active setting: the commented + # template is certbot's own and carries no value. + active = re.match(r"\s*renew_before_expiry\s*=", line) + if setting is not None and re.match( + r"\s*#?\s*renew_before_expiry\s*=", line + ): if not replaced: out.append(setting) replaced = True continue + if setting is None and active: + removed = True + continue out.append(line) - if not replaced: + if setting is not None and not replaced: # Must land before the first section header; the key is top-level. insert_at = next( (i for i, line in enumerate(out) if line.strip().startswith("[")), len(out), ) out.insert(insert_at, setting) + replaced = True + + if out == lines: + # Nothing to do: already correct, or already absent. Rewriting the + # file every pass would only add noise and a chance to corrupt it. + return try: with open(path, "w", encoding="utf-8") as fh: @@ -542,6 +562,13 @@ def apply_renewal_window(self, domain: str) -> None: except OSError as exc: print(f"Warning: cannot write {path}: {exc}", file=sys.stderr) return + + if removed: + print( + f"Renewal window for {domain} cleared; " + f"certbot's default applies again" + ) + return print(f"Renewal window for {domain} set to {days} days before expiry") def acme_account_exists(self) -> bool: diff --git a/custom-domain/dstack-ingress/scripts/tests/test_certman.py b/custom-domain/dstack-ingress/scripts/tests/test_certman.py new file mode 100644 index 0000000..4b744df --- /dev/null +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -0,0 +1,144 @@ +#!/usr/bin/env python3 +"""Unit tests for certman's renewal-window handling. + +Run: python3 scripts/tests/test_certman.py +""" + +import os +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")) + +import certman # noqa: E402 + +# A lineage config as certbot writes it: the key ships commented out, and +# everything after the first section header is out of bounds for a top-level key. +CERTBOT_CONF = """\ +version = 5.7.0 +archive_dir = /etc/letsencrypt/archive/example.com +cert = /etc/letsencrypt/live/example.com/cert.pem +# renew_before_expiry = 30 days + +[renewalparams] +authenticator = dns-cloudflare +""" + + +def _manager(path: str) -> certman.CertManager: + """A CertManager that reads and writes one temp file. + + __init__ builds a provider from the environment; none of that is involved + in rewriting a config file, so bypass it. + """ + mgr = object.__new__(certman.CertManager) + mgr._renewal_conf_path = lambda domain: path # noqa: SLF001 + return mgr + + +class RenewalWindowTest(unittest.TestCase): + def setUp(self): + self._saved = os.environ.get("RENEW_DAYS_BEFORE") + fd, self.path = tempfile.mkstemp(suffix=".conf") + os.close(fd) + self.write(CERTBOT_CONF) + + def tearDown(self): + os.unlink(self.path) + if self._saved is None: + os.environ.pop("RENEW_DAYS_BEFORE", None) + else: + os.environ["RENEW_DAYS_BEFORE"] = self._saved + + def write(self, text): + with open(self.path, "w", encoding="utf-8") as fh: + fh.write(text) + + def read(self): + with open(self.path, encoding="utf-8") as fh: + return fh.read() + + def apply(self, value): + if value is None: + os.environ.pop("RENEW_DAYS_BEFORE", None) + else: + os.environ["RENEW_DAYS_BEFORE"] = value + _manager(self.path).apply_renewal_window("example.com") + + def test_setting_is_written_above_the_first_section(self): + self.apply("365") + body = self.read() + self.assertIn("renew_before_expiry = 365 days", body) + self.assertLess( + body.index("renew_before_expiry"), body.index("[renewalparams]") + ) + + def test_setting_replaces_a_previous_value_without_duplicating(self): + self.apply("365") + self.apply("45") + body = self.read() + self.assertIn("renew_before_expiry = 45 days", body) + self.assertNotIn("365", body) + self.assertEqual(body.count("renew_before_expiry"), 1) + + def test_unsetting_removes_a_previously_written_value(self): + """The regression: the lineage config outlives the container. + + Leaving the setting behind kept the certificate permanently due, and + the documented way out -- unset the variable -- did nothing. + """ + self.apply("365") + self.assertIn("renew_before_expiry = 365 days", self.read()) + self.apply(None) + self.assertNotIn("renew_before_expiry = 365 days", self.read()) + + def test_unsetting_leaves_certbots_commented_template_alone(self): + self.apply(None) + self.assertIn("# renew_before_expiry = 30 days", self.read()) + + def test_unsetting_on_an_untouched_config_changes_nothing(self): + self.apply(None) + self.assertEqual(self.read(), CERTBOT_CONF) + + def test_invalid_value_leaves_the_config_alone(self): + self.apply("365") + before = self.read() + for bad in ("0", "-1", "later", "30 days"): + self.apply(bad) + self.assertEqual(self.read(), before, f"{bad!r} should be ignored") + + def test_missing_lineage_is_not_an_error(self): + os.unlink(self.path) + self.apply("365") + self.assertFalse(os.path.exists(self.path)) + open(self.path, "w").close() # tearDown unlinks it + + def test_wildcard_uses_the_bare_lineage_name(self): + self.assertEqual( + certman.CertManager._lineage_name("*.example.com"), "example.com" + ) + self.assertEqual( + certman.CertManager._lineage_name("example.com"), "example.com" + ) + + +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 + passing tests. Same guard as test_dnsguide.py.""" + + def test_no_duplicate_definitions(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) From 813b8cf1832871dfec05e043ec8f9cd9c483d2fe Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 28 Jul 2026 22:06:03 -0700 Subject: [PATCH 2/3] fix(dstack-ingress): rename the renewal config into place, and say who owns it Review follow-up. Writing with open(path, "w") truncates the live lineage config before anything replaces it, so a crash or a full disk mid-write leaves certbot with a config it cannot parse -- and a lineage it cannot parse is one it cannot renew. The except OSError branch could only report that; the content was already gone. Write a temporary file, fsync it, and os.replace it over the original, so a reader sees the old file or the new one and never a half-written one. Clearing is also deliberately not ownership-aware: with the variable unset we drop any active renew_before_expiry, including one a human put there, because nothing distinguishes the two. That is fine for the volume this image manages and wrong for a certbot directory maintained elsewhere, so the docstring now says so and points at the way out -- set RENEW_DAYS_BEFORE to the value you want. Tested: two more cases in test_certman.py (11 total) -- no scratch file left behind, and a write that cannot even create the temporary file leaves the original byte-identical. The harness now gives each case its own directory, since one of them makes that directory unwritable. --- .../dstack-ingress/scripts/certman.py | 23 ++++++++++++- .../scripts/tests/test_certman.py | 34 ++++++++++++++++--- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/custom-domain/dstack-ingress/scripts/certman.py b/custom-domain/dstack-ingress/scripts/certman.py index bd87019..12f08a7 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -500,6 +500,13 @@ def apply_renewal_window(self, domain: str) -> None: container, so a value written once would otherwise be permanent, and the documented way to leave the renewal window -- unset the variable -- would do nothing while the certificate stayed permanently due. + + This assumes the container owns the lineage config: clearing removes + any active renew_before_expiry, including one a human put there, since + nothing distinguishes the two. That holds for the volume this image + manages. Mounting a certbot directory maintained elsewhere is out of + scope -- set RENEW_DAYS_BEFORE to the value you want in that case, + rather than leaving it unset and expecting the file to be left alone. """ days = os.environ.get("RENEW_DAYS_BEFORE", "").strip() if days and (not days.isdigit() or int(days) < 1): @@ -556,11 +563,25 @@ def apply_renewal_window(self, domain: str) -> None: # file every pass would only add noise and a chance to corrupt it. return + # Write through a temporary file and rename over the original. Opening + # the live config with "w" truncates it before anything is written, so + # a crash or a full disk mid-write would leave certbot with a truncated + # lineage config -- and a lineage certbot cannot parse is one it cannot + # renew. os.replace is atomic, so a reader sees the old file or the new + # one, never a half-written one. + tmp = f"{path}.dstack-tmp" try: - with open(path, "w", encoding="utf-8") as fh: + with open(tmp, "w", encoding="utf-8") as fh: fh.write("\n".join(out) + "\n") + fh.flush() + os.fsync(fh.fileno()) + os.replace(tmp, path) except OSError as exc: print(f"Warning: cannot write {path}: {exc}", file=sys.stderr) + try: + os.unlink(tmp) + except OSError: + pass return if removed: diff --git a/custom-domain/dstack-ingress/scripts/tests/test_certman.py b/custom-domain/dstack-ingress/scripts/tests/test_certman.py index 4b744df..3bbef2d 100644 --- a/custom-domain/dstack-ingress/scripts/tests/test_certman.py +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -5,6 +5,7 @@ """ import os +import shutil import sys import tempfile import unittest @@ -40,12 +41,15 @@ def _manager(path: str) -> certman.CertManager: class RenewalWindowTest(unittest.TestCase): def setUp(self): self._saved = os.environ.get("RENEW_DAYS_BEFORE") - fd, self.path = tempfile.mkstemp(suffix=".conf") - os.close(fd) + # Its own directory, so a test may make the directory unwritable + # without touching anything it does not own. + self.dir = tempfile.mkdtemp() + self.path = os.path.join(self.dir, "example.com.conf") self.write(CERTBOT_CONF) def tearDown(self): - os.unlink(self.path) + os.chmod(self.dir, 0o700) + shutil.rmtree(self.dir, ignore_errors=True) if self._saved is None: os.environ.pop("RENEW_DAYS_BEFORE", None) else: @@ -112,7 +116,29 @@ def test_missing_lineage_is_not_an_error(self): os.unlink(self.path) self.apply("365") self.assertFalse(os.path.exists(self.path)) - open(self.path, "w").close() # tearDown unlinks it + + def test_write_is_atomic_and_leaves_no_scratch_file(self): + """The live config is renamed into place, never truncated in place. + + Opening it with "w" would empty it before the replacement was written, + and a lineage config certbot cannot parse is one it cannot renew. + """ + self.apply("365") + self.assertFalse( + os.path.exists(self.path + ".dstack-tmp"), + "temporary file left behind", + ) + self.assertTrue(self.read().endswith("\n")) + self.assertIn("[renewalparams]", self.read()) + + def test_a_failed_write_leaves_the_original_intact(self): + original = self.read() + os.chmod(self.dir, 0o500) # cannot create the temp file + try: + self.apply("365") + finally: + os.chmod(self.dir, 0o700) + self.assertEqual(self.read(), original) def test_wildcard_uses_the_bare_lineage_name(self): self.assertEqual( From d6dd438f4d0c5ef840a05abc0f423eae172dcdfb Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Wed, 29 Jul 2026 13:53:01 +0800 Subject: [PATCH 3/3] Potential fix for pull request finding 'CodeQL / Empty except' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- custom-domain/dstack-ingress/scripts/certman.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/custom-domain/dstack-ingress/scripts/certman.py b/custom-domain/dstack-ingress/scripts/certman.py index 12f08a7..4968447 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -580,8 +580,11 @@ def apply_renewal_window(self, domain: str) -> None: print(f"Warning: cannot write {path}: {exc}", file=sys.stderr) try: os.unlink(tmp) - except OSError: - pass + except OSError as cleanup_exc: + print( + f"Warning: cleanup failed for temporary file {tmp}: {cleanup_exc}", + file=sys.stderr, + ) return if removed: