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>
This commit is contained in:
Claude Opus 5
+127
-50
@@ -33,9 +33,21 @@
|
||||
# staging: $(dirname $SSL_CERTS_DIR)/cert-staging [$CERT_STAGING_DIR]
|
||||
# backups: $(dirname $SSL_CERTS_DIR)/cert-backups [$CERT_BACKUP_DIR]
|
||||
# Both siblings of the certs dir, so they are on the same filesystem and the
|
||||
# final mv is a rename(2) - atomic. If the mv ever fails (EXDEV because someone
|
||||
# mounted the certs dir separately, permissions, ...) we FAIL LOUDLY and leave
|
||||
# the live pem alone. There is deliberately no "just write it directly" path.
|
||||
# final mv is a rename(2) - atomic. There is deliberately no "just write it
|
||||
# directly into the certs dir" fallback path.
|
||||
#
|
||||
# The same-filesystem property is CHECKED, not assumed (see cert_publish step
|
||||
# (c)). An earlier revision of this comment claimed a cross-device mv would
|
||||
# "fail loudly and leave the live pem alone". It does not: GNU mv falls back to
|
||||
# copy-then-unlink across filesystems, so it OPENS THE DESTINATION FOR WRITING
|
||||
# and only then discovers it cannot finish - e.g. with a full destination
|
||||
# filesystem the live pem is already overwritten when mv reports failure. That
|
||||
# is precisely the truncation this library exists to prevent, so the device
|
||||
# numbers of the staging dir and the certs dir are compared with stat(1) before
|
||||
# anything is written, and a mismatch aborts the publish. Both directories are
|
||||
# env-overridable ($CERT_STAGING_DIR / $SSL_CERTS_DIR), so "they are siblings"
|
||||
# is not something the code can take on faith. This mirrors the explicit
|
||||
# st_dev check in publish_pem_bundle() on the Python side.
|
||||
|
||||
# Logging: the callers define their own log_info/log_error. Only provide
|
||||
# fallbacks so this library is usable standalone (e.g. from a test or a shell).
|
||||
@@ -72,23 +84,37 @@ cert_backup_dir() {
|
||||
#
|
||||
# Returns 0 if FILE is publishable as an HAProxy pem bundle.
|
||||
#
|
||||
# Layer 1 (MANDATORY, pure shell/grep, always available): structural checks.
|
||||
# A missing or failed structural check is a HARD FAIL. This layer is
|
||||
# what actually covers the truncation / partial-write / key-less
|
||||
# failure modes this library exists to prevent.
|
||||
# Layer 2 (BEST EFFORT): cryptographic pairing via the openssl CLI.
|
||||
# If openssl runs and says the cert and key do not match, that is a
|
||||
# HARD FAIL. If the openssl BINARY IS ABSENT we log a loud warning and
|
||||
# accept the bundle on the structural checks alone.
|
||||
# Layer 1 (structure, pure shell/grep): covers the truncation / partial-write /
|
||||
# key-less failure modes this library exists to prevent.
|
||||
# Layer 2 (cryptographic pairing via the openssl CLI): covers what layer 1
|
||||
# cannot see. Structural checks are weak on their own - a bundle of
|
||||
# EMPTY pem blocks ("-----BEGIN CERTIFICATE-----" immediately followed
|
||||
# by "-----END CERTIFICATE-----") satisfies every grep below and is
|
||||
# caught only by openssl.
|
||||
#
|
||||
# Rationale for not hard-failing on a missing checker: the container
|
||||
# image (see Dockerfile) installs haproxy, certbot and socat, but not
|
||||
# necessarily the openssl CLI. Refusing to publish when the checker is
|
||||
# missing would stall every renewal fleet-wide and let certificates
|
||||
# expire - a guaranteed outage - which is strictly worse than the risk
|
||||
# it prevents, since a mismatched pair can only arise from a
|
||||
# mis-assembled source tree, whereas the truncation modes we are
|
||||
# actually defending against are fully covered by layer 1.
|
||||
# BOTH LAYERS ARE MANDATORY. An absent openssl binary is a hard failure, not a
|
||||
# skip.
|
||||
#
|
||||
# The previous revision made layer 2 best-effort and justified it with "the
|
||||
# container image installs haproxy, certbot and socat, but not necessarily the
|
||||
# openssl CLI". That premise is false. openssl 3.x is present in the image: it
|
||||
# is a dependency of ca-certificates, which certbot needs, and
|
||||
# generate_self_signed_cert() in haproxy_manager.py shells out to `openssl req`
|
||||
# with check=True during first-run setup, so a container that reached the point
|
||||
# of publishing a bundle has always had it. The "unavailable" branch therefore
|
||||
# never fired in production, which means the fail-open was safe only by
|
||||
# accident - and a comment that justifies a decision on a false premise is
|
||||
# worse than no comment, because the next person extends the reasoning.
|
||||
#
|
||||
# Making it mandatory does mean a hypothetical image without openssl stops
|
||||
# publishing renewals. That is the right trade: it fails immediately and
|
||||
# loudly, into the monitored error log, on the first renewal run, whereas
|
||||
# publishing an unpaired or empty-block bundle takes the whole :443 bind (i.e.
|
||||
# every site on the host) down at the next reload. There is deliberately no
|
||||
# python `cryptography` fallback: the app runs on /usr/local/bin/python3 (the
|
||||
# base image's 3.12), where cryptography is NOT importable - it is installed
|
||||
# for Debian's /usr/bin/python3 as a certbot dependency. Coding to a hardcoded
|
||||
# /usr/bin/python3 would just be a second unverified premise.
|
||||
cert_bundle_valid() {
|
||||
local file="$1"
|
||||
|
||||
@@ -100,55 +126,71 @@ cert_bundle_valid() {
|
||||
log_error "Certificate bundle $file does not exist (or is not a regular file)"
|
||||
return 1
|
||||
fi
|
||||
if [ ! -s "$file" ]; then
|
||||
|
||||
# openssl is checked BEFORE any content check so a broken image is reported
|
||||
# as a broken image rather than as a bad certificate.
|
||||
if ! command -v openssl >/dev/null 2>&1; then
|
||||
log_error "openssl binary not found - REFUSING to publish $file." \
|
||||
"The cert/key pairing check (openssl x509 -pubkey vs openssl pkey -pubout)" \
|
||||
"is mandatory; structural checks alone cannot tell a real bundle from" \
|
||||
"empty pem blocks. Install openssl in this image."
|
||||
return 1
|
||||
fi
|
||||
|
||||
# Read the file ONCE and run every check against that snapshot.
|
||||
#
|
||||
# This used to open $file six times (two [ ] tests, three greps, two
|
||||
# openssl invocations). cert_bundle_valid() is called on the LIVE pem in
|
||||
# cert_publish() step (d), where a concurrent publisher can replace it
|
||||
# between two of those opens - each open then sees a different file. The
|
||||
# observable symptom was a spurious "private key does not match the
|
||||
# certificate" ERROR in the monitored error log for a pair that was fine:
|
||||
# openssl x509 read the old bundle and openssl pkey the new one.
|
||||
local content
|
||||
if ! content="$(cat -- "$file" 2>/dev/null)"; then
|
||||
log_error "Certificate bundle $file could not be read"
|
||||
return 1
|
||||
fi
|
||||
if [ -z "$content" ]; then
|
||||
log_error "Certificate bundle $file is empty"
|
||||
return 1
|
||||
fi
|
||||
|
||||
# --- layer 1: structure -------------------------------------------------
|
||||
if ! grep -qF -- '-----BEGIN CERTIFICATE-----' "$file"; then
|
||||
if ! grep -qF -- '-----BEGIN CERTIFICATE-----' <<< "$content"; then
|
||||
log_error "Certificate bundle $file contains no certificate block"
|
||||
return 1
|
||||
fi
|
||||
if ! grep -qF -- '-----END CERTIFICATE-----' "$file"; then
|
||||
if ! grep -qF -- '-----END CERTIFICATE-----' <<< "$content"; then
|
||||
log_error "Certificate bundle $file has an unterminated certificate block (truncated?)"
|
||||
return 1
|
||||
fi
|
||||
|
||||
local key_begin key_end
|
||||
key_begin="$(grep -m1 -oE -- '-----BEGIN (RSA |EC )?PRIVATE KEY-----' "$file")"
|
||||
key_begin="$(grep -m1 -oE -- '-----BEGIN (RSA |EC )?PRIVATE KEY-----' <<< "$content")"
|
||||
if [ -z "$key_begin" ]; then
|
||||
log_error "Certificate bundle $file contains no private key block"
|
||||
return 1
|
||||
fi
|
||||
key_end="${key_begin/BEGIN/END}"
|
||||
if ! grep -qF -- "$key_end" "$file"; then
|
||||
if ! grep -qF -- "$key_end" <<< "$content"; then
|
||||
log_error "Certificate bundle $file has an unterminated private key block (truncated?)"
|
||||
return 1
|
||||
fi
|
||||
|
||||
# --- layer 2: cert/key pairing ------------------------------------------
|
||||
if ! command -v openssl >/dev/null 2>&1; then
|
||||
# Once per process: this fires per domain otherwise, and a renewal run
|
||||
# walks every certificate on the host.
|
||||
if [ -z "${_CERT_OPENSSL_WARNED:-}" ]; then
|
||||
_CERT_OPENSSL_WARNED=1
|
||||
log_warn "openssl binary not found - SKIPPING the cert/key pairing check" \
|
||||
"(openssl x509 -pubkey vs openssl pkey -pubout);" \
|
||||
"certificate bundles are being published on structural checks alone"
|
||||
fi
|
||||
return 0
|
||||
fi
|
||||
|
||||
# Fed from the same snapshot on stdin (openssl reads stdin when -in is
|
||||
# omitted) rather than re-opening $file, so layer 2 judges exactly the
|
||||
# bytes layer 1 judged. -passin pass: means an encrypted key fails fast
|
||||
# instead of prompting - a passphrase prompt in a cron job is a hang, not
|
||||
# an error.
|
||||
local cert_pub key_pub
|
||||
# </dev/null on both: openssl pkey prompts for a passphrase on an encrypted
|
||||
# key, and a prompt in a cron job is a hang, not an error.
|
||||
if ! cert_pub="$(openssl x509 -in "$file" -noout -pubkey 2>/dev/null </dev/null)" \
|
||||
if ! cert_pub="$(openssl x509 -noout -pubkey 2>/dev/null <<< "$content")" \
|
||||
|| [ -z "$cert_pub" ]; then
|
||||
log_error "Certificate bundle $file: openssl could not read the certificate"
|
||||
return 1
|
||||
fi
|
||||
if ! key_pub="$(openssl pkey -in "$file" -pubout -passin pass: 2>/dev/null </dev/null)" \
|
||||
if ! key_pub="$(openssl pkey -pubout -passin pass: 2>/dev/null <<< "$content")" \
|
||||
|| [ -z "$key_pub" ]; then
|
||||
log_error "Certificate bundle $file: openssl could not read the private key"
|
||||
return 1
|
||||
@@ -174,7 +216,7 @@ cert_publish() {
|
||||
fi
|
||||
|
||||
local cert_file="$1" key_file="$2" dest_file="$3"
|
||||
local staging_dir backup_dir tmp base
|
||||
local staging_dir backup_dir dest_dir tmp base
|
||||
|
||||
# (a) sources must exist and be non-empty before we touch anything.
|
||||
if [ ! -s "$cert_file" ]; then
|
||||
@@ -189,13 +231,42 @@ cert_publish() {
|
||||
# (b) assemble in the staging dir - NOT in the certs dir, which HAProxy
|
||||
# scans wholesale.
|
||||
staging_dir="$(cert_staging_dir)"
|
||||
if ! mkdir -p "$staging_dir"; then
|
||||
log_error "cert_publish: cannot create staging directory $staging_dir"
|
||||
dest_dir="$(dirname "$dest_file")"
|
||||
if ! mkdir -p "$staging_dir" || ! mkdir -p "$dest_dir"; then
|
||||
log_error "cert_publish: cannot create staging directory $staging_dir or destination directory $dest_dir"
|
||||
return 1
|
||||
fi
|
||||
# Sweep temps orphaned by a kill -9 / OOM in an earlier run. Restricted to
|
||||
# the mktemp suffix shape inside our own staging dir.
|
||||
find "$staging_dir" -maxdepth 1 -type f -name '*.??????' -mmin +1440 -delete 2>/dev/null
|
||||
|
||||
# (c) the final swap is `mv`, which is only a rename(2) - and therefore only
|
||||
# atomic - within one filesystem. Across filesystems GNU mv copies:
|
||||
# it truncates and writes the DESTINATION, then unlinks the source, so a
|
||||
# failure part-way through (ENOSPC is the realistic one) leaves exactly
|
||||
# the half-written live pem this library exists to prevent. Both paths
|
||||
# are env-overridable, so check instead of assuming. Same check as the
|
||||
# st_dev comparison in publish_pem_bundle() on the Python side.
|
||||
local staging_dev dest_dev
|
||||
staging_dev="$(stat -Lc '%d' "$staging_dir" 2>/dev/null)"
|
||||
dest_dev="$(stat -Lc '%d' "$dest_dir" 2>/dev/null)"
|
||||
if [ -z "$staging_dev" ] || [ -z "$dest_dev" ]; then
|
||||
log_error "cert_publish: cannot stat $staging_dir and/or $dest_dir - refusing to publish $dest_file"
|
||||
return 1
|
||||
fi
|
||||
if [ "$staging_dev" != "$dest_dev" ]; then
|
||||
log_error "cert_publish: staging dir $staging_dir and destination dir $dest_dir" \
|
||||
"are on different filesystems, so the bundle cannot be swapped in atomically." \
|
||||
"Refusing to publish $dest_file (live file left untouched);" \
|
||||
"point CERT_STAGING_DIR at a directory on the same filesystem as $dest_dir"
|
||||
return 1
|
||||
fi
|
||||
|
||||
# Sweep temps orphaned by a kill -9 / OOM in an earlier run. Two shapes,
|
||||
# because two publishers share this directory: mktemp's six-X suffix from
|
||||
# this library, and `<name>.<random>.tmp` from write_config_atomically() on
|
||||
# the Python side (tempfile.mkstemp(prefix=name + '.', suffix='.tmp')).
|
||||
# Matching only the mktemp shape - as this did - left every Python-side
|
||||
# temp behind forever.
|
||||
find "$staging_dir" -maxdepth 1 -type f \
|
||||
\( -name '*.??????' -o -name '*.tmp' \) -mmin +1440 -delete 2>/dev/null
|
||||
|
||||
base="$(basename "$dest_file")"
|
||||
tmp="$(mktemp "${staging_dir}/${base}.XXXXXX" 2>/dev/null)"
|
||||
@@ -218,14 +289,14 @@ cert_publish() {
|
||||
return 1
|
||||
fi
|
||||
|
||||
# (c) never promote something HAProxy would choke on.
|
||||
# (d) never promote something HAProxy would choke on.
|
||||
if ! cert_bundle_valid "$tmp"; then
|
||||
log_error "cert_publish: assembled bundle for $dest_file failed validation (live file left untouched)"
|
||||
rm -f "$tmp"
|
||||
return 1
|
||||
fi
|
||||
|
||||
# (d) back up the bundle we are about to replace - but only if it is itself
|
||||
# (e) back up the bundle we are about to replace - but only if it is itself
|
||||
# valid. Overwriting a good backup with garbage would turn "restore the
|
||||
# backup" into "restore a different broken file". Same semantics as
|
||||
# create_backup(require_valid=True) in haproxy_manager.py.
|
||||
@@ -252,12 +323,18 @@ cert_publish() {
|
||||
# who can read these files is not something a write-safety fix should do as
|
||||
# a side effect - and it must match write_config_atomically() on the Python
|
||||
# side, which preserves the mode the same way.
|
||||
#
|
||||
# -L (follow symlinks) matters: stat without it reports the mode of the
|
||||
# SYMLINK, which is 0777 on Linux and is not a permission at all. A live
|
||||
# pem that is a symlink therefore produced a world-WRITABLE 0777 private
|
||||
# key sitting in the crt directory. With -L we copy the mode of the file
|
||||
# the link points at, which is the mode an operator actually chose.
|
||||
local mode
|
||||
mode="$(stat -c '%a' "$dest_file" 2>/dev/null)"
|
||||
mode="$(stat -Lc '%a' "$dest_file" 2>/dev/null)"
|
||||
[ -n "$mode" ] || mode=644
|
||||
chmod "$mode" "$tmp" 2>/dev/null
|
||||
|
||||
# (e) atomic swap. Same filesystem by construction; if it still fails,
|
||||
# (f) atomic swap. Same filesystem, verified in (c); if it still fails,
|
||||
# stop - do not fall back to writing into the certs dir.
|
||||
if ! mv -f "$tmp" "$dest_file"; then
|
||||
log_error "cert_publish: failed to move $tmp into place as $dest_file" \
|
||||
|
||||
Reference in New Issue
Block a user