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):