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..4968447 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,21 @@ 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. + + 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 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 +517,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,29 +531,67 @@ 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 + + # 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 as cleanup_exc: + print( + f"Warning: cleanup failed for temporary file {tmp}: {cleanup_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") 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..3bbef2d --- /dev/null +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -0,0 +1,170 @@ +#!/usr/bin/env python3 +"""Unit tests for certman's renewal-window handling. + +Run: python3 scripts/tests/test_certman.py +""" + +import os +import shutil +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") + # 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.chmod(self.dir, 0o700) + shutil.rmtree(self.dir, ignore_errors=True) + 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)) + + 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( + 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)