From b07278619215ea2443d8ecded511efcfc0b3aeea Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 04:47:26 -0700 Subject: [PATCH] fix(domain-removal): stop deleting certificates other live sites are served from DELETE /api/domain ran, unconditionally, for any ssl_enabled row: os.remove(ssl_cert_path) certbot delete --cert-name --non-interactive `domains.domain` is UNIQUE. `domains.ssl_cert_path` is not, and nothing anywhere guarded against two rows naming the same file. Sharing is not an edge case -- it is the normal shape of the table, because request_ssl_bundle() deliberately creates it: one SAN certificate is issued as `--cert-name `, published once to /etc/haproxy/certs/.pem, and then EVERY included name's row is pointed at that same path ("Mark every name in the bundle as ssl_enabled, all pointing at the same combined .pem"). Measured read-only on the live SQLite in the haproxy-manager containers on 2026-08-23: * whp01: 157 domain rows, 150 ssl_enabled, 71 distinct cert paths. 39 of those paths are referenced by MORE THAN ONE row, covering 118 of the 150 SSL-enabled rows. Worst cases: brain-jar.com.pem and arclightcourt.com.pem with 10 domains each, hackerpublicradio.org.pem with 6, anhonesthost.com.pem with 5. * whp02: 33 rows, 31 ssl_enabled, 15 distinct paths, 11 shared across 27 rows -- including threeworldsoneheart.org.pem, referenced by the apex, its www, and mail.threeworldsoneheart.org. A production cleanup of that mail.* row was stopped short precisely because removing it would have unlinked the PEM the serving site is using. So removing one domain unlinked a file up to nine other configured domains were being served from. HAProxy binds the crt directory (`bind ... ssl crt /etc/haproxy/certs`), so the loss is not noticed until the next reload or restart, at which point the listener refuses to come up or those names fall back to the wrong certificate. `certbot delete` is the worse half. It destroys the lineage's archive, live symlinks and renewal config; recovery is a fresh, rate-limited ACME order. The old code passed `--cert-name `, which is also simply the wrong lineage for a SAN member: 81 of whp01's 150 SSL-enabled rows have a cert path whose basename is not their own domain, so for those the call was a silent no-op -- while for a bundle PRIMARY it deleted the one lineage still renewing the certificate every other name in the bundle is served with. The fix refcounts, after the row is deleted so the query answers "who else still needs this": * lineage_name_for_cert_path() -- the lineage is the published bundle's basename minus .pem, the same derivation _quarantine_superseded_certs() already uses, not the domain being removed. * domains_referencing_cert_path() / domains_referencing_lineage() -- the remaining rows that name that file, and that lineage. * remove_domain() unlinks only when the list is empty, `certbot delete`s only when the list is empty, logs the retained names explicitly when it skips, and reports them as certificate_retained_for / lineage_retained_for in the API response. ssl_enabled is deliberately not filtered on in the refcount: the two mistakes are not symmetric. A stale PEM left in the crt directory costs a few kilobytes; an unlinked live one is HTTPS down for every name it serves. Cleanup is deferred, not cancelled -- removing the last name on a bundle still unlinks the file and deletes the lineage. No row on either production host has ssl_enabled=1 with an empty ssl_cert_path, so the "no path, no attributable lineage" branch changes nothing on the current fleet. Tests (scripts/test-cert-write-safety.py, +9, suite now 31, all offline): last reference -> file unlinked and lineage deleted; shared file survives removal of a SAN member AND of the bundle primary, byte-for-byte, with the edge still starting; shared lineage is not certbot-deleted; the production mail.* shape; removing every name eventually cleans up; an unrelated bundle is never collateral damage. Assertions are on os.path.exists, file contents and the recorded certbot argv, never on which branch ran. Mutation-tested, all five mutants killed: 1. guard absent entirely (suite run with HAPROXY_MANAGER_DIR pointed at main) -> 6 failures, incl. "example.com is still configured and still served from this file". 2. refcount taken before the row is deleted -> 5 failures, incl. "the last reference is gone - now it may be removed". 3. certbot guard removed, file guard kept -> 3 failures, incl. [] != ['delete --cert-name example.com --non-interactive']. 4. lineage taken from the domain name instead of the cert path -> 3 failures, incl. 'delete --cert-name example.com' != 'delete --cert-name www.example.com'. 5. file-unlink guard removed, certbot guard kept -> 4 failures, incl. "two sites are still served from this bundle". Other suites unchanged and green: test-config-rollback, test-cert-scripts, test-stick-table-contract, test-runtime-map-contract. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 1 + VERSION | 2 +- haproxy_manager.py | 130 ++++++++++++++++++--- scripts/test-cert-write-safety.py | 180 ++++++++++++++++++++++++++++++ 4 files changed, 295 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ff3f3aa..f4c43cf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,6 +9,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co - **Certificate Request Testing**: `./scripts/test-certificate-request.sh` - Tests certificate generation endpoints - **Stick-table contract**: `python3 scripts/test-stick-table-contract.py` - offline; holds the templates' `store` clauses, `STICK_TABLE_FIELD_CONTRACT`, and every consumer to each other. Run it after touching any `stick-table` line. - **Runtime-map contract**: `python3 scripts/test-runtime-map-contract.py` - offline; asserts the runtime map commands are `@1`-prefixed, reference the map by FILE PATH (never `#`), carry the value `1`, and that every captured rejection is classified as a failure. Run it after touching any `add map`/`del map`/`clear map` path. +- **Certificate destruction safety**: `python3 scripts/test-cert-write-safety.py` - offline; asserts a live `.pem` is never truncated, removed, or its certbot lineage deleted while any configured domain still references it (one bundle serves many names, so `ssl_cert_path` is routinely shared). Run it after touching any `os.remove`/`certbot delete`/PEM-write path. - **Manual Testing**: Run `curl` commands against `http://localhost:8000` endpoints as shown in README.md ### Reading stick tables (and why it is easy to get silently wrong) diff --git a/VERSION b/VERSION index 9b7e9f6..0d92381 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2026.08.10 +2026.08.11 diff --git a/haproxy_manager.py b/haproxy_manager.py index 6016b94..2e023bf 100644 --- a/haproxy_manager.py +++ b/haproxy_manager.py @@ -1624,6 +1624,61 @@ def request_certificates(): else: return jsonify(response), 500 # All failed +def lineage_name_for_cert_path(cert_path): + """The certbot lineage name implied by a published bundle path. + + Every writer in this module publishes to ``{SSL_CERTS_DIR}/.pem`` and + issues that lineage with ``--cert-name `` (see request_ssl() and + request_ssl_bundle()); _quarantine_superseded_certs() derives the lineage + from the filename the same way. So the basename minus ``.pem`` IS the + lineage, and it is NOT necessarily the domain being removed: a bundle + issued as ``--cert-name example.com`` also serves www.example.com and any + other SAN, all of whose DB rows point at ``/etc/haproxy/certs/example.com.pem``. + """ + if not cert_path: + return None + base = os.path.basename(cert_path) + return base[:-len('.pem')] if base.endswith('.pem') else base + + +def domains_referencing_cert_path(cursor, cert_path): + """Domains (still in the table) whose ssl_cert_path is exactly `cert_path`. + + Call this AFTER the row being removed has been deleted, so the answer is + "who else still needs this file". + + ssl_enabled is deliberately NOT filtered on. HAProxy binds the whole crt + directory, so a file is load-bearing for any row that names it; and the + asymmetry of the two mistakes is total - keeping a stale PEM costs nothing, + unlinking a live one is HTTPS down for every name it serves. + """ + if not cert_path: + return [] + cursor.execute( + 'SELECT domain FROM domains WHERE ssl_cert_path = ? ORDER BY domain', + (cert_path,)) + return [row[0] for row in cursor.fetchall()] + + +def domains_referencing_lineage(cursor, lineage): + """Domains (still in the table) served by certbot lineage `lineage`. + + Same shape as domains_referencing_cert_path(), one level further back: + `certbot delete --cert-name X` destroys the archive, live symlinks and + renewal config for X. If any remaining domain is served by a bundle + published from that lineage, deleting it means the next renewal silently + stops happening for all of them and the material cannot be recovered + without a fresh, rate-limited ACME order. + """ + if not lineage: + return [] + cursor.execute( + "SELECT domain, ssl_cert_path FROM domains " + "WHERE ssl_cert_path IS NOT NULL AND ssl_cert_path != ''") + return sorted({domain for domain, path in cursor.fetchall() + if lineage_name_for_cert_path(path) == lineage}) + + @app.route('/api/domain', methods=['DELETE']) @require_api_key def remove_domain(): @@ -1662,33 +1717,74 @@ def remove_domain(): # Delete domain cursor.execute('DELETE FROM domains WHERE id = ?', (domain_id,)) + # Refcount the certificate BEFORE anything is unlinked or deleted, + # with the row above already gone so the query answers "who else + # still needs this". One .pem serves many names: request_ssl_bundle() + # issues a single SAN cert and points EVERY included domain's row at + # the same /etc/haproxy/certs/.pem. Removing one of those + # names used to os.remove() that file and `certbot delete` its + # lineage unconditionally - taking HTTPS down for every other name + # in the bundle, and destroying the only recoverable copy with it. + cert_path_users = domains_referencing_cert_path(cursor, ssl_cert_path) + lineage = lineage_name_for_cert_path(ssl_cert_path) if ssl_cert_path else None + # The lineage to delete is the one this domain's bundle came from, + # not the domain's own name - for a SAN member those differ. + lineage_users = domains_referencing_lineage(cursor, lineage) + + cert_retained_for = [] + lineage_retained_for = [] + # Delete SSL certificate from HAProxy certs directory if ssl_enabled and ssl_cert_path: - try: - os.remove(ssl_cert_path) - logger.info(f"Removed HAProxy certificate file: {ssl_cert_path}") - except OSError as e: - logger.warning(f"Failed to remove certificate file {ssl_cert_path}: {e}") + if cert_path_users: + cert_retained_for = cert_path_users + logger.info( + "Kept HAProxy certificate file %s while removing %s: still " + "referenced by %d other domain(s): %s", + ssl_cert_path, domain, len(cert_path_users), + ', '.join(cert_path_users)) + else: + try: + os.remove(ssl_cert_path) + logger.info(f"Removed HAProxy certificate file: {ssl_cert_path}") + except OSError as e: + logger.warning(f"Failed to remove certificate file {ssl_cert_path}: {e}") # Remove certificate from certbot if ssl_enabled: - try: - result = subprocess.run( - ['certbot', 'delete', '--cert-name', domain, '--non-interactive'], - capture_output=True, text=True - ) - if result.returncode == 0: - logger.info(f"Removed Let's Encrypt certificate for {domain}") - else: - logger.warning(f"Failed to remove Let's Encrypt certificate for {domain}: {result.stderr}") - except Exception as e: - logger.warning(f"Error removing Let's Encrypt certificate for {domain}: {e}") + if not lineage: + logger.info( + "Skipping certbot delete for %s: no certificate path on the " + "removed row, so no lineage can be attributed to it", domain) + elif lineage_users: + lineage_retained_for = lineage_users + logger.info( + "Kept Let's Encrypt lineage %s while removing %s: still " + "serving %d other domain(s): %s", + lineage, domain, len(lineage_users), ', '.join(lineage_users)) + else: + try: + result = subprocess.run( + ['certbot', 'delete', '--cert-name', lineage, '--non-interactive'], + capture_output=True, text=True + ) + if result.returncode == 0: + logger.info(f"Removed Let's Encrypt certificate {lineage} for {domain}") + else: + logger.warning(f"Failed to remove Let's Encrypt certificate {lineage} for {domain}: {result.stderr}") + except Exception as e: + logger.warning(f"Error removing Let's Encrypt certificate {lineage} for {domain}: {e}") # Regenerate HAProxy config generate_config() log_operation('remove_domain', True, f'Domain {domain} removed successfully') - return jsonify({'status': 'success', 'message': 'Domain configuration removed'}) + return jsonify({ + 'status': 'success', + 'message': 'Domain configuration removed', + 'certificate_retained_for': cert_retained_for, + 'lineage_retained_for': lineage_retained_for, + }) except Exception as e: log_operation('remove_domain', False, str(e)) diff --git a/scripts/test-cert-write-safety.py b/scripts/test-cert-write-safety.py index 9a94ea0..aea6f8a 100755 --- a/scripts/test-cert-write-safety.py +++ b/scripts/test-cert-write-safety.py @@ -27,6 +27,8 @@ These tests pin the invariants: * nothing is published that is not a complete, validated cert+key pair; * no old certificate file is removed and no lineage deleted until the replacement is validated, in place, and actually loaded by HAProxy; + * removing ONE domain never unlinks a .pem, or deletes a certbot lineage, + that other still-configured domains are being served from; * only final .pem files ever exist in the crt directory. Running @@ -894,6 +896,184 @@ class TestBundleValidation(CertPublishTestCase): 'a private key must never be group/world writable') +class TestSharedCertificateSurvivesDomainRemoval(CertPublishTestCase): + """Bug 5: DELETE /api/domain unlinked a PEM other live sites were served from. + + `domains.domain` is UNIQUE; `domains.ssl_cert_path` is not, and nothing + ever made it so. request_ssl_bundle() issues one SAN certificate and points + every included name's row at the same /etc/haproxy/certs/.pem, so + sharing is not an edge case - it is the normal shape of the table. Measured + on production the day this was written: 39 of 71 distinct cert paths on one + host were referenced by more than one domain row (118 of 150 SSL-enabled + rows), and one shared path was + /etc/haproxy/certs/threeworldsoneheart.org.pem, referenced by the live + apex, its www, and a mail.* alias. + + remove_domain() did, unconditionally: + + os.remove(ssl_cert_path) + certbot delete --cert-name + + Removing the mail.* alias would therefore have deleted the PEM the apex was + serving on, and (had the alias been the bundle primary) the lineage behind + it - HTTPS down for every other name in the bundle, with no local copy and + only a fresh, rate-limited ACME order to recover from. + + These tests assert file-system and certbot-invocation outcomes, not which + branch was taken. + """ + + BUNDLE = '/etc/haproxy/certs' # documentation only; SSL_CERTS_DIR is stubbed + + def _cert_for(self, primary): + """Publish a bundle for `primary` and return its path.""" + return self.publish_live_bundle(primary) + + def remove(self, domain): + return self.client.delete('/api/domain', json={'domain': domain}) + + # -- last reference: the cleanup must still happen -------------------- + + def test_last_reference_removal_unlinks_the_pem(self): + cert = self._cert_for('example.com') + self.add_domain('example.com', 'be_example', ssl_cert_path=cert) + + resp = self.remove('example.com') + + self.assertEqual(200, resp.status_code, resp.data) + self.assertFalse( + os.path.exists(cert), + 'nothing else referenced this bundle - it must be cleaned up, or ' + 'the crt directory accumulates certs for domains that are gone') + + def test_last_reference_removal_deletes_the_lineage(self): + cert = self._cert_for('example.com') + self.add_domain('example.com', 'be_example', ssl_cert_path=cert) + + self.remove('example.com') + + self.assertEqual( + ['delete --cert-name example.com --non-interactive'], + self.certbot_deletes(), + 'the last name on a lineage went away - the lineage should go too') + + def test_lineage_deleted_is_the_bundles_not_the_domains_own_name(self): + """Removing a SAN member must target the lineage that issued the file.""" + cert = self._cert_for('example.com') + self.add_domain('www.example.com', 'be_www', ssl_cert_path=cert) + + self.remove('www.example.com') + + self.assertEqual( + ['delete --cert-name example.com --non-interactive'], + self.certbot_deletes(), + 'the lineage is named after the bundle primary (--cert-name), not ' + 'after whichever SAN happened to be removed last') + + # -- shared reference: nothing may be destroyed ----------------------- + + def test_shared_pem_survives_removal_of_one_name(self): + cert = self._cert_for('example.com') + before = self.read(cert) + self.add_domain('example.com', 'be_apex', ssl_cert_path=cert) + self.add_domain('www.example.com', 'be_www', ssl_cert_path=cert) + + resp = self.remove('www.example.com') + + self.assertEqual(200, resp.status_code, resp.data) + self.assertTrue( + os.path.exists(cert), + 'example.com is still configured and still served from this file') + self.assertEqual(before, self.read(cert), + 'the surviving bundle must be byte-for-byte intact') + self.assertTrue(self.edge_would_start(), + 'the edge must still load the crt directory') + + def test_shared_pem_survives_removal_of_the_bundle_primary(self): + """The worst shape: the name being removed IS the lineage/file name.""" + cert = self._cert_for('example.com') + before = self.read(cert) + self.add_domain('example.com', 'be_apex', ssl_cert_path=cert) + self.add_domain('www.example.com', 'be_www', ssl_cert_path=cert) + + self.remove('example.com') + + self.assertTrue( + os.path.exists(cert), + 'www.example.com is still configured and is served from this exact ' + 'file - removing the apex must not unlink it') + self.assertEqual(before, self.read(cert)) + self.assertTrue(self.edge_would_start()) + + def test_shared_lineage_is_not_certbot_deleted(self): + cert = self._cert_for('example.com') + self.add_domain('example.com', 'be_apex', ssl_cert_path=cert) + self.add_domain('www.example.com', 'be_www', ssl_cert_path=cert) + + self.remove('example.com') + + self.assertEqual( + [], self.certbot_deletes(), + 'certbot delete destroys archive, live and renewal config for a ' + 'lineage that is still renewing the certificate www.example.com ' + 'is served with') + + def test_production_shape_mail_alias_removal(self): + """The exact row that stopped a production cleanup. + + mail.threeworldsoneheart.org carried + ssl_cert_path=/etc/haproxy/certs/threeworldsoneheart.org.pem - the live + PEM of a different, serving site. + """ + cert = self._cert_for('threeworldsoneheart.org') + before = self.read(cert) + self.add_domain('threeworldsoneheart.org', 'be_apex', ssl_cert_path=cert) + self.add_domain('www.threeworldsoneheart.org', 'be_www', ssl_cert_path=cert) + self.add_domain('mail.threeworldsoneheart.org', 'be_mail', ssl_cert_path=cert) + + self.remove('mail.threeworldsoneheart.org') + + self.assertTrue(os.path.exists(cert), + 'two sites are still served from this bundle') + self.assertEqual(before, self.read(cert)) + self.assertEqual([], self.certbot_deletes()) + self.assertTrue(self.edge_would_start()) + + def test_removing_every_name_eventually_cleans_up(self): + """The guard defers cleanup, it does not cancel it.""" + cert = self._cert_for('example.com') + self.add_domain('example.com', 'be_apex', ssl_cert_path=cert) + self.add_domain('www.example.com', 'be_www', ssl_cert_path=cert) + + self.remove('www.example.com') + self.assertTrue(os.path.exists(cert), 'apex still needs it') + + self.remove('example.com') + self.assertFalse(os.path.exists(cert), + 'the last reference is gone - now it may be removed') + self.assertEqual( + ['delete --cert-name example.com --non-interactive'], + self.certbot_deletes()) + + def test_unrelated_domains_certificate_is_untouched(self): + """Sharing is by exact path; different bundles stay independent.""" + mine = self._cert_for('example.com') + theirs = self._cert_for('other.example') + theirs_before = self.read(theirs) + self.add_domain('example.com', 'be_mine', ssl_cert_path=mine) + self.add_domain('other.example', 'be_theirs', ssl_cert_path=theirs) + + self.remove('example.com') + + self.assertFalse(os.path.exists(mine)) + self.assertTrue(os.path.exists(theirs), + 'a different bundle must not be collateral damage') + self.assertEqual(theirs_before, self.read(theirs)) + self.assertEqual( + ['delete --cert-name example.com --non-interactive'], + self.certbot_deletes()) + + @unittest.skipIf(TESTING_FOREIGN_TREE, 'HAPROXY_MANAGER_DIR points at another tree') class TestPublisherApiIsPresent(unittest.TestCase): -- 2.52.0