43e522104b96698ac17fcd42c3133784bb2db5d1
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  shadowdaoandClaude Opus 5
|
b072786192 |
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>
|
||
|
shadowdaoandClaude Opus 5
|
4f0949d534 |
fix(certs): stop the test suite bricking the container; make the pairing check real
Gate findings on fix/cert-write-safety. One blocker, one latent truncation
path, a false premise in a load-bearing comment, and a set of tests that were
passing without testing anything.
THE BLOCKER - scripts/test-cert-scripts.py bricked the production container
------------------------------------------------------------------------
The openssl-availability tests build a stripped PATH out of SYMLINKS to real
system binaries (cat, grep, mktemp, mv, cp, rm, mkdir, basename, dirname, find,
date, chmod). _cleanup_tmp() then walked the temp tree calling os.chmod(p,
0o600) - and os.chmod FOLLOWS SYMLINKS. Run once as root in the real image,
which is where this file ships (COPY scripts /haproxy/scripts) and where an
operator would most plausibly run it after deploying a cert fix, it stripped
the exec bit off twelve core binaries INCLUDING chmod itself, so it could not
be undone from inside the container:
/haproxy/scripts/cert-publish-lib.sh: line 109: /usr/bin/grep: Permission denied
bash: /usr/bin/chmod: Permission denied
Certificate publishing stayed dead until the container was recreated. It was
invisible on a workstation because an unprivileged chmod of a root-owned file
fails EPERM straight into `except OSError: pass` - which is also why the
advertised "32 tests pass" was only ever true off-container. In the image the
shipped file measured FAILED (failures=22, skipped=1). Cleanup now skips
symlinks; the suites are green in the image and the binaries survive.
S1 - the shell half had no same-filesystem guard
------------------------------------------------
The header claimed a cross-device mv would "FAIL LOUDLY and leave the live pem
alone". GNU mv does the opposite: across filesystems it copies, so it opens and
truncates the DESTINATION and only then discovers it cannot finish - measured
as a 204800-byte partial live.pem, sentinel gone, before mv reported ENOSPC.
cert_publish() now compares stat -Lc %d of the staging and certs dirs before
writing anything, mirroring the st_dev check the Python half already had, and
the comment says what mv actually does. Latent today (both dirs share a device
on all five hosts) but both are env-overridable.
openssl is present - correct the premise, make the check mandatory
------------------------------------------------------------------
Both halves justified a fail-open with "the image does not necessarily install
the openssl CLI". It does: openssl 3.5.6 in the running container, pulled in by
ca-certificates which certbot needs, and generate_self_signed_cert() already
shells to `openssl req` with check=True during setup. The 'unavailable' branch
never fired, so the pairing check has always run - and that, not the stated
reasoning, is what made the fail-open harmless. Structural validation alone is
weak: a bundle of EMPTY pem blocks passes every structural rule and is caught
only by openssl. The check is now mandatory in both halves and a missing binary
is a loud refusal. No `cryptography` fallback: the app runs on
/usr/local/bin/python3 (3.12) where it is not importable - it belongs to
Debian's /usr/bin/python3 - and reaching for that would be a second unverified
premise.
Smaller items
-------------
* cert_bundle_valid() read the file six times; a concurrent swap between two of
them made openssl x509 and openssl pkey judge different files and log a bogus
"private key does not match the certificate" into the monitored error log. It
now reads one snapshot and feeds openssl from it on stdin.
* except OSError -> except (OSError, UnicodeDecodeError): a BINARY-corrupt live
pem made backup_existing_pem() raise out of publish_pem_bundle() entirely, so
the republish that would have healed the host was the one thing that could
not run. The shell half recovers fine.
* stat -c %a on a symlinked live pem reports the LINK's 0777 and produced a
world-writable private key in the crt directory; now stat -Lc.
* The staging reaper's '*.??????' glob matched mktemp names but not the Python
side's '<name>.<random>.tmp', so those leaked forever. Matches both now.
* renew-certificates.sh and sync-certificates.sh exited 0 even when every
domain failed to publish, so "0 updated, 12 failed" looked identical to a
clean run to cron, to host-renew-certificates.sh (which branches on it) and
to monitoring - a host could silently stop publishing renewals until the
certificates expired. They now exit 1 if any domain failed, still after
publishing the ones that worked.
Test-quality
------------
Four TestCertPublishLibrary tests passed with cert-publish-lib.sh DELETED -
they asserted only rc != 0, and `command not found` is 127. All four now assert
the rejection REASON via assert_rejected(), and setUp() fails if the library is
missing. test_missing_openssl_still_rejects... is replaced by
test_empty_pem_blocks_are_rejected, which pins the case that makes the pairing
check necessary.
Also: the staging-containment test used startswith(certs + os.sep), so
cert_staging_dir() returning the crt directory ITSELF - the exact hazard -
still passed; test_successful_renewal_still_publishes built its "renewed" cert
with a no-op .replace() and could not tell a renewal that published nothing;
test_no_temp_file_survives_a_failed_publish failed before the staging dir
existed and asserted [] == []; FIX_ONLY was skipUnless(hasattr(hm,
'publish_pem_bundle')), so renaming that function turned 10 of 17 tests into
skips while the run still printed OK. Each is fixed and each fix is
mutation-proved: the mutation that the old assertion waved through now fails.
File mode is pinned in both suites (it was pinned nowhere), and the rename
failure is injected with a stub mv instead of chmod 0500, which root ignored -
so that test no longer skips itself precisely where it matters.
Verification
------------
IN THE BUILT IMAGE, as root (the acceptance bar):
scripts/test-cert-scripts.py 38 tests, OK, 0 skipped
scripts/test-cert-write-safety.py 22 tests, OK, 0 skipped
scripts/test-config-rollback.py 17 tests, OK (neighbour, unchanged)
Workstation: 38/38 OK for the shell suite; the Python suite needs flask.
Against the pre-fix tree (main): shell 38 failures; python failures=4, errors=1,
skipped=15 - the FIX_ONLY skips are the 14 fix-only tests plus the API guard.
The old python suite against the pre-fix tree measures skipped=10, confirming
the gate's count.
32 mutation checks, all behaving as intended: every fix breaks a test when
reverted, and every rewritten test fails under the mutation its predecessor
passed. py_compile clean, bash -n clean, shellcheck clean, no new pyflakes
warnings (same 4 pre-existing).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
shadowdaoandClaude Opus 5
|
22dab685d0 |
fix(certs): never truncate a live PEM; validate before publishing
Every path that refreshed a combined certificate did
with open(combined_path, 'w') as combined: # TRUNCATES
subprocess.run(['cat', cert, key], stdout=combined) # rc ignored
where combined_path is the bundle HAProxy is currently serving. The live file
was emptied before any source material had been read and the cat status was
never checked, so an unreadable source, a zero-length privkey, a full disk or a
killed container left a truncated or key-less PEM in place. HAProxy loads
/etc/haproxy/certs as a directory, so one unusable file there fails the whole
ssl bind - HTTPS down for every site on the host, and unlike a bad haproxy.cfg
it is not recoverable by config rollback.
The bundle endpoint compounded it: superseded .pem files were unlinked and
their lineages `certbot delete`d before anything had checked the replacement,
destroying both copies of a working certificate. Recovery there means fresh,
rate-limited ACME orders.
Bundles are now assembled in a staging directory beside the crt directory (never
inside it - HAProxy would try to load a temp file), validated there, and swapped
in with os.replace(). Validation is mandatory structural checks in pure Python
plus a best-effort openssl key/leaf pairing check; a missing openssl warns
loudly and does not silently pass. The previous bundle is copied to
/etc/haproxy/cert-backups first. Superseded certs are moved aside rather than
deleted, and `certbot delete` runs only after HAProxy has reloaded onto the
replacement. The same guarantees are implemented for the cron/renewal shell
path in scripts/cert-publish-lib.sh, which also gates the reload on `haproxy -c`.
Reuses write_config_atomically() from the config-rollback fix (extended with
staging_dir/validate) rather than adding a second atomic writer.
Also heals a zero-byte QUIC cluster-secret file, which previously made
get_or_create_cluster_secret() return '' forever.
Tests: scripts/test-cert-write-safety.py (17) and scripts/test-cert-scripts.py
(32) in the existing stdlib-unittest, stub-binary convention. Both bug classes
reproduce against the pre-fix tree via HAPROXY_MANAGER_DIR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|