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 <domain> --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 <primary>`, published once to
/etc/haproxy/certs/<primary>.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 <domain>`, 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) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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/<primary>.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 <domain>
|
||||
|
||||
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):
|
||||
|
||||
Reference in New Issue
Block a user