From 3fdaaded50a4c8dd903e365cd9ad7a942eff1ba3 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Wed, 14 Oct 2026 18:43:04 -0700 Subject: [PATCH] fix(ingress): give wildcard certificates their own certbot lineage With both x.com and *.x.com in DOMAINS, certbot names the lineages x.com and x.com-0001, but certman._lineage_name mapped both names to x.com: one of them was served the other's certificate, the evidence attested the wrong certificate, and renewals targeted the wrong lineage. _lineage_name now finds the lineage whose certificate holds exactly the domain, so lineages certbot named itself keep working without being renamed or re-issued. A new lineage is named explicitly: _wildcard.x.com for a wildcard (an underscore cannot appear in a certificate host name), the domain otherwise, with -NNNN appended if another certificate already holds that name. certonly passes it as --cert-name unless it is a --csr issuance, which keeps no lineage. 2.4-2.6 wrote a wildcard's derived-key files under the bare name, so the legacy live/ cleanup looks there. Signed-off-by: Kevin Wang --- .../dstack-ingress/scripts/certman.py | 57 +++++++-- .../scripts/tests/test_certman.py | 109 +++++++++++++++--- 2 files changed, 141 insertions(+), 25 deletions(-) diff --git a/custom-domain/dstack-ingress/scripts/certman.py b/custom-domain/dstack-ingress/scripts/certman.py index b493c68..572af37 100644 --- a/custom-domain/dstack-ingress/scripts/certman.py +++ b/custom-domain/dstack-ingress/scripts/certman.py @@ -36,6 +36,17 @@ def renewal_due(cert: x509.Certificate, now: datetime) -> bool: return now >= not_before + (lifetime / 2 if lifetime < timedelta(days=10) else lifetime * 2 / 3) +def _cert_names(path: str) -> Optional[List[str]]: + """The DNS names a certificate file is for, or None if it cannot be read.""" + try: + with open(path, "rb") as fh: + cert = x509.load_pem_x509_certificate(fh.read()) + return cert.extensions.get_extension_for_class( + x509.SubjectAlternativeName).value.get_values_for_type(x509.DNSName) + except (OSError, ValueError, x509.ExtensionNotFound): + return None + + def staging_enabled() -> bool: """Whether to use Let's Encrypt staging. @@ -219,9 +230,10 @@ def _build_certbot_command(self, action: str, domain: str, email: str, # 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 "--csr" not in extra_args: + base_cmd.extend(["--cert-name", self._lineage_name(domain)]) if action == "renew": - base_cmd.extend(["--cert-name", self._lineage_name(domain), - "--no-random-sleep-on-renew"]) + base_cmd.append("--no-random-sleep-on-renew") # Explicit on `renew` too: certbot would otherwise take the server # saved in the lineage, whatever the configuration says now. base_cmd.extend(["--server", acme_server()]) @@ -263,16 +275,18 @@ def _build_certbot_command(self, action: str, domain: str, email: str, else: base_cmd.append("--register-unsafely-without-email") base_cmd.extend(["-d", domain]) - if action == "renew": + if "--csr" not in extra_args: # 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. - # + # then reported against whichever domain happened to ask. On + # `certonly` it picks the lineage (see _lineage_name); --csr + # issues outside any lineage. + base_cmd.extend(["--cert-name", self._lineage_name(domain)]) + if action == "renew": # Without a TTY, certbot also sleeps up to 8 minutes before a due # renewal, which overruns _certbot_timeout and gets the run killed. - base_cmd.extend(["--cert-name", self._lineage_name(domain), - "--no-random-sleep-on-renew"]) + base_cmd.append("--no-random-sleep-on-renew") base_cmd.extend(["--server", acme_server()]) if getattr(self.provider, 'CERTBOT_PROPAGATION_SECONDS'): @@ -401,10 +415,11 @@ def _clear_legacy_live(self, domain: str) -> None: to create the lineage over them left an empty renewal config, which would push the next lineage to -0001. """ - conf = self._renewal_conf_path(domain) + base = domain[2:] if domain.startswith("*.") else domain + conf = os.path.join(LETSENCRYPT_DIR, "renewal", f"{base}.conf") if os.path.isfile(conf) and os.path.getsize(conf) == 0: os.remove(conf) - live = self._live_dir(domain) + live = os.path.join(LETSENCRYPT_DIR, "live", base) privkey = os.path.join(live, "privkey.pem") if not os.path.isfile(privkey) or os.path.islink(privkey): return @@ -512,8 +527,28 @@ def certificate_exists(self, domain: str) -> bool: @staticmethod def _lineage_name(domain: str) -> str: - """certbot stores a wildcard lineage under the bare name.""" - return domain[2:] if domain.startswith("*.") else domain + """The lineage holding exactly this domain, else the name to create. + + Left to itself certbot named a wildcard's lineage after the bare name, + the same as the base domain's, so with both in DOMAINS one became + -0001. Existing lineages are therefore found by content. A new + wildcard lineage is _wildcard.: an underscore cannot appear in a + certificate host name, so it never collides with a domain. + """ + base = domain[2:] if domain.startswith("*.") else domain + new = f"_wildcard.{base}" if domain.startswith("*.") else domain + renewal = os.path.join(LETSENCRYPT_DIR, "renewal") + pattern = re.compile(rf"(_wildcard\.)?{re.escape(base)}(-\d{{4}})?") + existing = sorted(os.path.basename(p)[:-len(".conf")] + for p in glob.glob(os.path.join(renewal, "*.conf"))) + for name in [new] + [n for n in existing if pattern.fullmatch(n)]: + if _cert_names(os.path.join(LETSENCRYPT_DIR, "live", name, "cert.pem")) == [domain]: + return name + name, n = new, 0 + while name in existing: + n += 1 + name = f"{new}-{n:04d}" + return name @classmethod def _live_dir(cls, domain: str) -> str: diff --git a/custom-domain/dstack-ingress/scripts/tests/test_certman.py b/custom-domain/dstack-ingress/scripts/tests/test_certman.py index 2d9632f..58744e8 100644 --- a/custom-domain/dstack-ingress/scripts/tests/test_certman.py +++ b/custom-domain/dstack-ingress/scripts/tests/test_certman.py @@ -141,14 +141,6 @@ def test_a_failed_write_leaves_the_original_intact(self): 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" - ) - def test_wildcard_has_its_own_deterministic_dir(self): with mock.patch.dict(os.environ, {"DETERMINISTIC_TLS_KEY": "true"}): self.assertEqual(certman.CertManager.cert_dir("*.example.com"), @@ -191,6 +183,10 @@ 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) + self.root = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.root) + self.addCleanup(setattr, certman, "LETSENCRYPT_DIR", certman.LETSENCRYPT_DIR) + certman.LETSENCRYPT_DIR = self.root def tearDown(self): for v, old in self._saved.items(): @@ -213,15 +209,38 @@ def test_renew_is_scoped_to_one_lineage(self): 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): + def test_renew_uses_the_wildcard_lineage(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): + self.assertEqual(cmd[cmd.index("--cert-name") + 1], "_wildcard.example.com") + + def test_certonly_names_the_lineage(self): + """certbot's own pick for a wildcard is the base domain's name.""" + for delegation in ("", "deleg.example.net"): + os.environ["DELEGATION_ZONE"] = delegation + cmd = _command_manager()._build_certbot_command( + "certonly", "*.example.com", "") + self.assertEqual(cmd[cmd.index("--cert-name") + 1], "_wildcard.example.com") + self.assertEqual(cmd[cmd.index("-d") + 1], "*.example.com") + + def test_csr_issuance_has_no_lineage(self): + os.environ["DETERMINISTIC_TLS_KEY"] = "true" cmd = _command_manager()._build_certbot_command( - "certonly", "a.example.com", "") + "certonly", "*.example.com", "", ("--csr", "x.csr")) self.assertNotIn("--cert-name", cmd) - self.assertIn("-d", cmd) + + def test_replacing_keeps_the_lineage(self): + mgr = _command_manager() + mgr.install_plugin = lambda: True + mgr.provider.validate_credentials = lambda: True + mgr.setup_credentials = lambda: True + cmds = [] + with mock.patch.object(certman.subprocess, "run", + lambda cmd, **kw: cmds.append(cmd) or mock.Mock(returncode=0)): + self.assertTrue(mgr.obtain_certificate("*.x.com", "", replace=True)) + cmd = cmds[0] + self.assertIn("--force-renewal", cmd) + self.assertIn("--break-my-certs", cmd) + self.assertEqual(cmd[cmd.index("--cert-name") + 1], "_wildcard.x.com") def test_delegation_renew_is_scoped_too(self): os.environ["DELEGATION_ZONE"] = "deleg.example.net" @@ -313,6 +332,68 @@ def test_delegation_renew_skips_the_random_sleep(self): self.assertIn("--no-random-sleep-on-renew", cmd) +def _cert_pem(name, key): + from datetime import datetime, timedelta, timezone + from cryptography import x509 + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.x509.oid import NameOID + + now = datetime.now(timezone.utc) + subject = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, name)]) + return (x509.CertificateBuilder() + .subject_name(subject).issuer_name(subject) + .public_key(key.public_key()).serial_number(1) + .not_valid_before(now).not_valid_after(now + timedelta(days=90)) + .add_extension(x509.SubjectAlternativeName([x509.DNSName(name)]), critical=False) + .sign(key, hashes.SHA256()) + .public_bytes(serialization.Encoding.PEM)) + + +class LineageNameTest(EnvTestCase): + """Existing lineages are found by content; new ones get a name of their own.""" + + def setUp(self): + super().setUp() + from cryptography.hazmat.primitives.asymmetric import ec + + self.key = ec.generate_private_key(ec.SECP256R1()) + self.mgr = object.__new__(certman.CertManager) + + def lineage(self, lineage, name): + os.makedirs(f"{self.root}/live/{lineage}") + os.makedirs(f"{self.root}/renewal", exist_ok=True) + with open(f"{self.root}/live/{lineage}/cert.pem", "wb") as fh: + fh.write(_cert_pem(name, self.key)) + open(f"{self.root}/renewal/{lineage}.conf", "w").close() + + def test_new_wildcard_lineage_is_apart_from_the_base(self): + self.assertEqual(self.mgr._lineage_name("*.x.com"), "_wildcard.x.com") + self.assertEqual(self.mgr._lineage_name("x.com"), "x.com") + + def test_lineages_certbot_named_itself_are_found(self): + self.lineage("x.com", "*.x.com") + self.lineage("x.com-0001", "x.com") + self.assertEqual(self.mgr._lineage_name("*.x.com"), "x.com") + self.assertEqual(self.mgr._lineage_name("x.com"), "x.com-0001") + + def test_a_name_taken_by_another_certificate_is_not_reused(self): + self.lineage("x.com", "*.x.com") + self.assertEqual(self.mgr._lineage_name("x.com"), "x.com-0001") + + def test_legacy_wildcard_files_under_the_bare_name_move_to_the_wildcards(self): + os.environ["DETERMINISTIC_TLS_KEY"] = "true" + live = f"{self.root}/live/x.com" + os.makedirs(live) + for item in ("cert", "fullchain"): + with open(f"{live}/{item}.pem", "wb") as fh: + fh.write(_cert_pem("*.x.com", self.key)) + with open(f"{live}/privkey.pem", "wb") as fh: + fh.write(certman.deterministic_key.key_pem(self.key)) + self.mgr._clear_legacy_live("*.x.com") + self.assertFalse(os.path.exists(live)) + self.assertTrue(os.path.exists(f"{self.root}/deterministic/_wildcard.x.com/fullchain.pem")) + + class CertbotTimeoutTest(EnvTestCase): """A fixed 300s cap could not fit every provider's own propagation wait."""